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

["Xiang, Haihao" <haihao.xiang-at-intel.com@ffmpeg.org>]

On Sun, 2022-03-13 at 20:45 +0000, Mark Thompson wrote:
> On 11/03/2022 09:00, Fei Wang wrote:
> > From: Linjie Fu <linjie.fu@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@intel.com>
> > Signed-off-by: Fei Wang <fei.w.wang@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?  

According to 
https://github.com/intel/media-driver/blob/master/media_driver/agnostic/common/codec/hal/codechal_vdenc_hevc.cpp#L3072-L3087
, L0 and L1 should be the same for vdenc hevc on some platforms, so user can't
use past and future reference together, which is why you experienced the failure
after applying version 2 

Thanks
Haihao


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