[FFmpeg-devel] [PATCH 25/31] cbs: Add function to update video codec parameters
Mark Thompson
sw at jkqxz.net
Mon Jul 29 11:21:20 EEST 2019
On 29/07/2019 02:33, Andreas Rheinhardt wrote:
> 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.
Hmm. I think the real reason that seemed wrong to me was that it isn't really a CBS function, rather a BSF function - it can be used by things with no other CBS interaction, such as prores_metadata. So, maybe it should go in bsf.h rather than cbs.h?
ff_cbs_fragment_uninit(), on the other hand, really is a CBS function, so even though it doesn't currently use the context/logging argument it still feels right to pass it.
>>> + 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.
No future standard will backward-compatibly allow color_range > 255 because it's an 8-bit field in all current use.
I'm purely considering the behaviour for a new N + 1 value, since other components do allow that (for example, in <http://git.videolan.org/?p=ffmpeg.git;a=blob;f=libavcodec/options_table.h;h=4a266eca16918f6a1f9410e86973c61c0ac457d9;hb=HEAD#l355>).
>>> + 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?
Same argument applies, I think?
>>> + 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.
Yeah, the name was the hard bit - that's why it ended up being "foo" in the example :P
Thanks,
- Mark
More information about the ffmpeg-devel
mailing list