Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Sun, 24 Mar 2024 14:04:37 +0100 Andreas Rheinhardt
wrote:
> Niklas Haas:
> > On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt
> > 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@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Fri, 09 Feb 2024 13:11:38 +0100 Niklas Haas wrote:
> On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt
> 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?
>
> 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?
>
> 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.
>
> 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?
> }
Ping for review?
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt
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?
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?
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.
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?
}
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Thu, 08 Feb 2024 13:33:51 +0100 Andreas Rheinhardt
wrote:
> Sorry for not answering earlier.
> My intention is to allow users who only want to deal with the common
> case of a cartesian product to continue to do so, but to also support
> other usecases.
> The public function would look like
>
> int avcodec_get_supported_config(const AVCodecContext *avctx, const
> AVCodec *codec, int **supported_configs, unsigned desired_configs,
> unsigned flags, void *logctx);
>
> avctx can be NULL (in which case this allows to return all potential
> configurations, irrespective of e.g. the level of strictness).
> codec can be omitted if avctx->codec is set, but if both are supplied
> and avctx->codec is set, they have to match (like in avcodec_open2()).
> desired_configs is a bitfield of configs that the user wants to get
> information about; your patch would have to add flags for color_ranges
> and color_spaces.
> supported_configs will on return point to something like an array of
> struct { int desired_config0, desired_config1,...;}. The order of the
> entries will be fixed (say coincide with the order of the bits in the
> desired_configs bitfields).
> If one member is the unspec value for its type, then this means that it
> works with everything.
> supported_configs will be allocated; ownership passes to the user.
> Using a multidimensional sentinel (that would depend on desired_configs)
> is clumsy, so there will be two supported ways for this (depends upon a
> flag to be supplied in flags): One method that really adds a
> multidimensional sentinel, the other method that writes the number of
> entries into **supported_configs, so that the first entry starts at
> (*supported_configs)[1]. This allows users that only want to deal with
> the factors of a cartesian product separately to continue to do so.
OTOH this design will necessarily either result in exponential
explosion, or end up requiring the caller to make assumptions about
which fields are independent (and should thus be queried separately),
the moment a codec imposes *any* restriction (cartesian or not) on
multiple fields at the same time.
I also think that a `test()` callback, as I previously proposed, is also
overkill and doesn't actually solve anything. Codecs can already error
out on invalid configurations at open() time, and any practical use of
such an API would also end up having to make the cartesian assumption
one way or the other.
So in summary, I still think that we should enforce the assumption that
these fields form a cartesian set - it's simple, fast, useful and
doesn't overengineer for hypothetical constraints that we couldn't realistically
address one way or the other. Afaict, all current codecs support
a cartesian set of metadata, but feel free to correct me if I'm wrong.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Mon, 05 Feb 2024 19:37:48 +0100 Niklas Haas wrote:
> On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt
> 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.
>
> What signature would you propose for such a callback?
Well, there are two fundamental approaches here:
1. The callback somehow sets or returns a list of supported colorspaces.
2. The callback accepts a particular configuration and simply returns if
it's supported or not, with fields set to AVCOL_*_UNKNOWN being
ignored.
The first case has the pro of simplicity, and the ability to enumerate
more easily, but the drawback of only being scalable if we return
a cartesian set (i.e. set each attribute list independently, rather than
generating one big list).
The second case has the pro of flexibility, and the ability to handle
non-cartesian use cases, but the con of being slightly more awkward to
recover a list (for e.g. filtering purpose). Fortunately, we can recover
the minimal cartesian superset of the actual supported set in O(n) time,
and then still verify the exact format chosen at encode time.
One possibility could be to add a `test()` callback which simply tests
if the current codec context has valid parameters. For example:
struct AVCodecContext {
...
int (*test)(const AVCodecContext *ctx);
}
But this suggests a very stateful API, where you have to mutate
AVCodecContext in order to query what formats would be supported.
I don't like this.
So probably it makes more sense to just directly ingest a new struct
for this purpose, which we can then extend easily. (Or should we just
ingest AVFrame directly, i.e. int (*test_avframe)(ctx, AVFrame *frame)?)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
On Mon, 05 Feb 2024 19:04:30 +0100 Andreas Rheinhardt 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. What signature would you propose for such a callback? I disagree with using a list for the range being wasteful. In terms of binary size, they are shared (see ff_color_range_mpeg/jpeg definition). At best you are saving a few bits from using an int bitmask instead of a pointer. The main reason to use a list is API simplicity and consistency. Working with a bitmask is more awkward and would require more specialized code as opposed to the current design which allows simply re-using macros like DEF_CHOOSE_FORMATS in fftools, which is designed to handle lists. Not to mention the equivalent libavfilter API also being list-based. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
Re: [FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
It would make me happy if we could get this API addition in before 7.0, even though it's non-functional on its own, as it sets the groundwork to be able to start properly deprecating YUVJ. After this series, the next steps are: 1. Make decoders stop outputting YUVJ frames 2. Mark the actual YUVJ pixel formats as deprecated, add a removal API define, and guard internal usages by AV_NOWARN_DEPRECATED 3. After sufficient time passes, remove YUVJ formats fully I have a branch prepared for each. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
[FFmpeg-devel] [PATCH 1/2] avcodec: add YUV color space metadata to AVCodec
From: Niklas Haas
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 adding YUV range, we might as well add the
YUV color matrix as well - since some codecs (e.g. VP8, JPEG) only
support certain values.
I decided to preserve the ambiguous and misleading "color_spaces" name,
for symmetry with AVFrame.colorspace. (Though this would IMO be better
called "color_matrix" or "color_system")
I also decided to omit the other AVColor* fields for now, because
vf_scale cannot handle auto-conversion between primaries/transfer/etc.
There is little value in adding metadata we cannot do anything with, and
no harm in extending the API again in the future. In theory, vf_scale
can handle conversion between chroma locations, but also the signalling
for this is annoying, so I'll defer it to a future commit.
---
doc/APIchanges | 3 +++
libavcodec/codec.h | 6 ++
libavcodec/version.h | 2 +-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/doc/APIchanges b/doc/APIchanges
index 1f5724324a..7849ce47d9 100644
--- a/doc/APIchanges
+++ b/doc/APIchanges
@@ -2,6 +2,9 @@ The last version increases of all libraries were on 2023-02-09
API changes, most recent first:
+2024-02-xx - xx - lavc 60.40.100 - avcodec.h
+ Add AVCodec.color_ranges and AVCodec.color_spaces.
+
2024-02-04 - xx - lavc 60.39.100 - packet.h
Add AV_PKT_DATA_AMBIENT_VIEWING_ENVIRONMENT.
diff --git a/libavcodec/codec.h b/libavcodec/codec.h
index 8034f1a53c..8bd678de7a 100644
--- a/libavcodec/codec.h
+++ b/libavcodec/codec.h
@@ -235,6 +235,12 @@ typedef struct AVCodec {
* Array of supported channel layouts, terminated with a zeroed layout.
*/
const AVChannelLayout *ch_layouts;
+
+/**
+ * Array of supported YUV color formats. Ignored for RGB/Gray formats.
+ */
+const enum AVColorRange *color_ranges; ///< terminated by
AVCOL_RANGE_UNSPECIFIED
+const enum AVColorSpace *color_spaces; ///< terminated by
AVCOL_SPC_UNSPECIFIED
} AVCodec;
/**
diff --git a/libavcodec/version.h b/libavcodec/version.h
index f2f14eaed1..19f3f4a272 100644
--- a/libavcodec/version.h
+++ b/libavcodec/version.h
@@ -29,7 +29,7 @@
#include "version_major.h"
-#define LIBAVCODEC_VERSION_MINOR 39
+#define LIBAVCODEC_VERSION_MINOR 40
#define LIBAVCODEC_VERSION_MICRO 100
#define LIBAVCODEC_VERSION_INT AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
--
2.43.0
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
