diff mbox series

[FFmpeg-devel,v2,02/13] avcodec: add extended AVCodec color metadata

Message ID 20231013142706.23971-3-ffmpeg@haasn.xyz
State New
Headers show
Series YUVJ removal | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Niklas Haas Oct. 13, 2023, 2:24 p.m. UTC
From: Niklas Haas <git@haasn.dev>

This is motivated primarily by a desire for YUVJ removal, which will
require signalling the supported color ranges as part of the codec
capabilities. But since we're here anyway, we might as well add all of
the metadata, which I foresee seeing more use in the future (e.g.
automatic conversion from HDR to SDR when encoding to formats that don't
support AVCOL_TRC_SMPTE2084, ...)
---
 doc/APIchanges       | 4 ++++
 libavcodec/codec.h   | 7 +++++++
 libavcodec/version.h | 4 ++--
 3 files changed, 13 insertions(+), 2 deletions(-)

Comments

Andreas Rheinhardt Oct. 13, 2023, 5:10 p.m. UTC | #1
Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> This is motivated primarily by a desire for YUVJ removal, which will
> require signalling the supported color ranges as part of the codec
> capabilities. But since we're here anyway, we might as well add all of
> the metadata, which I foresee seeing more use in the future (e.g.
> automatic conversion from HDR to SDR when encoding to formats that don't
> support AVCOL_TRC_SMPTE2084, ...)
> ---
>  doc/APIchanges       | 4 ++++
>  libavcodec/codec.h   | 7 +++++++
>  libavcodec/version.h | 4 ++--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 9b109e6fa7..f91e855e70 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,10 @@ The last version increases of all libraries were on 2023-02-09
>  
>  API changes, most recent first:
>  
> +2023-10-xx - xxxxxxxxxx - lavc 60.23.100 - avcodec.h
> +  Add AVCodec.csps, AVCodec.color_ranges, AVCodec.chroma_locs, AVCodec.primaries,
> +  AVCodec.trcs.
> +
>  2023-10-06 - xxxxxxxxxx - lavc 60.30.101 - avcodec.h
>    AVCodecContext.coded_side_data may now be used during decoding, to be set
>    by user before calling avcodec_open2() for initialization.
> diff --git a/libavcodec/codec.h b/libavcodec/codec.h
> index 8034f1a53c..5bc8f21868 100644
> --- a/libavcodec/codec.h
> +++ b/libavcodec/codec.h
> @@ -235,6 +235,13 @@ typedef struct AVCodec {
>       * Array of supported channel layouts, terminated with a zeroed layout.
>       */
>      const AVChannelLayout *ch_layouts;
> +
> +    /* Extended colorspace support metadata */
> +    const enum AVColorSpace *csps;                  ///< array of supported color spaces, or NULL if unknown, array is terminated by AVCOL_SPC_UNSPECIFIED
> +    const enum AVColorRange *color_ranges;          ///< array of supported color ranges, or NULL if unknown, array is terminated by 0
> +    const enum AVChromaLocation *chroma_locs;       ///< array of supported chroma locations, or NULL if unknown, array is terminated by 0
> +    const enum AVColorPrimaries *primaries;         ///< array of supported color primaries, or NULL if unknown, array is terminated by 0
> +    const enum AVColorTransferCharacteristic *trcs; ///< array of supported transfer characteristics, or NULL if known, array is terminated by 0
>  } AVCodec;
>  

This design has several drawbacks:
1. It adds stuff that will only be set by a tiny minority of AVCodec's
to all of them.
2. It is based around the underlying assumption that the set of
permissible states (tupels) is a cartesian product of a set of color
spaces, a set of color ranges etc. This is wrong: E.g. VP9 disallows
limited-range RGB (it is syntactically impossible to set the color range
when using RGB color space).
3. I don't see how the MJPEG encoder behaviour where the valid formats
de facto depend upon strictness can be encoded in this way; isn't the
aim to get rid of the necessity of the workaround in ffmpeg cli?

1. and 2. suggests using some form of function that returns a list of
supported tupels; if said function uses an AVCodecContext* parameter,
said list can depend upon the state of the AVCodecContext given to it,
thereby solving 3. to the extent that one can get the supported
combinations given AVCodecContext options (but I do not see a good way
to signal which options modify the supported combinations).

- Andreas
Vittorio Giovara Oct. 13, 2023, 6:52 p.m. UTC | #2
On Fri, Oct 13, 2023 at 1:09 PM Andreas Rheinhardt <
andreas.rheinhardt@outlook.com> wrote:

> 2. It is based around the underlying assumption that the set of
> permissible states (tupels) is a cartesian product of a set of color
> spaces, a set of color ranges etc. This is wrong: E.g. VP9 disallows
> limited-range RGB (it is syntactically impossible to set the color range
> when using RGB color space).


small note on VP9 and other color-constrained codecs, it's still always
possible to set these parameters via the container, i.e. through the mp4
color box.
Niklas Haas Oct. 14, 2023, 10:29 a.m. UTC | #3
On Fri, 13 Oct 2023 19:10:33 +0200 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> This design has several drawbacks:
> 1. It adds stuff that will only be set by a tiny minority of AVCodec's
> to all of them.
> 2. It is based around the underlying assumption that the set of
> permissible states (tupels) is a cartesian product of a set of color
> spaces, a set of color ranges etc. This is wrong: E.g. VP9 disallows
> limited-range RGB (it is syntactically impossible to set the color range
> when using RGB color space).
> 3. I don't see how the MJPEG encoder behaviour where the valid formats
> de facto depend upon strictness can be encoded in this way; isn't the
> aim to get rid of the necessity of the workaround in ffmpeg cli?
> 
> 1. and 2. suggests using some form of function that returns a list of
> supported tupels; if said function uses an AVCodecContext* parameter,
> said list can depend upon the state of the AVCodecContext given to it,
> thereby solving 3. to the extent that one can get the supported
> combinations given AVCodecContext options (but I do not see a good way
> to signal which options modify the supported combinations).

There are two other designs I can think of:

1. Enumerate all possible combinations as a list. To avoid combinatoric
   explosion, setting any field to 'UNSPECIFIED' implies no restriction
   on that field. So the default (no list) would be equivalent to a list
   with a single entry containing values of UNSPECIFIED for every entry.

2. Provide a single function which merely checks if a given combination
   is supported or not.

#2 would work for the short term but runs into the same risk of
exponential explosion if we need to start finding a common format
between different filters.

So maybe #1 is the correct approach here. It would also simplify
extending the filter API, as we would only need one set of list
managing/merging/compat testing boilerplate for all of the colorspace
metadata.

I will try implementing #1 on a separate branch.
Niklas Haas Oct. 14, 2023, 11:46 a.m. UTC | #4
On Fri, 13 Oct 2023 19:10:33 +0200 Andreas Rheinhardt <andreas.rheinhardt@outlook.com> wrote:
> 2. It is based around the underlying assumption that the set of
> permissible states (tupels) is a cartesian product of a set of color
> spaces, a set of color ranges etc. This is wrong: E.g. VP9 disallows
> limited-range RGB (it is syntactically impossible to set the color range
> when using RGB color space).

Well, upon further consideration, I don't think this is enough to break
the cartesian approach, because RGB is always full range by convention.
Note how vf_scale, vf_zscale and vf_libplacebo all force the color range
for RGB inputs to full. So this is not an exception, rather it is the
rule. In other words, for RGB input, the colorspace and color_range
restrictions should simply be ignored, as they conceptually apply to YUV
formats only.

Note also that, thinking a little bit ahead, independent list would make
AVFilter negotiation *much* easier as we could just re-use
AVFilterFormats for each field without worry - whereas a "list of
tuples" approach requires introducing a new struct to group such
metadata, a new type of AVFilterFormats list + all supporting functions,
and a lot more boilerplate overall.

So we need to think very carefully if there actually are any
sufficiently strong motivating cases to introduce such heavy machinery.

> 3. I don't see how the MJPEG encoder behaviour where the valid formats
> de facto depend upon strictness can be encoded in this way; isn't the
> aim to get rid of the necessity of the workaround in ffmpeg cli?

Note that ffmpeg cli presently initializes the filter graph well before
the AVCodecContext is set up with options, let alone opened. (Presently,
the logic for overriding the pixfmt list directly looks up the "strict"
field in the options dict)

So that limits the design space somewhat for elegant solutions here.
Either we make the "return list of supported formats" callback in
AVCodec simply accept the strict_std_compliance setting directly, or we
extend the static list of colorspaces itself by an extra strictness
field. Probably the former is better than the latter of these two
approaches.
Leo Izen Oct. 14, 2023, 1:31 p.m. UTC | #5
On 10/13/23 10:24, Niklas Haas wrote:
> From: Niklas Haas <git@haasn.dev>
> 
> This is motivated primarily by a desire for YUVJ removal, which will
> require signalling the supported color ranges as part of the codec
> capabilities. But since we're here anyway, we might as well add all of
> the metadata, which I foresee seeing more use in the future (e.g.
> automatic conversion from HDR to SDR when encoding to formats that don't
> support AVCOL_TRC_SMPTE2084, ...)
> ---
> +
> +    /* Extended colorspace support metadata */
> +    const enum AVColorSpace *csps;                  ///< array of supported color spaces, or NULL if unknown, array is terminated by AVCOL_SPC_UNSPECIFIED
> +    const enum AVColorRange *color_ranges;          ///< array of supported color ranges, or NULL if unknown, array is terminated by 0
> +    const enum AVChromaLocation *chroma_locs;       ///< array of supported chroma locations, or NULL if unknown, array is terminated by 0
> +    const enum AVColorPrimaries *primaries;         ///< array of supported color primaries, or NULL if unknown, array is terminated by 0
> +    const enum AVColorTransferCharacteristic *trcs; ///< array of supported transfer characteristics, or NULL if known, array is terminated by 0
>   } AVCodec;


Any particular reason we're using AVCOL_SPC_UNSPECIFIED to terminate 
csps, but not using AVCOL_PRI_UNSPECIFIED for the primaries and the 
equivalent for the TRC? It seems a bit more consistent than using RESERVED0

- Leo Izen (Traneptora)
Niklas Haas Oct. 14, 2023, 1:40 p.m. UTC | #6
On Sat, 14 Oct 2023 09:31:32 -0400 Leo Izen <leo.izen@gmail.com> wrote:
> On 10/13/23 10:24, Niklas Haas wrote:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > This is motivated primarily by a desire for YUVJ removal, which will
> > require signalling the supported color ranges as part of the codec
> > capabilities. But since we're here anyway, we might as well add all of
> > the metadata, which I foresee seeing more use in the future (e.g.
> > automatic conversion from HDR to SDR when encoding to formats that don't
> > support AVCOL_TRC_SMPTE2084, ...)
> > ---
> > +
> > +    /* Extended colorspace support metadata */
> > +    const enum AVColorSpace *csps;                  ///< array of supported color spaces, or NULL if unknown, array is terminated by AVCOL_SPC_UNSPECIFIED
> > +    const enum AVColorRange *color_ranges;          ///< array of supported color ranges, or NULL if unknown, array is terminated by 0
> > +    const enum AVChromaLocation *chroma_locs;       ///< array of supported chroma locations, or NULL if unknown, array is terminated by 0
> > +    const enum AVColorPrimaries *primaries;         ///< array of supported color primaries, or NULL if unknown, array is terminated by 0
> > +    const enum AVColorTransferCharacteristic *trcs; ///< array of supported transfer characteristics, or NULL if known, array is terminated by 0
> >   } AVCodec;
> 
> 
> Any particular reason we're using AVCOL_SPC_UNSPECIFIED to terminate 
> csps, but not using AVCOL_PRI_UNSPECIFIED for the primaries and the 
> equivalent for the TRC? It seems a bit more consistent than using RESERVED0

To be clear, we are - AVCOL_PRI/TRC_UNSPECIFIED are equal to 0, unlike
the other fields. But I could change the comments here for clarity.
Anton Khirnov Oct. 20, 2023, 1:54 p.m. UTC | #7
Quoting Niklas Haas (2023-10-14 13:46:34)
> > 3. I don't see how the MJPEG encoder behaviour where the valid formats
> > de facto depend upon strictness can be encoded in this way; isn't the
> > aim to get rid of the necessity of the workaround in ffmpeg cli?
> 
> Note that ffmpeg cli presently initializes the filter graph well before
> the AVCodecContext is set up with options, let alone opened. (Presently,
> the logic for overriding the pixfmt list directly looks up the "strict"
> field in the options dict)
> 
> So that limits the design space somewhat for elegant solutions here.
> Either we make the "return list of supported formats" callback in
> AVCodec simply accept the strict_std_compliance setting directly, or we
> extend the static list of colorspaces itself by an extra strictness
> field. Probably the former is better than the latter of these two
> approaches.

FWIW I find that behaviour to be a disgusting hack and the cleanest way
to address it would IMO be a separate mjpeg_experimental AVCodec
instance. That is assuming anyone actually needs this functionality.
Maybe we could also just drop it?
Anton Khirnov Oct. 20, 2023, 2:01 p.m. UTC | #8
Quoting Andreas Rheinhardt (2023-10-13 19:10:33)
> Niklas Haas:
> > From: Niklas Haas <git@haasn.dev>
> > 
> > This is motivated primarily by a desire for YUVJ removal, which will
> > require signalling the supported color ranges as part of the codec
> > capabilities. But since we're here anyway, we might as well add all of
> > the metadata, which I foresee seeing more use in the future (e.g.
> > automatic conversion from HDR to SDR when encoding to formats that don't
> > support AVCOL_TRC_SMPTE2084, ...)
> > ---
> >  doc/APIchanges       | 4 ++++
> >  libavcodec/codec.h   | 7 +++++++
> >  libavcodec/version.h | 4 ++--
> >  3 files changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/doc/APIchanges b/doc/APIchanges
> > index 9b109e6fa7..f91e855e70 100644
> > --- a/doc/APIchanges
> > +++ b/doc/APIchanges
> > @@ -2,6 +2,10 @@ The last version increases of all libraries were on 2023-02-09
> >  
> >  API changes, most recent first:
> >  
> > +2023-10-xx - xxxxxxxxxx - lavc 60.23.100 - avcodec.h
> > +  Add AVCodec.csps, AVCodec.color_ranges, AVCodec.chroma_locs, AVCodec.primaries,
> > +  AVCodec.trcs.
> > +
> >  2023-10-06 - xxxxxxxxxx - lavc 60.30.101 - avcodec.h
> >    AVCodecContext.coded_side_data may now be used during decoding, to be set
> >    by user before calling avcodec_open2() for initialization.
> > diff --git a/libavcodec/codec.h b/libavcodec/codec.h
> > index 8034f1a53c..5bc8f21868 100644
> > --- a/libavcodec/codec.h
> > +++ b/libavcodec/codec.h
> > @@ -235,6 +235,13 @@ typedef struct AVCodec {
> >       * Array of supported channel layouts, terminated with a zeroed layout.
> >       */
> >      const AVChannelLayout *ch_layouts;
> > +
> > +    /* Extended colorspace support metadata */
> > +    const enum AVColorSpace *csps;                  ///< array of supported color spaces, or NULL if unknown, array is terminated by AVCOL_SPC_UNSPECIFIED
> > +    const enum AVColorRange *color_ranges;          ///< array of supported color ranges, or NULL if unknown, array is terminated by 0
> > +    const enum AVChromaLocation *chroma_locs;       ///< array of supported chroma locations, or NULL if unknown, array is terminated by 0
> > +    const enum AVColorPrimaries *primaries;         ///< array of supported color primaries, or NULL if unknown, array is terminated by 0
> > +    const enum AVColorTransferCharacteristic *trcs; ///< array of supported transfer characteristics, or NULL if known, array is terminated by 0
> >  } AVCodec;
> >  
> 
> This design has several drawbacks:
> 1. It adds stuff that will only be set by a tiny minority of AVCodec's
> to all of them.
> 2. It is based around the underlying assumption that the set of
> permissible states (tupels) is a cartesian product of a set of color
> spaces, a set of color ranges etc. This is wrong: E.g. VP9 disallows
> limited-range RGB (it is syntactically impossible to set the color range
> when using RGB color space).
> 3. I don't see how the MJPEG encoder behaviour where the valid formats
> de facto depend upon strictness can be encoded in this way; isn't the
> aim to get rid of the necessity of the workaround in ffmpeg cli?
> 
> 1. and 2. suggests using some form of function that returns a list of
> supported tupels;

Another argument in favor of a function instead of arrays directly in
AVCodec is that for some codecs this is determined at runtime. A
function would allow us to get rid of FFCodec.init_static_data and make
FFCodec const.
Michael Niedermayer Oct. 20, 2023, 2:11 p.m. UTC | #9
On Fri, Oct 20, 2023 at 03:54:16PM +0200, Anton Khirnov wrote:
> Quoting Niklas Haas (2023-10-14 13:46:34)
> > > 3. I don't see how the MJPEG encoder behaviour where the valid formats
> > > de facto depend upon strictness can be encoded in this way; isn't the
> > > aim to get rid of the necessity of the workaround in ffmpeg cli?
> > 
> > Note that ffmpeg cli presently initializes the filter graph well before
> > the AVCodecContext is set up with options, let alone opened. (Presently,
> > the logic for overriding the pixfmt list directly looks up the "strict"
> > field in the options dict)
> > 
> > So that limits the design space somewhat for elegant solutions here.
> > Either we make the "return list of supported formats" callback in
> > AVCodec simply accept the strict_std_compliance setting directly, or we
> > extend the static list of colorspaces itself by an extra strictness
> > field. Probably the former is better than the latter of these two
> > approaches.
> 
> FWIW I find that behaviour to be a disgusting hack and the cleanest way
> to address it would IMO be a separate mjpeg_experimental AVCodec
> instance. That is assuming anyone actually needs this functionality.
> Maybe we could also just drop it?

The usecase i remember was lossless jpeg, but this seems to support
it by default now

If there is still a usecase, then i agree *jpeg_experimental would be an
option. or maybe jpeg_somethingrange
maybe this is usefull for encoding some mjpeg variants. Somehow i think
they didnt all use the same range

thx

[...]
diff mbox series

Patch

diff --git a/doc/APIchanges b/doc/APIchanges
index 9b109e6fa7..f91e855e70 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,10 @@  The last version increases of all libraries were on 2023-02-09
 
 API changes, most recent first:
 
+2023-10-xx - xxxxxxxxxx - lavc 60.23.100 - avcodec.h
+  Add AVCodec.csps, AVCodec.color_ranges, AVCodec.chroma_locs, AVCodec.primaries,
+  AVCodec.trcs.
+
 2023-10-06 - xxxxxxxxxx - lavc 60.30.101 - avcodec.h
   AVCodecContext.coded_side_data may now be used during decoding, to be set
   by user before calling avcodec_open2() for initialization.
diff --git a/libavcodec/codec.h b/libavcodec/codec.h
index 8034f1a53c..5bc8f21868 100644
--- a/libavcodec/codec.h
+++ b/libavcodec/codec.h
@@ -235,6 +235,13 @@  typedef struct AVCodec {
      * Array of supported channel layouts, terminated with a zeroed layout.
      */
     const AVChannelLayout *ch_layouts;
+
+    /* Extended colorspace support metadata */
+    const enum AVColorSpace *csps;                  ///< array of supported color spaces, or NULL if unknown, array is terminated by AVCOL_SPC_UNSPECIFIED
+    const enum AVColorRange *color_ranges;          ///< array of supported color ranges, or NULL if unknown, array is terminated by 0
+    const enum AVChromaLocation *chroma_locs;       ///< array of supported chroma locations, or NULL if unknown, array is terminated by 0
+    const enum AVColorPrimaries *primaries;         ///< array of supported color primaries, or NULL if unknown, array is terminated by 0
+    const enum AVColorTransferCharacteristic *trcs; ///< array of supported transfer characteristics, or NULL if known, array is terminated by 0
 } AVCodec;
 
 /**
diff --git a/libavcodec/version.h b/libavcodec/version.h
index 6b46100aae..497389d3f3 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,8 +29,8 @@ 
 
 #include "version_major.h"
 
-#define LIBAVCODEC_VERSION_MINOR  30
-#define LIBAVCODEC_VERSION_MICRO 102
+#define LIBAVCODEC_VERSION_MINOR  31
+#define LIBAVCODEC_VERSION_MICRO 100
 
 #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
                                                LIBAVCODEC_VERSION_MINOR, \