diff mbox series

[FFmpeg-devel,3/3] mmaldec with plain yuv420p without copy

Message ID ca7c4ece.AMUAAJVkjlsAAAAAAAAAALF60VUAARpcY_sAAAAAAAeRJgBgJaZe@mailjet.com
State New
Headers show
Series mjpeg_mmal and avoid a frame copy in mmaldec
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Lluís Batlle i Rossell Feb. 11, 2021, 9:48 p.m. UTC
From: Lluís Batlle i Rossell <viric@viric.name>

---
 libavcodec/mmaldec.c | 48 ++++++++++++++++++++------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

Comments

Mark Thompson Feb. 11, 2021, 10:34 p.m. UTC | #1
On 11/02/2021 21:48, Lluís Batlle i Rossel wrote:
> From: Lluís Batlle i Rossell <viric@viric.name>
> 
> ---
>   libavcodec/mmaldec.c | 48 ++++++++++++++++++++------------------------
>   1 file changed, 22 insertions(+), 26 deletions(-)
> 
> diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
> index 4dfaacbb41..097b990f92 100644
> --- a/libavcodec/mmaldec.c
> +++ b/libavcodec/mmaldec.c
> @@ -119,10 +119,11 @@ static void ffmmal_release_frame(void *opaque, uint8_t *data)
>   
>   // Setup frame with a new reference to buffer. The buffer must have been
>   // allocated from the given pool.
> -static int ffmmal_set_ref(AVFrame *frame, FFPoolRef *pool,
> -                          MMAL_BUFFER_HEADER_T *buffer)
> +static int ffmmal_set_ref(AVCodecContext *avctx, AVFrame *frame,
> +        FFPoolRef *pool, MMAL_BUFFER_HEADER_T *buffer)
>   {
>       FFBufferRef *ref = av_mallocz(sizeof(*ref));
> +

Random whitespace change?

>       if (!ref)
>           return AVERROR(ENOMEM);
>   
> @@ -140,8 +141,19 @@ static int ffmmal_set_ref(AVFrame *frame, FFPoolRef *pool,
>       atomic_fetch_add_explicit(&ref->pool->refcount, 1, memory_order_relaxed);
>       mmal_buffer_header_acquire(buffer);
>   
> -    frame->format = AV_PIX_FMT_MMAL;
> -    frame->data[3] = (uint8_t *)ref->buffer;
> +    frame->format = avctx->pix_fmt;
> +
> +    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P) {
> +        int w = FFALIGN(avctx->width, 32);
> +        int h = FFALIGN(avctx->height, 16);
> +
> +        av_image_fill_arrays(frame->data, frame->linesize,
> +                buffer->data + buffer->type->video.offset[0],
> +                avctx->pix_fmt, w, h, 1);
> +    } else {
> +        frame->data[3] = (uint8_t *)ref->buffer;
> +    }
> +
>       return 0;
>   }
>   
> @@ -633,30 +645,14 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,

The function name is very misleading after this change.

>       frame->interlaced_frame = ctx->interlaced_frame;
>       frame->top_field_first = ctx->top_field_first;
>   
> -    if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
> -        if (!ctx->pool_out)
> -            return AVERROR_UNKNOWN; // format change code failed with OOM previously
> -
> -        if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
> -            goto done;
> -
> -        if ((ret = ffmmal_set_ref(frame, ctx->pool_out, buffer)) < 0)
> -            goto done;
> -    } else {
> -        int w = FFALIGN(avctx->width, 32);
> -        int h = FFALIGN(avctx->height, 16);
> -        uint8_t *src[4];
> -        int linesize[4];
> +    if (!ctx->pool_out)
> +        return AVERROR_UNKNOWN; // format change code failed with OOM previously
>   
> -        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
> -            goto done;
> +    if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
> +        goto done;
>   
> -        av_image_fill_arrays(src, linesize,
> -                             buffer->data + buffer->type->video.offset[0],
> -                             avctx->pix_fmt, w, h, 1);
> -        av_image_copy(frame->data, frame->linesize, src, linesize,
> -                      avctx->pix_fmt, avctx->width, avctx->height);
> -    }
> +    if ((ret = ffmmal_set_ref(avctx, frame, ctx->pool_out, buffer)) < 0)
> +        goto done;
>   
>       frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts;
>   #if FF_API_PKT_PTS
> 

What happens if the user holds on to multiple of the output frames, perhaps because they are being queued into some other operation?  The number of buffers appears fixed, so what happens if they run out?

- Mark
Lluís Batlle i Rossell Feb. 11, 2021, 10:49 p.m. UTC | #2
On Thu, Feb 11, 2021 at 10:34:11PM +0000, Mark Thompson wrote:
> On 11/02/2021 21:48, Lluís Batlle i Rossel wrote:
> > From: Lluís Batlle i Rossell <viric@viric.name>
> > 
> > +
> 
> Random whitespace change?

likely. I can fix that.

> 
> >       if (!ref)
> >           return AVERROR(ENOMEM);
> > @@ -140,8 +141,19 @@ static int ffmmal_set_ref(AVFrame *frame, FFPoolRef *pool,
> >       atomic_fetch_add_explicit(&ref->pool->refcount, 1, memory_order_relaxed);
> >       mmal_buffer_header_acquire(buffer);
> > -    frame->format = AV_PIX_FMT_MMAL;
> > -    frame->data[3] = (uint8_t *)ref->buffer;
> > +    frame->format = avctx->pix_fmt;
> > +
> > +    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P) {
> > +        int w = FFALIGN(avctx->width, 32);
> > +        int h = FFALIGN(avctx->height, 16);
> > +
> > +        av_image_fill_arrays(frame->data, frame->linesize,
> > +                buffer->data + buffer->type->video.offset[0],
> > +                avctx->pix_fmt, w, h, 1);
> > +    } else {
> > +        frame->data[3] = (uint8_t *)ref->buffer;
> > +    }
> > +
> >       return 0;
> >   }
> > @@ -633,30 +645,14 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
> 
> The function name is very misleading after this change.

Definitely. I can fix that.

> > +    if ((ret = ffmmal_set_ref(avctx, frame, ctx->pool_out, buffer)) < 0)
> > +        goto done;
> >       frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts;
> >   #if FF_API_PKT_PTS
> > 
> 
> What happens if the user holds on to multiple of the output frames, perhaps because they are being queued into some other operation?  The number of buffers appears fixed, so what happens if they run out?

I don't know. I don't know how to trigger that situation. That would
affect also the "mmal" pixfmt, already present.
diff mbox series

Patch

diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
index 4dfaacbb41..097b990f92 100644
--- a/libavcodec/mmaldec.c
+++ b/libavcodec/mmaldec.c
@@ -119,10 +119,11 @@  static void ffmmal_release_frame(void *opaque, uint8_t *data)
 
 // Setup frame with a new reference to buffer. The buffer must have been
 // allocated from the given pool.
-static int ffmmal_set_ref(AVFrame *frame, FFPoolRef *pool,
-                          MMAL_BUFFER_HEADER_T *buffer)
+static int ffmmal_set_ref(AVCodecContext *avctx, AVFrame *frame,
+        FFPoolRef *pool, MMAL_BUFFER_HEADER_T *buffer)
 {
     FFBufferRef *ref = av_mallocz(sizeof(*ref));
+
     if (!ref)
         return AVERROR(ENOMEM);
 
@@ -140,8 +141,19 @@  static int ffmmal_set_ref(AVFrame *frame, FFPoolRef *pool,
     atomic_fetch_add_explicit(&ref->pool->refcount, 1, memory_order_relaxed);
     mmal_buffer_header_acquire(buffer);
 
-    frame->format = AV_PIX_FMT_MMAL;
-    frame->data[3] = (uint8_t *)ref->buffer;
+    frame->format = avctx->pix_fmt;
+
+    if (avctx->pix_fmt == AV_PIX_FMT_YUV420P) {
+        int w = FFALIGN(avctx->width, 32);
+        int h = FFALIGN(avctx->height, 16);
+
+        av_image_fill_arrays(frame->data, frame->linesize,
+                buffer->data + buffer->type->video.offset[0],
+                avctx->pix_fmt, w, h, 1);
+    } else {
+        frame->data[3] = (uint8_t *)ref->buffer;
+    }
+
     return 0;
 }
 
@@ -633,30 +645,14 @@  static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
     frame->interlaced_frame = ctx->interlaced_frame;
     frame->top_field_first = ctx->top_field_first;
 
-    if (avctx->pix_fmt == AV_PIX_FMT_MMAL) {
-        if (!ctx->pool_out)
-            return AVERROR_UNKNOWN; // format change code failed with OOM previously
-
-        if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
-            goto done;
-
-        if ((ret = ffmmal_set_ref(frame, ctx->pool_out, buffer)) < 0)
-            goto done;
-    } else {
-        int w = FFALIGN(avctx->width, 32);
-        int h = FFALIGN(avctx->height, 16);
-        uint8_t *src[4];
-        int linesize[4];
+    if (!ctx->pool_out)
+        return AVERROR_UNKNOWN; // format change code failed with OOM previously
 
-        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
-            goto done;
+    if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
+        goto done;
 
-        av_image_fill_arrays(src, linesize,
-                             buffer->data + buffer->type->video.offset[0],
-                             avctx->pix_fmt, w, h, 1);
-        av_image_copy(frame->data, frame->linesize, src, linesize,
-                      avctx->pix_fmt, avctx->width, avctx->height);
-    }
+    if ((ret = ffmmal_set_ref(avctx, frame, ctx->pool_out, buffer)) < 0)
+        goto done;
 
     frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts;
 #if FF_API_PKT_PTS