[FFmpeg-devel,11/31] cbs: Remove superfluous checks for ff_cbs_delete_unit

Submitted by Andreas Rheinhardt on June 19, 2019, 11:45 p.m.

Details

Message ID 20190619234521.15619-3-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt June 19, 2019, 11:45 p.m.
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(-)

Comments

Mark Thompson July 7, 2019, 9:24 p.m.
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

Patch hide | download patch | download mbox

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) {