| Message ID | 20161226171434.935-3-cus@passwd.hu |
|---|---|
| State | Superseded |
| Headers | show |
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? > 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.) > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > libavfilter/formats.c | 20 ++++++-------------- > libavfilter/tests/formats.c | 3 +++ > tests/ref/fate/filter-formats | 5 ++++- > 3 files changed, 13 insertions(+), 15 deletions(-) > > diff --git a/libavfilter/formats.c b/libavfilter/formats.c > index 840780d..8be606f 100644 > --- a/libavfilter/formats.c > +++ b/libavfilter/formats.c > @@ -662,24 +662,16 @@ int ff_parse_sample_rate(int *ret, const char *arg, void *log_ctx) > int ff_parse_channel_layout(int64_t *ret, int *nret, const char *arg, > void *log_ctx) > { > - char *tail; > int64_t chlayout; > + int nb_channels; > > - 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. > - 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) > + return AVERROR(EINVAL); > } > *ret = chlayout; > if (nret) > - *nret = av_get_channel_layout_nb_channels(chlayout); > + *nret = nb_channels; > + > return 0; > } > diff --git a/libavfilter/tests/formats.c b/libavfilter/tests/formats.c > index 0e8ba4a..5450742 100644 > --- a/libavfilter/tests/formats.c > +++ b/libavfilter/tests/formats.c > @@ -39,6 +39,9 @@ int main(void) > "-1c", > "60c", > "65c", > + "2C", > + "60C", > + "65C", > "5.1", > "stereo", > "1+1+1+1", > diff --git a/tests/ref/fate/filter-formats b/tests/ref/fate/filter-formats > index 4c303d8..17ff5b2 100644 > --- a/tests/ref/fate/filter-formats > +++ b/tests/ref/fate/filter-formats > @@ -75,8 +75,11 @@ quad(side) > 0 = ff_parse_channel_layout(0000000000000004, 1, 1c); > 0 = ff_parse_channel_layout(0000000000000003, 2, 2c); > -1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, -1c); > -0 = ff_parse_channel_layout(0000000000000000, 60, 60c); > +-1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 60c); > -1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 65c); > +0 = ff_parse_channel_layout(0000000000000000, 2, 2C); > +0 = ff_parse_channel_layout(0000000000000000, 60, 60C); > +-1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 65C); > 0 = ff_parse_channel_layout(000000000000003F, 6, 5.1); > 0 = ff_parse_channel_layout(0000000000000003, 2, stereo); > 0 = ff_parse_channel_layout(0000000000000001, 1, 1+1+1+1); Regards,
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
diff --git a/libavfilter/formats.c b/libavfilter/formats.c index 840780d..8be606f 100644 --- a/libavfilter/formats.c +++ b/libavfilter/formats.c @@ -662,24 +662,16 @@ int ff_parse_sample_rate(int *ret, const char *arg, void *log_ctx) int ff_parse_channel_layout(int64_t *ret, int *nret, const char *arg, void *log_ctx) { - char *tail; int64_t chlayout; + int nb_channels; - 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) { - 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); + return AVERROR(EINVAL); } *ret = chlayout; if (nret) - *nret = av_get_channel_layout_nb_channels(chlayout); + *nret = nb_channels; + return 0; } diff --git a/libavfilter/tests/formats.c b/libavfilter/tests/formats.c index 0e8ba4a..5450742 100644 --- a/libavfilter/tests/formats.c +++ b/libavfilter/tests/formats.c @@ -39,6 +39,9 @@ int main(void) "-1c", "60c", "65c", + "2C", + "60C", + "65C", "5.1", "stereo", "1+1+1+1", diff --git a/tests/ref/fate/filter-formats b/tests/ref/fate/filter-formats index 4c303d8..17ff5b2 100644 --- a/tests/ref/fate/filter-formats +++ b/tests/ref/fate/filter-formats @@ -75,8 +75,11 @@ quad(side) 0 = ff_parse_channel_layout(0000000000000004, 1, 1c); 0 = ff_parse_channel_layout(0000000000000003, 2, 2c); -1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, -1c); -0 = ff_parse_channel_layout(0000000000000000, 60, 60c); +-1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 60c); -1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 65c); +0 = ff_parse_channel_layout(0000000000000000, 2, 2C); +0 = ff_parse_channel_layout(0000000000000000, 60, 60C); +-1 = ff_parse_channel_layout(FFFFFFFFFFFFFFFF, -1, 65C); 0 = ff_parse_channel_layout(000000000000003F, 6, 5.1); 0 = ff_parse_channel_layout(0000000000000003, 2, stereo); 0 = ff_parse_channel_layout(0000000000000001, 1, 1+1+1+1);
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. 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. Signed-off-by: Marton Balint <cus@passwd.hu> --- libavfilter/formats.c | 20 ++++++-------------- libavfilter/tests/formats.c | 3 +++ tests/ref/fate/filter-formats | 5 ++++- 3 files changed, 13 insertions(+), 15 deletions(-)