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

[wm4]

On Thu, 2 Mar 2017 09:58:28 -0800
Aman Gupta <ffmpeg at tmm1.net> wrote:

> On Thu, Mar 2, 2017 at 1:34 AM, wm4 <nfxjfg at googlemail.com> wrote:
> 
> > On Tue, 21 Feb 2017 13:40:08 -0800
> > Aman Gupta <ffmpeg at tmm1.net> wrote:
> >  
> > > On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje at gmail.com>
> > > wrote:
> > >  
> > > > Hi,
> > > >
> > > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg at tmm1.net> wrote:
> > > >  
> > > >> 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;
> > > >> +
> > > >> +    ret = av_frame_ref(dst, src);
> > > >>      if (ret < 0)
> > > >>          return ret;  
> > > >
> > > >
> > > > This is a total hack :) Is there a way to hide this into VT-specific  
> > code  
> > > > outside h264*.[ch]?
> > > >  
> > >
> > > The way the VT hwaccel works is a total hack, as noted in my commit  
> > message.  
> > >
> > > AFAICT, given how the hwaccel APIs work, there's no way to do this  
> > outside  
> > > the h264 decoder. But I'm happy to try fixing this a different way if
> > > anyone has a suggestion.  
> >
> > So, should we push this?
> >  
> 
> I'm struggling to find a less hacky way to implement this, so my vote would
> be to move forward. Changing the error to AVERROR_EXTERNAL makes sense too.

I'll apply the patch in 24h then, with the error code changed.

It's a regrettable hack, but probably better than violating the API.
With the patch applied, it'll behave like normal hwaccels do, and I
suppose the internals can always be changed later. So I think there's a
good case for applying this.