| Message ID | 20190619234521.15619-17-andreas.rheinhardt@gmail.com |
|---|---|
| State | New |
| Headers | show |
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@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
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@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
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@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
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, + 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) + 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); + /** * Write the bitstream of a fragment to a packet. *
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@gmail.com> --- libavcodec/cbs.c | 35 +++++++++++++++++++++++++++++++++++ libavcodec/cbs.h | 15 +++++++++++++++ 2 files changed, 50 insertions(+)