[FFmpeg-devel] [PATCH 1/3] libavcodec/cbs: Add ability to keep the units array.

[Mark Thompson]

On 05/02/2019 20:08, Andreas Rheinhardt wrote:
> Currently, a fragment's unit array is constantly reallocated during
> splitting of a packet. This commit adds the ability to keep the unit
> array by distinguishing between the number of allocated and the number
> of valid units in the unit array.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
>  libavcodec/av1_metadata_bsf.c       |  4 +--
>  libavcodec/av1_parser.c             |  4 +--
>  libavcodec/cbs.c                    | 51 ++++++++++++++++++-----------
>  libavcodec/cbs.h                    | 19 ++++++++---
>  libavcodec/filter_units_bsf.c       |  6 ++--
>  libavcodec/h264_metadata_bsf.c      |  4 +--
>  libavcodec/h264_redundant_pps_bsf.c |  4 +--
>  libavcodec/h265_metadata_bsf.c      |  4 +--
>  libavcodec/mpeg2_metadata_bsf.c     |  4 +--
>  libavcodec/trace_headers_bsf.c      |  4 +--
>  libavcodec/vaapi_encode_h264.c      |  8 ++---
>  libavcodec/vaapi_encode_h265.c      |  8 ++---
>  libavcodec/vaapi_encode_mjpeg.c     |  2 +-
>  libavcodec/vaapi_encode_mpeg2.c     |  4 +--
>  libavcodec/vp9_metadata_bsf.c       |  2 +-
>  15 files changed, 76 insertions(+), 52 deletions(-)
> 
> ...
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index ecbf57c293..b61dedb1eb 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -137,15 +137,20 @@ static void cbs_unit_uninit(CodedBitstreamContext *ctx,
>  }
>  
>  void ff_cbs_fragment_uninit(CodedBitstreamContext *ctx,
> -                            CodedBitstreamFragment *frag)
> +                            CodedBitstreamFragment *frag,
> +                            int completely)
>  {
>      int i;
>  
>      for (i = 0; i < frag->nb_units; i++)
>          cbs_unit_uninit(ctx, &frag->units[i]);
> -    av_freep(&frag->units);
>      frag->nb_units = 0;
>  
> +    if (completely) {
> +        av_freep(&frag->units);
> +        frag->units_allocated = 0;
> +    }
> +
>      av_buffer_unref(&frag->data_ref);
>      frag->data             = NULL;
>      frag->data_size        = 0;
> @@ -548,20 +553,34 @@ static int cbs_insert_unit(CodedBitstreamContext *ctx,
>  {
>      CodedBitstreamUnit *units;
>  
> -    units = av_malloc_array(frag->nb_units + 1, sizeof(*units));
> -    if (!units)
> -        return AVERROR(ENOMEM);
> +    if (frag->nb_units < frag->units_allocated) {
> +        units = frag->units;
> +
> +        if (position < frag->nb_units)
> +            memmove(units + position + 1, frag->units + position,
> +                    (frag->nb_units - position) * sizeof(*units));
> +    } else {
> +        units = av_malloc_array(frag->nb_units + 1, sizeof(*units));
> +        if (!units)
> +            return AVERROR(ENOMEM);
> +
> +        ++frag->units_allocated;

Given your aim, I wonder whether it would be sensible to have this increase the size by more the one each time - maybe double it or add a small constant (four or eight)?  The unit structures themselves are not particularly large, so this might save a decent number of allocations at the cost of wasting perhaps few hundred bytes of memory.

>  
> -    if (position > 0)
> -        memcpy(units, frag->units, position * sizeof(*units));
> -    if (position < frag->nb_units)
> -        memcpy(units + position + 1, frag->units + position,
> -               (frag->nb_units - position) * sizeof(*units));
> +        if (position > 0)
> +            memcpy(units, frag->units, position * sizeof(*units));
> +
> +        if (position < frag->nb_units)
> +            memcpy(units + position + 1, frag->units + position,
> +                   (frag->nb_units - position) * sizeof(*units));
> +    }
>  
>      memset(units + position, 0, sizeof(*units));
>  
> -    av_freep(&frag->units);
> -    frag->units = units;
> +    if (units != frag->units) {
> +        av_free(frag->units);
> +        frag->units = units;
> +    }
> +
>      ++frag->nb_units;
>  
>      return 0;
> @@ -652,16 +671,10 @@ int ff_cbs_delete_unit(CodedBitstreamContext *ctx,
>  
>      --frag->nb_units;
>  
> -    if (frag->nb_units == 0) {
> -        av_freep(&frag->units);
> -
> -    } else {
> +    if (frag->nb_units > 0)
>          memmove(frag->units + position,
>                  frag->units + position + 1,
>                  (frag->nb_units - position) * sizeof(*frag->units));
>  
> -        // Don't bother reallocating the unit array.
> -    }
> -
>      return 0;
>  }
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 53ac360bb1..229cb129aa 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -145,10 +145,19 @@ typedef struct CodedBitstreamFragment {
>       * and has not been decomposed.
>       */
>      int              nb_units;
> +
> +    /**
> +     * Number of allocated units.
> +     *
> +     * Must always be >= nb_units; designed for internal use by cbs.
> +     */
> +     int      units_allocated;

I think call this nb_units_allocated for consistent styling.

> +
>      /**
> -     * Pointer to an array of units of length nb_units.
> +     * Pointer to an array of units of length units_allocated.
> +     * Only the first nb_units are valid.
>       *
> -     * Must be NULL if nb_units is zero.
> +     * Must be NULL if units_allocated is zero.
>       */
>      CodedBitstreamUnit *units;
>  } CodedBitstreamFragment;
> @@ -294,10 +303,12 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>  
>  
>  /**
> - * Free all allocated memory in a fragment.
> + * Free all allocated memory in a fragment except possibly the unit array
> + * itself. The unit array is only freed if completely is set.
>   */
>  void ff_cbs_fragment_uninit(CodedBitstreamContext *ctx,
> -                            CodedBitstreamFragment *frag);
> +                            CodedBitstreamFragment *frag,
> +                            int completely);

This last parameter seems a little bit magical - while the implementation is similar, in terms of actual use these are doing quite different things (in one case it's freed and you can discard it, in the other it isn't).  So, I think this function would be better split into two with clearer names.

The current name doesn't fit particularly well with either what it does at the moment (frees all content) or what your modified version does (resets all content but doesn't free things which can be reused).  So, perhaps two functions - something like "free" or "destroy" for the former, with "clear" or "reset" for the latter?  The naming is slightly confused by the structure being transparent and allocated by the user, but that doesn't affect actual use in code too much.

Thanks,

- Mark