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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
