| Message ID | 20220311090046.262777-1-fei.w.wang@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,v4,1/4] lavc/vaapi_encode_h265: Add GPB frame support for hevc_vaapi | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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? 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
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".
> -----Original Message----- > From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang, > Haihao > Sent: Monday, March 14, 2022 10:15 AM > To: ffmpeg-devel@ffmpeg.org > Subject: Re: [FFmpeg-devel] [PATCH v4 1/4] lavc/vaapi_encode_h265: Add GPB > frame support for hevc_vaapi > > 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. If driver doesn't support B frames with two different reference lists, use gpb instead of regular B is a best way. In V3, I turned B frames to P, but this will break gop structure. If user set I/P/B frames, while the output only contains I/P frames will be much confuse. > > > > > + > > > 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? VA_PREDICTION_DIRECTION_PREVIOUS/ VA_PREDICTION_DIRECTION_FUTURE/ VA_PREDICTION_DIRECTION_BI_NOT_EMPTY are used to indicate if driver has the limitation on how to set regular P and B frame's reference list when the queried max reference list ref_l0 and ref_l1 both are not zero. If queried value is VA_PREDICTION_DIRECTION_PREVIOUS only, this means driver doesn't support B frame with different l0/l1, need to set l1 = 10. VA_PREDICTION_DIRECTION_PREVIOUS | VA_PREDICTION_DIRECTION_FUTURE means different l0/l1 is support for B frame. And if queried value is VA_PREDICTION_DIRECTION_BI_NOT_EMPTY, this means driver doesn't support P frame with l0 only. And in debug message, maybe use "Driver only support same reference list for B-frames\n" will be more clear. Fei > > > > > + > > > + 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".
On 14/03/2022 02:15, Xiang, Haihao wrote: > 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, Right, so this is actually a different constraint which isn't indicated so far. It's not just the BI_NOT_EMPTY constraint that P slices cannot be used, it's also that in B slices RefPicList1 has to be identical to RefPicList0. Perhaps this should be added to libva? > so user can't > use past and future reference together, Where is this coming from? I don't see how past vs. future references are relevant at all (since the driver can't see that anyway), only the matching content of the lists. Maybe in this case the right behaviour would be to concatenate what would normally be in the two lists and then put that in both of them (assuming it supports the necessary reference counts, but this particular hardware appears to allow 3/3 so it would). > which is why you experienced the failure > after applying version 2 Yep - and is also why the P-frame only case works, since that puts the same single entry in both lists. - Mark
On 14/03/2022 11:07, Wang, Fei W wrote: >> -----Original Message----- >> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Xiang, >> Haihao >> Sent: Monday, March 14, 2022 10:15 AM >> To: ffmpeg-devel@ffmpeg.org >> Subject: Re: [FFmpeg-devel] [PATCH v4 1/4] lavc/vaapi_encode_h265: Add GPB >> frame support for hevc_vaapi >> >> 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. > > If driver doesn't support B frames with two different reference lists, use gpb instead > of regular B is a best way. In V3, I turned B frames to P, but this will break gop structure. > If user set I/P/B frames, while the output only contains I/P frames will be much confuse. If the driver requires that RefPicList0 and RefPicList1 in B slices are the same then isn't the obvious solution to concatenate the expected lists and put the result in both actual lists (assuming it fits)? That continues to support B frames in the obvious way (and transparently to the user). The hardware will have to do a bit more motion search, but the fake P slices already require that anyway so I assume it is sensibly optimised. >>>> + >>>> 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? > > VA_PREDICTION_DIRECTION_PREVIOUS/ VA_PREDICTION_DIRECTION_FUTURE/ VA_PREDICTION_DIRECTION_BI_NOT_EMPTY > are used to indicate if driver has the limitation on how to set regular P and B frame's reference list when the queried max > reference list ref_l0 and ref_l1 both are not zero. > > If queried value is VA_PREDICTION_DIRECTION_PREVIOUS only, this means driver doesn't support B frame with different l0/l1, > need to set l1 = 10. > VA_PREDICTION_DIRECTION_PREVIOUS | VA_PREDICTION_DIRECTION_FUTURE means different l0/l1 is support for B frame. > > And if queried value is VA_PREDICTION_DIRECTION_BI_NOT_EMPTY, this means driver doesn't support P frame with l0 only. Hmm. So if we ignore both the names and the documentation as unhelpful then the meanings you are intending are: Attribute unset -> no restrictions. Attribute set -> PREVIOUS bit must be set: P and B slices are both supported but RefPicList0 and RefPicList1 must in B slices must be equal. Then additional bits modify that: FUTURE bit set -> RefPicList0 and RefPicList1 in B slices may differ. BI_NOT_EMPTY bit set -> P slices are not supported. (If this is actually the intended meaning then maybe someone could add this to the documentation in libva?) The real Intel hardware we've then got for H.265 is: Gen9, Gen9 vdenc, Gen11, Gen12: unset -> no restrictions. Gen11 vdenc: PREVIOUS | BI_NOT_EMPTY -> P slices are not supported, B slices must have RefPicList0 equal to RefPicList1. Gen12 vdenc: PREVIOUS | FUTURE | BI_NOT_EMPTY -> P slices are not supported, B slices can do whatever. For Gen12 vdenc you fake up P frames by using B slices with two equal lists, and make B frames normally. That looks sensible. For Gen11 vdenc you are faking up P frames in the same way, but then with B frames your patch pretends they are also P frames and ignores all but the first reference, which IMO is not sensible because it will multiply bitrate by several times at a given quality. Hence suggestion above? > And in debug message, maybe use "Driver only support same reference list for B-frames\n" will be more clear. Yeah, given the trickiness of this it's probably a good idea if the messages are very clear about what is going on. >>>> + >>>> + 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
> > Hmm. So if we ignore both the names and the documentation as unhelpful then > the meanings you are intending are: > > Attribute unset -> no restrictions. > Attribute set -> PREVIOUS bit must be set: P and B slices are both supported > but RefPicList0 and RefPicList1 must in B slices must be equal. > > Then additional bits modify that: > > FUTURE bit set -> RefPicList0 and RefPicList1 in B slices may differ. > BI_NOT_EMPTY bit set -> P slices are not supported. > > (If this is actually the intended meaning then maybe someone could add this to > the documentation in libva?) I submitted https://github.com/intel/libva/pull/581 to update the doc, you are welcome to review it. Thanks Haihao
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); + 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"); + } + } + + 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 + if (ctx->codec->flags & FLAG_INTRA_ONLY || avctx->gop_size <= 1) { av_log(avctx, AV_LOG_VERBOSE, "Using intra frames only.\n"); @@ -1855,14 +1907,24 @@ static av_cold int vaapi_encode_init_gop_structure(AVCodecContext *avctx) return AVERROR(EINVAL); } else if (!(ctx->codec->flags & FLAG_B_PICTURES) || ref_l1 < 1 || avctx->max_b_frames < 1) { - av_log(avctx, AV_LOG_VERBOSE, "Using intra and P-frames " - "(supported references: %d / %d).\n", ref_l0, ref_l1); + if (ctx->p_to_gpb) + av_log(avctx, AV_LOG_VERBOSE, "Using intra and B-frames " + "(supported references: %d / %d).\n", + ref_l0, ref_l1); + else + av_log(avctx, AV_LOG_VERBOSE, "Using intra and P-frames " + "(supported references: %d / %d).\n", ref_l0, ref_l1); ctx->gop_size = avctx->gop_size; ctx->p_per_i = INT_MAX; ctx->b_per_p = 0; } else { - av_log(avctx, AV_LOG_VERBOSE, "Using intra, P- and B-frames " - "(supported references: %d / %d).\n", ref_l0, ref_l1); + if (ctx->p_to_gpb) + av_log(avctx, AV_LOG_VERBOSE, "Using intra and B-frames " + "(supported references: %d / %d).\n", + ref_l0, ref_l1); + else + av_log(avctx, AV_LOG_VERBOSE, "Using intra, P- and B-frames " + "(supported references: %d / %d).\n", ref_l0, ref_l1); ctx->gop_size = avctx->gop_size; ctx->p_per_i = INT_MAX; ctx->b_per_p = avctx->max_b_frames; diff --git a/libavcodec/vaapi_encode.h b/libavcodec/vaapi_encode.h index b41604a883..4ce1df0c6f 100644 --- a/libavcodec/vaapi_encode.h +++ b/libavcodec/vaapi_encode.h @@ -328,6 +328,8 @@ typedef struct VAAPIEncodeContext { int idr_counter; int gop_counter; int end_of_stream; + int p_to_gpb; + int b_to_gpb; // Whether the driver supports ROI at all. int roi_allowed; diff --git a/libavcodec/vaapi_encode_h265.c b/libavcodec/vaapi_encode_h265.c index e1dc53dfa9..25b9a707d2 100644 --- a/libavcodec/vaapi_encode_h265.c +++ b/libavcodec/vaapi_encode_h265.c @@ -717,7 +717,10 @@ static int vaapi_encode_h265_init_picture_params(AVCodecContext *avctx, hpic->pic_type = 1; } else { VAAPIEncodePicture *irap_ref; - av_assert0(pic->refs[0] && pic->refs[1]); + if (!ctx->b_to_gpb) + av_assert0(pic->refs[0] && pic->refs[1]); + else + av_assert0(pic->refs[0]); for (irap_ref = pic; irap_ref; irap_ref = irap_ref->refs[1]) { if (irap_ref->type == PICTURE_TYPE_I) break; @@ -886,6 +889,7 @@ static int vaapi_encode_h265_init_slice_params(AVCodecContext *avctx, VAAPIEncodePicture *pic, VAAPIEncodeSlice *slice) { + VAAPIEncodeContext *ctx = avctx->priv_data; VAAPIEncodeH265Context *priv = avctx->priv_data; VAAPIEncodeH265Picture *hpic = pic->priv_data; const H265RawSPS *sps = &priv->raw_sps; @@ -908,6 +912,9 @@ static int vaapi_encode_h265_init_slice_params(AVCodecContext *avctx, sh->slice_type = hpic->slice_type; + if (sh->slice_type == HEVC_SLICE_P && ctx->p_to_gpb) + sh->slice_type = HEVC_SLICE_B; + sh->slice_pic_order_cnt_lsb = hpic->pic_order_cnt & (1 << (sps->log2_max_pic_order_cnt_lsb_minus4 + 4)) - 1; @@ -1066,11 +1073,26 @@ static int vaapi_encode_h265_init_slice_params(AVCodecContext *avctx, av_assert0(pic->type == PICTURE_TYPE_P || pic->type == PICTURE_TYPE_B); vslice->ref_pic_list0[0] = vpic->reference_frames[0]; + if (ctx->p_to_gpb && pic->type == PICTURE_TYPE_P) + // Reference for GPB B-frame, L0 == L1 + vslice->ref_pic_list1[0] = vpic->reference_frames[0]; } if (pic->nb_refs >= 2) { // Forward reference for B-frame. av_assert0(pic->type == PICTURE_TYPE_B); vslice->ref_pic_list1[0] = vpic->reference_frames[1]; + if (ctx->b_to_gpb && pic->type == PICTURE_TYPE_B) + // Reference for GPB B-frame, L0 == L1 + vslice->ref_pic_list1[0] = vpic->reference_frames[0]; + } + + if ((pic->type == PICTURE_TYPE_P && ctx->p_to_gpb) || + (pic->type == PICTURE_TYPE_B && ctx->b_to_gpb)) { + vslice->slice_type = HEVC_SLICE_B; + for (i = 0; i < FF_ARRAY_ELEMS(vslice->ref_pic_list0); i++) { + vslice->ref_pic_list1[i].picture_id = vslice->ref_pic_list0[i].picture_id; + vslice->ref_pic_list1[i].flags = vslice->ref_pic_list0[i].flags; + } } return 0;