[FFmpeg-devel] [RFC] encoder profile validation

[Marton Balint]

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.

>
>> 
>> So I'd rather not user AVCodec->profiles for this validation. I am 
>> thinking about two possible solutions:
>
> It seems preferable to me to fix the above issues and not have multiple
> fields with subtly different meanings that are just bound to cause
> confusion and bugs.

I agree that having the list of suppored profiles in two places is wrong, 
so in the long run, I would simply deprecate AVCodec->profiles, and use 
only the AVOptions to list the supported profiles. Also see my previous 
patchset which moves profile names directly to encoders:

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

So the supported profiles would be contained in one place, AVOptions.
Even querying the list should be doable, if needed, and 
av_get_profile_name can also be modified to get the profile name from the 
AVOptions.

Regards,
Marton