[FFmpeg-devel] [PATCH 1/2] libavfilter/formats: Fix parsing of channel specifications with a trailing 'c'.

[Simon Thelen]

On 15-06-09 at 22:50, Michael Niedermayer wrote:
> On Tue, Jun 09, 2015 at 04:55:56AM +0200, Simon Thelen wrote:
> > Fix an off-by-one in checking tail for trailing characters
> 
> > and ensure
> > that the parsing helper is only called for unknown channel layouts.
> 
> in which case does this make a difference / how can i reproduce the
> issue this fixes ?
For example:
ffmpeg -i stereo_audio.flac -af pan=1c|c0=0.9*c0+0.1*c1 out.wav
(Input and output file format don't matter)

Without this patch, this will produce silent audio.
> > 
> > Note: This removes the check ensuring that the channel layout is > 0 and
> > < 63.
> > 
> > Signed-off-by: Simon Thelen <ffmpeg-dev at c-14.de>
> > ---
> > If the check ensuring 0 < chlayout < 63 is necessary, I can send a v2
> > adding it back
> 
> i think its a good idea to keep the check or why shuld it be removed ?
I removed the check because a count <= 0 or >63 would have caused the
function to enter the bottom portion the same way it does now. Though,
if it only entered the function on count > 0, I could probably remove it
entirely because it only enters that portion of the code when strtol
returns 0. To be honest, I can't think of a valid reason for the
existence of that piece of code in the first place since it sets
out_channel_layout to 0 for valid input/output. If it were intended to
catch channel_layouts unknown to FFmpeg, it should have been placed
after the call to av_get_channel_layout, and not before it. (Note,
removing it does not cause any breakage on my end.)

> > 
> >  libavfilter/formats.c | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> > 
> > diff --git a/libavfilter/formats.c b/libavfilter/formats.c
> > index 4f9773b..2e00f30 100644
> > --- a/libavfilter/formats.c
> > +++ b/libavfilter/formats.c
> > @@ -637,20 +637,17 @@ int ff_parse_channel_layout(int64_t *ret, int *nret, const char *arg,
> >                              void *log_ctx)
> >  {
> >      char *tail;
> > -    int64_t chlayout, count;
> > -
> > -    if (nret) {
> > -        count = strtol(arg, &tail, 10);
> > -        if (*tail == 'c' && !tail[1] && count > 0 && count < 63) {
> > -            *nret = count;
> > -            *ret = 0;
> > -            return 0;
> > -        }
> > -    }
> > +    int64_t chlayout;
> > +
> >      chlayout = av_get_channel_layout(arg);
> >      if (chlayout == 0) {
> >          chlayout = strtol(arg, &tail, 10);
> > -        if (*tail || chlayout == 0) {
> > +        if (*(tail + 1) || chlayout == 0) {
> 
> doesnt *(tail + 1) read from potentially after the array ?
Fixed in v2

-- 
Simon Thelen