| Message ID | 20190619234521.15619-3-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On 20/06/2019 00:45, Andreas Rheinhardt wrote: > ff_cbs_delete_unit never fails if the index of the unit to delete is > valid; document this behaviour explicitly and remove the checks for > whether ff_cbs_delete_unit failed, because all the callers of > ff_cbs_delete_unit already made sure the index to be valid. And add some > comments to the callers to ensure that it stays that way. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/av1_metadata_bsf.c | 12 +++++------- > libavcodec/cbs.h | 2 ++ > libavcodec/filter_units_bsf.c | 1 + > libavcodec/h264_metadata_bsf.c | 10 ++++------ > libavcodec/h264_redundant_pps_bsf.c | 5 ++--- > libavcodec/h265_metadata_bsf.c | 2 ++ > 6 files changed, 16 insertions(+), 16 deletions(-) > > diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c > index bb2ca2075b..9345095277 100644 > --- a/libavcodec/av1_metadata_bsf.c > +++ b/libavcodec/av1_metadata_bsf.c > @@ -151,6 +151,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > // If a Temporal Delimiter is present, it must be the first OBU. > if (frag->units[0].type == AV1_OBU_TEMPORAL_DELIMITER) { > if (ctx->td == REMOVE) > + // This call never fails because we have already excluded > + // fragments without a single unit. > ff_cbs_delete_unit(ctx->cbc, frag, 0); I would prefer to always keep braces in things of the form: if (cond) // Something something // Blah blah actual code; > } else if (ctx->td == INSERT) { > td = (AV1RawOBU) { > @@ -167,13 +169,9 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > > if (ctx->delete_padding) { > for (i = frag->nb_units - 1; i >= 0; i--) { > - if (frag->units[i].type == AV1_OBU_PADDING) { > - err = ff_cbs_delete_unit(ctx->cbc, frag, i); > - if (err < 0) { > - av_log(bsf, AV_LOG_ERROR, "Failed to delete Padding OBU.\n"); > - goto fail; > - } > - } > + if (frag->units[i].type == AV1_OBU_PADDING) > + // This call never fails as the fragment has a unit at pos. i. > + ff_cbs_delete_unit(ctx->cbc, frag, i); > } > } > > diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h > index 5260a39c63..e1e6055ceb 100644 > --- a/libavcodec/cbs.h > +++ b/libavcodec/cbs.h > @@ -380,6 +380,8 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, > > /** > * Delete a unit from a fragment and free all memory it uses. > + * > + * Never returns failure if position is >= 0 and < frag->nb_units. > */ > int ff_cbs_delete_unit(CodedBitstreamContext *ctx, > CodedBitstreamFragment *frag, > diff --git a/libavcodec/filter_units_bsf.c b/libavcodec/filter_units_bsf.c > index 380f23e5a7..3068e464f0 100644 > --- a/libavcodec/filter_units_bsf.c > +++ b/libavcodec/filter_units_bsf.c > @@ -124,6 +124,7 @@ static int filter_units_filter(AVBSFContext *bsf, AVPacket *pkt) > } > if (ctx->mode == REMOVE ? j < ctx->nb_types > : j >= ctx->nb_types) > + // This call never fails as the fragment has a unit at pos. i. > ff_cbs_delete_unit(ctx->cbc, frag, i); > } > > diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c > index f7ca1f0f09..e3c29cc9ad 100644 > --- a/libavcodec/h264_metadata_bsf.c > +++ b/libavcodec/h264_metadata_bsf.c > @@ -309,6 +309,8 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > // If an AUD is present, it must be the first NAL unit. > if (au->units[0].type == H264_NAL_AUD) { > if (ctx->aud == REMOVE) > + // This call never fails because we have already excluded > + // access units without a single unit. > ff_cbs_delete_unit(ctx->cbc, au, 0); > } else { > if (ctx->aud == INSERT) { > @@ -428,12 +430,8 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) > for (i = au->nb_units - 1; i >= 0; i--) { > if (au->units[i].type == H264_NAL_FILLER_DATA) { > // Filler NAL units. > - err = ff_cbs_delete_unit(ctx->cbc, au, i); > - if (err < 0) { > - av_log(bsf, AV_LOG_ERROR, "Failed to delete " > - "filler NAL.\n"); > - goto fail; > - } > + // This call never fails as the fragment has a unit at pos. i. > + ff_cbs_delete_unit(ctx->cbc, au, i); > continue; > } > > diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c > index db8717d69a..f59c4f57c0 100644 > --- a/libavcodec/h264_redundant_pps_bsf.c > +++ b/libavcodec/h264_redundant_pps_bsf.c > @@ -95,9 +95,8 @@ static int h264_redundant_pps_filter(AVBSFContext *bsf, AVPacket *out) > if (!au_has_sps) { > av_log(bsf, AV_LOG_VERBOSE, "Deleting redundant PPS " > "at %"PRId64".\n", in->pts); > - err = ff_cbs_delete_unit(ctx->input, au, i); > - if (err < 0) > - goto fail; > + // This call never fails as the fragment has a unit at pos. i. > + ff_cbs_delete_unit(ctx->input, au, i); > } > } > if (nal->type == H264_NAL_SLICE || > diff --git a/libavcodec/h265_metadata_bsf.c b/libavcodec/h265_metadata_bsf.c > index 0683cc2f9d..3436f3d0a3 100644 > --- a/libavcodec/h265_metadata_bsf.c > +++ b/libavcodec/h265_metadata_bsf.c > @@ -256,6 +256,8 @@ static int h265_metadata_filter(AVBSFContext *bsf, AVPacket *out) > // If an AUD is present, it must be the first NAL unit. > if (au->units[0].type == HEVC_NAL_AUD) { > if (ctx->aud == REMOVE) > + // This call never fails because we have already excluded > + // access units without a single unit. > ff_cbs_delete_unit(ctx->cbc, au, 0); > } else { > if (ctx->aud == INSERT) { > 10-11 LGTM. - Mark
diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c index bb2ca2075b..9345095277 100644 --- a/libavcodec/av1_metadata_bsf.c +++ b/libavcodec/av1_metadata_bsf.c @@ -151,6 +151,8 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) // If a Temporal Delimiter is present, it must be the first OBU. if (frag->units[0].type == AV1_OBU_TEMPORAL_DELIMITER) { if (ctx->td == REMOVE) + // This call never fails because we have already excluded + // fragments without a single unit. ff_cbs_delete_unit(ctx->cbc, frag, 0); } else if (ctx->td == INSERT) { td = (AV1RawOBU) { @@ -167,13 +169,9 @@ static int av1_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) if (ctx->delete_padding) { for (i = frag->nb_units - 1; i >= 0; i--) { - if (frag->units[i].type == AV1_OBU_PADDING) { - err = ff_cbs_delete_unit(ctx->cbc, frag, i); - if (err < 0) { - av_log(bsf, AV_LOG_ERROR, "Failed to delete Padding OBU.\n"); - goto fail; - } - } + if (frag->units[i].type == AV1_OBU_PADDING) + // This call never fails as the fragment has a unit at pos. i. + ff_cbs_delete_unit(ctx->cbc, frag, i); } } diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h index 5260a39c63..e1e6055ceb 100644 --- a/libavcodec/cbs.h +++ b/libavcodec/cbs.h @@ -380,6 +380,8 @@ int ff_cbs_insert_unit_data(CodedBitstreamContext *ctx, /** * Delete a unit from a fragment and free all memory it uses. + * + * Never returns failure if position is >= 0 and < frag->nb_units. */ int ff_cbs_delete_unit(CodedBitstreamContext *ctx, CodedBitstreamFragment *frag, diff --git a/libavcodec/filter_units_bsf.c b/libavcodec/filter_units_bsf.c index 380f23e5a7..3068e464f0 100644 --- a/libavcodec/filter_units_bsf.c +++ b/libavcodec/filter_units_bsf.c @@ -124,6 +124,7 @@ static int filter_units_filter(AVBSFContext *bsf, AVPacket *pkt) } if (ctx->mode == REMOVE ? j < ctx->nb_types : j >= ctx->nb_types) + // This call never fails as the fragment has a unit at pos. i. ff_cbs_delete_unit(ctx->cbc, frag, i); } diff --git a/libavcodec/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c index f7ca1f0f09..e3c29cc9ad 100644 --- a/libavcodec/h264_metadata_bsf.c +++ b/libavcodec/h264_metadata_bsf.c @@ -309,6 +309,8 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) // If an AUD is present, it must be the first NAL unit. if (au->units[0].type == H264_NAL_AUD) { if (ctx->aud == REMOVE) + // This call never fails because we have already excluded + // access units without a single unit. ff_cbs_delete_unit(ctx->cbc, au, 0); } else { if (ctx->aud == INSERT) { @@ -428,12 +430,8 @@ static int h264_metadata_filter(AVBSFContext *bsf, AVPacket *pkt) for (i = au->nb_units - 1; i >= 0; i--) { if (au->units[i].type == H264_NAL_FILLER_DATA) { // Filler NAL units. - err = ff_cbs_delete_unit(ctx->cbc, au, i); - if (err < 0) { - av_log(bsf, AV_LOG_ERROR, "Failed to delete " - "filler NAL.\n"); - goto fail; - } + // This call never fails as the fragment has a unit at pos. i. + ff_cbs_delete_unit(ctx->cbc, au, i); continue; } diff --git a/libavcodec/h264_redundant_pps_bsf.c b/libavcodec/h264_redundant_pps_bsf.c index db8717d69a..f59c4f57c0 100644 --- a/libavcodec/h264_redundant_pps_bsf.c +++ b/libavcodec/h264_redundant_pps_bsf.c @@ -95,9 +95,8 @@ static int h264_redundant_pps_filter(AVBSFContext *bsf, AVPacket *out) if (!au_has_sps) { av_log(bsf, AV_LOG_VERBOSE, "Deleting redundant PPS " "at %"PRId64".\n", in->pts); - err = ff_cbs_delete_unit(ctx->input, au, i); - if (err < 0) - goto fail; + // This call never fails as the fragment has a unit at pos. i. + ff_cbs_delete_unit(ctx->input, au, i); } } if (nal->type == H264_NAL_SLICE || diff --git a/libavcodec/h265_metadata_bsf.c b/libavcodec/h265_metadata_bsf.c index 0683cc2f9d..3436f3d0a3 100644 --- a/libavcodec/h265_metadata_bsf.c +++ b/libavcodec/h265_metadata_bsf.c @@ -256,6 +256,8 @@ static int h265_metadata_filter(AVBSFContext *bsf, AVPacket *out) // If an AUD is present, it must be the first NAL unit. if (au->units[0].type == HEVC_NAL_AUD) { if (ctx->aud == REMOVE) + // This call never fails because we have already excluded + // access units without a single unit. ff_cbs_delete_unit(ctx->cbc, au, 0); } else { if (ctx->aud == INSERT) {
ff_cbs_delete_unit never fails if the index of the unit to delete is valid; document this behaviour explicitly and remove the checks for whether ff_cbs_delete_unit failed, because all the callers of ff_cbs_delete_unit already made sure the index to be valid. And add some comments to the callers to ensure that it stays that way. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/av1_metadata_bsf.c | 12 +++++------- libavcodec/cbs.h | 2 ++ libavcodec/filter_units_bsf.c | 1 + libavcodec/h264_metadata_bsf.c | 10 ++++------ libavcodec/h264_redundant_pps_bsf.c | 5 ++--- libavcodec/h265_metadata_bsf.c | 2 ++ 6 files changed, 16 insertions(+), 16 deletions(-)