diff mbox series

[FFmpeg-devel,RFC] lavc/options_table: Add basic candidate h264 profiles

Message ID 20200506053753.2194-1-linjie.fu@intel.com
State New
Headers show
Series [FFmpeg-devel,RFC] lavc/options_table: Add basic candidate h264 profiles
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Fu, Linjie May 6, 2020, 5:37 a.m. UTC
Allows specifying avctx->profile with string type profile for
h264 encoders.

Private field/option "profile" may be abled to be removed for basic
h264 profiles, directly for encoders like libopenh264/h264_vaapi,
or with an map to hardware profile like h264_qsv.

Signed-off-by: Linjie Fu <linjie.fu@intel.com>
---
One concern is some encoders have options for specific profiles,
like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
hence they may not be able to remove the private option easily.

Please help to comment.

 libavcodec/options_table.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Martin Storsjö May 6, 2020, 9:40 a.m. UTC | #1
On Wed, 6 May 2020, Linjie Fu wrote:

> Allows specifying avctx->profile with string type profile for
> h264 encoders.
>
> Private field/option "profile" may be abled to be removed for basic
> h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> or with an map to hardware profile like h264_qsv.
>
> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> ---
> One concern is some encoders have options for specific profiles,
> like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> hence they may not be able to remove the private option easily.
>
> Please help to comment.
>
> libavcodec/options_table.h | 4 ++++
> 1 file changed, 4 insertions(+)

This change in itself looks sensible to me (but I'd like for someone else 
to comment as well). Even if it might not be able to get rid of the 
private option for all encoders, it should at least simplify the easiest 
cases.

// Martin
mypopy@gmail.com May 6, 2020, 10:04 a.m. UTC | #2
On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st> wrote:
>
> On Wed, 6 May 2020, Linjie Fu wrote:
>
> > Allows specifying avctx->profile with string type profile for
> > h264 encoders.
> >
> > Private field/option "profile" may be abled to be removed for basic
> > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > or with an map to hardware profile like h264_qsv.
> >
> > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > ---
> > One concern is some encoders have options for specific profiles,
> > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > hence they may not be able to remove the private option easily.
> >
> > Please help to comment.
> >
> > libavcodec/options_table.h | 4 ++++
> > 1 file changed, 4 insertions(+)
>
> This change in itself looks sensible to me (but I'd like for someone else
> to comment as well). Even if it might not be able to get rid of the
> private option for all encoders, it should at least simplify the easiest
> cases.
>
> // Martin
I think we have a discussion about this case,
https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-c74b-b3cb-46fc5f3ea932@gmail.com/
mypopy@gmail.com May 6, 2020, 10:13 a.m. UTC | #3
On Wed, May 6, 2020 at 6:04 PM mypopy@gmail.com <mypopy@gmail.com> wrote:
>
> On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st> wrote:
> >
> > On Wed, 6 May 2020, Linjie Fu wrote:
> >
> > > Allows specifying avctx->profile with string type profile for
> > > h264 encoders.
> > >
> > > Private field/option "profile" may be abled to be removed for basic
> > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > or with an map to hardware profile like h264_qsv.
> > >
> > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > ---
> > > One concern is some encoders have options for specific profiles,
> > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > hence they may not be able to remove the private option easily.
> > >
> > > Please help to comment.
> > >
> > > libavcodec/options_table.h | 4 ++++
> > > 1 file changed, 4 insertions(+)
> >
> > This change in itself looks sensible to me (but I'd like for someone else
> > to comment as well). Even if it might not be able to get rid of the
> > private option for all encoders, it should at least simplify the easiest
> > cases.
> >
> > // Martin
> I think we have a discussion about this case,
> https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-c74b-b3cb-46fc5f3ea932@gmail.com/

I agree with Hendrik Leppkes, and I think "main" more natural than
"h264_main"  for 264 codec profile from a user viewpoint, both of
command line tools or  application program interface
Fu, Linjie May 6, 2020, 10:50 a.m. UTC | #4
> From: mypopy@gmail.com <mypopy@gmail.com>
> Sent: Wednesday, May 6, 2020 18:13
> To: FFmpeg development discussions and patches <ffmpeg-
> devel@ffmpeg.org>
> Cc: Fu, Linjie <linjie.fu@intel.com>
> Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic
> candidate h264 profiles
> 
> On Wed, May 6, 2020 at 6:04 PM mypopy@gmail.com <mypopy@gmail.com>
> wrote:
> >
> > On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st> wrote:
> > >
> > > On Wed, 6 May 2020, Linjie Fu wrote:
> > >
> > > > Allows specifying avctx->profile with string type profile for
> > > > h264 encoders.
> > > >
> > > > Private field/option "profile" may be abled to be removed for basic
> > > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > > or with an map to hardware profile like h264_qsv.
> > > >
> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > > ---
> > > > One concern is some encoders have options for specific profiles,
> > > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > > hence they may not be able to remove the private option easily.
> > > >
> > > > Please help to comment.
> > > >
> > > > libavcodec/options_table.h | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > >
> > > This change in itself looks sensible to me (but I'd like for someone else
> > > to comment as well). Even if it might not be able to get rid of the
> > > private option for all encoders, it should at least simplify the easiest
> > > cases.
> > >
> > > // Martin
> > I think we have a discussion about this case,
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-
> c74b-b3cb-46fc5f3ea932@gmail.com/

Ahh, thanks for this input since it seems to be well-discussed.

> I agree with Hendrik Leppkes, and I think "main" more natural than
> "h264_main"  for 264 codec profile from a user viewpoint, both of
> command line tools or  application program interface

Indeed, being natural makes sense.
(otherwise we would have used numeric values instead)

- Linjie
Limin Wang May 6, 2020, 2:57 p.m. UTC | #5
On Wed, May 06, 2020 at 06:13:10PM +0800, mypopy@gmail.com wrote:
> On Wed, May 6, 2020 at 6:04 PM mypopy@gmail.com <mypopy@gmail.com> wrote:
> >
> > On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st> wrote:
> > >
> > > On Wed, 6 May 2020, Linjie Fu wrote:
> > >
> > > > Allows specifying avctx->profile with string type profile for
> > > > h264 encoders.
> > > >
> > > > Private field/option "profile" may be abled to be removed for basic
> > > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > > or with an map to hardware profile like h264_qsv.
> > > >
> > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > > ---
> > > > One concern is some encoders have options for specific profiles,
> > > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > > hence they may not be able to remove the private option easily.
> > > >
> > > > Please help to comment.
> > > >
> > > > libavcodec/options_table.h | 4 ++++
> > > > 1 file changed, 4 insertions(+)
> > >
> > > This change in itself looks sensible to me (but I'd like for someone else
> > > to comment as well). Even if it might not be able to get rid of the
> > > private option for all encoders, it should at least simplify the easiest
> > > cases.
> > >
> > > // Martin
> > I think we have a discussion about this case,
> > https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-c74b-b3cb-46fc5f3ea932@gmail.com/
> 
> I agree with Hendrik Leppkes, and I think "main" more natural than
> "h264_main"  for 264 codec profile from a user viewpoint, both of
> command line tools or  application program interface

So what's the conclusion? I have submit a patch to add mpeg2 profile
 52 and Marton prefer to use global profile with mpeg2_xxxx. By Hendrik's
comments, I think it's more reasonable to use private profile.

> _______________________________________________
> 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".
Fu, Linjie May 6, 2020, 3:46 p.m. UTC | #6
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
> lance.lmwang@gmail.com
> Sent: Wednesday, May 6, 2020 22:57
> To: ffmpeg-devel@ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic
> candidate h264 profiles
> 
> On Wed, May 06, 2020 at 06:13:10PM +0800, mypopy@gmail.com wrote:
> > On Wed, May 6, 2020 at 6:04 PM mypopy@gmail.com
> <mypopy@gmail.com> wrote:
> > >
> > > On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st>
> wrote:
> > > >
> > > > On Wed, 6 May 2020, Linjie Fu wrote:
> > > >
> > > > > Allows specifying avctx->profile with string type profile for
> > > > > h264 encoders.
> > > > >
> > > > > Private field/option "profile" may be abled to be removed for basic
> > > > > h264 profiles, directly for encoders like libopenh264/h264_vaapi,
> > > > > or with an map to hardware profile like h264_qsv.
> > > > >
> > > > > Signed-off-by: Linjie Fu <linjie.fu@intel.com>
> > > > > ---
> > > > > One concern is some encoders have options for specific profiles,
> > > > > like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
> > > > > hence they may not be able to remove the private option easily.
> > > > >
> > > > > Please help to comment.
> > > > >
> > > > > libavcodec/options_table.h | 4 ++++
> > > > > 1 file changed, 4 insertions(+)
> > > >
> > > > This change in itself looks sensible to me (but I'd like for someone else
> > > > to comment as well). Even if it might not be able to get rid of the
> > > > private option for all encoders, it should at least simplify the easiest
> > > > cases.
> > > >
> > > > // Martin
> > > I think we have a discussion about this case,
> > > https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-
> c74b-b3cb-46fc5f3ea932@gmail.com/
> >
> > I agree with Hendrik Leppkes, and I think "main" more natural than
> > "h264_main"  for 264 codec profile from a user viewpoint, both of
> > command line tools or  application program interface
> 
> So what's the conclusion? I have submit a patch to add mpeg2 profile
>  52 and Marton prefer to use global profile with mpeg2_xxxx. By Hendrik's
> comments, I think it's more reasonable to use private profile.

I'd also prefer an option without prefix which seems more natural, especially
for the codecs that already have implemented it as a private option. Otherwise
it seems to be a sudden break for users to use "h264_main"/"mepg2_main"
instead of "main" unless we declared some deprecated flags and get rid of
the private options later.

Please help to comment.

- Linjie
Marton Balint May 6, 2020, 6:19 p.m. UTC | #7
On Wed, 6 May 2020, Fu, Linjie wrote:

>> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of
>> lance.lmwang@gmail.com
>> Sent: Wednesday, May 6, 2020 22:57
>> To: ffmpeg-devel@ffmpeg.org
>> Subject: Re: [FFmpeg-devel] [PATCH, RFC] lavc/options_table: Add basic
>> candidate h264 profiles
>>
>> On Wed, May 06, 2020 at 06:13:10PM +0800, mypopy@gmail.com wrote:
>>> On Wed, May 6, 2020 at 6:04 PM mypopy@gmail.com
>> <mypopy@gmail.com> wrote:
>>>>
>>>> On Wed, May 6, 2020 at 5:40 PM Martin Storsjö <martin@martin.st>
>> wrote:
>>>>>
>>>>> On Wed, 6 May 2020, Linjie Fu wrote:
>>>>>
>>>>>> Allows specifying avctx->profile with string type profile for
>>>>>> h264 encoders.
>>>>>>
>>>>>> Private field/option "profile" may be abled to be removed for basic
>>>>>> h264 profiles, directly for encoders like libopenh264/h264_vaapi,
>>>>>> or with an map to hardware profile like h264_qsv.
>>>>>>
>>>>>> Signed-off-by: Linjie Fu <linjie.fu@intel.com>
>>>>>> ---
>>>>>> One concern is some encoders have options for specific profiles,
>>>>>> like "high444p" for nvenc_h264 and "constrained_high" for h264_amf,
>>>>>> hence they may not be able to remove the private option easily.
>>>>>>
>>>>>> Please help to comment.
>>>>>>
>>>>>> libavcodec/options_table.h | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> This change in itself looks sensible to me (but I'd like for someone else
>>>>> to comment as well). Even if it might not be able to get rid of the
>>>>> private option for all encoders, it should at least simplify the easiest
>>>>> cases.
>>>>>
>>>>> // Martin
>>>> I think we have a discussion about this case,
>>>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/fa14da65-9e1a-
>> c74b-b3cb-46fc5f3ea932@gmail.com/
>>>
>>> I agree with Hendrik Leppkes, and I think "main" more natural than
>>> "h264_main"  for 264 codec profile from a user viewpoint, both of
>>> command line tools or  application program interface
>>
>> So what's the conclusion? I have submit a patch to add mpeg2 profile
>>  52 and Marton prefer to use global profile with mpeg2_xxxx. By Hendrik's
>> comments, I think it's more reasonable to use private profile.
>
> I'd also prefer an option without prefix which seems more natural, especially
> for the codecs that already have implemented it as a private option. Otherwise
> it seems to be a sudden break for users to use "h264_main"/"mepg2_main"
> instead of "main" unless we declared some deprecated flags and get rid of
> the private options later.
>

I was against the private option because there is no clear path in what to 
support in the future.

Storing the profile in two places is problematic becuase priority between 
the two is not consistently enforced. So I guess for some codecs the 
private profile will have priority, for others, the avctx profile.

If libavcodec should store all profiles for a codec, can't we make the 
profile option a string and do the parsing later from code? This way we 
can eliminate the per-codec profile options and use the shorter options at 
the same time them having different meaning according to the used codec.

I can wrap up a patch if this seems like a viable way forward.

Thanks,
Marton
diff mbox series

Patch

diff --git a/libavcodec/options_table.h b/libavcodec/options_table.h
index 54366747ca..8d41f2755c 100644
--- a/libavcodec/options_table.h
+++ b/libavcodec/options_table.h
@@ -282,6 +282,10 @@  static const AVOption avcodec_options[] = {
 {"mpeg4_asp",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_MPEG4_ADVANCED_SIMPLE }, INT_MIN, INT_MAX, V|E, "profile"},
 {"main10",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_HEVC_MAIN_10 }, INT_MIN, INT_MAX, V|E, "profile"},
 {"msbc",  NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_SBC_MSBC }, INT_MIN, INT_MAX, A|E, "profile"},
+{"h264_constrained_baseline", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_CONSTRAINED_BASELINE }, INT_MIN, INT_MAX, V|E, "profile"},
+{"h264_baseline",             NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_BASELINE },             INT_MIN, INT_MAX, V|E, "profile"},
+{"h264_main",                 NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_MAIN },                 INT_MIN, INT_MAX, V|E, "profile"},
+{"h264_high",                 NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_PROFILE_H264_HIGH },                 INT_MIN, INT_MAX, V|E, "profile"},
 {"level", NULL, OFFSET(level), AV_OPT_TYPE_INT, {.i64 = FF_LEVEL_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "level"},
 {"unknown", NULL, 0, AV_OPT_TYPE_CONST, {.i64 = FF_LEVEL_UNKNOWN }, INT_MIN, INT_MAX, V|A|E, "level"},
 {"lowres", "decode at 1= 1/2, 2=1/4, 3=1/8 resolutions", OFFSET(lowres), AV_OPT_TYPE_INT, {.i64 = 0 }, 0, INT_MAX, V|A|D},