[FFmpeg-devel] Fix for incorrect Vorbis decoding.

[Alex Converse]

On Thu, Nov 11, 2010 at 1:25 PM, David Conrad <lessen42 at gmail.com> wrote:
> On Nov 11, 2010, at 1:15 PM, Gregory Maxwell wrote:
>
>> Last night Alex Converse mentioned on IRC that a Ogg/Theora+Vorbis
>> file on Wikipedia was producing a garbled decode
>> (http://upload.wikimedia.org/wikipedia/commons/7/79/Big_Buck_Bunny_small.ogv).
>> ?I performed an automated sweep and identified many files for which
>> the ffmpeg decode produced bad sounding output.
>>
>> e.g. compare
>> http://myrandomnode.dyndns.org:8080/~gmaxwell/ffmpeg_broken_decode/f065.wav
>> http://myrandomnode.dyndns.org:8080/~gmaxwell/ffmpeg_broken_decode/v065.wav
>>
>> I didn't have a chance to look at it more in depth at the time. But
>> Monty listened to the files and said they sounded like an error
>> decoding the floor slope. Fortunately the logic in the ffmpeg vorbis
>> decoder is mostly identical to libvorbis, so it was fairly simple to
>> compare it.
>>
>> The root cause turned out to be an overflow in the floor 1
>> computation. I've attached a working fix, though my feelings won't be
>> bruised if you prefer another fix for stylistic reasons.
>
> It it's probably better to use plain int rather than the fast types; IIRC on some platforms they're mapped to 64-bit types which is certainly not the fastest for division.
>

Those fast int types always seem to be creating all kinds of crazy problems.

All the recent fixes to vorbisdec seem to deal with those crazy types.

> But the patch looks OK.
>
>> It seems to me that the ffmpeg implementation rather aggressively uses
>> the smallest types? and it seems to me that this may be resulting in
>> other issues, some of them may be rare or difficult to catch. So I
>> make no promises the the decode is now completely correct. However, I
>> did run automated testing against a few hundred files and found no
>> cases where the patched code gave significantly worse results than
>> libvorbis.
>
> The int_fast*_t types are pretty annoying, since their actual sizes are platform-dependant...

uint_fast8_t x = 3;
int y = -2;

if (y > x)
    printf("sizeof uint_fast8_t>= sizeof int");
else
    printf("sizeof uint_fast8_t < sizeof int");

fun

see r24716