| Message ID | 20170221184837.46049-1-ffmpeg@tmm1.net |
|---|---|
| State | Accepted |
| Headers | show |
Hi, On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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]? Ronald
On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi, > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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. > > Ronald >
On Tue, 21 Feb 2017 10:48:37 -0800 Aman Gupta <ffmpeg@tmm1.net> wrote: > From: Aman Gupta <aman@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.)
On Tue, 21 Feb 2017 13:40:08 -0800 Aman Gupta <ffmpeg@tmm1.net> wrote: > On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje@gmail.com> > wrote: > > > Hi, > > > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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?
On Thu, Mar 2, 2017 at 1:34 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Tue, 21 Feb 2017 13:40:08 -0800 > Aman Gupta <ffmpeg@tmm1.net> wrote: > > > On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje@gmail.com> > > wrote: > > > > > Hi, > > > > > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Thu, 2 Mar 2017 09:58:28 -0800 Aman Gupta <ffmpeg@tmm1.net> wrote: > On Thu, Mar 2, 2017 at 1:34 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > On Tue, 21 Feb 2017 13:40:08 -0800 > > Aman Gupta <ffmpeg@tmm1.net> wrote: > > > > > On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje@gmail.com> > > > wrote: > > > > > > > Hi, > > > > > > > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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.
On Thu, 2 Mar 2017 09:58:28 -0800 Aman Gupta <ffmpeg@tmm1.net> wrote: > On Thu, Mar 2, 2017 at 1:34 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > On Tue, 21 Feb 2017 13:40:08 -0800 > > Aman Gupta <ffmpeg@tmm1.net> wrote: > > > > > On Tue, Feb 21, 2017 at 1:04 PM, Ronald S. Bultje <rsbultje@gmail.com> > > > wrote: > > > > > > > Hi, > > > > > > > > On Tue, Feb 21, 2017 at 1:48 PM, Aman Gupta <ffmpeg@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. Pushed, with error code changed and a minor version bump added.
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]); } 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; + + 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;
From: Aman Gupta <aman@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(-)