On Thu, Nov 30, 2017 at 10:40 AM, Li, Zhong <zhong...@intel.com> wrote: >> -----Original Message----- >> From: ffmpeg-devel [mailto:ffmpeg-devel-boun...@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.s...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
