[FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec

[Andreas Rheinhardt]

Niklas Haas:
> On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
>> This presumes the relevant states to be a cartesian product. Which need
>> not be true. A callback would be better; this would also allow to base
>> the list on other values already set in an AVCodecContext. And if this
>> were extended, it would also allow to remove init_static_data one day.
>> It is furthermore quite wasteful to store color_ranges in a list,
>> although there are only very few states for it.
> 
> There is also the consideration to be made that using a callback is
> inconsistent with the established design. Consider that framerates,
> pix_fmts, samplerates, sample fmts and channel layouts are all currently
> provided as static arrays in AVCodec. There is a natural symmetry
> between these items and the ones I intend to add (yuv matrix, range,
> chroma location, primaries and gamma) - all of them are descriptive of
> the way data is encoded, and are therefore also (or should be)
> negotiable filter link properties.
> 
> If we add a new callback API, should we then extend it to also include
> all of the existing items from the above list? Is there a reason that
> yuv range supports needs to be more dynamic than the others?
> 

It should support everything; and I'd like to remove the other (public)
static lists, too (after the necessary deprecations).

> Food for thought: mjpeg is not the only codec that puts restrictions on
> the format support based on the strictness level. For example,
> yuv4mpegpipe_muxer errors out with a strictness warning if you use
> a non-standard pixel format. And arguably, in this case, this is
> **preferred** behavior over "silently" inserting a scale filter to
> convert to a supported format, as the whole point of y4m2 is to
> encapsulate raw data as-is.
> 
> Should we:
> 
> 1. Add a new dynamic callback that can query lists for all of the above
>    in a way dependent on the strictness level, and use it as
>    a replacement for the static lists currently in AVCodec?
> 
> 2. Continue with the status quo of having these lists be static, plus
>    dynamic checks at open() time, and continue using the "convenience
>    hack" of having ffmpeg_tools automatically restrict limited range mjpeg?
> 

I really want this convenience hack removed.

> It is not immediately obvious to me that an automatic conversion to
> a supported format is *necessarily* preferred to erroring out unless the
> user specifies a lower strictness level.

I agree. (In fact, on default strictness, the current code inserts a
scale filter even if one explicitly adds "-color_range tv".)

> 
> As for an API, I think that rather than having an AVCodecContext-aware
> callback at all, I would just make callbacks that directly ingest the
> strictness level in AVCodec. That makes it far less of a black box about
> which fields of the AVCodecContext are relevant here.
> 
> i.e.
> 
> struct AVCodec {
>     const enum AVColorRange (*get_color_ranges)(int strictness);
>     const enum AVColorSpace (*get_color_spaces)(int strictness);
>     // ditto for the other parameters?
> }

Your callbacks would hardcode that the only thing that matters is
strictness. And it would be very expensive, because these fields would
be in every AVCodec, even though only a minority of AVCodecs (namely
video encoders) would use them. (supported_framerates is even only set
by two encoders. What a waste.) Adding an API like

int avcodec_get_supported_config(const AVCodecContext *avctx, const
AVCodec *codec, void **supported_configs, unsigned *num_configs, enum
AVCodecConfigs desired_config,
unsigned flags, void *logctx);
(enum AVCodecConfigs would contain a value for pix fmts, sample fmts etc.)

allows to keep the details hidden and therefore use a compact way to
store it.

- Andreas