| Message ID | 25a2cf1b-f2ec-a250-6094-fb5be4605b89@jkqxz.net |
|---|---|
| State | Accepted |
| Commit | 3ff8fbbf5a7bc40c09db74d4952364997fd3c611 |
| Headers | show |
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > Of Mark Thompson > Sent: Thursday, November 30, 2017 7:30 AM > To: ffmpeg-devel@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Nov 30, 2017 at 10:40 AM, Li, Zhong <zhong.li@intel.com> wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf >> Of Mark Thompson >> Sent: Thursday, November 30, 2017 7:30 AM >> To: ffmpeg-devel@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@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. But 41 is never 4.1 for HEVC, so using a scheme that was basically "invented" doesn't seem right (probably as a carry-over from H264). When decoding, avctx->level contains 123, not 41, for example, or for a closer match, NVENC accepts -profile:v 123 or -profile:v 4.1, but not 41. I think its more important to stay consistent here (both to the HEVC spec and other HEVC encoders), instead of catering to an imaginary value that is never actually used in relation to HEVC. - Hendrik
> -----Original Message----- > From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of > Hendrik Leppkes > Sent: Thursday, November 30, 2017 6:12 PM > To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> > Subject: Re: [FFmpeg-devel] [PATCH 2/2] vaapi_h265: Add named options for > setting profile and level > > On Thu, Nov 30, 2017 at 10:40 AM, Li, Zhong <zhong.li@intel.com> wrote: > >> -----Original Message----- > >> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf > >> Of Mark Thompson > >> Sent: Thursday, November 30, 2017 7:30 AM > >> To: ffmpeg-devel@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@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. > > > But 41 is never 4.1 for HEVC, so using a scheme that was basically > "invented" doesn't seem right (probably as a carry-over from H264). > When decoding, avctx->level contains 123, not 41, for example, or for > a closer match, NVENC accepts -profile:v 123 or -profile:v 4.1, but > not 41. > I think its more important to stay consistent here (both to the HEVC > spec and other HEVC encoders), instead of catering to an imaginary > value that is never actually used in relation to HEVC. I agree with you on this. It would be better if we can keep consistent here. For this level information, if user want to set level to 5.1 in hevc. They can do as: "-level 5.1" or "-level 153". I tried Mark's patch, it works as expected. - Ruiling > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
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" },