| Message ID | 20200706005305.9136-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/cbs: Remove unused function parameters | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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@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
On 7/6/2020 6:52 PM, Mark Thompson wrote: > 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@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. Your approval is more than enough. I'll do the same for cbs_av1 while at it, which should allow us to revert 4e2bef6a82. > > Thanks, > > - Mark > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mark Thompson: > 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@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.) > I opted for "input" and "output", thereby making the naming consistent with h264_redundant_pps. - Andreas
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; 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)
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@gmail.com> --- This is an alternative to James patch [1] which instead uses separate reader and writer parameter sets in the same CodedBitstreamContext. 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(-)