[FFmpeg-devel] [PATCH 2/2] vaapi_h265: Add named options for setting profile and level

[Li, Zhong]

> -----Original Message-----
> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces at ffmpeg.org] On Behalf
> Of Mark Thompson
> Sent: Thursday, November 30, 2017 7:30 AM
> To: ffmpeg-devel at ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH 2/2] vaapi_h265: Add named options for
> setting profile and level
> 
> Also fixes the default, which previously contained a nonsense value.
> ---
> On 29/11/17 03:51, Li, Zhong wrote:
> >> On 28/11/17 07:46, Ruiling Song wrote:
> >>> Signed-off-by: Ruiling Song <ruiling.song at intel.com>
> >>> ---
> >>>  libavcodec/vaapi_encode_h265.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/vaapi_encode_h265.c
> >>> b/libavcodec/vaapi_encode_h265.c index 3ae92a7..32b8bc6 100644
> >>> --- a/libavcodec/vaapi_encode_h265.c
> >>> +++ b/libavcodec/vaapi_encode_h265.c
> >>> @@ -219,7 +219,7 @@ static int
> >> vaapi_encode_h265_init_sequence_params(AVCodecContext *avctx)
> >>>          .general_non_packed_constraint_flag = 1,
> >>>          .general_frame_only_constraint_flag = 1,
> >>>
> >>> -        .general_level_idc     = avctx->level,
> >>> +        .general_level_idc     = avctx->level * 3,
> >>>      };
> >>>
> >>> vps->profile_tier_level.general_profile_compatibility_flag[avctx->pr
> >>> vps->of
> >>> ile & 31] = 1;
> >>>
> >>>
> >> The documentation has always said "profile and level set the values
> >> of general_profile_idc and general_level_idc respectively"
> >> (<http://ffmpeg.org/ffmpeg-codecs.html#VAAPI-encoders>) - the code
> >> disagreed for a while, but that was made consistent in
> >> 00179664bccd1dd6fa0d1c40db453528757bf6f7.
> >>
> >> Why do you want to change it to be general_level_idc / 3 instead?
> >
> > As HEVC spec, "general_level_idc and sub_layer_level_idc[ i ] shall be
> > set equal to a value of 30 times the level number specified in Table A.4."
> > And use the tool "mediainfo" to check the encoded bitstream, it show the
> level is 1.7 if set the option "-level" to be 51.
> >
> > Maybe the documentation
> ((<http://ffmpeg.org/ffmpeg-codecs.html#VAAPI-encoders>)) also needs to
> be changed?
> 
> Hmm, right - the default is wrong, so you get a nonsense value there.
> 
> How about we fix that and add named constants to make it clearer, like this?

I think it is a good idea, except one situation：
Suppose user set the level to be 41, they may want an encoded bitstream with level 4.1, right? 
But actually the output level is 1.4 (using mediainfo to check this), currently we are forcing them to set the option to be 4.1 or 123, right? 
Haven't verify your patch, just infer from the code. 

> 
> Thanks,
> 
> - Mark
> 
> 
> 
>  libavcodec/vaapi_encode_h265.c | 44
> +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_h265.c
> b/libavcodec/vaapi_encode_h265.c index 3ae92a7a09..8e98b0230d 100644
> --- a/libavcodec/vaapi_encode_h265.c
> +++ b/libavcodec/vaapi_encode_h265.c
> @@ -63,6 +63,8 @@ typedef struct VAAPIEncodeH265Context {  typedef
> struct VAAPIEncodeH265Options {
>      int qp;
>      int aud;
> +    int profile;
> +    int level;
>  } VAAPIEncodeH265Options;
> 
> 
> @@ -880,10 +882,17 @@ static const VAAPIEncodeType
> vaapi_encode_type_h265 = {
> 
>  static av_cold int vaapi_encode_h265_init(AVCodecContext *avctx)  {
> -    VAAPIEncodeContext *ctx = avctx->priv_data;
> +    VAAPIEncodeContext     *ctx = avctx->priv_data;
> +    VAAPIEncodeH265Options *opt =
> +        (VAAPIEncodeH265Options*)ctx->codec_options_data;
> 
>      ctx->codec = &vaapi_encode_type_h265;
> 
> +    if (avctx->profile == FF_PROFILE_UNKNOWN)
> +        avctx->profile = opt->profile;
> +    if (avctx->level == FF_LEVEL_UNKNOWN)
> +        avctx->level = opt->level;
> +
>      switch (avctx->profile) {
>      case FF_PROFILE_HEVC_MAIN:
>      case FF_PROFILE_UNKNOWN:
> @@ -946,12 +955,41 @@ static const AVOption
> vaapi_encode_h265_options[] = {
>      { "aud", "Include AUD",
>        OFFSET(aud), AV_OPT_TYPE_INT, { .i64 = 0 }, 0, 1, FLAGS },
> 
> +    { "profile", "Set profile (general_profile_idc)",
> +      OFFSET(profile), AV_OPT_TYPE_INT,
> +      { .i64 = FF_PROFILE_HEVC_MAIN }, 0x00, 0xff, FLAGS, "profile" },
> +
> +#define PROFILE(name, value)  name, NULL, 0, AV_OPT_TYPE_CONST, \
> +      { .i64 = value }, 0, 0, FLAGS, "profile"
> +    { PROFILE("main",               FF_PROFILE_HEVC_MAIN) },
> +    { PROFILE("main10",             FF_PROFILE_HEVC_MAIN_10) },
> +#undef PROFILE
> +
> +    { "level", "Set level (general_level_idc)",
> +      OFFSET(level), AV_OPT_TYPE_INT,
> +      { .i64 = 153 }, 0x00, 0xff, FLAGS, "level" },
> +
> +#define LEVEL(name, value) name, NULL, 0, AV_OPT_TYPE_CONST, \
> +      { .i64 = value }, 0, 0, FLAGS, "level"
> +    { LEVEL("1",    30) },
> +    { LEVEL("2",    60) },
> +    { LEVEL("2.1",  63) },
> +    { LEVEL("3",    90) },
> +    { LEVEL("3.1",  93) },
> +    { LEVEL("4",   120) },
> +    { LEVEL("4.1", 123) },
> +    { LEVEL("5",   150) },
> +    { LEVEL("5.1", 153) },
> +    { LEVEL("5.2", 156) },
> +    { LEVEL("6",   180) },
> +    { LEVEL("6.1", 183) },
> +    { LEVEL("6.2", 186) },
> +#undef LEVEL
> +
>      { NULL },
>  };
> 
>  static const AVCodecDefault vaapi_encode_h265_defaults[] = {
> -    { "profile",        "1"   },
> -    { "level",          "51"  },
>      { "b",              "0"   },
>      { "bf",             "2"   },
>      { "g",              "120" },
> --
> 2.11.0
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel