[FFmpeg-devel] [PATCH 13/14] mmal: add option copy_frame to support retrieving sw frames w/o copy

[Wang Bin]

2017-12-16 19:47 GMT+08:00 wm4 <nfxjfg at googlemail.com>:
> On Sat, 16 Dec 2017 13:48:05 +0800
> Wang Bin <wbsecg1 at gmail.com> wrote:
>
>> 2017-12-16 2:50 GMT+08:00 wm4 <nfxjfg at googlemail.com>:
>> > On Fri, 15 Dec 2017 15:05:50 +0800
>> > wbsecg1 at gmail.com wrote:
>> >
>> >> From: wang-bin <wbsecg1 at gmail.com>
>> >>
>> >> mmal buffer->data is already in host memory. AFAIK decoders implemented in omx must
>> >> be configured to output frames to either memory or something directly used by renderer,
>> >> for example mediacodec surface, mmal buffer and omxil eglimage.
>> >> test result: big buck bunny 1080p fps increases from about 100 to 110 if copy_frame is
>> >> turned off
>> >> ---
>> >>  libavcodec/mmaldec.c | 31 +++++++++++++++++++++++--------
>> >>  1 file changed, 23 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/libavcodec/mmaldec.c b/libavcodec/mmaldec.c
>> >> index c1cfb09283..9cd6c6558f 100644
>> >> --- a/libavcodec/mmaldec.c
>> >> +++ b/libavcodec/mmaldec.c
>> >> @@ -69,6 +69,7 @@ typedef struct MMALDecodeContext {
>> >>      AVClass *av_class;
>> >>      int extra_buffers;
>> >>      int extra_decoder_buffers;
>> >> +    int copy_frame;
>> >>
>> >>      MMAL_COMPONENT_T *decoder;
>> >>      MMAL_QUEUE_T *queue_decoded_frames;
>> >> @@ -139,7 +140,6 @@ 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;
>> >>      return 0;
>> >>  }
>> >> @@ -650,20 +650,34 @@ static int ffmal_copy_frame(AVCodecContext *avctx,  AVFrame *frame,
>> >>
>> >>          if ((ret = ffmmal_set_ref(frame, ctx->pool_out, buffer)) < 0)
>> >>              goto done;
>> >> +        frame->format = AV_PIX_FMT_MMAL;
>> >>      } else {
>> >>          int w = FFALIGN(avctx->width, 32);
>> >>          int h = FFALIGN(avctx->height, 16);
>> >>          uint8_t *src[4];
>> >>          int linesize[4];
>> >>
>> >> -        if ((ret = ff_get_buffer(avctx, frame, 0)) < 0)
>> >> -            goto done;
>> >> +        if (ctx->copy_frame) {
>> >> +            if ((ret = ff_get_buffer(avctx, frame, 0)) < 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);
>> >> +            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);
>> >> +        } else {
>> >> +            if ((ret = ff_decode_frame_props(avctx, frame)) < 0)
>> >> +                goto done;
>> >> +            /* buffer->type->video.offset/pitch[i]; is always 0 */
>> >> +            av_image_fill_arrays(src, linesize,
>> >> +                                buffer->data + buffer->type->video.offset[0],
>> >> +                                avctx->pix_fmt, w, h, 1);
>> >> +            if ((ret = ffmmal_set_ref(frame, ctx->pool_out, buffer)) < 0)
>> >> +                goto done;
>> >> +            memcpy(frame->data, src, sizeof(src));
>> >> +            memcpy(frame->linesize, linesize, sizeof(linesize));
>> >> +        }
>> >>      }
>> >>
>> >>      frame->pts = buffer->pts == MMAL_TIME_UNKNOWN ? AV_NOPTS_VALUE : buffer->pts;
>> >> @@ -842,6 +856,7 @@ AVHWAccel ff_wmv3_mmal_hwaccel = {
>> >>  static const AVOption options[]={
>> >>      {"extra_buffers", "extra buffers", offsetof(MMALDecodeContext, extra_buffers), AV_OPT_TYPE_INT, {.i64 = 10}, 0, 256, 0},
>> >>      {"extra_decoder_buffers", "extra MMAL internal buffered frames", offsetof(MMALDecodeContext, extra_decoder_buffers), AV_OPT_TYPE_INT, {.i64 = 10}, 0, 256, 0},
>> >> +    {"copy_frame", "copy deocded data to avframe", offsetof(MMALDecodeContext, copy_frame), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 256, 0},
>> >>      {NULL}
>> >>  };
>> >>
>> >
>> > Didn't check too closely what exactly the patch does, but adding an
>> > option for it sounds very wrong. The user select in the get_format
>> > callback whether a GPU surface is output (MMAL pixfmt), or software.
>>
>> Avoid copying data from mmal buffer->data to avframe data. Instead,
>> just fill strides and address of each plane in avframe, and add a
>> reference to mmal buffer.
>
> Does it make sure to keep the mmal buffer pool alive then? Otherwise a
> decoded AVFrame would become invalid after closing and destroying the
> decoder.

Yes. ffmmal_set_ref() is called like hw pixfmt code and mmal buffer's
reference is increased. otherwise the displayed frame is weired
because the buffer may be invalid as you say. I tested it in mpv.

> Why would this need a new option?

For testing purposes. You can compare the performance with the option.