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

[Andreas Rheinhardt]

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.
> 
I know. I thought that's how you wanted the API to look like. After
all, ff_cbs_fragment_uninit always used a context argument without
needing it (cbs_unit_uninit doesn't use its context argument at all
and it is the only place where ff_cbs_fragment_uninit used it; I of
course kept this behaviour when replacing ff_cbs_fragment_uninit). Now
that it seems that this was unintentional, I will send a patch to
remove the argument.

>> +                                    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).
> 
Do you want to omit checking entirely? Or should I still check against
the sentinel? I prefer the latter. I dislike setting an enum to
something else than an enum constant and moreover, if a future
standard would allow (say) color_range > 255 (or a bsf would simply
forget to check the value), but libavcodec does not, then it might be
that the compiler uses char as underlying type for the enum which of
course could not hold such a value.

>> +    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;
>> +
And how about the gap in the color_primaries values?

>> +    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?
> 
Sounds good to me. Naming suggestions welcome.

- Andreas