Re: [FFmpeg-devel] [PATCH v2 02/17] avcodec: add avcodec_get_supported_config()

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Niklas Haas:
> From: Niklas Haas <git@haasn.dev>
> 
> This replaces the myriad of existing lists in AVCodec by a unified API
> call, allowing us to (ultimately) trim down the sizeof(AVCodec) quite
> substantially, while also making this more trivially extensible.
> 
> In addition to the already covered lists, add two new entries for color
> space and color range, mirroring the newly added negotiable fields in
> libavfilter.
> 
> I decided to drop the explicit length field from the API proposed by
> Andreas Rheinhardt, because having it in place ended up complicating
> both the codec side and the client side implementations, while also
> being strictly less flexible (it's trivial to recover a length given
> a terminator, but requires allocation to add a terminator given
> a length). Using a terminator also presents less of a porting challenge
> for existing users of the current API.
> 
> Once the deprecation period passes for the existing public fields, the
> rough plan is to move the commonly used fields (such as
> pix_fmt/sample_fmt) into FFCodec, possibly as a union of audio and video
> configuration types, and then implement the rarely used fields with
> custom callbacks.
> ---
>  doc/APIchanges              |  5 +++
>  libavcodec/avcodec.c        | 75 +++++++++++++++++++++++++++++++++++++
>  libavcodec/avcodec.h        | 27 +++++++++++++
>  libavcodec/codec.h          | 19 ++++++++--
>  libavcodec/codec_internal.h | 24 ++++++++++++
>  libavcodec/version.h        |  4 +-
>  6 files changed, 148 insertions(+), 6 deletions(-)
> 
> diff --git a/doc/APIchanges b/doc/APIchanges
> index 0a39b6d7ab8..fdeae67159d 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -2,6 +2,11 @@ The last version increases of all libraries were on 2024-03-07
>  
>  API changes, most recent first:
>  
> +2024-04-xx - xxxxxxxxxx - lavc 59.6.100 - avcodec.h
> +  Add avcodec_get_supported_config() and enum AVCodecConfig; deprecate
> +  AVCodec.pix_fmts, AVCodec.sample_fmts, AVCodec.supported_framerates,
> +  AVCodec.supported_samplerates and AVCodec.ch_layouts.
> +
>  2024-04-03 - xxxxxxxxxx - lavu 59.13.100 - pixfmt.h
>    Add AVCOL_SPC_IPT_C2, AVCOL_SPC_YCGCO_RE and AVCOL_SPC_YCGCO_RO
>    to map new matrix coefficients defined by H.273 v3.
> diff --git a/libavcodec/avcodec.c b/libavcodec/avcodec.c
> index 525fe516bd2..96728546d6c 100644
> --- a/libavcodec/avcodec.c
> +++ b/libavcodec/avcodec.c
> @@ -700,3 +700,78 @@ int attribute_align_arg avcodec_receive_frame(AVCodecContext *avctx, AVFrame *fr
>          return ff_decode_receive_frame(avctx, frame);
>      return ff_encode_receive_frame(avctx, frame);
>  }
> +
> +#define WRAP_CONFIG(allowed_type, field)    \
> +    do {                                    \
> +        if (codec->type != (allowed_type))  \
> +            return AVERROR(EINVAL);         \
> +        *out_configs = (field);             \
> +        return 0;                           \
> +    } while (0)
> +
> +static const enum AVColorRange color_range_jpeg[] = {
> +    AVCOL_RANGE_JPEG, AVCOL_RANGE_UNSPECIFIED
> +};
> +
> +static const enum AVColorRange color_range_mpeg[] = {
> +    AVCOL_RANGE_MPEG, AVCOL_RANGE_UNSPECIFIED
> +};
> +
> +static const enum AVColorRange color_range_all[] = {
> +    AVCOL_RANGE_MPEG, AVCOL_RANGE_JPEG, AVCOL_RANGE_UNSPECIFIED
> +};
> +
> +static const enum AVColorRange *color_range_table[] = {
> +    [AVCOL_RANGE_MPEG] = color_range_mpeg,
> +    [AVCOL_RANGE_JPEG] = color_range_jpeg,
> +    [AVCOL_RANGE_MPEG | AVCOL_RANGE_JPEG] = color_range_all,
> +};
> +
> +int ff_default_get_supported_config(const AVCodecContext *avctx,
> +                                    const AVCodec *codec,
> +                                    enum AVCodecConfig config,
> +                                    unsigned flags,
> +                                    const void **out_configs)
> +{
> +    switch (config) {
> +FF_DISABLE_DEPRECATION_WARNINGS
> +    case AV_CODEC_CONFIG_PIX_FORMAT:
> +        WRAP_CONFIG(AVMEDIA_TYPE_VIDEO, codec->pix_fmts);
> +    case AV_CODEC_CONFIG_FRAME_RATE:
> +        WRAP_CONFIG(AVMEDIA_TYPE_VIDEO, codec->supported_framerates);
> +    case AV_CODEC_CONFIG_SAMPLE_RATE:
> +        WRAP_CONFIG(AVMEDIA_TYPE_AUDIO, codec->supported_samplerates);
> +    case AV_CODEC_CONFIG_SAMPLE_FORMAT:
> +        WRAP_CONFIG(AVMEDIA_TYPE_AUDIO, codec->sample_fmts);
> +    case AV_CODEC_CONFIG_CHANNEL_LAYOUT:
> +        WRAP_CONFIG(AVMEDIA_TYPE_AUDIO, codec->ch_layouts);
> +FF_ENABLE_DEPRECATION_WARNINGS
> +
> +    case AV_CODEC_CONFIG_COLOR_RANGE:
> +        if (codec->type != AVMEDIA_TYPE_VIDEO)
> +            return AVERROR(EINVAL);
> +        *out_configs = color_range_table[ffcodec(codec)->color_ranges];
> +        return 0;
> +
> +    case AV_CODEC_CONFIG_COLOR_SPACE:
> +        *out_configs = NULL;
> +        return 0;
> +    default:
> +        return AVERROR(EINVAL);
> +    }
> +}
> +
> +int avcodec_get_supported_config(const AVCodecContext *avctx, const AVCodec *codec,
> +                                 enum AVCodecConfig config, unsigned flags,
> +                                 const void **out)
> +{
> +    const FFCodec *codec2;
> +    if (!codec)
> +        codec = avctx->codec;
> +    codec2 = ffcodec(codec);
> +    if (codec2->get_supported_config) {
> +        return codec2->get_supported_config(avctx, codec, config, flags, out);
> +    } else {
> +        return ff_default_get_supported_config(avctx, codec, config, flags, out);
> +    }
> +}
> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 83dc487251c..64f31375fc6 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -2690,6 +2690,33 @@ int avcodec_get_hw_frames_parameters(AVCodecContext *avctx,
>                                       enum AVPixelFormat hw_pix_fmt,
>                                       AVBufferRef **out_frames_ref);
>  
> +enum AVCodecConfig {
> +    AV_CODEC_CONFIG_PIX_FORMAT,     ///< AVPixelFormat, terminated by AV_PIX_FMT_NONE
> +    AV_CODEC_CONFIG_FRAME_RATE,     ///< AVRational, terminated by {0, 0}
> +    AV_CODEC_CONFIG_SAMPLE_RATE,    ///< int, terminated by 0
> +    AV_CODEC_CONFIG_SAMPLE_FORMAT,  ///< AVSampleFormat, terminated by AV_SAMPLE_FMT_NONE
> +    AV_CODEC_CONFIG_CHANNEL_LAYOUT, ///< AVChannelLayout, terminated by {0}
> +    AV_CODEC_CONFIG_COLOR_RANGE,    ///< AVColorRange, terminated by AVCOL_RANGE_UNSPECIFIED
> +    AV_CODEC_CONFIG_COLOR_SPACE,    ///< AVColorSpace, terminated by AVCOL_SPC_UNSPECIFIED
> +};
> +
> +/**
> + * Retrieve a list of all supported values for a given configuration type.
> + *
> + * @param avctx An optional context to use. Values such as
> + *              `strict_std_compliance` may affect the result. If NULL,
> + *              default values are used.
> + * @param codec The codec to query, or NULL to use avctx->codec.
> + * @param config The configuration to query.
> + * @param flags Currently unused; should be set to zero.
> + * @param out_configs On success, set to a list of configurations, terminated
> + *                    by a config-specific terminator, or NULL if all
> + *                    possible values are supported.
> + */
> +int avcodec_get_supported_config(const AVCodecContext *avctx,
> +                                 const AVCodec *codec, enum AVCodecConfig config,
> +                                 unsigned flags, const void **out_configs);

1. This approach constrains us in several ways, most notably it forces
us to always have static lists for every possible combination. E.g. take
a look at the wavpack encoder: It accepts certain native channel layouts
or certain unspecified channel counts within a range (the encoder
accepts up to 255, the format allows up to 4096). It makes no sense to
have a static list of 4096 unspecified channel layouts. Getting rid of
this requirement seems good, even if it causes allocations.
2. A length field is not "strictly less flexible", to the contrary: It
allows to keep our options open. The length field should be optional and
for the convenience of the user (meaning the list would still be
terminated by a sentinel). But it would allow to e.g. add a flag in the
future that says that *out_configs is already set to some array whose
length is given by the length field which would allow to avoid the
allocation that I just proposed in 1. (e.g. the number of sample formats
is very small and can be put into a stack array; the number of pixel
formats is also small and can be queried at runtime, so that an array of
maximum size can be allocated once to query multiple codecs).

> +
>  
>  
>  /**
> diff --git a/libavcodec/codec.h b/libavcodec/codec.h
> index 6f9b42760d7..f7541ffc42b 100644
> --- a/libavcodec/codec.h
> +++ b/libavcodec/codec.h
> @@ -205,10 +205,19 @@ typedef struct AVCodec {
>       */
>      int capabilities;
>      uint8_t max_lowres;                     ///< maximum value for lowres supported by the decoder
> -    const AVRational *supported_framerates; ///< array of supported framerates, or NULL if any, array is terminated by {0,0}
> -    const enum AVPixelFormat *pix_fmts;     ///< array of supported pixel formats, or NULL if unknown, array is terminated by -1
> -    const int *supported_samplerates;       ///< array of supported audio samplerates, or NULL if unknown, array is terminated by 0
> -    const enum AVSampleFormat *sample_fmts; ///< array of supported sample formats, or NULL if unknown, array is terminated by -1
> +
> +    /**
> +     * Deprecated codec capabilities.
> +     */
> +    attribute_deprecated
> +    const AVRational *supported_framerates; ///< @deprecated use avcodec_get_supported_config()
> +    attribute_deprecated
> +    const enum AVPixelFormat *pix_fmts;     ///< @deprecated use avcodec_get_supported_config()
> +    attribute_deprecated
> +    const int *supported_samplerates;       ///< @deprecated use avcodec_get_supported_config()
> +    attribute_deprecated
> +    const enum AVSampleFormat *sample_fmts; ///< @deprecated use avcodec_get_supported_config()
> +
>      const AVClass *priv_class;              ///< AVClass for the private context
>      const AVProfile *profiles;              ///< array of recognized profiles, or NULL if unknown, array is terminated by {AV_PROFILE_UNKNOWN}
>  
> @@ -226,7 +235,9 @@ typedef struct AVCodec {
>  
>      /**
>       * Array of supported channel layouts, terminated with a zeroed layout.
> +     * @deprecated use avcodec_get_supported_config()
>       */
> +    attribute_deprecated
>      const AVChannelLayout *ch_layouts;
>  } AVCodec;
>  
> diff --git a/libavcodec/codec_internal.h b/libavcodec/codec_internal.h
> index 2134f184094..bac3e30ba2c 100644
> --- a/libavcodec/codec_internal.h
> +++ b/libavcodec/codec_internal.h
> @@ -22,6 +22,7 @@
>  #include <stdint.h>
>  
>  #include "libavutil/attributes.h"
> +#include "avcodec.h"
>  #include "codec.h"
>  #include "config.h"
>  
> @@ -270,8 +271,31 @@ typedef struct FFCodec {
>       * List of supported codec_tags, terminated by FF_CODEC_TAGS_END.
>       */
>      const uint32_t *codec_tags;
> +
> +    /**
> +     * Custom callback for avcodec_get_supported_config(). If absent,
> +     * ff_default_get_supported_config() will be used.
> +     */
> +    int (*get_supported_config)(const AVCodecContext *avctx,
> +                                const AVCodec *codec,
> +                                enum AVCodecConfig config,
> +                                unsigned flags,
> +                                const void **out_configs);
>  } FFCodec;
>  
> +/**
> + * Default implementation for avcodec_get_supported_config(). Will return the
> + * relevant fields from AVCodec if present, or NULL otherwise.
> + *
> + * For AVCODEC_CONFIG_COLOR_RANGE, the output will depend on the bitmask in
> + * FFCodec.color_ranges, with a value of 0 returning NULL.
> + */
> +int ff_default_get_supported_config(const AVCodecContext *avctx,
> +                                    const AVCodec *codec,
> +                                    enum AVCodecConfig config,
> +                                    unsigned flags,
> +                                    const void **out_configs);
> +
>  #if CONFIG_SMALL
>  #define CODEC_LONG_NAME(str) .p.long_name = NULL
>  #else
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 84a1c02ce4a..da54f878874 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -29,8 +29,8 @@
>  
>  #include "version_major.h"
>  
> -#define LIBAVCODEC_VERSION_MINOR   5
> -#define LIBAVCODEC_VERSION_MICRO 101
> +#define LIBAVCODEC_VERSION_MINOR   6
> +#define LIBAVCODEC_VERSION_MICRO 100
>  
>  #define LIBAVCODEC_VERSION_INT  AV_VERSION_INT(LIBAVCODEC_VERSION_MAJOR, \
>                                                 LIBAVCODEC_VERSION_MINOR, \

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".