| Message ID | 20181201192810.852-3-andreas.rheinhardt@googlemail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 12/1/2018 4:28 PM, Andreas Rheinhardt wrote: > Up until now, this BSF only changed the bitstream, not the > AVCodecParameters. The result is that e.g. some muxers use outdated > information to write header information that conflicts with (and precedes) > the new information at the bitstream level, so that the updates might > not lead to the desired change. > This commit changes this. It adds a mechanism by which the new > information can be used to update the AVCodecParameters, too. This is > the new default and an option has been added to revert to the old > behaviour. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com> > --- > doc/bitstream_filters.texi | 6 +++++ > libavcodec/av1_metadata_bsf.c | 43 +++++++++++++++++++++++++++++++---- > 2 files changed, 45 insertions(+), 4 deletions(-) > > diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi > index 15c578aa8a..80df345f26 100644 > --- a/doc/bitstream_filters.texi > +++ b/doc/bitstream_filters.texi > @@ -87,6 +87,12 @@ the timing info in the sequence header. > Set the number of ticks in each picture, to indicate that the stream > has a fixed framerate. Ignored if @option{tick_rate} is not also set. > > +@item full_update > +If this is set, the AVCodecParameters are updated in addition to the > +bitstream. If unset, muxers might add header information based upon the > +old AVCodecParameters that contradicts and potentially precedes the > +changes made at the bitstream level. On by default. > + > @end table > > @section chomp > diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c > index 52d383661f..0c8d00acea 100644 > --- a/libavcodec/av1_metadata_bsf.c > +++ b/libavcodec/av1_metadata_bsf.c > @@ -46,11 +46,15 @@ typedef struct AV1MetadataContext { > > AVRational tick_rate; > int num_ticks_per_picture; > + > + int full_update; > } AV1MetadataContext; > > > static int av1_metadata_update_sequence_header(AVBSFContext *bsf, > - AV1RawSequenceHeader *seq) > + AV1RawSequenceHeader *seq, > + int *color_range, > + int *chroma_sample_position) > { > AV1MetadataContext *ctx = bsf->priv_data; > AV1RawColorConfig *clc = &seq->color_config; > @@ -82,6 +86,8 @@ static int av1_metadata_update_sequence_header(AVBSFContext *bsf, > "on RGB streams encoded in BT.709 sRGB.\n"); > } else { > clc->color_range = ctx->color_range; > + if (color_range) > + *color_range = ctx->color_range; > } > } > > @@ -91,6 +97,8 @@ static int av1_metadata_update_sequence_header(AVBSFContext *bsf, > "can only be set for 4:2:0 streams.\n"); > } else { > clc->chroma_sample_position = ctx->chroma_sample_position; > + if (chroma_sample_position) > + *chroma_sample_position = ctx->chroma_sample_position; > } > } > > @@ -135,7 +143,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *out) > for (i = 0; i < frag->nb_units; i++) { > if (frag->units[i].type == AV1_OBU_SEQUENCE_HEADER) { > obu = frag->units[i].content; > - err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header); > + err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header, > + NULL, NULL); > if (err < 0) > goto fail; > } > @@ -184,7 +193,7 @@ static int av1_metadata_init(AVBSFContext *bsf) > AV1MetadataContext *ctx = bsf->priv_data; > CodedBitstreamFragment *frag = &ctx->access_unit; > AV1RawOBU *obu; > - int err, i; > + int err, i, color_range = -1, chroma_sample_position = -1; > > err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_AV1, bsf); > if (err < 0) > @@ -200,7 +209,8 @@ static int av1_metadata_init(AVBSFContext *bsf) > for (i = 0; i < frag->nb_units; i++) { > if (frag->units[i].type == AV1_OBU_SEQUENCE_HEADER) { > obu = frag->units[i].content; > - err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header); > + err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header, > + &color_range, &chroma_sample_position); > if (err < 0) > goto fail; > } > @@ -213,6 +223,28 @@ static int av1_metadata_init(AVBSFContext *bsf) > } > } > > + if (ctx->full_update) { > + if (chroma_sample_position >= 0) { > + const int conversion_table[4] = { AVCHROMA_LOC_UNSPECIFIED, > + AVCHROMA_LOC_LEFT, > + AVCHROMA_LOC_TOPLEFT, > + AVCHROMA_LOC_UNSPECIFIED }; > + chroma_sample_position = conversion_table[chroma_sample_position]; > + } > + > + if (ctx->color_range >= 0) > + ctx->color_range++; > + > + ff_cbs_update_video_parameters(ctx->cbc, bsf->par_out, -1, -1, -1, -1, > + -1, color_range, ctx->color_primaries, > + ctx->transfer_characteristics, > + ctx->matrix_coefficients, > + chroma_sample_position, -1); > + > + if (ctx->color_range != -1) > + ctx->color_range--; > + } > + > err = 0; > fail: > ff_cbs_fragment_uninit(ctx->cbc, frag); > @@ -273,6 +305,9 @@ static const AVOption av1_metadata_options[] = { > OFFSET(num_ticks_per_picture), AV_OPT_TYPE_INT, > { .i64 = -1 }, -1, INT_MAX, FLAGS }, > > + { "full_update", "Update not only bitstream, but also AVCodecParameters.", > + OFFSET(full_update), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, FLAGS}, I don't think this needs an option. It should be the default behavior. The BSF framework is written in a way that changing AVCodecParameters values is expected (Or necessary, like in cases where it adds extradata). > + > { NULL } > }; > >
James Almer: > On 12/1/2018 4:28 PM, Andreas Rheinhardt wrote: >> Up until now, this BSF only changed the bitstream, not the >> AVCodecParameters. The result is that e.g. some muxers use outdated >> information to write header information that conflicts with (and precedes) >> the new information at the bitstream level, so that the updates might >> not lead to the desired change. >> This commit changes this. It adds a mechanism by which the new >> information can be used to update the AVCodecParameters, too. This is >> the new default and an option has been added to revert to the old >> behaviour. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com> ... >> +@item full_update >> +If this is set, the AVCodecParameters are updated in addition to the >> +bitstream. If unset, muxers might add header information based upon the >> +old AVCodecParameters that contradicts and potentially precedes the >> +changes made at the bitstream level. On by default. >> + ... >> + { "full_update", "Update not only bitstream, but also AVCodecParameters.", >> + OFFSET(full_update), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, FLAGS}, > > I don't think this needs an option. It should be the default behavior. > The BSF framework is written in a way that changing AVCodecParameters > values is expected (Or necessary, like in cases where it adds extradata). My rationale was that these are bitstream filters; they are meant to change the bitstream, but with this new option, they can also lead to a change at the container level. Now most users will want to align the information of these two layers (and this is the reason that updating is the default behaviour), but there might be a niche case where this is not desired. So I added this option. But of course I don't insist on it and am fine with either way. - Andreas PS: If changing AVCodecParameters is expected, then how do I change the aspect ratio without breaking muxing?
diff --git a/doc/bitstream_filters.texi b/doc/bitstream_filters.texi index 15c578aa8a..80df345f26 100644 --- a/doc/bitstream_filters.texi +++ b/doc/bitstream_filters.texi @@ -87,6 +87,12 @@ the timing info in the sequence header. Set the number of ticks in each picture, to indicate that the stream has a fixed framerate. Ignored if @option{tick_rate} is not also set. +@item full_update +If this is set, the AVCodecParameters are updated in addition to the +bitstream. If unset, muxers might add header information based upon the +old AVCodecParameters that contradicts and potentially precedes the +changes made at the bitstream level. On by default. + @end table @section chomp diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c index 52d383661f..0c8d00acea 100644 --- a/libavcodec/av1_metadata_bsf.c +++ b/libavcodec/av1_metadata_bsf.c @@ -46,11 +46,15 @@ typedef struct AV1MetadataContext { AVRational tick_rate; int num_ticks_per_picture; + + int full_update; } AV1MetadataContext; static int av1_metadata_update_sequence_header(AVBSFContext *bsf, - AV1RawSequenceHeader *seq) + AV1RawSequenceHeader *seq, + int *color_range, + int *chroma_sample_position) { AV1MetadataContext *ctx = bsf->priv_data; AV1RawColorConfig *clc = &seq->color_config; @@ -82,6 +86,8 @@ static int av1_metadata_update_sequence_header(AVBSFContext *bsf, "on RGB streams encoded in BT.709 sRGB.\n"); } else { clc->color_range = ctx->color_range; + if (color_range) + *color_range = ctx->color_range; } } @@ -91,6 +97,8 @@ static int av1_metadata_update_sequence_header(AVBSFContext *bsf, "can only be set for 4:2:0 streams.\n"); } else { clc->chroma_sample_position = ctx->chroma_sample_position; + if (chroma_sample_position) + *chroma_sample_position = ctx->chroma_sample_position; } } @@ -135,7 +143,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *out) for (i = 0; i < frag->nb_units; i++) { if (frag->units[i].type == AV1_OBU_SEQUENCE_HEADER) { obu = frag->units[i].content; - err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header); + err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header, + NULL, NULL); if (err < 0) goto fail; } @@ -184,7 +193,7 @@ static int av1_metadata_init(AVBSFContext *bsf) AV1MetadataContext *ctx = bsf->priv_data; CodedBitstreamFragment *frag = &ctx->access_unit; AV1RawOBU *obu; - int err, i; + int err, i, color_range = -1, chroma_sample_position = -1; err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_AV1, bsf); if (err < 0) @@ -200,7 +209,8 @@ static int av1_metadata_init(AVBSFContext *bsf) for (i = 0; i < frag->nb_units; i++) { if (frag->units[i].type == AV1_OBU_SEQUENCE_HEADER) { obu = frag->units[i].content; - err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header); + err = av1_metadata_update_sequence_header(bsf, &obu->obu.sequence_header, + &color_range, &chroma_sample_position); if (err < 0) goto fail; } @@ -213,6 +223,28 @@ static int av1_metadata_init(AVBSFContext *bsf) } } + if (ctx->full_update) { + if (chroma_sample_position >= 0) { + const int conversion_table[4] = { AVCHROMA_LOC_UNSPECIFIED, + AVCHROMA_LOC_LEFT, + AVCHROMA_LOC_TOPLEFT, + AVCHROMA_LOC_UNSPECIFIED }; + chroma_sample_position = conversion_table[chroma_sample_position]; + } + + if (ctx->color_range >= 0) + ctx->color_range++; + + ff_cbs_update_video_parameters(ctx->cbc, bsf->par_out, -1, -1, -1, -1, + -1, color_range, ctx->color_primaries, + ctx->transfer_characteristics, + ctx->matrix_coefficients, + chroma_sample_position, -1); + + if (ctx->color_range != -1) + ctx->color_range--; + } + err = 0; fail: ff_cbs_fragment_uninit(ctx->cbc, frag); @@ -273,6 +305,9 @@ static const AVOption av1_metadata_options[] = { OFFSET(num_ticks_per_picture), AV_OPT_TYPE_INT, { .i64 = -1 }, -1, INT_MAX, FLAGS }, + { "full_update", "Update not only bitstream, but also AVCodecParameters.", + OFFSET(full_update), AV_OPT_TYPE_INT, { .i64 = 1 }, 0, 1, FLAGS}, + { NULL } };
Up until now, this BSF only changed the bitstream, not the AVCodecParameters. The result is that e.g. some muxers use outdated information to write header information that conflicts with (and precedes) the new information at the bitstream level, so that the updates might not lead to the desired change. This commit changes this. It adds a mechanism by which the new information can be used to update the AVCodecParameters, too. This is the new default and an option has been added to revert to the old behaviour. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@googlemail.com> --- doc/bitstream_filters.texi | 6 +++++ libavcodec/av1_metadata_bsf.c | 43 +++++++++++++++++++++++++++++++---- 2 files changed, 45 insertions(+), 4 deletions(-)