[FFmpeg-devel] [PATCH 22/22] avfilter/formats: Avoid allocations when merging channel layouts

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12020-08-12):
>> When one merges two AVFilterChannelLayouts structs, there is no need to
>> allocate a new one. Instead one can reuse one of the two given ones.
>> If one does this, one also doesn't need to update the references of the
>> AVFilterChannelLayouts that is reused. Therefore this commit reuses the
>> structure with the higher refcount.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
>> ---
>> This can of course be applied independently of 7-21.
>>
>>  libavfilter/formats.c | 48 +++++++++++++++++++------------------------
>>  1 file changed, 21 insertions(+), 27 deletions(-)
>>
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index 00d050e439..2d33dd7afe 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -48,13 +48,13 @@ do {                                                                       \
>>      av_freep(&a);                                                          \
>>  } while (0)
>>  
>> -#define MERGE_REF(ret, a, fmts, type, fail)                                \
>> +#define MERGE_REF(ret, a, fmts, type, fail_statement)                      \
>>  do {                                                                       \
>>      type ***tmp;                                                           \
>>                                                                             \
>>      if (!(tmp = av_realloc_array(ret->refs, ret->refcount + a->refcount,   \
>>                                   sizeof(*tmp))))                           \
>> -        goto fail;                                                         \
>> +        { fail_statement }                                                 \
>>      ret->refs = tmp;                                                       \
>>      MERGE_REF_NO_ALLOC(ret, a, fmts);                                      \
>>  } while (0)
>> @@ -157,10 +157,10 @@ AVFilterFormats *ff_merge_samplerates(AVFilterFormats *a,
>>      if (a->nb_formats && b->nb_formats) {
>>          MERGE_FORMATS(ret, a, b, formats, nb_formats, AVFilterFormats, fail);
>>      } else if (a->nb_formats) {
>> -        MERGE_REF(a, b, formats, AVFilterFormats, fail);
>> +        MERGE_REF(a, b, formats, AVFilterFormats, return NULL;);
>>          ret = a;
>>      } else {
>> -        MERGE_REF(b, a, formats, AVFilterFormats, fail);
>> +        MERGE_REF(b, a, formats, AVFilterFormats, return NULL;);
>>          ret = b;
>>      }
>>  
>> @@ -177,7 +177,7 @@ fail:
>>  AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>                                                   AVFilterChannelLayouts *b)
>>  {
>> -    AVFilterChannelLayouts *ret = NULL;
>> +    uint64_t *channel_layouts;
>>      unsigned a_all = a->all_layouts + a->all_counts;
>>      unsigned b_all = b->all_layouts + b->all_counts;
>>      int ret_max, ret_nb = 0, i, j, round;
>> @@ -201,15 +201,13 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>                  return NULL;
>>              b->nb_channel_layouts = j;
>>          }
>> -        MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, fail);
>> +        MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, return NULL;);
>>          return b;
>>      }
>>  
>>      ret_max = a->nb_channel_layouts + b->nb_channel_layouts;
>> -    if (!(ret = av_mallocz(sizeof(*ret))) ||
>> -        !(ret->channel_layouts = av_malloc_array(ret_max,
>> -                                                 sizeof(*ret->channel_layouts))))
>> -        goto fail;
> 
>> +    if (!(channel_layouts = av_malloc_array(ret_max, sizeof(channel_layouts))))
> 
> sizeof(*channel_layouts)

True. Fixed locally.

> 
>> +        return NULL;
>>  
>>      /* a[known] intersect b[known] */
>>      for (i = 0; i < a->nb_channel_layouts; i++) {
>> @@ -217,7 +215,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>              continue;
>>          for (j = 0; j < b->nb_channel_layouts; j++) {
>>              if (a->channel_layouts[i] == b->channel_layouts[j]) {
>> -                ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>> +                channel_layouts[ret_nb++] = a->channel_layouts[i];
>>                  a->channel_layouts[i] = b->channel_layouts[j] = 0;
>>              }
>>          }
>> @@ -232,7 +230,7 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>              bfmt = FF_COUNT2LAYOUT(av_get_channel_layout_nb_channels(fmt));
>>              for (j = 0; j < b->nb_channel_layouts; j++)
>>                  if (b->channel_layouts[j] == bfmt)
>> -                    ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>> +                    channel_layouts[ret_nb++] = a->channel_layouts[i];
>>          }
>>          /* 1st round: swap to prepare 2nd round; 2nd round: put it back */
>>          FFSWAP(AVFilterChannelLayouts *, a, b);
>> @@ -243,27 +241,23 @@ AVFilterChannelLayouts *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>>              continue;
>>          for (j = 0; j < b->nb_channel_layouts; j++)
>>              if (a->channel_layouts[i] == b->channel_layouts[j])
>> -                ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>> +                channel_layouts[ret_nb++] = a->channel_layouts[i];
>>      }
>>  
>> -    ret->nb_channel_layouts = ret_nb;
>> -    if (!ret->nb_channel_layouts)
>> +    if (!ret_nb)
>>          goto fail;
>>  
>> -    ret->refs = av_realloc_array(NULL, a->refcount + b->refcount,
>> -                                 sizeof(*ret->refs));
>> -    if (!ret->refs)
>> -        goto fail;
>> -    MERGE_REF_NO_ALLOC(ret, a, channel_layouts);
>> -    MERGE_REF_NO_ALLOC(ret, b, channel_layouts);
>> -    return ret;
> 
>> +    if (a->refcount > b->refcount)
>> +        FFSWAP(AVFilterChannelLayouts *, a, b);
> 
> I do not think it is just as simple as that: if you swap, then b->refs
> points to the references of a and a->refs points to the references of b.
> Then the MERGE_REF() below will update a->refs, i.e. the references of
> b, which are already valid, and not b->refs, which need to.

No, I just swap the pointers, not the structures themselves. It just
ensures a->refcount <= b->refcount in the following code. (The same
swapping btw already happens at the beginning of the function.)

> 
> Better leave this optimization for later. Especially if we consider
> using a linked list as I suggested.
> 
>> +
>> +    MERGE_REF(b, a, channel_layouts, AVFilterChannelLayouts, goto fail;);
>> +    av_freep(&b->channel_layouts);
>> +    b->channel_layouts    = channel_layouts;
>> +    b->nb_channel_layouts = ret_nb;
>> +    return b;
>>  
>>  fail:
>> -    if (ret) {
>> -        av_assert1(!ret->refs);
>> -        av_freep(&ret->channel_layouts);
>> -        av_freep(&ret);
>> -    }
>> +    av_free(channel_layouts);
>>      return NULL;
>>  }
> 
> LGTM apart from that.
> 
> Regards,
>