[FFmpeg-devel] [PATCH] lavc/videotoolbox: fix threaded decoding
Aman Gupta
ffmpeg at tmm1.net
Tue Feb 6 01:59:48 EET 2018
On Fri, Feb 2, 2018 at 6:50 PM, Rodger Combs <rodger.combs at gmail.com> wrote:
> AVHWAccel.end_frame can run on a worker thread. The assumption of the
> frame threading code is that the worker thread will change the AVFrame
> image data, not the AVFrame fields. So the AVFrame fields are not synced
> back to the main thread. But this breaks videotoolbox due to its special
> requirements (everything else is fine). It actually wants to update
> AVFrame fields.
>
> The actual videotoolbox frame is now stored in the dummy AVBufferRef, so
> it mimics what happens in non-videotoolbox cases. (Changing the
> AVBufferRef contents is a bit like changing the image data.) The
> post_process callback copies that reference to the proper AVFrame field.
> Based on a patch by wm4.
>
Patch seems reasonable, although I did not try on my devices.
> ---
> libavcodec/h264dec.c | 3 ---
> libavcodec/videotoolbox.c | 68 ++++++++++++++++++++++++++++++
> +++++------------
> libavcodec/vt_internal.h | 1 -
> 3 files changed, 51 insertions(+), 21 deletions(-)
>
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 8c9c6d9f3b..7494c7a8f2 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -838,9 +838,6 @@ static int output_frame(H264Context *h, AVFrame *dst,
> H264Picture *srcp)
> AVFrame *src = srcp->f;
> int ret;
>
> - if (src->format == AV_PIX_FMT_VIDEOTOOLBOX && src->buf[0]->size == 1)
> - return AVERROR_INVALIDDATA;
> -
>
Without this we used to see segfaults before, since API users could receive
dummy frames with no data. I assume videotoolbox_postproc_frame somehow
raises this error now before the frame can make it to the consumer?
I explicitly returned INVALIDDATA in this case, because I observed that the
VT decoder would only fail to produce frames when it deemed the input
invalid.
> ret = av_frame_ref(dst, src);
> if (ret < 0)
> return ret;
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index afec1edf3f..f82c31c5df 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -45,8 +45,10 @@ enum { kCMVideoCodecType_HEVC = 'hvc1' };
>
> static void videotoolbox_buffer_release(void *opaque, uint8_t *data)
> {
> - CVPixelBufferRef cv_buffer = (CVImageBufferRef)data;
> + CVPixelBufferRef cv_buffer = *(CVPixelBufferRef *)data;
> CVPixelBufferRelease(cv_buffer);
> +
> + av_free(data);
> }
>
> static int videotoolbox_buffer_copy(VTContext *vtctx,
> @@ -69,19 +71,47 @@ static int videotoolbox_buffer_copy(VTContext *vtctx,
> return 0;
> }
>
> +static int videotoolbox_postproc_frame(void *avctx, AVFrame *frame)
> +{
> + CVPixelBufferRef ref = *(CVPixelBufferRef *)frame->buf[0]->data;
> +
> + if (!ref) {
> + av_log(avctx, AV_LOG_ERROR, "No frame decoded?\n");
> + av_frame_unref(frame);
> + return AVERROR_EXTERNAL;
>
I would prefer to return AVERROR_INVALIDDATA here as outlined earlier,
mostly because in my application I assume AVERROR_EXTERNAL to mean the
decoder is badly broken and I should use a software decoder instead.
The av_log here could probably also be demoted to a warning. I've seen this
happen a lot at the beginning of some live streams.
> + }
> +
> + frame->data[3] = (uint8_t*)ref;
> +
> + return 0;
> +}
> +
> int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame)
> {
> + size_t size = sizeof(CVPixelBufferRef);
> + uint8_t *data = NULL;
> + AVBufferRef *buf = NULL;
> int ret = ff_attach_decode_data(frame);
> + FrameDecodeData *fdd;
> if (ret < 0)
> return ret;
>
> + data = av_mallocz(size);
> + if (!data)
> + return AVERROR(ENOMEM);
> + buf = av_buffer_create(data, size, videotoolbox_buffer_release, NULL,
> 0);
> + if (!buf) {
> + av_freep(&data);
> + return AVERROR(ENOMEM);
> + }
> + frame->buf[0] = buf;
> +
> + fdd = (FrameDecodeData*)frame->private_ref->data;
> + fdd->post_process = videotoolbox_postproc_frame;
> +
> frame->width = avctx->width;
> frame->height = avctx->height;
> frame->format = avctx->pix_fmt;
> - frame->buf[0] = av_buffer_alloc(1);
> -
> - if (!frame->buf[0])
> - return AVERROR(ENOMEM);
>
> return 0;
> }
> @@ -285,20 +315,24 @@ CFDataRef ff_videotoolbox_hvcc_extradata_create(AVCodecContext
> *avctx)
> return data;
> }
>
> -int ff_videotoolbox_buffer_create(VTContext *vtctx, AVFrame *frame)
> +static int videotoolbox_set_frame(AVCodecContext *avctx, AVFrame *frame)
> {
> - av_buffer_unref(&frame->buf[0]);
> -
> - frame->buf[0] = av_buffer_create((uint8_t*)vtctx->frame,
> - sizeof(vtctx->frame),
> - videotoolbox_buffer_release,
> - NULL,
> - AV_BUFFER_FLAG_READONLY);
> - if (!frame->buf[0]) {
> - return AVERROR(ENOMEM);
> + VTContext *vtctx = avctx->internal->hwaccel_priv_data;
> + if (!frame->buf[0] || frame->data[3]) {
> + av_log(avctx, AV_LOG_ERROR, "videotoolbox: invalid state\n");
>
Might be worth expanding the error message here so it's obvious which one
of the checks failed.
> + av_frame_unref(frame);
> + return AVERROR_EXTERNAL;
> + }
> +
> + CVPixelBufferRef *ref = (CVPixelBufferRef *)frame->buf[0]->data;
> +
> + if (*ref) {
> + av_log(avctx, AV_LOG_ERROR, "videotoolbox: frame already set?\n");
> + av_frame_unref(frame);
> + return AVERROR_EXTERNAL;
> }
>
> - frame->data[3] = (uint8_t*)vtctx->frame;
> + *ref = vtctx->frame;
> vtctx->frame = NULL;
>
> return 0;
> @@ -406,7 +440,7 @@ static int videotoolbox_buffer_create(AVCodecContext
> *avctx, AVFrame *frame)
> AVHWFramesContext *cached_frames;
> int ret;
>
> - ret = ff_videotoolbox_buffer_create(vtctx, frame);
> + ret = videotoolbox_set_frame(avctx, frame);
> if (ret < 0)
> return ret;
>
> diff --git a/libavcodec/vt_internal.h b/libavcodec/vt_internal.h
> index 929fe423fc..fb64735b8c 100644
> --- a/libavcodec/vt_internal.h
> +++ b/libavcodec/vt_internal.h
> @@ -46,7 +46,6 @@ typedef struct VTContext {
>
> int ff_videotoolbox_alloc_frame(AVCodecContext *avctx, AVFrame *frame);
> int ff_videotoolbox_uninit(AVCodecContext *avctx);
> -int ff_videotoolbox_buffer_create(VTContext *vtctx, AVFrame *frame);
> int ff_videotoolbox_h264_start_frame(AVCodecContext *avctx,
> const uint8_t *buffer,
> uint32_t size);
> --
> 2.16.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
More information about the ffmpeg-devel
mailing list