Re: [FFmpeg-devel] [PATCH 11/31] cbs: Remove superfluous checks for ff_cbs_delete_unit
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
> ---
> 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
[FFmpeg-devel] [PATCH 11/31] cbs: Remove superfluous checks for ff_cbs_delete_unit
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
---
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);
} 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
