[FFmpeg-devel] [PATCH 5/6] avfilter/formats: Simplify cleanup for ff_merge_* functions

[Andreas Rheinhardt]

Nicolas George:
> Andreas Rheinhardt (12020-08-08):
>> I was thinking about this part of the MERGE_FORMATS macro:
>>
>>         for (i = 0; i < a->nb; i++)
>>             for (j = 0; j < b->nb; j++)
>>                 if (a->fmts[i] == b->fmts[j]) {
>>                     if(k >= FFMIN(a->nb, b->nb)){
>>                         av_log(NULL, AV_LOG_ERROR, "Duplicate formats in
>> %s detected\n", __FUNCTION__);
>>                         av_free(ret->fmts);
>>                         av_free(ret);
>>                         return NULL;
>>                     }
>>                     ret->fmts[k++] = a->fmts[i];
>>                 }
>>
>> Said check is not sufficient to ensure that there are no duplicates in
>> ret->fmts, of course. It has been added in the merge commit 1cbf7fb4345
>> as an additional safety check to ensure that one does not write beyond
>> the end of ret->fmts (it has FFMIN(a->nb, b->nb) entries). Yet this goal
>> could also be achieved in a simpler way: If a->nb <= b->nb, one could
>> simply rewrite the above to
> 
> This is only a protection against internal bugs, since lists should not
> contain duplicates in the first place. As such it should have been an
> assert. If we no longer need it, it would probably be better to make a
> conditional complete test for duplicates somewhere.
> 
>>         for (i = 0; i < a->nb; i++)
>>             for (j = 0; j < b->nb; j++)
>>                 if (a->fmts[i] == b->fmts[j]) {
>>                     ret->fmts[k++] = a->fmts[i];
>>                     break;
>>                 }
>>
>> This ensures that every entry of a will end up at most once in ret. And
>> if one were simply allowed to switch a and b, one could ensure a->nb <=
>> b->nb easily. ff_merge_channel_layouts() already does something similar.
> 
> I noticed something similar when refreshing my memory about this.
> Indeed, breaking out of the loop when a match is found would make the
> same guarantee, and a nice optimization for a quadratic algorithm.
> 
> Now, I can confirm, the order of formats in the array is not relevant
> for the rest of the negotiation.
> 

Sure about that? The filter-framerate-up test fails (the output pixel
format is now YUV9 instead of I420) when I switch a and b in
MERGE_FORMATS if b->nb < a->nb. What you said seems to only apply to
sample rates, not format negotiations.

One can of course still avoid the allocations, if one is either willing
to keep the bigger of the two formats buffers or if one reallocates the
buffer to the correct (usually even smaller) size afterwards. I'll go
with the latter.

- Andreas