[FFmpeg-devel] [PATCH 01/27] avcodec: add color_range to AVCodec struct and use it

[wm4]

On Mon, 11 Dec 2017 08:32:52 -0500
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> Hi,
> 
> On Sat, Dec 9, 2017 at 10:37 AM, Paul B Mahol <onemda at gmail.com> wrote:
> 
> > @@ -3376,6 +3376,7 @@ typedef struct AVCodec {
> >      uint8_t max_lowres;                     ///< maximum value for lowres
> > supported by the decoder
> >      const AVClass *priv_class;              ///< AVClass for the private
> > context
> >      const AVProfile *profiles;              ///< array of recognized
> > profiles, or NULL if unknown, array is terminated by {FF_PROFILE_UNKNOWN}
> > +    int color_range;                        ///< supported color range by
> > encoder, 0 means any is supported
> >
> >      /*****************************************************************
> >       * No fields below this line are part of the public API. They  
> 
> 
> First: please use the properly typed enum (enum AVColorRange).

(That makes it actually less safe from an ABI perspective, but that's a
lost cause anyway, since enums are used in most places where it matters
for ABI.)

> Second: so I understand what you're trying to do here. For decoders, if
> they only support one range (or colorspace, or whatever), we can set it in
> the decode function. But for encoders, we need some introspectable way to
> make sure the input to the codec is of the proper format, else we can only
> error, right? So, if some codec only supports mpeg range but the input is
> jpeg, something needs to convert it.

libavcodec doesn't do such conversions. I guess you can be lucky if
trying to encode AVFrames with unsupported parameters gives a
meaningful error.

> Are we going to do that for all other color-related properties also? What
> if vp8 only supports bt601 but the input is bt709? Will we add a
> color_space to AVCodec?

In theory we should. Of course the goal is merely getting rid of the J
formats, which sort of provided a way to signal the color range via
pixel format. But in general, the API for signaling supported input
data for encoders is pretty bad. E.g. some encoders have random
restrictions on image size, etc., which brings us to...:

> Or do we need a more generic mechanism to validate
> an input AVFrame config (AVFrame without data)? If we want a more generic
> design, we'd better do it now before we add 10 new ints to AVCodec and then
> remove them again... (Same is true for audio also, what if the codec only
> supports some samplerates/channel configs etc? Maybe something more generic
> is useful there too.)

Yes, we would need that. You have to keep in mind that just listing
supported parameters separately is not enough either. Encoders could
support or not-support certain parameter combinations in non-trivial
ways. It's basically an N-dimensional search space (insert Paul
aneurism here).

I don't know what would be the best way to design such an API. It can
get easily too complex, while still not supporting all cases. I guess
that's the reason why it the metadata remained as "stupid" as it is,
because raising the complexity won't fix it either.

In summary, it's probably OK to extend the current API in the way Paul
wants to do.