[FFmpeg-devel] [PATCH] lavfi/avfiltergraph: always reduce all_layouts to a single layout

[Marton Balint]

On Sun, 27 Nov 2016, Nicolas George wrote:

> Le tridi 3 frimaire, an CCXXV, Marton Balint a écrit :
>> I thought we are trying to move away from workarounds introduced only to be
>> able to be compatible with libav API. So this is clearly libav dirving
>> ffmpeg development, which is not fortunate IMHO.
>>
>> I also think that the chance of an exploitable filter because of this is
>> small. An audio filter with no query_formats is quite rare in itself. Even
>> if such a filter got merged, making it work with unknown channel layouts is
>> a feature which we would want anyway, becase ffmpeg does support unknown
>> channel layouts.
>>
>> Yes, it is not me who does the merges, but IMHO this does not add too much
>> burden for the people who does it. Hendrik, Clement, what do you think?
>>
>> And even if such an issue got in the codebase, isn't this something that
>> coverity should be able to easily detect most of the times?
>
> In principle, I think we would want to get rid of the work-ardounds and
> have all filters support unknown layouts. But we also want our users to
> have a working and reliable tool. Meeting both objectives requires
> auditing and testing.
>
> If we make unknown layouts the default and a filter that does not work
> with them sneaks through, it will sometimes use 0 as the number of
> channels. Depending on how much it does so, it can have several
> unfortunate consequences, most probably either a generic error message
> (probably "invalid argument" or "out of memory"), a corrupted output or
> a crash (and unless proven otherwise, a crash must be assumed to be
> exploitable).
>
> Automated testing can not help us catching them. Neither FATE nor
> coverity, the way they currently work.
>
> On the other hand, detecting filters that do not work correctly is not
> very hard in itself. The use of av_get_channel_layout_nb_channels() is a
> very reliable condition. But it could be hidden in a call to an external
> function. Making them work is usually rather easy too: replace
> av_get_channel_layout_nb_channels() with the corresponding "channels"
> field.
>
> Here is, to the best of my knowledge, the current state of affairs. With
> that information, we can decide to make unknown layouts the default. But
> that is not my decision alone.
>
> If it happens, I will try to help paying attention to new filters, but I
> can not promise I will succeed catching them all.
>
> Also, if we decide to proceed, I think we should not just make
> all_counts the default: the all_counts / all_layouts logic is rather
> complex, and with all_counts the default it becomes essentially dead
> code. But the decision comes first.
>

There can be filters where .query_format is defined and they still can 
refer to all_formats and not all_counts, so i am not sure we can remove 
the all_formats/all_counts logic so easily.

Anyway, how can we move forward here? Apparently there is not too much 
interest in the topic... Since this can be easily changed later, I can 
also rework the patch to add the .query_formats callback to all filters 
currently not supporting unkown layouts until this is decided. What do you 
think? Obviously I prefer my original approach, but since it is not too 
much work, I can change the patch as well.

Thanks,
Marton