Message ID | 1586926427-20170-9-git-send-email-linjie.fu@intel.com |
---|---|
State | New |
Headers | show |
Series | Enhancement for libopenh264 encoder | expand |
Context | Check | Description |
---|---|---|
andriy/default | pending | |
andriy/make | success | Make finished |
andriy/make_fate | success | Make fate finished |
On Wed, 15 Apr 2020, Linjie Fu wrote: > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > --- > libavcodec/libopenh264enc.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c > index 0fe8cf4..4d337d2 100644 > --- a/libavcodec/libopenh264enc.c > +++ b/libavcodec/libopenh264enc.c > @@ -113,6 +113,22 @@ static av_cold int svc_encode_init_profile(AVCodecContext *avctx, SEncParamExt * > { > SVCContext *s = avctx->priv_data; > > + /* Allow specifying the libopenh264 profile through AVCodecContext. */ > + if (FF_PROFILE_UNKNOWN == s->profile && > + FF_PROFILE_UNKNOWN != avctx->profile) > + switch (avctx->profile) { > + case FF_PROFILE_H264_CONSTRAINED_BASELINE: > + s->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; > + break; > + case FF_PROFILE_H264_HIGH: > + s->profile = FF_PROFILE_H264_HIGH; > + break; > + default: > + av_log(avctx, AV_LOG_WARNING, > + "Unsupported avctx->profile: %d.\n", avctx->profile); > + break; > + } > + > if (s->profile == FF_PROFILE_UNKNOWN) > s->profile = s->cabac ? FF_PROFILE_H264_HIGH : > FF_PROFILE_H264_CONSTRAINED_BASELINE; With this in place, why even do the previous commit, why not just go for only using avctx->profile? Although as far as I can see, that field doesn't seem to have string options for sepcifying H264 profiles, so it can only be set via code or by specifying a numberic value - is that right? Wouldn't it just be best to add the H264 profiles to options_table.h to keep allowing it to be set via a string, but remove the internal s->profile field here, as patch 7/9 already breaks handling of the profile field by stopping recognizing "main" and only recognizing "high" anyway. // Martin
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of > Martin Storsjö > Sent: Tuesday, April 28, 2020 04:13 > To: FFmpeg development discussions and patches <ffmpeg- > devel@ffmpeg.org> > Cc: Fu, Linjie <linjie.fu@intel.com> > Subject: Re: [FFmpeg-devel] [PATCH v4 8/9] lavc/libopenh264enc: allow > specifying the profile through AVCodecContext > > On Wed, 15 Apr 2020, Linjie Fu wrote: > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com> > > --- > > libavcodec/libopenh264enc.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c > > index 0fe8cf4..4d337d2 100644 > > --- a/libavcodec/libopenh264enc.c > > +++ b/libavcodec/libopenh264enc.c > > @@ -113,6 +113,22 @@ static av_cold int > svc_encode_init_profile(AVCodecContext *avctx, SEncParamExt * > > { > > SVCContext *s = avctx->priv_data; > > > > + /* Allow specifying the libopenh264 profile through AVCodecContext. > */ > > + if (FF_PROFILE_UNKNOWN == s->profile && > > + FF_PROFILE_UNKNOWN != avctx->profile) > > + switch (avctx->profile) { > > + case FF_PROFILE_H264_CONSTRAINED_BASELINE: > > + s->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; > > + break; > > + case FF_PROFILE_H264_HIGH: > > + s->profile = FF_PROFILE_H264_HIGH; > > + break; > > + default: > > + av_log(avctx, AV_LOG_WARNING, > > + "Unsupported avctx->profile: %d.\n", avctx->profile); > > + break; > > + } > > + > > if (s->profile == FF_PROFILE_UNKNOWN) > > s->profile = s->cabac ? FF_PROFILE_H264_HIGH : > > FF_PROFILE_H264_CONSTRAINED_BASELINE; > > With this in place, why even do the previous commit, why not just go for > only using avctx->profile? Although as far as I can see, that field > doesn't seem to have string options for sepcifying H264 profiles, so it > can only be set via code or by specifying a numberic value - is that > right? Yes, code/numberic value works in this way, however maybe not straight forward enough for user. > Wouldn't it just be best to add the H264 profiles to options_table.h to > keep allowing it to be set via a string, but remove the internal > s->profile field here, as patch 7/9 already breaks handling of the profile > field by stopping recognizing "main" and only recognizing "high" anyway. Most sw codecs like libx264/libx265 and hw codecs like h264_vaapi/h264_nvenc have the logic of : 1. derive the settings from avctx->profile which is determined in options_table.h; 2. If not, check the private option for specific encoder; And specifically for libopenh264enc, we allow one more logic: 3. determine the profile by s->cabac; I admit it'll be good if settled this down to parse all possible profiles for each codec (maybe according to libavcodec/profiles.c), hence we may remove similar/redundant logics in specific encoder(x264/h264_vaapi/h264_nvenc). Please help to comment whether it makes sense for you all, since this would impact many codecs. - Linjie
diff --git a/libavcodec/libopenh264enc.c b/libavcodec/libopenh264enc.c index 0fe8cf4..4d337d2 100644 --- a/libavcodec/libopenh264enc.c +++ b/libavcodec/libopenh264enc.c @@ -113,6 +113,22 @@ static av_cold int svc_encode_init_profile(AVCodecContext *avctx, SEncParamExt * { SVCContext *s = avctx->priv_data; + /* Allow specifying the libopenh264 profile through AVCodecContext. */ + if (FF_PROFILE_UNKNOWN == s->profile && + FF_PROFILE_UNKNOWN != avctx->profile) + switch (avctx->profile) { + case FF_PROFILE_H264_CONSTRAINED_BASELINE: + s->profile = FF_PROFILE_H264_CONSTRAINED_BASELINE; + break; + case FF_PROFILE_H264_HIGH: + s->profile = FF_PROFILE_H264_HIGH; + break; + default: + av_log(avctx, AV_LOG_WARNING, + "Unsupported avctx->profile: %d.\n", avctx->profile); + break; + } + if (s->profile == FF_PROFILE_UNKNOWN) s->profile = s->cabac ? FF_PROFILE_H264_HIGH : FF_PROFILE_H264_CONSTRAINED_BASELINE;
Signed-off-by: Linjie Fu <linjie.fu@intel.com> --- libavcodec/libopenh264enc.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)