[FFmpeg-devel] [PATCH 2/2] avcodec/h26[45]_metadata_bsf: Use separate contexts for reading/writing
James Almer
jamrial at gmail.com
Tue Jul 7 05:48:33 EEST 2020
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 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.
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 at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
More information about the ffmpeg-devel
mailing list