Nicolas George:
> Andreas Rheinhardt (12020-08-13):
>> The channel layouts accepted by ff_merge_channel_layouts() are of two
>> types: Ordinary channel layouts and generic channel layouts. These are
>> layouts that match all layouts with a certain number of channels.
>> Therefore parsing these channel layouts is not done in one go; instead
>> first the intersection of the ordinary layouts of the first input
>> list of channel layouts with the ordinary layouts of the second list is
>> determined, then the intersection of the ordinary layouts of the first
>> one and the generic layouts of the second one etc. In order to mark the
>> ordinary channel layouts that have already been matched as used they are
>> zeroed. The inner loop that does this is as follows:
>>
>> 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];
>> a->channel_layouts[i] = b->channel_layouts[j] = 0;
>> }
>> }
>>
>> (Here ret->channel_layouts is the array containing the intersection of
>> the two input arrays.)
>>
>> Yet the problem with this code is that after a match has been found, the
>> loop continues the search with the new value a->channel_layouts[i].
>> The intention of zeroing these elements was to make sure that elements
>> already paired at this stage are ignored later. And while they are indeed
>> ignored when pairing ordinary and generic channel layouts later, it has
>> the exact opposite effect when pairing ordinary channel layouts.
>>
>> To see this consider the channel layouts A B C D E and E D C B A. In the
>> first round, A and A will be paired and added to ret->channel_layouts.
>> In the second round, the input arrays are 0 B C D E and E D C B 0.
>> At first B and B will be matched and zeroed, but after doing so matching
>> continues, but this time it will search for 0, which will match with the
>> last entry of the second array. ret->channel_layouts now contains A B 0.
>> In the third round, C 0 0 will be added to ret->channel_layouts etc.
>> This gives a quadratic amount of elements, yet the amount of elements
>> allocated for said array is only the sum of the sizes of a and b.
>>
>> This issue can e.g. be reproduced by
>> ffmpeg -f lavfi -i anullsrc=cl=7.1 \
>> -af 'aformat=cl=mono|stereo|2.1|3.0|4.0,aformat=cl=4.0|3.0|2.1|stereo|mono' \
>> -f null -
>
> Oh, good catch. It is a bug indeed.
>
>> The fix is easy: break out of the inner loop after having found a match.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinha...@gmail.com>
>> ---
>> 1. This patch is intended to be applied before
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-August/267790.html
>>
>
>> 2. After this patch the code will stay within the buffer provided that
>> no list contains one and the same generic channel layout multiple times
>> (ordinary channel layouts may appear arbitrary often and there may even
>> be ordinary channel layouts and generic channel layouts of the same
>> channel count in the same list). But is this actually ensured?
>> (Given that the concept of generic channel layouts is not part of the
>> public API, but just valid in AVFilterChannelLayouts the question can be
>> rephrased as: Is it possible for the (API) user to somehow create
>> generic channel layouts?)
>
> I thought it was ensured by the rest of the code, where the lists of
> supported formats are very limited. But I forgot what you found: a few
> filters accept them from the application or the user. In particular,
> bufferink can inject generic layouts.
>
> I will make a patch that checks the validity after calling
> query_formats.
>
>>
>> libavfilter/formats.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/libavfilter/formats.c b/libavfilter/formats.c
>> index 00d050e439..f39396ed81 100644
>> --- a/libavfilter/formats.c
>> +++ b/libavfilter/formats.c
>> @@ -219,6 +219,7 @@ AVFilterChannelLayouts
>> *ff_merge_channel_layouts(AVFilterChannelLayouts *a,
>> if (a->channel_layouts[i] == b->channel_layouts[j]) {
>> ret->channel_layouts[ret_nb++] = a->channel_layouts[i];
>> a->channel_layouts[i] = b->channel_layouts[j] = 0;
>> + break;
>> }
>> }
>> }
>
> I think you can add the break to the other two loops too.
>
Yes. And this even makes sure that we always stay within the buffer
regardless of whether the lists contain duplicate generic elements.
- Andreas
_______________________________________________
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".
