[FFmpeg-devel] [PATCH] avcodec/h264, videotoolbox: do not return invalid frames on failure

[wm4]

On Wed, 18 Nov 2015 15:09:48 +0100
Michael Niedermayer <michael at niedermayer.cc> wrote:

> On Tue, Nov 17, 2015 at 03:19:29PM +0100, wm4 wrote:
> > If videotoolbox_common_end_frame failed, then the AVFrame was returned
> > to the API user with the dummy buffer (in AVFrame.buf[0]) still set, and
> > the decode call indicating success.
> > 
> > These "half-set" AVFrames with dummy buffer are a videotoolbox specific
> > hack, because the decoder requires an allocated AVFrame for its internal
> > logic. Videotoolbox on the other hand allocates its frame itself
> > internally, and outputs it only on end_frame. At this point, the dummy
> > buffer is replaced with the real frame (unless decoding fails).
> > ---
> > Better solutions welcome. For now, this fixes the API returning garbage
> > which can crash or confuse API users,
> > ---
> >  libavcodec/h264_refs.c    | 3 ++-
> >  libavcodec/videotoolbox.c | 2 ++
> >  2 files changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavcodec/h264_refs.c b/libavcodec/h264_refs.c
> > index 619f2ed..9d0641a 100644
> > --- a/libavcodec/h264_refs.c
> > +++ b/libavcodec/h264_refs.c
> > @@ -515,7 +515,8 @@ 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);
> > -        ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);
> > +        if (h->short_ref[0]->f->buf[0])
> > +            ff_h264_ref_picture(h, &h->last_pic_for_ec, h->short_ref[0]);
> >      }  
> 
> no objections from me, but ive no means to test videotoolbox nor am
> i a videotoolbox developer or maintainer

The real question is whether it's really allowed to turn the AVFrame
into a frame with no data at this point. It seemed to be fine, but
crashed uninit at the point above (can't ref an unrefed picture; thus
the additional check to prevent the assert firing).