[FFmpeg-devel] [RFC] encoder profile validation

[Marton Balint]

On Wed, 20 May 2020, Anton Khirnov wrote:

> Quoting Marton Balint (2020-05-20 10:35:07)
>> 
>> 
>> On Wed, 20 May 2020, Anton Khirnov wrote:
>> 
>> > Quoting Marton Balint (2020-05-18 20:32:53)
>> >> 
>> >> 
>> >> On Mon, 18 May 2020, Anton Khirnov wrote:
>> >> 
>> >> > Quoting Marton Balint (2020-05-16 15:52:22)
>> >> >> Hi,
>> >> >> 
>> >> >> As you may know, a recent patchset enabled AVCodecContext->profile 
>> >> >> constants to reside in encoders.
>> >> >> 
>> >> >> In order to make a full transition to avctx->profile even in existing 
>> >> >> encoders which might use a private profile setting, we have to make sure 
>> >> >> only supported avctx->profile values are passed to encoders.
>> >> >> 
>> >> >> The fact that avctx->profile is not validated is already an issue, and 
>> >> >> assertions/segmentation faults can already happen in existing encoders 
>> >> >> (e.g.: aac, mpeg) if unsupported values are passed.
>> >> >> 
>> >> >> AVCodec have a .profiles attribute which supposed to contain the list of 
>> >> >> supported profiles. However this is problematic because
>> >> >> - AVCodecContext->profile is not validated against this list
>> >> >> - not all encoders define the list
>> >> >> - even if there is a list it might be defined as NULL_IF_CONFIG_SMALL
>> >> >> - some encoders support more or less than its currently defined list
>> >> >
>> >> > All of these sound like bugs that can and should be fixed.
>> >> >
>> >> >> - AVCodec->profiles contains AVProfiles which supposed to have a textual
>> >> >>    representation of each profile, which can cause profile name
>> >> >>    duplications/inconsistencies against libavcodec/profiles.c if the list
>> >> >>    is different to the one in codecs profile list.
>> >> >
>> >> > Why should it be different? Isn't that a bug that should be fixed?
>> >> 
>> >> The list can be different because possibly not all the profiles which are 
>> >> recognized by the decoder is supported by the encoder. Also the encoder 
>> >> can define pseudo profiles which it can support, for example the AAC 
>> >> encoders have support for FF_PROFILE_MPEG2_AAC_LOW which is LC profile 
>> >> with some features disabled.
>> >
>> > Don't understand - I would think AVCodec.profiles would list the
>> > profiles supported by that specific AVCodec instance. Is that not the
>> > case?
>> 
>> Yes, but you cannot share AVProfile lists among encoders or decoders of 
>> the same codec because they support different profiles.
>
> Sure, but how is that relevant to the issue in question?
> Every decoder or encoder would just have its own profiles list. Am I
> missing something?

You asked that why the AVProfiles list of the encoders should be different 
to the AVProfile lists stored in libavcodec/profiles.c. Or at least that's 
how I understood your question...

Anyway, I'd still argue against the usage of AVCodec->profiles 
and AVProfiles in general. Because in an encoder we'd have to list the 
supported profiles as AVOptions in order to be able to specify the profile 
names:

(See my previous patchset [1] for the macro)

static const AVOption options[] = {
     FF_AVCTX_PROFILE_OPTION("dnxhd",     NULL, VIDEO, FF_PROFILE_DNXHD)
     FF_AVCTX_PROFILE_OPTION("dnxhr_444", NULL, VIDEO, FF_PROFILE_DNXHR_444)
     FF_AVCTX_PROFILE_OPTION("dnxhr_hqx", NULL, VIDEO, FF_PROFILE_DNXHR_HQX)
     FF_AVCTX_PROFILE_OPTION("dnxhr_hq",  NULL, VIDEO, FF_PROFILE_DNXHR_HQ)
     FF_AVCTX_PROFILE_OPTION("dnxhr_sq",  NULL, VIDEO, FF_PROFILE_DNXHR_SQ)
     FF_AVCTX_PROFILE_OPTION("dnxhr_lb",  NULL, VIDEO, FF_PROFILE_DNXHR_LB)
     { NULL }
}

And we'd also have to list the supported profiles as AVProfiles to specify 
the valid profiles.

const AVProfile profiles[] = {
   { FF_PROFILE_DNXHD,      "DNXHD"},
   { FF_PROFILE_DNXHR_LB,   "DNXHR LB"},
   { FF_PROFILE_DNXHR_SQ,   "DNXHR SQ"},
   { FF_PROFILE_DNXHR_HQ,   "DNXHR HQ" },
   { FF_PROFILE_DNXHR_HQX,  "DNXHR HQX"},
   { FF_PROFILE_DNXHR_444,  "DNXHR 444"},
   { FF_PROFILE_UNKNOWN },
};

So we'd have the list of supported profiles in an encoder two times which 
seems ugly and can become inconsistent. Also it seems cleaner to me to to do the 
profile validation via AVOptions, rather than hardcoding checking the 
profiles list in avcodec_open2.

Regards,
Marton

[1] https://patchwork.ffmpeg.org/project/ffmpeg/list/?series=1193