[FFmpeg-devel] [PATCH v4 1/4] lavc/vaapi_encode_h265: Add GPB frame support for hevc_vaapi

[Mark Thompson]

On 11/03/2022 09:00, Fei Wang wrote:
> From: Linjie Fu <linjie.fu at intel.com>
> 
> Use GPB frames to replace regular P/B frames if backend driver does not
> support it.
> 
> - GPB:
>      Generalized P and B picture. Regular P/B frames replaced by B
>      frames with previous-predict only, L0 == L1. Normal B frames
>      still have 2 different ref_lists and allow bi-prediction
> 
> Signed-off-by: Linjie Fu <linjie.fu at intel.com>
> Signed-off-by: Fei Wang <fei.w.wang at intel.com>
> ---
> update:
> 1. Add b to gpb.
> 2. Optimise debug message.
> 
>   libavcodec/vaapi_encode.c      | 74 +++++++++++++++++++++++++++++++---
>   libavcodec/vaapi_encode.h      |  2 +
>   libavcodec/vaapi_encode_h265.c | 24 ++++++++++-
>   3 files changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode.c b/libavcodec/vaapi_encode.c
> index 3bf379b1a0..bdba9726b2 100644
> --- a/libavcodec/vaapi_encode.c
> +++ b/libavcodec/vaapi_encode.c
> @@ -848,9 +848,13 @@ static void vaapi_encode_set_b_pictures(AVCodecContext *avctx,
>               pic->b_depth = current_depth;
>   
>               vaapi_encode_add_ref(avctx, pic, start, 1, 1, 0);
> -            vaapi_encode_add_ref(avctx, pic, end,   1, 1, 0);
>               vaapi_encode_add_ref(avctx, pic, prev,  0, 0, 1);
>   
> +            if (!ctx->b_to_gpb)
> +                vaapi_encode_add_ref(avctx, pic, end, 1, 1, 0);
> +            else
> +                vaapi_encode_add_ref(avctx, pic, end, 0, 1, 0);

This is doing something extremely dubious.  If b-to-gpb is set, then don't use the future reference?  That means these pictures will only have the past reference, and the coding efficiency will often be much worse.

E.g. if you have the default structure (max_b_frames = 2, max_b_depth = 1) then in a sequence of four pictures you get:

1 referring to something previous
4 referring to 1
2 referring to 1 and 4
3 referring to 1 and 4

and this change means you lose the 2 -> 4 and 3 -> 4 references.  Therefore, a change in the picture between 1 and 2 will end up coded three times in 2, 3 and 4 rather than just being coded in 4 and then referred to by the others.

> +
>               for (ref = end->refs[1]; ref; ref = ref->refs[1])
>                   vaapi_encode_add_ref(avctx, pic, ref, 0, 1, 0);
>           }
> @@ -871,8 +875,11 @@ static void vaapi_encode_set_b_pictures(AVCodecContext *avctx,
>   
>           vaapi_encode_add_ref(avctx, pic, pic,   0, 1, 0);
>           vaapi_encode_add_ref(avctx, pic, start, 1, 1, 0);
> -        vaapi_encode_add_ref(avctx, pic, end,   1, 1, 0);
>           vaapi_encode_add_ref(avctx, pic, prev,  0, 0, 1);
> +        if (!ctx->b_to_gpb)
> +            vaapi_encode_add_ref(avctx, pic, end, 1, 1, 0);
> +        else
> +            vaapi_encode_add_ref(avctx, pic, end, 0, 1, 0);
>   
>           for (ref = end->refs[1]; ref; ref = ref->refs[1])
>               vaapi_encode_add_ref(avctx, pic, ref, 0, 1, 0);
> @@ -1845,6 +1852,51 @@ static av_cold int vaapi_encode_init_gop_structure(AVCodecContext *avctx)
>           ref_l1 = attr.value >> 16 & 0xffff;
>       }
>   
> +    ctx->p_to_gpb = 0;
> +    ctx->b_to_gpb = 0;
> +
> +#if VA_CHECK_VERSION(1, 9, 0)
> +    if (!(ctx->codec->flags & FLAG_INTRA_ONLY ||
> +        avctx->gop_size <= 1)) {
> +        attr = (VAConfigAttrib) { VAConfigAttribPredictionDirection };
> +        vas = vaGetConfigAttributes(ctx->hwctx->display,
> +                                    ctx->va_profile,
> +                                    ctx->va_entrypoint,
> +                                    &attr, 1);
> +        if (vas != VA_STATUS_SUCCESS) {
> +            av_log(avctx, AV_LOG_WARNING, "Failed to query prediction direction "
> +                   "attribute: %d (%s).\n", vas, vaErrorStr(vas));
> +            return AVERROR_EXTERNAL;
> +        } else if (attr.value == VA_ATTRIB_NOT_SUPPORTED) {
> +            av_log(avctx, AV_LOG_VERBOSE, "Driver does not report any additional "
> +                   "prediction constraints.\n");
> +        } else {
> +            if (((ref_l0 > 0 || ref_l1 > 0) && !(attr.value & VA_PREDICTION_DIRECTION_PREVIOUS)) ||
> +                ((ref_l1 == 0) && (attr.value & (VA_PREDICTION_DIRECTION_FUTURE | VA_PREDICTION_DIRECTION_BI_NOT_EMPTY)))) {
> +                av_log(avctx, AV_LOG_ERROR, "Driver report incorrect prediction "
> +                       "direction attribute.\n");
> +                return AVERROR_EXTERNAL;
> +            }
> +
> +            if (!(attr.value & VA_PREDICTION_DIRECTION_FUTURE)) {
> +                if (ref_l0 > 0 && ref_l1 > 0) {
> +                    ctx->b_to_gpb = 1;
> +                    av_log(avctx, AV_LOG_VERBOSE, "Driver support previous prediction "
> +                           "only for B-frames.\n");
> +                }
> +            }

Please note that the PREDICTION_DIRECTION_FUTURE/PREVIOUS options don't mean anything for H.265.

The driver isn't told which direction the prediction is in, it's only told about some reference frames and how to refer to them.  Whether those frames are in the past or future is unspecified and irrelevant - a P frame can refer to a single future frame if it wants.

(I tried to argue this when it was added, but given that they are meaningless I didn't argue very hard.)

I suspect you are trying to use this as a test of something else.  Perhaps you could explain what the test you actually want is?

> +
> +            if (attr.value & VA_PREDICTION_DIRECTION_BI_NOT_EMPTY) {
> +                if (ref_l0 > 0 && ref_l1 > 0) {
> +                    ctx->p_to_gpb = 1;
> +                    av_log(avctx, AV_LOG_VERBOSE, "Driver does not support P-frames, "
> +                           "replacing them with previous prediction only B-frames.\n");
> +                }
> +            }
> +        }
> +    }
> +#endif
> ...

- Mark