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

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

[David Rosca]

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(-)

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;
         }
     }
 
-- 
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: Don't try to merge fields in DPB for non-field pics

[Mark Thompson]

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;
>          }
>      }
>  
_______________________________________________
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: Don't try to merge fields in DPB for non-field pics

[David Rosca]

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