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.rheinha...@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,
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
