| Message ID | 20240507060059.34246-2-nowrep@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] lavc/vaapi_h264: Fix merging fields in DPB with missing references | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
On 07/05/2024 07:00, David Rosca wrote: > If there are missing references, h264 decode does error concealment > by copying previous refs which means there will be duplicated surfaces > and this code would try to merge them instead of correctly appending > to DPB. Make sure the fields were actually merged before early return. > --- > libavcodec/vaapi_h264.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c > index b47531ce1c..c62a320e97 100644 > --- a/libavcodec/vaapi_h264.c > +++ b/libavcodec/vaapi_h264.c > @@ -112,8 +112,9 @@ static int dpb_add(DPB *dpb, const H264Picture *pic) > } else { > va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt; > } > + return 0; > } > - return 0; > + break; > } > } > I agree that the old code did nasty things, but can you explain a bit more about the reasoning for what you've got now? I'm thinking that given duplication a top field could appear twice, so you'd want to merge twice if you have the matching bottom field. Thanks, - Mark
On Tue, May 7, 2024 at 10:22 PM Mark Thompson <sw@jkqxz.net> wrote: > > On 07/05/2024 07:00, David Rosca wrote: > > If there are missing references, h264 decode does error concealment > > by copying previous refs which means there will be duplicated surfaces > > and this code would try to merge them instead of correctly appending > > to DPB. Make sure the fields were actually merged before early return. > > --- > > libavcodec/vaapi_h264.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c > > index b47531ce1c..c62a320e97 100644 > > --- a/libavcodec/vaapi_h264.c > > +++ b/libavcodec/vaapi_h264.c > > @@ -112,8 +112,9 @@ static int dpb_add(DPB *dpb, const H264Picture *pic) > > } else { > > va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt; > > } > > + return 0; > > } > > - return 0; > > + break; > > } > > } > > > > I agree that the old code did nasty things, but can you explain a bit more about the reasoning for what you've got now? > > I'm thinking that given duplication a top field could appear twice, so you'd want to merge twice if you have the matching bottom field. Right, that may be possible. Also I've looked at vdpau code and there it checks frame_num (in addition to surface) when searching for the other field, which seems better to avoid trying to merge when it should not. Posted v2 with this change + multiple merge. Thanks, David > > Thanks, > > - Mark > _______________________________________________ > 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".
diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c index b47531ce1c..c62a320e97 100644 --- a/libavcodec/vaapi_h264.c +++ b/libavcodec/vaapi_h264.c @@ -112,8 +112,9 @@ static int dpb_add(DPB *dpb, const H264Picture *pic) } else { va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt; } + return 0; } - return 0; + break; } }