[FFmpeg-devel] [RFC] encoder profile validation

[Marton Balint]

On Wed, 20 May 2020, mypopy at gmail.com wrote:

> On Tue, May 19, 2020 at 2:33 AM Marton Balint <cus at passwd.hu> wrote:
>>
>>
>>
>> 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.

> So this change will break the API if deprecate AVCodec->profiles? I
> know some guys used the FFmpeg API for encoding, it's bad new for them

Which change? According to my plan, validating AVCodecContext->profile 
will not not touch AVCodec->profiles. A potential next change can 
deprecate it, but it is a deprecation, not an instant removal.

Regards,
Marton