| Message ID | 20210309023600.16661-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/h264_slice: don't copy frame data during error concealment | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On Tue, Mar 9, 2021 at 3:39 AM James Almer <jamrial@gmail.com> wrote: > > In addition to the fact that av_image_copy() cannot handle hardware pixel > formats, h->short_ref[0]->f may not even be writable at this point. > > Based on a patch by Hendrik Leppkes. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is an alternative to "avcodec/h264_slice: properly handle missing > reference frames with hwaccel", given that I noticed that the target frame is > not writable for example when running fate-h264-missing-frame. > > To keep the current behavior of copying the frame data instead of making a > reference, I also tried to do ff_thread_release_buffer() -> > ff_thread_get_buffer() -> av_frame_copy(), which worked with software decoding, > but when using the d3d11va hwaccel the av_frame_copy() call would fail. > > There is a warning above this code that makes it sound like making references > is not ideal, but considering h->short_ref[0] is not writable here it feels > like it could be an outdated comment that someone forgot to remove. > Looks more thorough then my original change. I was worried about the comment also, but I think it may have been from before the days of ref-counting, frame threading, and all that (the comment appeared with the original code in e2983d6eac7b0bb563886c6f97c4ce0385b2018d, 2010) Perhaps the comment should be removed if we are going to reference now anyway? - Hendrik
We will test vaapi for this patch. But I am more curious about the software decoder behaviors. This approach just ref the yuv. Not the intermedia data(like mv, macroblock type) Will it have problem if missed frame selected as colPic (Figure 8-2 in spec). > -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Hendrik Leppkes > Sent: Tuesday, March 9, 2021 3:44 PM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] avcodec/h264_slice: don't copy frame > data during error concealment > > On Tue, Mar 9, 2021 at 3:39 AM James Almer <jamrial@gmail.com> wrote: > > > > In addition to the fact that av_image_copy() cannot handle hardware > > pixel formats, h->short_ref[0]->f may not even be writable at this point. > > > > Based on a patch by Hendrik Leppkes. > > > > Signed-off-by: James Almer <jamrial@gmail.com> > > --- > > This is an alternative to "avcodec/h264_slice: properly handle missing > > reference frames with hwaccel", given that I noticed that the target > > frame is not writable for example when running fate-h264-missing-frame. > > > > To keep the current behavior of copying the frame data instead of > > making a reference, I also tried to do ff_thread_release_buffer() -> > > ff_thread_get_buffer() -> av_frame_copy(), which worked with software > > decoding, but when using the d3d11va hwaccel the av_frame_copy() call > would fail. > > > > There is a warning above this code that makes it sound like making > > references is not ideal, but considering h->short_ref[0] is not > > writable here it feels like it could be an outdated comment that someone > forgot to remove. > > > > Looks more thorough then my original change. I was worried about the > comment also, but I think it may have been from before the days of ref- > counting, frame threading, and all that (the comment appeared with the > original code in e2983d6eac7b0bb563886c6f97c4ce0385b2018d, 2010) > > Perhaps the comment should be removed if we are going to reference now > anyway? > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email ffmpeg-devel-request@ffmpeg.org > with subject "unsubscribe".
On Mon, Mar 08, 2021 at 11:36:00PM -0300, James Almer wrote: > In addition to the fact that av_image_copy() cannot handle hardware pixel > formats, h->short_ref[0]->f may not even be writable at this point. > > Based on a patch by Hendrik Leppkes. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is an alternative to "avcodec/h264_slice: properly handle missing > reference frames with hwaccel", given that I noticed that the target frame is > not writable for example when running fate-h264-missing-frame. > > To keep the current behavior of copying the frame data instead of making a > reference, I also tried to do ff_thread_release_buffer() -> > ff_thread_get_buffer() -> av_frame_copy(), which worked with software decoding, > but when using the d3d11va hwaccel the av_frame_copy() call would fail. > > There is a warning above this code that makes it sound like making references > is not ideal, but considering h->short_ref[0] is not writable here it feels > like it could be an outdated comment that someone forgot to remove. > > libavcodec/h264_slice.c | 12 +++++------- > 1 file changed, 5 insertions(+), 7 deletions(-) This seems to produce infinite loops with some fuzzed samples ill mail you one privatly thx [...]
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xu, > Guangxin > Sent: Tuesday, March 9, 2021 5:13 PM > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH] avcodec/h264_slice: don't copy frame > data during error concealment > > We will test vaapi for this patch. > > But I am more curious about the software decoder behaviors. > This approach just ref the yuv. Not the intermedia data(like mv, macroblock > type) Will it have problem if missed frame selected as colPic (Figure 8-2 in > spec). > It's no regression for from ffmpeg vaapi. And it will fix following frame gap clips for vaapi MR3_TANDBERG_B MR4_TANDBERG_C MR5_TANDBERG_C
diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c index fa7a639053..a2f4ffa6d6 100644 --- a/libavcodec/h264_slice.c +++ b/libavcodec/h264_slice.c @@ -1599,13 +1599,11 @@ static int h264_field_start(H264Context *h, const H264SliceContext *sl, ff_thread_await_progress(&prev->tf, INT_MAX, 0); if (prev->field_picture) ff_thread_await_progress(&prev->tf, INT_MAX, 1); - av_image_copy(h->short_ref[0]->f->data, - h->short_ref[0]->f->linesize, - (const uint8_t **)prev->f->data, - prev->f->linesize, - prev->f->format, - prev->f->width, - prev->f->height); + ff_thread_release_buffer(h->avctx, &h->short_ref[0]->tf); + h->short_ref[0]->tf.f = h->short_ref[0]->f; + ret = ff_thread_ref_frame(&h->short_ref[0]->tf, &prev->tf); + if (ret < 0) + return ret; h->short_ref[0]->poc = prev->poc + 2U; } else if (!h->frame_recovered && !h->avctx->hwaccel) ff_color_frame(h->short_ref[0]->f, c);
In addition to the fact that av_image_copy() cannot handle hardware pixel formats, h->short_ref[0]->f may not even be writable at this point. Based on a patch by Hendrik Leppkes. Signed-off-by: James Almer <jamrial@gmail.com> --- This is an alternative to "avcodec/h264_slice: properly handle missing reference frames with hwaccel", given that I noticed that the target frame is not writable for example when running fate-h264-missing-frame. To keep the current behavior of copying the frame data instead of making a reference, I also tried to do ff_thread_release_buffer() -> ff_thread_get_buffer() -> av_frame_copy(), which worked with software decoding, but when using the d3d11va hwaccel the av_frame_copy() call would fail. There is a warning above this code that makes it sound like making references is not ideal, but considering h->short_ref[0] is not writable here it feels like it could be an outdated comment that someone forgot to remove. libavcodec/h264_slice.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)