On Fri, 1 May 2020, lance.lmw...@gmail.com wrote:

On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:


On Fri, 1 May 2020, lance.lmw...@gmail.com wrote:

> On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmw...@gmail.com wrote:
> > From: Limin Wang <lance.lmw...@gmail.com>
> > > > Signed-off-by: Limin Wang <lance.lmw...@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

Reply via email to