[FFmpeg-devel] [PATCH] fix vorbis decoder amplitude bits handling

[Uoti Urpala]

On Wed, 2010-11-17 at 23:11 -0800, Alex Converse wrote:
> On Tue, Nov 16, 2010 at 2:42 AM, Yuriy Kaminskiy <yumkam at mail.ru> wrote:
> >
> > Baptiste Coudurier wrote:

> > > Index: libavcodec/vorbis_dec.c
> > > ===================================================================
> > > --- libavcodec/vorbis_dec.c   (revision 25756)
> > > +++ libavcodec/vorbis_dec.c   (working copy)
> > > @@ -563,6 +563,11 @@
> > >                       "Floor 0 amplitude bits is 0.\n");
> > >                return -1;
> > >              }
> > > +            if (floor_setup->data.t0.amplitude_bits > 32) {

> > Not enough, s/>/>=/, result of 1<<32 is undefined (and I'm sure code below
> > really broken on x86 when amplitude_bits == 32, resulting in divide-by-zero):
> > === cut ===
> >                    q = exp((((amplitude*vf->amplitude_offset) /
> >                              (((1 << vf->amplitude_bits) - 1) * sqrt(p + q)))
> >                             - vf->amplitude_offset) * .11512925f);
> > === cut ===

> Libvorbis does that. When in Rome...

It's blatantly wrong if amplitude_bits >= 32, so such values should at
least be rejected cleanly.

It should be straightforward to add support for full 64 bits too. Of
course any extra accuracy from the additional bits would be lost because
the code uses only single-precision floats, but at least the files could
be decoded - I doubt such files currently exist though since libvorbis
wouldn't decode them correctly either.

The code quoted above has an additional problem: the
"amplitude*vf->amplitude_offset" multiplication is done in integers
rather than floats. vf->amplitude_offset is an 8-bit value, so the
integer multiplication can already overflow with amplitude_bits == 25.