[FFmpeg-devel] [PATCH 2/2] avcodec/h26[45]_metadata_bsf: Use separate contexts for reading/writing
Mark Thompson
sw at jkqxz.net
Tue Jul 7 00:52:19 EEST 2020
On 06/07/2020 01:53, Andreas Rheinhardt wrote:
> Currently, both bsfs used the same CodedBitstreamContext for reading and
> writing; as a consequence, the state of the writer's context at the
> beginning of writing a fragment is exactly the state of the reader after
> having read the fragment; in particular, the writer might not have
> encountered one of its active parameter sets yet.
>
> This is not nice and may lead to invalid output even when the input
> is completely spec-compliant: Think of an access unit containing
> a primary coded picture referencing a PPS with id id (that is known from
> an earlier access unit/from extradata), then a new version of the PPS
> with id id and then a redundant coded picture that is also referencing
> the PPS with id id. This is spec-compliant, as the standard allows to
> overwrite a PPS with a different PPS in between coded pictures and not
> only at the beginning of an access unit. In this scenario, the reader
> would read the primary coded picture with the old PPS and the redundant
> coded picture with the new PPS (as it should); yet the writer would
> write both with the new PPS as extradata which might lead to errors or
> to invalid data being output without any error (e.g. if the two PPS
> differed in redundant_pic_cnt_present_flag).
>
> The above scenario does not directly translate to HEVC as long as one
> restricts oneself to input with nuh_layer_id == 0 only (as cbs_h265
> does: it currently strips away any NAL unit with nuh_layer_id > 0 when
> decomposing); if one doesn't the same issue as above can happen.
>
> If one also allowed input packets to contain more than one access unit,
> issues like the above can happen even without redundant coded
> pictures/multiple layers.
>
> Therefore this commit uses separate contexts for reader and writer.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> This is an alternative to James patch [1] which instead uses separate
> reader and writer parameter sets in the same CodedBitstreamContext.
I do prefer this approach.
> The diff would be bigger if it were not for the preceding patch (in this
> case one would have to choose one of the two contexts to add/delete
> units and as soon as one has to do this, one notices that none of the
> two choices make any sense).
>
> libavcodec/h264_metadata_bsf.c | 23 ++++++++++++++---------
> libavcodec/h265_metadata_bsf.c | 23 ++++++++++++++---------
> 2 files changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index 09aa1765fd..9f081a3879 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -49,7 +49,8 @@ enum {
> typedef struct H264MetadataContext {
> const AVClass *class;
>
> - CodedBitstreamContext *cbc;
> + CodedBitstreamContext *cbc_in;
> + CodedBitstreamContext *cbc_out;
The old name "cbc" there was just a placeholder because it couldn't be "". Given that, just "in" and "out" might be nicer, or "read" and "write"? (Feel free to ignore this if you don't agree.)
> CodedBitstreamFragment access_unit;
>
> int done_first_au;
> @@ -289,7 +290,7 @@ static int h264_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
> if (!side_data_size)
> return 0;
>
> - err = ff_cbs_read(ctx->cbc, au, side_data, side_data_size);
> + err = ff_cbs_read(ctx->cbc_in, au, side_data, side_data_size);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read extradata from packet side data.\n");
> return err;
> @@ -303,7 +304,7 @@ static int h264_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
> }
> }
>
> - err = ff_cbs_write_fragment_data(ctx->cbc, au);
> + err = ff_cbs_write_fragment_data(ctx->cbc_out, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write extradata into packet side data.\n");
> return err;
> @@ -334,7 +335,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> if (err < 0)
> goto fail;
>
> - err = ff_cbs_read_packet(ctx->cbc, au, pkt);
> + err = ff_cbs_read_packet(ctx->cbc_in, au, pkt);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n");
> goto fail;
> @@ -602,7 +603,7 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> }
> }
>
> - err = ff_cbs_write_packet(ctx->cbc, pkt, au);
> + err = ff_cbs_write_packet(ctx->cbc_out, pkt, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
> goto fail;
> @@ -626,12 +627,15 @@ static int h264_metadata_init(AVBSFContext *bsf)
> CodedBitstreamFragment *au = &ctx->access_unit;
> int err, i;
>
> - err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_H264, bsf);
> + err = ff_cbs_init(&ctx->cbc_in, AV_CODEC_ID_H264, bsf);
> + if (err < 0)
> + return err;
> + err = ff_cbs_init(&ctx->cbc_out, AV_CODEC_ID_H264, bsf);
> if (err < 0)
> return err;
>
> if (bsf->par_in->extradata) {
> - err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in);
> + err = ff_cbs_read_extradata(ctx->cbc_in, au, bsf->par_in);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
> goto fail;
> @@ -645,7 +649,7 @@ static int h264_metadata_init(AVBSFContext *bsf)
> }
> }
>
> - err = ff_cbs_write_extradata(ctx->cbc, bsf->par_out, au);
> + err = ff_cbs_write_extradata(ctx->cbc_out, bsf->par_out, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write extradata.\n");
> goto fail;
> @@ -663,7 +667,8 @@ static void h264_metadata_close(AVBSFContext *bsf)
> H264MetadataContext *ctx = bsf->priv_data;
>
> ff_cbs_fragment_free(&ctx->access_unit);
> - ff_cbs_close(&ctx->cbc);
> + ff_cbs_close(&ctx->cbc_in);
> + ff_cbs_close(&ctx->cbc_out);
> }
>
> #define OFFSET(x) offsetof(H264MetadataContext, x)
> diff --git a/libavcodec/h265_metadata_bsf.c b/libavcodec/h265_metadata_bsf.c
> index b48a0bd3e9..57b248542c 100644
> --- a/libavcodec/h265_metadata_bsf.c
> +++ b/libavcodec/h265_metadata_bsf.c
> @@ -40,7 +40,8 @@ enum {
> typedef struct H265MetadataContext {
> const AVClass *class;
>
> - CodedBitstreamContext *cbc;
> + CodedBitstreamContext *cbc_in;
> + CodedBitstreamContext *cbc_out;
> CodedBitstreamFragment access_unit;
>
> H265RawAUD aud_nal;
> @@ -350,7 +351,7 @@ static int h265_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
> if (!side_data_size)
> return 0;
>
> - err = ff_cbs_read(ctx->cbc, au, side_data, side_data_size);
> + err = ff_cbs_read(ctx->cbc_in, au, side_data, side_data_size);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read extradata from packet side data.\n");
> return err;
> @@ -372,7 +373,7 @@ static int h265_metadata_update_side_data(AVBSFContext *bsf, AVPacket *pkt)
> }
> }
>
> - err = ff_cbs_write_fragment_data(ctx->cbc, au);
> + err = ff_cbs_write_fragment_data(ctx->cbc_out, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write extradata into packet side data.\n");
> return err;
> @@ -402,7 +403,7 @@ static int h265_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> if (err < 0)
> goto fail;
>
> - err = ff_cbs_read_packet(ctx->cbc, au, pkt);
> + err = ff_cbs_read_packet(ctx->cbc_in, au, pkt);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read packet.\n");
> goto fail;
> @@ -473,7 +474,7 @@ static int h265_metadata_filter(AVBSFContext *bsf, AVPacket *pkt)
> }
> }
>
> - err = ff_cbs_write_packet(ctx->cbc, pkt, au);
> + err = ff_cbs_write_packet(ctx->cbc_out, pkt, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write packet.\n");
> goto fail;
> @@ -495,12 +496,15 @@ static int h265_metadata_init(AVBSFContext *bsf)
> CodedBitstreamFragment *au = &ctx->access_unit;
> int err, i;
>
> - err = ff_cbs_init(&ctx->cbc, AV_CODEC_ID_HEVC, bsf);
> + err = ff_cbs_init(&ctx->cbc_in, AV_CODEC_ID_HEVC, bsf);
> + if (err < 0)
> + return err;
> + err = ff_cbs_init(&ctx->cbc_out, AV_CODEC_ID_HEVC, bsf);
> if (err < 0)
> return err;
>
> if (bsf->par_in->extradata) {
> - err = ff_cbs_read_extradata(ctx->cbc, au, bsf->par_in);
> + err = ff_cbs_read_extradata(ctx->cbc_in, au, bsf->par_in);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to read extradata.\n");
> goto fail;
> @@ -522,7 +526,7 @@ static int h265_metadata_init(AVBSFContext *bsf)
> }
> }
>
> - err = ff_cbs_write_extradata(ctx->cbc, bsf->par_out, au);
> + err = ff_cbs_write_extradata(ctx->cbc_out, bsf->par_out, au);
> if (err < 0) {
> av_log(bsf, AV_LOG_ERROR, "Failed to write extradata.\n");
> goto fail;
> @@ -540,7 +544,8 @@ static void h265_metadata_close(AVBSFContext *bsf)
> H265MetadataContext *ctx = bsf->priv_data;
>
> ff_cbs_fragment_free(&ctx->access_unit);
> - ff_cbs_close(&ctx->cbc);
> + ff_cbs_close(&ctx->cbc_in);
> + ff_cbs_close(&ctx->cbc_out);
> }
>
> #define OFFSET(x) offsetof(H265MetadataContext, x)
>
LGTM in any case, though wait for James to comment too.
Thanks,
- Mark
More information about the ffmpeg-devel
mailing list