[FFmpeg-devel] [PATCH v3 2/2] avcodec/mpeg12enc: Support mpeg2 encoder profile with const options
Marton Balint
cus at passwd.hu
Fri May 1 16:22:32 EEST 2020
On Fri, 1 May 2020, lance.lmwang at gmail.com wrote:
> On Fri, May 01, 2020 at 12:32:44PM +0200, Marton Balint wrote:
>>
>>
>> On Fri, 1 May 2020, lance.lmwang at gmail.com wrote:
>>
>> > On Sat, Apr 04, 2020 at 08:25:31AM +0800, lance.lmwang at gmail.com wrote:
>> > > From: Limin Wang <lance.lmwang at gmail.com>
>> > >
>> > > Signed-off-by: Limin Wang <lance.lmwang at 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).
>> > > + at item profile @var{integer}
>> > > +Select the mpeg2 profile to encode, possible values:
>> > > + at table @samp
>> > > + at item 422
>> > > + at item high
>> > > + at item main
>> > > + at item simple
>> > > + at 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 at 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
More information about the ffmpeg-devel
mailing list