[FFmpeg-devel] [PATCH 3/3] avfilter/formats: do not allow unknown layouts in ff_parse_channel_layout if nret is not set

[Marton Balint]

On Sat, 31 Dec 2016, Nicolas George wrote:

> Le sextidi 6 nivôse, an CCXXV, Marton Balint a écrit :
>> Current code returned the number of channels as channel layout in that case,
>> and if nret is not set then unknown layouts are typically not supported.
>
> Good catch.
>
> Let me see if I got this correctly:
>
> av_get_channel_layout("2c") = stereo
> av_get_channel_layout("2C") = 0
> av_get_channel_layout("13c") = 0
> av_get_channel_layout("13C") = 0
>
> av_get_extended_channel_layout("2c") = stereo / 2
> av_get_extended_channel_layout("2C") = 0 / 2
> av_get_extended_channel_layout("13c") = EINVAL
> av_get_extended_channel_layout("13C") = 0 / 13
>
> ff_parse_channel_layout():
>
>                     before                  after
> nret == NULL  2c     stereo                  stereo
> nret == NULL  2C     EINVAL                  EINVAL
> nret == NULL  13c    (int64)13 = FL+FC+LF    EINVAL
> nret == NULL  13C    EINVAL                  EINVAL
> nret != NULL  2c     stereo / 2              stereo / 2
> nret != NULL  2C     EINVAL                  0 / 2
> nret != NULL  13c    0 / 13                  EINVAL
> nret != NULL  13C    EINVAL                  0 / 13
>
> Do we agree?

Yes.

>
>> Also use the common parsing code. This breaks a very specific case, using
>> af_pan with an unknown channel layout such as '13c', from now on, only '13C'
>> will work.
>
> I think you could easily add a temporary workaround and a warning.
>
> (I suggest, from now on, we flag temporary workarounds and warnings with
> a standardized comment: "/* [TEMPORARY 2016-12 -> 2017-12]*/"; that way,
> we can find them using git grep.)

Ok.

>> -    chlayout = av_get_channel_layout(arg);
>> -    if (chlayout == 0) {
>> -        chlayout = strtol(arg, &tail, 10);
>
>> -        if (!(*tail == '\0' || *tail == 'c' && *(tail + 1) == '\0') || chlayout <= 0 || chlayout > 63) {
>
> This is losing the >63 test since av_get_extended_channel_layout() tests
> for >64. lavfi can not deal with channel layouts with channel #63 set,
> because the bit serves to mark channel counts disguised as layouts;
> channel #63 is not defined in lavu anyway. Unknown channel layouts
> should work with even more than 64 channels, but that would require
> testing.

As far as I see the old code allowed 63 channels but not 64. 
av_get_extended_channel_layout also works like this, although the 
conditions there are negated.

Do you want me to disallow 63 channels as well? Or allow 64?

I also thought about more than 63/64 channels, but unknown layouts still 
need some love even after my current patches, so I think we are not there 
yet.

>
>> -            av_log(log_ctx, AV_LOG_ERROR, "Invalid channel layout '%s'\n", arg);
>> -            return AVERROR(EINVAL);
>> -        }
>> -        if (nret) {
>> -            *nret = chlayout;
>> -            *ret = 0;
>> -            return 0;
>> -        }
>
>> +    if (av_get_extended_channel_layout(arg, &chlayout, &nb_channels) < 0 || (!chlayout && !nret)) {
>> +        av_log(log_ctx, AV_LOG_ERROR, "Invalid channel layout '%s'\n", arg);
>
> Could you split the test to distinguish the warning?
>
> av_get_extended_channel_layout < 0 -> "Invalid channel layout"
> (!chlayout && !nret) -> "channel count without layout unsupported"
>
> (it also makes it easier to add the temporary workaround)

ok. New patch is on the way.

Thanks,
Marton