diff mbox series

[FFmpeg-devel,v3,2/2] avcodec/mpeg12enc: Support mpeg2 encoder profile with const options

Message ID 20200404002531.32552-1-lance.lmwang@gmail.com
State New
Headers show
Series Untitled series #753
Related show

Commit Message

Limin Wang April 4, 2020, 12:25 a.m. UTC
From: Limin Wang <lance.lmwang@gmail.com>

Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
---
 doc/encoders.texi      |  8 ++++++++
 libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
 libavcodec/mpegvideo.h |  1 +
 3 files changed, 26 insertions(+), 5 deletions(-)

Comments

Limin Wang April 17, 2020, 2:16 p.m. UTC | #1
ping for merge, please.

On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  doc/encoders.texi      |  8 ++++++++
>  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
>  libavcodec/mpegvideo.h |  1 +
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index e23b6b3..5022b94 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
>  @item a53cc @var{boolean}
>  Import closed captions (which must be ATSC compatible format) into output.
>  Default is 1 (on).
> +@item profile @var{integer}
> +Select the mpeg2 profile to encode, possible values:
> +@table @samp
> +@item 422
> +@item high
> +@item main
> +@item simple
> +@end table
>  @end table
>  
>  @section png
> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> index 643ba81..9fe0c8b 100644
> --- a/libavcodec/mpeg12enc.c
> +++ b/libavcodec/mpeg12enc.c
> @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
>          }
>      }
>  
> -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
> +    if (s->profile == FF_PROFILE_UNKNOWN)
> +        s->profile = avctx->profile;
> +
> +    if (s->profile == FF_PROFILE_UNKNOWN) {
>          if (avctx->level != FF_LEVEL_UNKNOWN) {
>              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
>              return -1;
>          }
>          /* Main or 4:2:2 */
>          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
> +        return -1;
>      }
>  
>      if (avctx->level == FF_LEVEL_UNKNOWN) {
> -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>              if (avctx->width <= 720 && avctx->height <= 608)
>                  avctx->level = 5;                   /* Main */
>              else
>                  avctx->level = 2;                   /* High */
>          } else {
> -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>                  av_log(avctx, AV_LOG_ERROR,
>                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
>                  return -1;
> @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
>              put_header(s, EXT_START_CODE);
>              put_bits(&s->pb, 4, 1);                 // seq ext
>  
> -            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>  
> -            put_bits(&s->pb, 3, s->avctx->profile); // profile
> +            put_bits(&s->pb, 3, s->profile); // profile
>              put_bits(&s->pb, 4, s->avctx->level);   // level
>  
>              put_bits(&s->pb, 1, s->progressive_sequence);
> @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
>      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
>      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
>      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
> +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
> +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
> +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
> +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
> +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
>      FF_MPV_COMMON_OPTS
>      { NULL },
>  };
> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> index 29e692f..cee423e 100644
> --- a/libavcodec/mpegvideo.h
> +++ b/libavcodec/mpegvideo.h
> @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
>      int progressive_sequence;
>      int mpeg_f_code[2][2];
>      int a53_cc;
> +    int profile;
>  
>      // picture structure defines are loaded from mpegutils.h
>      int picture_structure;
> -- 
> 2.9.5
>
Limin Wang May 1, 2020, 9:57 a.m. UTC | #2
On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
> From: Limin Wang <lance.lmwang@gmail.com>
> 
> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> ---
>  doc/encoders.texi      |  8 ++++++++
>  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
>  libavcodec/mpegvideo.h |  1 +
>  3 files changed, 26 insertions(+), 5 deletions(-)
> 
> diff --git a/doc/encoders.texi b/doc/encoders.texi
> index e23b6b3..5022b94 100644
> --- a/doc/encoders.texi
> +++ b/doc/encoders.texi
> @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
>  @item a53cc @var{boolean}
>  Import closed captions (which must be ATSC compatible format) into output.
>  Default is 1 (on).
> +@item profile @var{integer}
> +Select the mpeg2 profile to encode, possible values:
> +@table @samp
> +@item 422
> +@item high
> +@item main
> +@item simple
> +@end table
>  @end table
>  
>  @section png
> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> index 643ba81..9fe0c8b 100644
> --- a/libavcodec/mpeg12enc.c
> +++ b/libavcodec/mpeg12enc.c
> @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
>          }
>      }
>  
> -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
> +    if (s->profile == FF_PROFILE_UNKNOWN)
> +        s->profile = avctx->profile;
> +
> +    if (s->profile == FF_PROFILE_UNKNOWN) {
>          if (avctx->level != FF_LEVEL_UNKNOWN) {
>              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
>              return -1;
>          }
>          /* Main or 4:2:2 */
>          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
> +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
> +        return -1;
>      }
>  
>      if (avctx->level == FF_LEVEL_UNKNOWN) {
> -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>              if (avctx->width <= 720 && avctx->height <= 608)
>                  avctx->level = 5;                   /* Main */
>              else
>                  avctx->level = 2;                   /* High */
>          } else {
> -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>                  av_log(avctx, AV_LOG_ERROR,
>                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
>                  return -1;
> @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
>              put_header(s, EXT_START_CODE);
>              put_bits(&s->pb, 4, 1);                 // seq ext
>  
> -            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>  
> -            put_bits(&s->pb, 3, s->avctx->profile); // profile
> +            put_bits(&s->pb, 3, s->profile); // profile
>              put_bits(&s->pb, 4, s->avctx->level);   // level
>  
>              put_bits(&s->pb, 1, s->progressive_sequence);
> @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
>      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
>      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
>      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
> +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
> +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
> +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
> +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
> +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
>      FF_MPV_COMMON_OPTS
>      { NULL },
>  };
> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> index 29e692f..cee423e 100644
> --- a/libavcodec/mpegvideo.h
> +++ b/libavcodec/mpegvideo.h
> @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
>      int progressive_sequence;
>      int mpeg_f_code[2][2];
>      int a53_cc;
> +    int profile;
>  
>      // picture structure defines are loaded from mpegutils.h
>      int picture_structure;
> -- 
> 2.9.5
> 

will apply the patch set.
Marton Balint May 1, 2020, 10:32 a.m. UTC | #3
On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:

> On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
>> From: Limin Wang <lance.lmwang@gmail.com>
>> 
>> Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> ---
>>  doc/encoders.texi      |  8 ++++++++
>>  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
>>  libavcodec/mpegvideo.h |  1 +
>>  3 files changed, 26 insertions(+), 5 deletions(-)
>> 
>> diff --git a/doc/encoders.texi b/doc/encoders.texi
>> index e23b6b3..5022b94 100644
>> --- a/doc/encoders.texi
>> +++ b/doc/encoders.texi
>> @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
>>  @item a53cc @var{boolean}
>>  Import closed captions (which must be ATSC compatible format) into output.
>>  Default is 1 (on).
>> +@item profile @var{integer}
>> +Select the mpeg2 profile to encode, possible values:
>> +@table @samp
>> +@item 422
>> +@item high
>> +@item main
>> +@item simple
>> +@end table
>>  @end table
>>
>>  @section png
>> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
>> index 643ba81..9fe0c8b 100644
>> --- a/libavcodec/mpeg12enc.c
>> +++ b/libavcodec/mpeg12enc.c
>> @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
>>          }
>>      }
>> 
>> -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
>> +    if (s->profile == FF_PROFILE_UNKNOWN)
>> +        s->profile = avctx->profile;
>> +
>> +    if (s->profile == FF_PROFILE_UNKNOWN) {
>>          if (avctx->level != FF_LEVEL_UNKNOWN) {
>>              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
>>              return -1;
>>          }
>>          /* Main or 4:2:2 */
>>          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
>> +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
>> +        return -1;
>>      }
>>
>>      if (avctx->level == FF_LEVEL_UNKNOWN) {
>> -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>> +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>>              if (avctx->width <= 720 && avctx->height <= 608)
>>                  avctx->level = 5;                   /* Main */
>>              else
>>                  avctx->level = 2;                   /* High */
>>          } else {
>> -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>> +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>>                  av_log(avctx, AV_LOG_ERROR,
>>                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
>>                  return -1;
>> @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
>>              put_header(s, EXT_START_CODE);
>>              put_bits(&s->pb, 4, 1);                 // seq ext
>> 
>> -            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> 
>> -            put_bits(&s->pb, 3, s->avctx->profile); // profile
>> +            put_bits(&s->pb, 3, s->profile); // profile
>>              put_bits(&s->pb, 4, s->avctx->level);   // level
>>
>>              put_bits(&s->pb, 1, s->progressive_sequence);
>> @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
>>      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
>>      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
>>      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
>> +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
>> +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
>> +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
>> +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
>> +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
>>      FF_MPV_COMMON_OPTS
>>      { NULL },
>>  };
>> diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
>> index 29e692f..cee423e 100644
>> --- a/libavcodec/mpegvideo.h
>> +++ b/libavcodec/mpegvideo.h
>> @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
>>      int progressive_sequence;
>>      int mpeg_f_code[2][2];
>>      int a53_cc;
>> +    int profile;
>>
>>      // picture structure defines are loaded from mpegutils.h
>>      int picture_structure;
>> -- 
>> 2.9.5
>> 
>
> will apply the patch set.

Hold on, this patch does not seem right. Is 422 as a named constant even 
works?

Regards,
Marton
Limin Wang May 1, 2020, 11:04 a.m. UTC | #4
On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:
> 
> 
> On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
> > > From: Limin Wang <lance.lmwang@gmail.com>
> > > 
> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > ---
> > >  doc/encoders.texi      |  8 ++++++++
> > >  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
> > >  libavcodec/mpegvideo.h |  1 +
> > >  3 files changed, 26 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > > index e23b6b3..5022b94 100644
> > > --- a/doc/encoders.texi
> > > +++ b/doc/encoders.texi
> > > @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
> > >  @item a53cc @var{boolean}
> > >  Import closed captions (which must be ATSC compatible format) into output.
> > >  Default is 1 (on).
> > > +@item profile @var{integer}
> > > +Select the mpeg2 profile to encode, possible values:
> > > +@table @samp
> > > +@item 422
> > > +@item high
> > > +@item main
> > > +@item simple
> > > +@end table
> > >  @end table
> > > 
> > >  @section png
> > > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> > > index 643ba81..9fe0c8b 100644
> > > --- a/libavcodec/mpeg12enc.c
> > > +++ b/libavcodec/mpeg12enc.c
> > > @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
> > >          }
> > >      }
> > > 
> > > -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
> > > +    if (s->profile == FF_PROFILE_UNKNOWN)
> > > +        s->profile = avctx->profile;
> > > +
> > > +    if (s->profile == FF_PROFILE_UNKNOWN) {
> > >          if (avctx->level != FF_LEVEL_UNKNOWN) {
> > >              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
> > >              return -1;
> > >          }
> > >          /* Main or 4:2:2 */
> > >          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> > > +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> > > +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
> > > +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
> > > +        return -1;
> > >      }
> > > 
> > >      if (avctx->level == FF_LEVEL_UNKNOWN) {
> > > -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> > > +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> > >              if (avctx->width <= 720 && avctx->height <= 608)
> > >                  avctx->level = 5;                   /* Main */
> > >              else
> > >                  avctx->level = 2;                   /* High */
> > >          } else {
> > > -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> > > +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> > >                  av_log(avctx, AV_LOG_ERROR,
> > >                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
> > >                  return -1;
> > > @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
> > >              put_header(s, EXT_START_CODE);
> > >              put_bits(&s->pb, 4, 1);                 // seq ext
> > > 
> > > -            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> > > +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> > > 
> > > -            put_bits(&s->pb, 3, s->avctx->profile); // profile
> > > +            put_bits(&s->pb, 3, s->profile); // profile
> > >              put_bits(&s->pb, 4, s->avctx->level);   // level
> > > 
> > >              put_bits(&s->pb, 1, s->progressive_sequence);
> > > @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
> > >      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
> > >      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
> > >      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
> > > +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
> > > +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
> > > +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
> > > +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
> > > +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
> > >      FF_MPV_COMMON_OPTS
> > >      { NULL },
> > >  };
> > > diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> > > index 29e692f..cee423e 100644
> > > --- a/libavcodec/mpegvideo.h
> > > +++ b/libavcodec/mpegvideo.h
> > > @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
> > >      int progressive_sequence;
> > >      int mpeg_f_code[2][2];
> > >      int a53_cc;
> > > +    int profile;
> > > 
> > >      // picture structure defines are loaded from mpegutils.h
> > >      int picture_structure;
> > > -- 
> > > 2.9.5
> > > 
> > 
> > will apply the patch set.
> 
> Hold on, this patch does not seem right. Is 422 as a named constant even
> works?

No problem, I'm waiting for comments still, what's the problem for 422? Below is my testing result:

$ ./ffmpeg -y -f lavfi -i testsrc=duration=1  -c:v mpeg2video -profile:v 422 a.ts
$ mediainfo a.ts
...
Format                                   : MPEG Video
Format version                           : Version 2
Format profile                           : 4:2:2@Main
Format settings, BVOP                    : No
...


> 
> Regards,
> Marton
> _______________________________________________
> 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".
Marton Balint May 1, 2020, 1:22 p.m. UTC | #5
On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:

> On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:
>> 
>> 
>> On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
>> > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > 
>> > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > > ---
>> > >  doc/encoders.texi      |  8 ++++++++
>> > >  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
>> > >  libavcodec/mpegvideo.h |  1 +
>> > >  3 files changed, 26 insertions(+), 5 deletions(-)
>> > > 
>> > > diff --git a/doc/encoders.texi b/doc/encoders.texi
>> > > index e23b6b3..5022b94 100644
>> > > --- a/doc/encoders.texi
>> > > +++ b/doc/encoders.texi
>> > > @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
>> > >  @item a53cc @var{boolean}
>> > >  Import closed captions (which must be ATSC compatible format) into output.
>> > >  Default is 1 (on).
>> > > +@item profile @var{integer}
>> > > +Select the mpeg2 profile to encode, possible values:
>> > > +@table @samp
>> > > +@item 422
>> > > +@item high
>> > > +@item main
>> > > +@item simple
>> > > +@end table
>> > >  @end table
>> > > 
>> > >  @section png
>> > > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
>> > > index 643ba81..9fe0c8b 100644
>> > > --- a/libavcodec/mpeg12enc.c
>> > > +++ b/libavcodec/mpeg12enc.c
>> > > @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
>> > >          }
>> > >      }
>> > > 
>> > > -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
>> > > +    if (s->profile == FF_PROFILE_UNKNOWN)
>> > > +        s->profile = avctx->profile;
>> > > +
>> > > +    if (s->profile == FF_PROFILE_UNKNOWN) {
>> > >          if (avctx->level != FF_LEVEL_UNKNOWN) {
>> > >              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
>> > >              return -1;
>> > >          }
>> > >          /* Main or 4:2:2 */
>> > >          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> > > +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> > > +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
>> > > +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
>> > > +        return -1;
>> > >      }
>> > > 
>> > >      if (avctx->level == FF_LEVEL_UNKNOWN) {
>> > > -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>> > > +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>> > >              if (avctx->width <= 720 && avctx->height <= 608)
>> > >                  avctx->level = 5;                   /* Main */
>> > >              else
>> > >                  avctx->level = 2;                   /* High */
>> > >          } else {
>> > > -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>> > > +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>> > >                  av_log(avctx, AV_LOG_ERROR,
>> > >                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
>> > >                  return -1;
>> > > @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
>> > >              put_header(s, EXT_START_CODE);
>> > >              put_bits(&s->pb, 4, 1);                 // seq ext
>> > > 
>> > > -            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> > > +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> > > 
>> > > -            put_bits(&s->pb, 3, s->avctx->profile); // profile
>> > > +            put_bits(&s->pb, 3, s->profile); // profile
>> > >              put_bits(&s->pb, 4, s->avctx->level);   // level
>> > > 
>> > >              put_bits(&s->pb, 1, s->progressive_sequence);
>> > > @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
>> > >      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
>> > >      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
>> > >      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
>> > > +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
>> > > +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
>> > > +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
>> > > +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
>> > > +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
>> > >      FF_MPV_COMMON_OPTS
>> > >      { NULL },
>> > >  };
>> > > diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
>> > > index 29e692f..cee423e 100644
>> > > --- a/libavcodec/mpegvideo.h
>> > > +++ b/libavcodec/mpegvideo.h
>> > > @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
>> > >      int progressive_sequence;
>> > >      int mpeg_f_code[2][2];
>> > >      int a53_cc;
>> > > +    int profile;
>> > > 
>> > >      // picture structure defines are loaded from mpegutils.h
>> > >      int picture_structure;
>> > > -- 
>> > > 2.9.5
>> > > 
>> > 
>> > will apply the patch set.
>> 
>> Hold on, this patch does not seem right. Is 422 as a named constant even
>> works?
>
> No problem, I'm waiting for comments still, what's the problem for 422? Below is my testing result:
>
> $ ./ffmpeg -y -f lavfi -i testsrc=duration=1  -c:v mpeg2video -profile:v 422 a.ts
> $ mediainfo a.ts
> ...
> Format                                   : MPEG Video
> Format version                           : Version 2
> Format profile                           : 4:2:2@Main
> Format settings, BVOP                    : No
> ...

Okay, it seems to work yet I think it is not very good practice to use 
constants which can be also parsed as integers.

In general about the patch. Is it decided that we should move away from 
using avctx->profile and use codec specific profiles? Because there are 
quite a number of available profiles which simply use avctx for other 
codecs.

Also it would be inconsistent if for some codecs avctx->profile had 
priority, where for other codecs, the priv_context->profile. DNXHD comes 
to mind.

So until these are decided, I'd rather not add local profile options and I 
think it is better to add the named constants with some prefix (mpeg2) to 
libavcodec/options_table.c, to follow the existing pattern.

On a side note, the patch tries to reject invalid profiles, but does not 
seem to do a very good job. MPEG2 profiles can only be 0..7, nothing else 
is allowed. I wonder if generic code should reject invalid 
avctx->profiles? After all, it is supposed to know the list of the allowed 
codec profiles, but maybe for some formats limiting avctx->profile to the 
list is too limiting?

Regards,
Marton
Limin Wang May 1, 2020, 2:55 p.m. UTC | #6
On Fri, May 01, 2020 at 03:22:32PM +0200, Marton Balint wrote:
> 
> 
> On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
> 
> > On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:
> > > 
> > > 
> > > On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
> > > 
> > > > On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
> > > > > From: Limin Wang <lance.lmwang@gmail.com>
> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
> > > > > ---
> > > > >  doc/encoders.texi      |  8 ++++++++
> > > > >  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
> > > > >  libavcodec/mpegvideo.h |  1 +
> > > > >  3 files changed, 26 insertions(+), 5 deletions(-)
> > > > > > > diff --git a/doc/encoders.texi b/doc/encoders.texi
> > > > > index e23b6b3..5022b94 100644
> > > > > --- a/doc/encoders.texi
> > > > > +++ b/doc/encoders.texi
> > > > > @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
> > > > >  @item a53cc @var{boolean}
> > > > >  Import closed captions (which must be ATSC compatible format) into output.
> > > > >  Default is 1 (on).
> > > > > +@item profile @var{integer}
> > > > > +Select the mpeg2 profile to encode, possible values:
> > > > > +@table @samp
> > > > > +@item 422
> > > > > +@item high
> > > > > +@item main
> > > > > +@item simple
> > > > > +@end table
> > > > >  @end table
> > > > > > >  @section png
> > > > > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> > > > > index 643ba81..9fe0c8b 100644
> > > > > --- a/libavcodec/mpeg12enc.c
> > > > > +++ b/libavcodec/mpeg12enc.c
> > > > > @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
> > > > >          }
> > > > >      }
> > > > > > > -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
> > > > > +    if (s->profile == FF_PROFILE_UNKNOWN)
> > > > > +        s->profile = avctx->profile;
> > > > > +
> > > > > +    if (s->profile == FF_PROFILE_UNKNOWN) {
> > > > >          if (avctx->level != FF_LEVEL_UNKNOWN) {
> > > > >              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
> > > > >              return -1;
> > > > >          }
> > > > >          /* Main or 4:2:2 */
> > > > >          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> > > > > +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
> > > > > +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
> > > > > +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
> > > > > +        return -1;
> > > > >      }
> > > > > > >      if (avctx->level == FF_LEVEL_UNKNOWN) {
> > > > > -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> > > > > +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
> > > > >              if (avctx->width <= 720 && avctx->height <= 608)
> > > > >                  avctx->level = 5;                   /* Main */
> > > > >              else
> > > > >                  avctx->level = 2;                   /* High */
> > > > >          } else {
> > > > > -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> > > > > +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
> > > > >                  av_log(avctx, AV_LOG_ERROR,
> > > > >                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
> > > > >                  return -1;
> > > > > @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
> > > > >              put_header(s, EXT_START_CODE);
> > > > >              put_bits(&s->pb, 4, 1);                 // seq ext
> > > > > > > -            put_bits(&s->pb, 1, s->avctx->profile ==
> > > FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> > > > > +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
> > > > > > > -            put_bits(&s->pb, 3, s->avctx->profile); //
> > > profile
> > > > > +            put_bits(&s->pb, 3, s->profile); // profile
> > > > >              put_bits(&s->pb, 4, s->avctx->level);   // level
> > > > > > >              put_bits(&s->pb, 1, s->progressive_sequence);
> > > > > @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
> > > > >      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
> > > > >      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
> > > > >      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
> > > > > +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
> > > > > +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
> > > > > +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
> > > > > +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
> > > > > +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
> > > > >      FF_MPV_COMMON_OPTS
> > > > >      { NULL },
> > > > >  };
> > > > > diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
> > > > > index 29e692f..cee423e 100644
> > > > > --- a/libavcodec/mpegvideo.h
> > > > > +++ b/libavcodec/mpegvideo.h
> > > > > @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
> > > > >      int progressive_sequence;
> > > > >      int mpeg_f_code[2][2];
> > > > >      int a53_cc;
> > > > > +    int profile;
> > > > > > >      // picture structure defines are loaded from
> > > mpegutils.h
> > > > >      int picture_structure;
> > > > > -- > > 2.9.5
> > > > > > > will apply the patch set.
> > > 
> > > Hold on, this patch does not seem right. Is 422 as a named constant even
> > > works?
> > 
> > No problem, I'm waiting for comments still, what's the problem for 422? Below is my testing result:
> > 
> > $ ./ffmpeg -y -f lavfi -i testsrc=duration=1  -c:v mpeg2video -profile:v 422 a.ts
> > $ mediainfo a.ts
> > ...
> > Format                                   : MPEG Video
> > Format version                           : Version 2
> > Format profile                           : 4:2:2@Main
> > Format settings, BVOP                    : No
> > ...
> 
> Okay, it seems to work yet I think it is not very good practice to use
> constants which can be also parsed as integers.

Yeah, now I understand your question for the 422 now.

> 
> In general about the patch. Is it decided that we should move away from
> using avctx->profile and use codec specific profiles? Because there are
> quite a number of available profiles which simply use avctx for other
> codecs.
> 
> Also it would be inconsistent if for some codecs avctx->profile had
> priority, where for other codecs, the priv_context->profile. DNXHD comes to
> mind.

At first, I have considered to add mpeg2_xxx in libavcodec/options_table.c,
like the main10 is used by HEVC, so I think it's misleading to add mpeg2_main10.
Then I notice x264 and some other code are using private profile for this, so
I try to follow the same way.

> 
> So until these are decided, I'd rather not add local profile options and I
> think it is better to add the named constants with some prefix (mpeg2) to
> libavcodec/options_table.c, to follow the existing pattern.

As you prefer to change in global, I will update the patch to use global way 
with prefix if no other developer's comments.

> 
> On a side note, the patch tries to reject invalid profiles, but does not
> seem to do a very good job. MPEG2 profiles can only be 0..7, nothing else is
> allowed. I wonder if generic code should reject invalid avctx->profiles?
> After all, it is supposed to know the list of the allowed codec profiles,
> but maybe for some formats limiting avctx->profile to the list is too
> limiting?

Yes, with codecs profile, we can check the valid of profile, but we can't check
it with global profile. 

> 
> Regards,
> Marton
> _______________________________________________
> 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".
Marton Balint May 1, 2020, 3:02 p.m. UTC | #7
On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:

> On Fri, May 01, 2020 at 03:22:32PM +0200, Marton Balint wrote:
>> 
>> 
>> On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
>> 
>> > On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:
>> > > 
>> > > 
>> > > On Fri, 1 May 2020, lance.lmwang@gmail.com wrote:
>> > > 
>> > > > On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang@gmail.com wrote:
>> > > > > From: Limin Wang <lance.lmwang@gmail.com>
>> > > > > > > Signed-off-by: Limin Wang <lance.lmwang@gmail.com>
>> > > > > ---
>> > > > >  doc/encoders.texi      |  8 ++++++++
>> > > > >  libavcodec/mpeg12enc.c | 22 +++++++++++++++++-----
>> > > > >  libavcodec/mpegvideo.h |  1 +
>> > > > >  3 files changed, 26 insertions(+), 5 deletions(-)
>> > > > > > > diff --git a/doc/encoders.texi b/doc/encoders.texi
>> > > > > index e23b6b3..5022b94 100644
>> > > > > --- a/doc/encoders.texi
>> > > > > +++ b/doc/encoders.texi
>> > > > > @@ -2753,6 +2753,14 @@ For maximum compatibility, use @samp{component}.
>> > > > >  @item a53cc @var{boolean}
>> > > > >  Import closed captions (which must be ATSC compatible format) into output.
>> > > > >  Default is 1 (on).
>> > > > > +@item profile @var{integer}
>> > > > > +Select the mpeg2 profile to encode, possible values:
>> > > > > +@table @samp
>> > > > > +@item 422
>> > > > > +@item high
>> > > > > +@item main
>> > > > > +@item simple
>> > > > > +@end table
>> > > > >  @end table
>> > > > > > >  @section png
>> > > > > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
>> > > > > index 643ba81..9fe0c8b 100644
>> > > > > --- a/libavcodec/mpeg12enc.c
>> > > > > +++ b/libavcodec/mpeg12enc.c
>> > > > > @@ -156,23 +156,30 @@ static av_cold int encode_init(AVCodecContext *avctx)
>> > > > >          }
>> > > > >      }
>> > > > > > > -    if (avctx->profile == FF_PROFILE_UNKNOWN) {
>> > > > > +    if (s->profile == FF_PROFILE_UNKNOWN)
>> > > > > +        s->profile = avctx->profile;
>> > > > > +
>> > > > > +    if (s->profile == FF_PROFILE_UNKNOWN) {
>> > > > >          if (avctx->level != FF_LEVEL_UNKNOWN) {
>> > > > >              av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
>> > > > >              return -1;
>> > > > >          }
>> > > > >          /* Main or 4:2:2 */
>> > > > >          avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> > > > > +        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
>> > > > > +    } else if (s->profile < FF_PROFILE_MPEG2_422) {
>> > > > > +        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
>> > > > > +        return -1;
>> > > > >      }
>> > > > > > >      if (avctx->level == FF_LEVEL_UNKNOWN) {
>> > > > > -        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>> > > > > +        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
>> > > > >              if (avctx->width <= 720 && avctx->height <= 608)
>> > > > >                  avctx->level = 5;                   /* Main */
>> > > > >              else
>> > > > >                  avctx->level = 2;                   /* High */
>> > > > >          } else {
>> > > > > -            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>> > > > > +            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
>> > > > >                  av_log(avctx, AV_LOG_ERROR,
>> > > > >                         "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
>> > > > >                  return -1;
>> > > > > @@ -321,9 +328,9 @@ static void mpeg1_encode_sequence_header(MpegEncContext *s)
>> > > > >              put_header(s, EXT_START_CODE);
>> > > > >              put_bits(&s->pb, 4, 1);                 // seq ext
>> > > > > > > -            put_bits(&s->pb, 1, s->avctx->profile ==
>> > > FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> > > > > +            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
>> > > > > > > -            put_bits(&s->pb, 3, s->avctx->profile); //
>> > > profile
>> > > > > +            put_bits(&s->pb, 3, s->profile); // profile
>> > > > >              put_bits(&s->pb, 4, s->avctx->level);   // level
>> > > > > > >              put_bits(&s->pb, 1, s->progressive_sequence);
>> > > > > @@ -1165,6 +1172,11 @@ static const AVOption mpeg2_options[] = {
>> > > > >      {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
>> > > > >      {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
>> > > > >      {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
>> > > > > +    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
>> > > > > +    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
>> > > > > +    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
>> > > > > +    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
>> > > > > +    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
>> > > > >      FF_MPV_COMMON_OPTS
>> > > > >      { NULL },
>> > > > >  };
>> > > > > diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
>> > > > > index 29e692f..cee423e 100644
>> > > > > --- a/libavcodec/mpegvideo.h
>> > > > > +++ b/libavcodec/mpegvideo.h
>> > > > > @@ -456,6 +456,7 @@ typedef struct MpegEncContext {
>> > > > >      int progressive_sequence;
>> > > > >      int mpeg_f_code[2][2];
>> > > > >      int a53_cc;
>> > > > > +    int profile;
>> > > > > > >      // picture structure defines are loaded from
>> > > mpegutils.h
>> > > > >      int picture_structure;
>> > > > > -- > > 2.9.5
>> > > > > > > will apply the patch set.
>> > > 
>> > > Hold on, this patch does not seem right. Is 422 as a named constant even
>> > > works?
>> > 
>> > No problem, I'm waiting for comments still, what's the problem for 422? Below is my testing result:
>> > 
>> > $ ./ffmpeg -y -f lavfi -i testsrc=duration=1  -c:v mpeg2video -profile:v 422 a.ts
>> > $ mediainfo a.ts
>> > ...
>> > Format                                   : MPEG Video
>> > Format version                           : Version 2
>> > Format profile                           : 4:2:2@Main
>> > Format settings, BVOP                    : No
>> > ...
>> 
>> Okay, it seems to work yet I think it is not very good practice to use
>> constants which can be also parsed as integers.
>
> Yeah, now I understand your question for the 422 now.
>
>> 
>> In general about the patch. Is it decided that we should move away from
>> using avctx->profile and use codec specific profiles? Because there are
>> quite a number of available profiles which simply use avctx for other
>> codecs.
>> 
>> Also it would be inconsistent if for some codecs avctx->profile had
>> priority, where for other codecs, the priv_context->profile. DNXHD comes to
>> mind.
>
> At first, I have considered to add mpeg2_xxx in libavcodec/options_table.c,
> like the main10 is used by HEVC, so I think it's misleading to add mpeg2_main10.
> Then I notice x264 and some other code are using private profile for this, so
> I try to follow the same way.
>
>> 
>> So until these are decided, I'd rather not add local profile options and I
>> think it is better to add the named constants with some prefix (mpeg2) to
>> libavcodec/options_table.c, to follow the existing pattern.
>
> As you prefer to change in global, I will update the patch to use global way 
> with prefix if no other developer's comments.
>
>> 
>> On a side note, the patch tries to reject invalid profiles, but does not
>> seem to do a very good job. MPEG2 profiles can only be 0..7, nothing else is
>> allowed. I wonder if generic code should reject invalid avctx->profiles?
>> After all, it is supposed to know the list of the allowed codec profiles,
>> but maybe for some formats limiting avctx->profile to the list is too
>> limiting?
>
> Yes, with codecs profile, we can check the valid of profile, but we can't check
> it with global profile.

I mean even for global avctx->profile the generic could could check if the 
value is in AVCodecDescriptor->profiles. This is not done at the moment.

Regards,
Marton
diff mbox series

Patch

diff --git a/doc/encoders.texi b/doc/encoders.texi
index e23b6b3..5022b94 100644
--- a/doc/encoders.texi
+++ b/doc/encoders.texi
@@ -2753,6 +2753,14 @@  For maximum compatibility, use @samp{component}.
 @item a53cc @var{boolean}
 Import closed captions (which must be ATSC compatible format) into output.
 Default is 1 (on).
+@item profile @var{integer}
+Select the mpeg2 profile to encode, possible values:
+@table @samp
+@item 422
+@item high
+@item main
+@item simple
+@end table
 @end table
 
 @section png
diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
index 643ba81..9fe0c8b 100644
--- a/libavcodec/mpeg12enc.c
+++ b/libavcodec/mpeg12enc.c
@@ -156,23 +156,30 @@  static av_cold int encode_init(AVCodecContext *avctx)
         }
     }
 
-    if (avctx->profile == FF_PROFILE_UNKNOWN) {
+    if (s->profile == FF_PROFILE_UNKNOWN)
+        s->profile = avctx->profile;
+
+    if (s->profile == FF_PROFILE_UNKNOWN) {
         if (avctx->level != FF_LEVEL_UNKNOWN) {
             av_log(avctx, AV_LOG_ERROR, "Set profile and level\n");
             return -1;
         }
         /* Main or 4:2:2 */
         avctx->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
+        s->profile = s->chroma_format == CHROMA_420 ? FF_PROFILE_MPEG2_MAIN : FF_PROFILE_MPEG2_422;
+    } else if (s->profile < FF_PROFILE_MPEG2_422) {
+        av_log(avctx, AV_LOG_ERROR, "Invalid mpeg2 profile set\n");
+        return -1;
     }
 
     if (avctx->level == FF_LEVEL_UNKNOWN) {
-        if (avctx->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
+        if (s->profile == FF_PROFILE_MPEG2_422) {   /* 4:2:2 */
             if (avctx->width <= 720 && avctx->height <= 608)
                 avctx->level = 5;                   /* Main */
             else
                 avctx->level = 2;                   /* High */
         } else {
-            if (avctx->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
+            if (s->profile != FF_PROFILE_MPEG2_HIGH && s->chroma_format != CHROMA_420) {
                 av_log(avctx, AV_LOG_ERROR,
                        "Only High(1) and 4:2:2(0) profiles support 4:2:2 color sampling\n");
                 return -1;
@@ -321,9 +328,9 @@  static void mpeg1_encode_sequence_header(MpegEncContext *s)
             put_header(s, EXT_START_CODE);
             put_bits(&s->pb, 4, 1);                 // seq ext
 
-            put_bits(&s->pb, 1, s->avctx->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
+            put_bits(&s->pb, 1, s->profile == FF_PROFILE_MPEG2_422); // escx 1 for 4:2:2 profile
 
-            put_bits(&s->pb, 3, s->avctx->profile); // profile
+            put_bits(&s->pb, 3, s->profile); // profile
             put_bits(&s->pb, 4, s->avctx->level);   // level
 
             put_bits(&s->pb, 1, s->progressive_sequence);
@@ -1165,6 +1172,11 @@  static const AVOption mpeg2_options[] = {
     {     "secam",        NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_SECAM      },  0, 0, VE, "video_format" },
     {     "mac",          NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_MAC        },  0, 0, VE, "video_format" },
     {     "unspecified",  NULL, 0, AV_OPT_TYPE_CONST,  {.i64 = VIDEO_FORMAT_UNSPECIFIED},  0, 0, VE, "video_format" },
+    { "profile",          "Set the profile",  OFFSET(profile),   AV_OPT_TYPE_INT,{ .i64 = FF_PROFILE_UNKNOWN }, FF_PROFILE_UNKNOWN, FF_PROFILE_MPEG2_SIMPLE, VE, "profile" },
+    {     "422",          "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_422     }, 0, 0, VE, "profile" },
+    {     "high",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_HIGH    }, 0, 0, VE, "profile" },
+    {     "main",         "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_MAIN    }, 0, 0, VE, "profile" },
+    {     "simple",       "",   0, AV_OPT_TYPE_CONST,{ .i64 = FF_PROFILE_MPEG2_SIMPLE  }, 0, 0, VE, "profile" },
     FF_MPV_COMMON_OPTS
     { NULL },
 };
diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h
index 29e692f..cee423e 100644
--- a/libavcodec/mpegvideo.h
+++ b/libavcodec/mpegvideo.h
@@ -456,6 +456,7 @@  typedef struct MpegEncContext {
     int progressive_sequence;
     int mpeg_f_code[2][2];
     int a53_cc;
+    int profile;
 
     // picture structure defines are loaded from mpegutils.h
     int picture_structure;