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