From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id B0730451B6 for ; Wed, 11 Jan 2023 10:29:10 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 3AD8968BD06; Wed, 11 Jan 2023 12:29:09 +0200 (EET) Received: from mail-pg1-f178.google.com (mail-pg1-f178.google.com [209.85.215.178]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 34CF668BBD9 for ; Wed, 11 Jan 2023 12:29:03 +0200 (EET) Received: by mail-pg1-f178.google.com with SMTP id s67so10223000pgs.3 for ; Wed, 11 Jan 2023 02:29:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=YsiN7kGZ4PmeIkWh6tGT3hkHT+Wmg/myQghUHbP5eAM=; b=hW2DJ7115nF+3jRW3NV97ZA+EaRHK2fZFa96e7KDBk2TThubOI8HUqsyb7DoQ3ktC1 eqbP+UMWzC6KFORP/qiLdGP8dAghJ6ysoin6arefgpow7clJNpS1Oi3/bgTaMUexAstZ yV3A9qA5QsAo3gHvumTO7hdvBdSXz6iYARNMcKwsFr9MCGF+tCzWaHXhnQjweE1qc4hl v2iHHcOGZ7fWQH2cU20Xq+qj6NmNcgIY79b9GdYAV4lVhaKaq7d/d3675v5OUcKvQ4Uh X075i3NyBjPvXttbiBhbskym2jkwFv7dL0vwi8IR79MA7aPqJM4Ydy1AftKVRHT+NYO9 79nQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YsiN7kGZ4PmeIkWh6tGT3hkHT+Wmg/myQghUHbP5eAM=; b=rJ2O6vzyz1n6hoLdB224c5x8Hz/xxsVSgSRiHN1FhaMSnp0SSUL1Lr5P4GYjQASjkC Ta9y/Jo3jcxInLGcBz8h4KhcY+R7ccwmLcHNWKbxzUEizDIjucYqaHDb7wBtWzCUYpI9 yPzMki9JMCadNijdEWHsIfvAV3ZVlsh50BimUwrnTvUA2b0Xysq5e9Xy1rhwOZoTWtf1 rpi7sy10mr4BSvjLE1vUokteNzEU4kEIs33BsrcSn8ftM4mqkjE3CroIi1KJ6SUMUV0p akq27ulmV4+LI+/vtzNsYAjrD7MBl9Pti2O00syvehg1oOhSAlj2tw5fA7CnZTXjTCiS nong== X-Gm-Message-State: AFqh2kqjSbm5MIonB9J2j0IWJua9fVXWbdpDdnqdOQvAjJF2M4TuI+IJ 7G8bD481Usl4IGCfhwJMLDRe9yKjRR0zKEkyj+2BNKr1/z/G9Q== X-Google-Smtp-Source: AMrXdXt+jBKMH9dwsUqo3eUTFma4+wqwwerxPPANDsvnmHcebRv0M/LMUzZl+W4RpDasV+QAvVwIWHbR7ueR0eUD9pQ= X-Received: by 2002:a63:ed53:0:b0:4ac:bd5a:e11 with SMTP id m19-20020a63ed53000000b004acbd5a0e11mr1675669pgk.556.1673432940906; Wed, 11 Jan 2023 02:29:00 -0800 (PST) MIME-Version: 1.0 References: <4a9e9778-c94e-f5b7-bce6-0f60a4a5371a@jkqxz.net> In-Reply-To: <4a9e9778-c94e-f5b7-bce6-0f60a4a5371a@jkqxz.net> From: Jianfeng Zheng Date: Wed, 11 Jan 2023 18:27:45 +0800 Message-ID: To: FFmpeg development discussions and patches Subject: Re: [FFmpeg-devel] [PATCH] vaapi: support VAProfileH264High10 X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Hi Mark, Patch v3 would be send. And please view the detailed reply follows: > > On 26/12/2022 06:59, Jean Jogh wrote: > > see https://github.com/intel/libva/pull/664 > > > > Signed-off-by: jianfeng.zheng > > --- > > configure | 13 +++++++++++++ > > libavcodec/h264_slice.c | 6 ++++++ > > libavcodec/vaapi_decode.c | 10 ++++++++++ > > libavcodec/vaapi_encode_h264.c | 24 ++++++++++++++++++++++-- > > libavcodec/vaapi_h264.c | 5 +++-- > > 5 files changed, 54 insertions(+), 4 deletions(-) > > > > diff --git a/configure b/configure > > index f08cdab3d1..ac199d97cb 100755 > > --- a/configure > > +++ b/configure > > @@ -2410,6 +2410,7 @@ HAVE_LIST=" > > texi2html > > xmllint > > zlib_gzip > > + va_profile_h264_high10 > > " > > > > # options emitted with CONFIG_ prefix but not available on the command line > > @@ -6958,6 +6959,18 @@ if enabled vaapi; then > > check_type "va/va.h va/va_enc_jpeg.h" "VAEncPictureParameterBufferJPEG" > > check_type "va/va.h va/va_enc_vp8.h" "VAEncPictureParameterBufferVP8" > > check_type "va/va.h va/va_enc_vp9.h" "VAEncPictureParameterBufferVP9" > > + > > + # > > + # Using 'VA_CHECK_VERSION' in source codes make things easy. But > > we have to wait > > + # until newly added VAProfile being distributed by VAAPI released version. > > + # > > + # Before or after that, we can use auto-detection to keep version > > compatibility. > > + # It always works. > > + # > > + disable va_profile_h264_high10 && enabled h264_vaapi_hwaccel > > + test_code cc va/va.h "VAProfile p = VAProfileH264High10" && > > + enable va_profile_h264_high10 > > To echo comments from others, I think it would be better follow the same form as other codecs and use the VA_CHECK_VERSION for the next release. Yes, corresponding VAProfileH264High10 patch has been merged into libva mater branch. Maybe we can wait if the new release don't come too late or just merge with 2nd patch. > > + > > fi > > > > if enabled_all opencl libdrm ; then > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c > > index 420758ba0a..b9236281b1 100644 > > --- a/libavcodec/h264_slice.c > > +++ b/libavcodec/h264_slice.c > > @@ -806,6 +806,12 @@ static enum AVPixelFormat > > get_pixel_format(H264Context *h, int force_callback) > > } else if (CHROMA422(h)) > > *fmt++ = AV_PIX_FMT_YUV422P10; > > else > > +#if CONFIG_H264_VAAPI_HWACCEL > > + // Just add as candidate. Whether VAProfileH264High10 usable or > > + // not is decided by vaapi_decode_make_config() defined in FFmpeg > > + // and vaQueryCodingProfile() defined in libva. > > + *fmt++ = AV_PIX_FMT_VAAPI; > > +#endif > > *fmt++ = AV_PIX_FMT_YUV420P10; > > Braces definitely needed here! Yes, thank you! > > break; > > case 12: > > diff --git a/libavcodec/vaapi_decode.c b/libavcodec/vaapi_decode.c > > index 134f10eca5..03bbbf2c41 100644 > > --- a/libavcodec/vaapi_decode.c > > +++ b/libavcodec/vaapi_decode.c > > @@ -402,6 +402,9 @@ static const struct { > > H264ConstrainedBaseline), > > MAP(H264, H264_MAIN, H264Main ), > > MAP(H264, H264_HIGH, H264High ), > > +#ifdef HAVE_VA_PROFILE_H264_HIGH10 > > + MAP(H264, H264_HIGH_10, H264High10 ), > > +#endif > > #if VA_CHECK_VERSION(0, 37, 0) > > MAP(HEVC, HEVC_MAIN, HEVCMain ), > > MAP(HEVC, HEVC_MAIN_10, HEVCMain10 ), > > @@ -510,6 +513,13 @@ static int vaapi_decode_make_config(AVCodecContext *avctx, > > if (exact_match) > > break; > > } > > +#ifdef HAVE_VA_PROFILE_H264_HIGH10 > > + //incase 8bit stream being decoded under VAProfileH264High10 > > + if (avctx->codec_id == AV_CODEC_ID_H264 && > > + (avctx->profile == FF_PROFILE_H264_EXTENDED || > > avctx->profile == FF_PROFILE_H264_BASELINE) && > > + matched_va_profile == VAProfileH264High) > > + break; > > +#endif > > Can you explain what this is trying to do? Why does it need a special case where H.265 doesn't? > As the comments point out, this is to incase 8bit stream being decoded under VAProfileH264High10 when "--allow_profile_mismatch" is specified. H265 don't need this becase H.265 hasn't defined so much 8-bit profiles like H.264, and won't meet the mismatch problem. > > } > > av_freep(&profile_list); > > > > diff --git a/libavcodec/vaapi_encode_h264.c b/libavcodec/vaapi_encode_h264.c > > index dd17be2190..d26ac5a98f 100644 > > --- a/libavcodec/vaapi_encode_h264.c > > +++ b/libavcodec/vaapi_encode_h264.c > > @@ -23,6 +23,7 @@ > > > > #include "libavutil/avassert.h" > > #include "libavutil/common.h" > > +#include "libavutil/pixdesc.h" > > #include "libavutil/internal.h" > > #include "libavutil/opt.h" > > > > @@ -290,10 +291,21 @@ static int > > vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > > H264RawPPS *pps = &priv->raw_pps; > > VAEncSequenceParameterBufferH264 *vseq = ctx->codec_sequence_params; > > VAEncPictureParameterBufferH264 *vpic = ctx->codec_picture_params; > > + const AVPixFmtDescriptor *desc; > > + int bit_depth; > > > > memset(sps, 0, sizeof(*sps)); > > memset(pps, 0, sizeof(*pps)); > > > > + desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format); > > + av_assert0(desc); > > + if (desc->nb_components == 1 || desc->log2_chroma_w != 1 || > > desc->log2_chroma_h != 1) { > > + av_log(avctx, AV_LOG_ERROR, "Chroma format of input pixel format " > > + "%s is not supported.\n", desc->name); > > + return AVERROR(EINVAL); > > + } > > + bit_depth = desc->comp[0].depth; > > + > > sps->nal_unit_header.nal_ref_idc = 3; > > sps->nal_unit_header.nal_unit_type = H264_NAL_SPS; > > > > @@ -303,11 +315,11 @@ static int > > vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > > avctx->profile == FF_PROFILE_H264_MAIN) > > sps->constraint_set1_flag = 1; > > > > - if (avctx->profile == FF_PROFILE_H264_HIGH) > > + if (avctx->profile == FF_PROFILE_H264_HIGH || avctx->profile == > > FF_PROFILE_H264_HIGH_10) > > sps->constraint_set3_flag = ctx->gop_size == 1; > > > > if (avctx->profile == FF_PROFILE_H264_MAIN || > > - avctx->profile == FF_PROFILE_H264_HIGH) { > > + avctx->profile == FF_PROFILE_H264_HIGH || avctx->profile == > > FF_PROFILE_H264_HIGH_10) { > > sps->constraint_set4_flag = 1; > > sps->constraint_set5_flag = ctx->b_per_p == 0; > > } > > @@ -348,6 +360,8 @@ static int > > vaapi_encode_h264_init_sequence_params(AVCodecContext *avctx) > > > > sps->seq_parameter_set_id = 0; > > sps->chroma_format_idc = 1; > > + sps->bit_depth_luma_minus8 = bit_depth - 8; > > + sps->bit_depth_chroma_minus8 = bit_depth - 8; > > > > sps->log2_max_frame_num_minus4 = 4; > > sps->pic_order_cnt_type = 0; > > @@ -1111,6 +1125,9 @@ static av_cold int > > vaapi_encode_h264_configure(AVCodecContext *avctx) > > } > > > > static const VAAPIEncodeProfile vaapi_encode_h264_profiles[] = { > > +#ifdef HAVE_VA_PROFILE_H264_HIGH10 > > + { FF_PROFILE_H264_HIGH_10, 10, 3, 1, 1, VAProfileH264High10 }, > > +#endif > > { FF_PROFILE_H264_HIGH, 8, 3, 1, 1, VAProfileH264High }, > > { FF_PROFILE_H264_MAIN, 8, 3, 1, 1, VAProfileH264Main }, > > { FF_PROFILE_H264_CONSTRAINED_BASELINE, > > @@ -1175,11 +1192,13 @@ static av_cold int > > vaapi_encode_h264_init(AVCodecContext *avctx) > > av_log(avctx, AV_LOG_ERROR, "H.264 extended profile " > > "is not supported.\n"); > > return AVERROR_PATCHWELCOME; > > +#ifndef HAVE_VA_PROFILE_H264_HIGH10 > > case FF_PROFILE_H264_HIGH_10: > > case FF_PROFILE_H264_HIGH_10_INTRA: > > av_log(avctx, AV_LOG_ERROR, "H.264 10-bit profiles " > > "are not supported.\n"); > > return AVERROR_PATCHWELCOME; > > +#endif > > This is doing early rejection of explicitly-specified profiles which libavcodec definitely can't support - I think just remove this block entirely, since it will be rejected later if not supported. > > (Presumably explicitly-specified High 10 Intra does work as expected? It looks like it should, but it would be good to check.) Yes, high 10 intra would be problem if explicitly-specified, so it should be keeped. > > case FF_PROFILE_H264_HIGH_422: > > case FF_PROFILE_H264_HIGH_422_INTRA: > > case FF_PROFILE_H264_HIGH_444: > > @@ -1267,6 +1286,7 @@ static const AVOption vaapi_encode_h264_options[] = { > > { PROFILE("constrained_baseline", FF_PROFILE_H264_CONSTRAINED_BASELINE) }, > > { PROFILE("main", FF_PROFILE_H264_MAIN) }, > > { PROFILE("high", FF_PROFILE_H264_HIGH) }, > > + { PROFILE("high10", FF_PROFILE_H264_HIGH_10) }, > > #undef PROFILE > > > > { "level", "Set level (level_idc)", > > Hmm. It looks like libva has the same deficiency here as in H.265 where it doesn't support the lowest QPs. Is there any way of fixing that on the libva side? More hint please? > > > diff --git a/libavcodec/vaapi_h264.c b/libavcodec/vaapi_h264.c > > index 9332aa6f31..3e44c8caf6 100644 > > --- a/libavcodec/vaapi_h264.c > > +++ b/libavcodec/vaapi_h264.c > > @@ -234,6 +234,7 @@ static int vaapi_h264_start_frame(AVCodecContext > > *avctx, > > VAPictureParameterBufferH264 pic_param; > > VAIQMatrixBufferH264 iq_matrix; > > int err; > > + int qp_bd_offset = 6 * (sps->bit_depth_luma - 8); > > > > pic->output_surface = ff_vaapi_get_surface_id(h->cur_pic_ptr->f); > > > > @@ -256,8 +257,8 @@ static int vaapi_h264_start_frame(AVCodecContext > > *avctx, > > .log2_max_pic_order_cnt_lsb_minus4 = > > sps->log2_max_poc_lsb - 4, > > .delta_pic_order_always_zero_flag = > > sps->delta_pic_order_always_zero_flag, > > }, > > - .pic_init_qp_minus26 = pps->init_qp - 26, > > - .pic_init_qs_minus26 = pps->init_qs - 26, > > + .pic_init_qp_minus26 = pps->init_qp - > > 26 - qp_bd_offset, > > + .pic_init_qs_minus26 = pps->init_qs - > > 26 - qp_bd_offset, > > The field is (signed) pic_init_qp_minus26 as found in the PPS in order to allow header parsing in hardware if it wants. You should not be applying QpBdOffset as if making QP'_y here, because that's only done at the MB level. > > Given that you've sent this and therefore it presumably works, I guess you must have a driver applying the reverse offset to recover the actual QP? Can you fix the driver not to do that? Yes I think we can move qp_bd_offset to the driver layer. > > > .chroma_qp_index_offset = > > pps->chroma_qp_index_offset[0], > > .second_chroma_qp_index_offset = > > pps->chroma_qp_index_offset[1], > > .pic_fields.bits = { > > Thanks, > > - 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". Thanks, Jianfeng _______________________________________________ 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".