[FFmpeg-cvslog] r15812 - in trunk/libavcodec: ac3dec.c ac3dec.h ac3dec_data.c ac3dec_data.h eac3dec.c

[Justin Ruggles]

Michael Niedermayer wrote:
> On Thu, Nov 13, 2008 at 04:18:13AM +0100, jbr wrote:
>> Author: jbr
>> Date: Thu Nov 13 04:18:13 2008
>> New Revision: 15812
>>
>> Log:
>> add support for spectral extension
> 
> This code looks like it completely lacks validity checks and likely
> exploitable at several points.
> I am not asking you to revert it but i would be happy if you did anyway.
> This code should have passed review before commiting IMHO
> 
> Below review is incomplete, there likely are more issues, also iam not
> mentioning the exploitable code as this patch needs to be reviewed completely
> for security issues (which i did not do) not just the one issue ive found
> fixed.
> 
> 
> [...]
> 
>> +    int channel_in_spx[AC3_MAX_CHANNELS];   ///< channel in spectral extension          (chinspx)
> 
> uint8_t
> 
> 
>> +    int spx_atten_code[AC3_MAX_CHANNELS];   ///< spx attenuation code                   (spxattencod)
> 
> int8_t
> and many others also waste space

I can see the benefit of reducing the memory footprint of the decode
context, but I can't recall you ever bringing up integer type size as an
issue except when talking about static or global arrays.  I could go
back and change almost every single field in the struct just to decrease
the context size...  I don't really care either way, but it would be
good to have some guidance on this.  Should I do it for all arrays
within the struct?  Just large arrays?  All fields?

> 
>> +    int spx_coords_exist[AC3_MAX_CHANNELS]; ///< indicates if a channel has spx coords  (spxcoe)
> 
> unused
> 
> 
>> +    int spx_start_subband;                  ///< spx beginning frequency band           (spxbegf)
>> +    int spx_start_freq;                     ///< spx start frequency bin
>> +    int spx_end_freq;                       ///< spx end frequency bin
>> +    int spx_copy_start_freq;                ///< spx starting frequency for copying     (copystartmant)
> 
>> +    int num_spx_subbands;                   ///< number of spectral extension subbands
> 
> unused, and ive not checked variables exhaustivles so there may be more

I think those are the only 2.  I'll double-check though.

> [...]
> 
>> +        if ((bin + bandsize) > s->spx_start_freq) {
>> +            copy_sizes[num_copy_sections++] = bin - s->spx_copy_start_freq;
>> +            bin = s->spx_copy_start_freq;
>> +            wrapflag[bnd] = 1;
>> +        }
>> +        for (i = 0; i < bandsize; i++) {
>> +            if (bin == s->spx_start_freq) {
>> +                copy_sizes[num_copy_sections++] = bin - s->spx_copy_start_freq;
>> +                bin = s->spx_copy_start_freq;
>> +            }
>> +            bin++;
>> +        }
> 
> these 2 look rather similar and the second looks inefficient

They are similar, but they check different conditions.  The first is for
resetting the copy point at the start of the band and for setting the
wrap flag.  The second is for wrapping the copying within the band.  I
have some ideas for alternative solutions.  I'll see if I can come up
with something better.

> 
> [...]
> 
>> +            bin = s->spx_start_freq - 2;
>> +            for (i = 0; i < 5; i++) {
>> +                s->fixed_coeffs[ch][bin] = ((int64_t)atten_tab[2-abs(i-2)] *
>> +                        (int64_t)s->fixed_coeffs[ch][bin]) >> 23;
>> +                bin++;
>> +            }
> 
>> +            /* apply notch at all other wrap points */
>> +            bin += s->spx_band_sizes[0];
>> +            for (bnd = 1; bnd < s->num_spx_bands; bnd++) {
>> +                if (wrapflag[bnd]) {
>> +                    bin -= 5;
> 
>> +                    for (i = 0; i < 5; i++) {
>> +                        s->fixed_coeffs[ch][bin] = (atten_tab[2-abs(i-2)] *
>> +                                (int64_t)s->fixed_coeffs[ch][bin]) >> 23;
>> +                        bin++;
>> +                    }
> 
> these 2 look like they could be in 1 loop, also it might be worth
> to benchmark this against a unrolled one

Yes, they can be merged by setting wrapflag[0]=1.  I'll also benchmark
unrolling the inner loop.

Thanks,
Justin