[FFmpeg-devel] [PATCH] lavc/vaapi_h264: Fix merging fields in DPB with missing references

[FFmpeg-devel] [PATCH] lavc/vaapi_h264: Fix merging fields in DPB with missing references

[David Rosca]

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;
         }
     }
 
-- 
2.45.0

_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_h264: Fix merging fields in DPB with missing references

[Mark Thompson]

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
_______________________________________________
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".

Re: [FFmpeg-devel] [PATCH] lavc/vaapi_h264: Fix merging fields in DPB with missing references

[David Rosca]

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".
_______________________________________________
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".