| Message ID | 20171215070550.34806-1-wbsecg1@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 15 Dec 2017 15:05:50 +0800 wbsecg1@gmail.com wrote: > From: wang-bin <wbsecg1@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.
2017-12-16 2:50 GMT+08:00 wm4 <nfxjfg@googlemail.com>: > On Fri, 15 Dec 2017 15:05:50 +0800 > wbsecg1@gmail.com wrote: > >> From: wang-bin <wbsecg1@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.
On Sat, 16 Dec 2017 13:48:05 +0800 Wang Bin <wbsecg1@gmail.com> wrote: > 2017-12-16 2:50 GMT+08:00 wm4 <nfxjfg@googlemail.com>: > > On Fri, 15 Dec 2017 15:05:50 +0800 > > wbsecg1@gmail.com wrote: > > > >> From: wang-bin <wbsecg1@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. Why would this need a new option?
2017-12-16 19:47 GMT+08:00 wm4 <nfxjfg@googlemail.com>: > On Sat, 16 Dec 2017 13:48:05 +0800 > Wang Bin <wbsecg1@gmail.com> wrote: > >> 2017-12-16 2:50 GMT+08:00 wm4 <nfxjfg@googlemail.com>: >> > On Fri, 15 Dec 2017 15:05:50 +0800 >> > wbsecg1@gmail.com wrote: >> > >> >> From: wang-bin <wbsecg1@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.
On Sat, 16 Dec 2017 20:26:56 +0800 Wang Bin <wbsecg1@gmail.com> wrote: > 2017-12-16 19:47 GMT+08:00 wm4 <nfxjfg@googlemail.com>: > > On Sat, 16 Dec 2017 13:48:05 +0800 > > Wang Bin <wbsecg1@gmail.com> wrote: > > > >> 2017-12-16 2:50 GMT+08:00 wm4 <nfxjfg@googlemail.com>: > >> > On Fri, 15 Dec 2017 15:05:50 +0800 > >> > wbsecg1@gmail.com wrote: > >> > > >> >> From: wang-bin <wbsecg1@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. Fine then I guess. It could still be surprising to the API user that holding a frame will keep a large chunk of memory alive (the whole pool), but I guess that's unavoidable with mmal's design. The API user could still copy the frame manually if it becomes a problem.
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} };
From: wang-bin <wbsecg1@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(-)