[FFmpeg-devel] lavc/videotoolbox: fix threaded decoding

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.
---
 libavcodec/h264dec.c      |  3 ---
 libavcodec/videotoolbox.c | 68 +++++++++++++++++++++++++++++++++++------------
 libavcodec/vt_internal.h  |  1 -
 3 files changed, 51 insertions(+), 21 deletions(-)

On Fri, Feb 2, 2018 at 6:50 PM, Rodger Combs <rodger.combs@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@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>