diff mbox series

[FFmpeg-devel] lavc/vaapi_h264: Don't try to merge fields in DPB for non-field pics

Message ID 20240505163656.765900-3-nowrep@gmail.com
State New
Headers show
Series [FFmpeg-devel] lavc/vaapi_h264: Don't try to merge fields in DPB for non-field pics | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

David Rosca May 5, 2024, 4:36 p.m. UTC 
This path can be hit when there are missing references while decoding
progressive stream and would completely break the DPB contents.
---
 libavcodec/vaapi_h264.c | 30 ++++++++++++++++--------------
 1 file changed, 16 insertions(+), 14 deletions(-)

Comments

Mark Thompson May 6, 2024, 7:55 p.m. UTC | #1 
On 05/05/2024 17:36, David Rosca wrote:
> This path can be hit when there are missing references while decoding
> progressive stream and would completely break the DPB contents.
> ---
>  libavcodec/vaapi_h264.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)

Can you share a stream which does this in a progressive case?

My understanding of this (though I suspect I am missing something) is that we are filling from the short/long DPB lists which should not contain duplicates (not RefPicListX, which can), so I'm not seeing how this case could be triggered.

If you do have two DPB entries which are different frames but refer to the same surface then that seems like a bug elsewhere.

Thanks,

- Mark


> diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
> index b47531ce1c..ca0076f57a 100644
> --- a/libavcodec/vaapi_h264.c
> +++ b/libavcodec/vaapi_h264.c
> @@ -98,22 +98,24 @@ static int dpb_add(DPB *dpb, const H264Picture *pic)
>      if (dpb->size >= dpb->max_size)
>          return -1;
>  
> -    for (i = 0; i < dpb->size; i++) {
> -        VAPictureH264 * const va_pic = &dpb->va_pics[i];
> -        if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
> -            VAPictureH264 temp_va_pic;
> -            fill_vaapi_pic(&temp_va_pic, pic, 0);
> -
> -            if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
> -                va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> -                /* Merge second field */
> -                if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
> -                    va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
> -                } else {
> -                    va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
> +    if (pic->field_picture) {
> +        for (i = 0; i < dpb->size; i++) {
> +            VAPictureH264 * const va_pic = &dpb->va_pics[i];
> +            if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
> +                VAPictureH264 temp_va_pic;
> +                fill_vaapi_pic(&temp_va_pic, pic, 0);
> +
> +                if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
> +                    va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> +                    /* Merge second field */
> +                    if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
> +                        va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
> +                    } else {
> +                        va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
> +                    }
>                  }
> +                return 0;
>              }
> -            return 0;
>          }
>      }
>
David Rosca May 7, 2024, 6:02 a.m. UTC | #2 
On Mon, May 6, 2024 at 9:55 PM Mark Thompson <sw@jkqxz.net> wrote:
>
> On 05/05/2024 17:36, David Rosca wrote:
> > This path can be hit when there are missing references while decoding
> > progressive stream and would completely break the DPB contents.
> > ---
> >  libavcodec/vaapi_h264.c | 30 ++++++++++++++++--------------
> >  1 file changed, 16 insertions(+), 14 deletions(-)
>
> Can you share a stream which does this in a progressive case?

https://github.com/nowrep/chiaki4deck/raw/samples/samples/missingframes.h264

>
> My understanding of this (though I suspect I am missing something) is that we are filling from the short/long DPB lists which should not contain duplicates (not RefPicListX, which can), so I'm not seeing how this case could be triggered.
>
> If you do have two DPB entries which are different frames but refer to the same surface then that seems like a bug elsewhere.

It comes from the error concealment in h264dec which copies previous
ref for missing refs. It can actually also happen for interlaced
streams. I've sent a new patch that should fix it for both progressive
and interlaced.

https://github.com/FFmpeg/FFmpeg/blob/b1037d4ebe7b7f9548ce1ed24a2929aedbe9a27a/libavcodec/h264_slice.c#L1520-L1559

Thanks,
David

>
> Thanks,
>
> - Mark
>
>
> > diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
> > index b47531ce1c..ca0076f57a 100644
> > --- a/libavcodec/vaapi_h264.c
> > +++ b/libavcodec/vaapi_h264.c
> > @@ -98,22 +98,24 @@ static int dpb_add(DPB *dpb, const H264Picture *pic)
> >      if (dpb->size >= dpb->max_size)
> >          return -1;
> >
> > -    for (i = 0; i < dpb->size; i++) {
> > -        VAPictureH264 * const va_pic = &dpb->va_pics[i];
> > -        if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
> > -            VAPictureH264 temp_va_pic;
> > -            fill_vaapi_pic(&temp_va_pic, pic, 0);
> > -
> > -            if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
> > -                va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> > -                /* Merge second field */
> > -                if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
> > -                    va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
> > -                } else {
> > -                    va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
> > +    if (pic->field_picture) {
> > +        for (i = 0; i < dpb->size; i++) {
> > +            VAPictureH264 * const va_pic = &dpb->va_pics[i];
> > +            if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
> > +                VAPictureH264 temp_va_pic;
> > +                fill_vaapi_pic(&temp_va_pic, pic, 0);
> > +
> > +                if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
> > +                    va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
> > +                    /* Merge second field */
> > +                    if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
> > +                        va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
> > +                    } else {
> > +                        va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
> > +                    }
> >                  }
> > +                return 0;
> >              }
> > -            return 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".
diff mbox series

Patch

diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c
index b47531ce1c..ca0076f57a 100644
--- a/libavcodec/vaapi_h264.c
+++ b/libavcodec/vaapi_h264.c
@@ -98,22 +98,24 @@  static int dpb_add(DPB *dpb, const H264Picture *pic)
     if (dpb->size >= dpb->max_size)
         return -1;
 
-    for (i = 0; i < dpb->size; i++) {
-        VAPictureH264 * const va_pic = &dpb->va_pics[i];
-        if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
-            VAPictureH264 temp_va_pic;
-            fill_vaapi_pic(&temp_va_pic, pic, 0);
-
-            if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
-                va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
-                /* Merge second field */
-                if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
-                    va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
-                } else {
-                    va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
+    if (pic->field_picture) {
+        for (i = 0; i < dpb->size; i++) {
+            VAPictureH264 * const va_pic = &dpb->va_pics[i];
+            if (va_pic->picture_id == ff_vaapi_get_surface_id(pic->f)) {
+                VAPictureH264 temp_va_pic;
+                fill_vaapi_pic(&temp_va_pic, pic, 0);
+
+                if ((temp_va_pic.flags ^ va_pic->flags) & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD)) {
+                    va_pic->flags |= temp_va_pic.flags & (VA_PICTURE_H264_TOP_FIELD | VA_PICTURE_H264_BOTTOM_FIELD);
+                    /* Merge second field */
+                    if (temp_va_pic.flags & VA_PICTURE_H264_TOP_FIELD) {
+                        va_pic->TopFieldOrderCnt    = temp_va_pic.TopFieldOrderCnt;
+                    } else {
+                        va_pic->BottomFieldOrderCnt = temp_va_pic.BottomFieldOrderCnt;
+                    }
                 }
+                return 0;
             }
-            return 0;
         }
     }