| Message ID | 20231013142706.23971-3-ffmpeg@haasn.xyz |
|---|---|
| State | New |
| Headers | show |
| Series | YUVJ removal | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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.
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.
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.
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)
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.
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?
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.
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 --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, \
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(-)