[FFmpeg-devel] [PATCH 25/31] cbs: Add function to update video codec parameters

[Mark Thompson]

On 20/06/2019 00:45, Andreas Rheinhardt wrote:
> If any of the *_metadata filter based upon cbs currently updates a video
> codec parameter like color information, the AVCodecParameters are not
> updated accordingly, so that e.g. muxers write header values based upon
> outdated information that may precede and thereby nullify the new values
> on the bitstream level.
> This commit adds a function to also update the video codec parameters
> so that the above situation can be fixed in a unified manner.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
>  libavcodec/cbs.c | 35 +++++++++++++++++++++++++++++++++++
>  libavcodec/cbs.h | 15 +++++++++++++++
>  2 files changed, 50 insertions(+)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 47679eca1b..37b080b5ba 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -342,6 +342,41 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>      return 0;
>  }
>  
> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,

The context argument isn't used at all.

> +                                    AVCodecParameters *par, int profile,
> +                                    int level, int width, int height,
> +                                    int field_order, int color_range,
> +                                    int color_primaries, int color_trc,
> +                                    int color_space, int chroma_location,
> +                                    int video_delay)
> +{
> +#define SET_IF_NONNEGATIVE(elem) \
> +    if (elem >= 0) \
> +        par->elem = elem;
> +    SET_IF_NONNEGATIVE(profile)
> +    SET_IF_NONNEGATIVE(level)
> +    SET_IF_NONNEGATIVE(width)
> +    SET_IF_NONNEGATIVE(height)
> +    SET_IF_NONNEGATIVE(field_order)
> +    SET_IF_NONNEGATIVE(video_delay)
> +#undef SET_IF_NONNEGATIVE
> +
> +#define SET_IF_VALID(elem, upper_bound) \
> +    if (0 <= elem && elem < upper_bound) \
> +        par->elem = elem;
> +    SET_IF_VALID(color_range,     AVCOL_RANGE_NB)
> +    SET_IF_VALID(color_trc,       AVCOL_TRC_NB)
> +    SET_IF_VALID(color_space,     AVCOL_SPC_NB)

I think we generally want to admit future values for the 23001-8 / H.273 fields (see range ..255 on all the metadata filters).

> +    SET_IF_VALID(chroma_location, AVCHROMA_LOC_NB)
> +#undef SET_IF_VALID
> +
> +    if (0 <= color_primaries && color_primaries <= AVCOL_PRI_SMPTE432
> +                             || color_primaries == AVCOL_PRI_JEDEC_P22)
> +        par->color_primaries = color_primaries;
> +
> +    return;
> +}
> +
>  int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>                          AVPacket *pkt,
>                          CodedBitstreamFragment *frag)
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index e1e6055ceb..1655f790cd 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -304,6 +304,21 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>                             AVCodecParameters *par,
>                             CodedBitstreamFragment *frag);
>  
> +/**
> + * Update the parameters of an AVCodecParameters structure
> + *
> + * If a parameter is negative, the corresponding member is not
> + * modified. For the color/chroma parameters, only values that
> + * are part of the relevant enumeration are written.
> + */
> +void ff_cbs_update_video_parameters(CodedBitstreamContext *ctx,
> +                                    AVCodecParameters *par, int profile,
> +                                    int level, int width, int height,
> +                                    int field_order, int color_range,
> +                                    int color_primaries, int color_trc,
> +                                    int color_space, int chroma_location,
> +                                    int video_delay);

I find the calls with -1, -1, -1 pretty horrible, and all the extra pointer arguments being passed around in the metadata filters are not very nice either.

To avoid both of those, how about something like this:

typedef struct foo {
    int profile;
    int level;
    int width;
    ...
} foo;

init_foo(foo *p)
{
    set all fields to minus one (or some other placeholder value)
}

update_codecpar_with_foo(AVCodecPar *par, const foo *p)
{
    as above
}

Then in each BSF:

struct BarMetadataContext {
    ...
    foo parameters;
} BarMetadataContext;

update_stuff()
{
    ...
    ctx->parameters.whatever = not-minus-one
}

init()
{
    init_foo(&ctx->parameters);
    update_stuff()
    update_codecpar_with_foo(bsf->par_out, &ctx->foo);
}

What do you think?

Thanks,

- Mark