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

[Nicolas George]

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 at 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,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20161231/5e641776/attachment.sig>