[FFmpeg-devel] [PATCH v2] avcodec/h264, videotoolbox: fix crash after VT decoder fails

[wm4]

On Tue, 21 Feb 2017 10:48:37 -0800
Aman Gupta <ffmpeg at tmm1.net> wrote:

> From: Aman Gupta <aman at tmm1.net>
> 
> The way videotoolbox hooks in as a hwaccel is pretty hacky. The VT decode
> API is not invoked until end_frame(), so alloc_frame() returns a dummy
> frame with a 1-byte buffer. When end_frame() is eventually called, the
> dummy buffer is replaced with the actual decoded data from
> VTDecompressionSessionDecodeFrame().
> 
> When the VT decoder fails, the frame returned to the h264 decoder from
> alloc_frame() remains invalid and should not be used. Before
> 9747219958060d8c4f697df62e7f172c2a77e6c7, it was accidentally being
> returned all the way up to the API user. After that commit, the dummy
> frame was unref'd so the user received an error.
> 
> However, since that commit, VT hwaccel failures started causing random
> segfaults in the h264 decoder. This happened more often on iOS where the
> VT implementation is more likely to throw errors on bitstream anomolies.
> A recent report of this issue can be see in
> http://ffmpeg.org/pipermail/libav-user/2016-November/009831.html
> 
> The issue here is that the dummy frame is still referenced internally by the
> h264 decoder, as part of the reflist and cur_pic_ptr. Deallocating the
> frame causes assertions like this one to trip later on during decoding:
> 
>   Assertion h->cur_pic_ptr->f->buf[0] failed at src/libavcodec/h264_slice.c:1340
> 
> With this commit, we leave the dummy 1-byte frame intact, but avoid returning it
> to the user.
> 
> This reverts commit 9747219958060d8c4f697df62e7f172c2a77e6c7.
> ---
>  libavcodec/h264_refs.c    | 3 +--
>  libavcodec/h264dec.c      | 7 ++++++-
>  libavcodec/videotoolbox.c | 2 --
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
> index 97bf588b51..ad296753c3 100644
> --- a/libavcodec/h264_refs.c
> +++ b/libavcodec/h264_refs.c
> @@ -571,8 +571,7 @@ void ff_h264_remove_all_refs(H264Context *h)
>  
>      if (h->short_ref_count && !h->last_pic_for_ec.f->data[0]) {
>          ff_h264_unref_picture(h, &h->last_pic_for_ec);
> -        if (h->short_ref[0]->f->buf[0])
> -            ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);
> +        ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);

In case others don't realize, this just undoes an earlier VT-specific
hack, and should not affect software decoding in any way.

>      }
>  
>      for (i = 0; i < h->short_ref_count; i++) {
> diff --git a/libavcodec/h264dec.c b/libavcodec/h264dec.c
> index 41c0964392..a0ae632fed 100644
> --- a/libavcodec/h264dec.c
> +++ b/libavcodec/h264dec.c
> @@ -850,7 +850,12 @@ static int output_frame(H264Context *h, AVFrame *dst, H264Picture *srcp)
>      AVFrame *src = srcp->f;
>      const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(src->format);
>      int i;
> -    int ret = av_frame_ref(dst, src);
> +    int ret;
> +
> +    if (src->format == AV_PIX_FMT_VIDEOTOOLBOX && src->buf[0]->size == 1)
> +        return AVERROR_INVALIDDATA;

Kind of un-nice, but I guess we can live with it for now? And it's
probably much better than trying to make the h264 code deal with an
unset buffer (which was what I tried earlier, but which is imperfect).

I'd probably return AVERROR_EXTERNAL (or maybe we should introduce an
error code that signals that a hw decoder failed).

> +
> +    ret = av_frame_ref(dst, src);
>      if (ret < 0)
>          return ret;
>  
> diff --git a/libavcodec/videotoolbox.c b/libavcodec/videotoolbox.c
> index 1288aa5087..d931dbd5f9 100644
> --- a/libavcodec/videotoolbox.c
> +++ b/libavcodec/videotoolbox.c
> @@ -351,8 +351,6 @@ static int videotoolbox_common_end_frame(AVCodecContext *avctx, AVFrame *frame)
>      AVVideotoolboxContext *videotoolbox = avctx->hwaccel_context;
>      VTContext *vtctx = avctx->internal->hwaccel_priv_data;
>  
> -    av_buffer_unref(&frame->buf[0]);
> -
>      if (!videotoolbox->session || !vtctx->bitstream)
>          return AVERROR_INVALIDDATA;
>  

In theory, ff_videotoolbox_buffer_create() could still leave buf[0]
unset if av_buffer_create() fails. (Which is rare, but theoretically
possible.)