[FFmpeg-devel,10/18] 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 make sure that the index is valid.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Several callers already ignored to check the return value.

 libavcodec/av1_metadata_bsf.c       | 9 ++-------
 libavcodec/cbs.h                    | 2 ++
 libavcodec/h264_metadata_bsf.c      | 7 +------
 libavcodec/h264_redundant_pps_bsf.c | 4 +---
 4 files changed, 6 insertions(+), 16 deletions(-)

On 6/17/2019 12:42 AM, 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 make sure that the index is valid.

Add a comment about why you're ignoring the return values in all three
filters. If for whatever reason the filters are changed in the future
and it can no longer be ensured the call will never fail, the developer
should be aware of it.

> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
> Several callers already ignored to check the return value.
> 
>  libavcodec/av1_metadata_bsf.c       | 9 ++-------
>  libavcodec/cbs.h                    | 2 ++
>  libavcodec/h264_metadata_bsf.c      | 7 +------
>  libavcodec/h264_redundant_pps_bsf.c | 4 +---
>  4 files changed, 6 insertions(+), 16 deletions(-)
> 
> diff --git a/libavcodec/av1_metadata_bsf.c b/libavcodec/av1_metadata_bsf.c
> index 842b80c201..dafddced63 100644
> --- a/libavcodec/av1_metadata_bsf.c
> +++ b/libavcodec/av1_metadata_bsf.c
> @@ -161,13 +161,8 @@ 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)
> +                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/h264_metadata_bsf.c b/libavcodec/h264_metadata_bsf.c
> index f7ca1f0f09..d05b75be14 100644
> --- a/libavcodec/h264_metadata_bsf.c
> +++ b/libavcodec/h264_metadata_bsf.c
> @@ -428,12 +428,7 @@ 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;
> -                }
> +                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..8526b5b4c4 100644
> --- a/libavcodec/h264_redundant_pps_bsf.c
> +++ b/libavcodec/h264_redundant_pps_bsf.c
> @@ -95,9 +95,7 @@ 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;
> +                ff_cbs_delete_unit(ctx->input, au, i);
>              }
>          }
>          if (nal->type == H264_NAL_SLICE ||
>

James Almer:
> On 6/17/2019 12:42 AM, 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 make sure that the index is valid.
> 
> Add a comment about why you're ignoring the return values in all three
> filters. If for whatever reason the filters are changed in the future
> and it can no longer be ensured the call will never fail, the developer
> should be aware of it.
> 
Ok. I just noticed that the call for deleting AV1 temporal delimiters
doesn't check whether the fragment has a unit at all (in fact, the
check before that call already presupposes it). The only way for
ff_cbs_read_packet to return an empty fragment and no error is if the
packet's buf has a size of zero (this is legal according to the API if
the packet has side-data). What should be done about packets that
don't result in a single unit and more specifically about zero-sized
packets? The obvious answer would be to simply add a check to
av1_metadata like the already existing in h264_metadata; but
discarding side-data only packets is no good.
(If the zero-sized packet had new extradata as side-data, then that's
something that may be parsed and rewritten; the other side-data stuff
should simply be passed through.)

- Andreas