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

[Niklas Haas]

On Sun, 24 Mar 2024 14:04:37 +0100 Andreas Rheinhardt <andreas.rheinhardt at outlook.com> wrote:
> 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.)

Having an extra `logctx` here seems redundant and inconsistent with
other avctx-taking functions, which log to the provided `avctx`.

> 
> allows to keep the details hidden and therefore use a compact way to
> store it.
> 
> - Andreas
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".