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

[Andreas Rheinhardt]

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.
>> @@ -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.
I thought about this and came to the conclusion that the amount of
allocations that could be saved is not really decent, but negligible:
Usually the size of the unit array stayed in the single digits (e.g.
for H.264 Blurays it's usually four slices, two parameter sets and a
few SEI units). The highest number I found (I didn't try to create
content with artificially many units) were 72 or so from an MPEG-2
Bluray, but that's already an outlier.
Sometimes (when splitting H2645 and when splitting VP9 superframes)
the number of units needed is already known before adding the first
one. But given the low numbers involved, I have not attempted to use
this in order to decrease the number of allocations.
> 
>>  
>> -    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.
> 
Ok. My rationale was consistency with H2645Packet.
>> +
>>      /**
>> -     * 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.
Ok. Hope to send an updated patch today.

- Andreas