[FFmpeg-devel] [PATCH] E-AC-3 spectral extension

Justin Ruggles justin.ruggles
Thu Dec 4 04:41:59 CET 2008


Michael Niedermayer wrote:
> On Sat, Nov 15, 2008 at 07:10:53PM -0500, Justin Ruggles wrote:
>> Hi,
>>
>> Here is an updated version of E-AC-3 spectral extension support.  I have
>> addressed the issues mentioned in Michael's review in ffmpeg-cvslog.
>>
>> - should not be expoitable now
>> - uses smaller int types for context arrays
>> - got rid of unused variables
>> - the code to determine copy_sizes and wrapflag is 70% faster
>> - merged the 2 loops for notch filters
>> - moved multiplies outside the loop for signal & noise blending
>>
>> Thanks,
>> Justin
> 
>> Index: libavcodec/ac3dec.c
>> ===================================================================
>> --- libavcodec/ac3dec.c	(revision 15818)
>> +++ libavcodec/ac3dec.c	(working copy)
>> @@ -728,9 +729,10 @@
>>                                    int ecpl, int start_subband, int end_subband,
>>                                    const uint8_t *default_band_struct,
>>                                    uint8_t *band_struct, int *num_subbands,
>> -                                  int *num_bands, int *band_sizes)
>> +                                  int *num_bands, uint8_t *band_sizes)
>>  {
>> -    int subbnd, bnd, n_subbands, n_bands, bnd_sz[22];
>> +    int subbnd, bnd, n_subbands, n_bands=0;
>> +    uint8_t bnd_sz[22];
>>  
>>      n_subbands = end_subband - start_subband;
>>  
>> @@ -769,7 +771,7 @@
>>      if (num_bands)
>>          *num_bands = n_bands;
>>      if (band_sizes)
>> -        memcpy(band_sizes, bnd_sz, sizeof(int)*n_bands);
>> +        memcpy(band_sizes, bnd_sz, n_bands);
>>  }
>>  
>>  /**
> 
> ok, though maybe this should be a seperate commit with related changes
> if any

applied

> 
>> @@ -818,15 +820,92 @@
>>  
>>      /* spectral extension strategy */
>>      if (s->eac3 && (!blk || get_bits1(gbc))) {
>> -        if (get_bits1(gbc)) {
>> -            av_log_missing_feature(s->avctx, "Spectral extension", 1);
>> -            return -1;
>> +        s->spx_in_use = get_bits1(gbc);
>> +        if (s->spx_in_use) {
>> +            int begf, endf;
>> +            int spx_end_subband;
>> +
>> +            /* determine which channels use spx */
>> +            if (s->channel_mode == AC3_CHMODE_MONO) {
>> +                s->channel_in_spx[1] = 1;
>> +            } else {
>> +                for (ch = 1; ch <= fbw_channels; ch++)
>> +                    s->channel_in_spx[ch] = get_bits1(gbc);
>> +            }
>> +
>> +            s->spx_copy_start_freq = get_bits(gbc, 2) * 12 + 25;
>> +            begf = get_bits(gbc, 3);
>> +            endf = get_bits(gbc, 3);
>> +            s->spx_start_subband = begf < 6 ? begf+2 : 2*begf-3;
>> +            spx_end_subband      = endf < 4 ? endf+5 : 2*endf+3;
>> +            if (s->spx_start_subband >= spx_end_subband) {
>> +                av_log(s->avctx, AV_LOG_ERROR, "invalid spectral extension range (%d >= %d)\n",
>> +                       s->spx_start_subband, spx_end_subband);
>> +                return -1;
>> +            }
> 
> with code like this i always ask myself if its enough or not
> I mean this code is conditinal on a get_bits1(gbc) apparently otherwise
> reusing the last values.
> and the return -1 does leave some changed vars in the context, so it
> pretty likely can leave invalid vars in the context.
> I dont know if they are used or not after a return -1
> but the code is not very solid like that either way as it depends on some
> variables in the context not being used ...
> so IMHO there either should be no invalid values left or it should be
> documented in comments why its safe currently and appropriate notes
> should be placed in the code to prevent future mistaken use of the
> variables, but i guess its easier just not to store invalid values
> in the context to begin with

I have simplified things by committing a change that prevents decoding
subsequent blocks in a frame after an invalid block is found.  It was
pointless to try anyways.  This way it doesn't matter if invalid values
are in the context since the decoder will always re-read them before
using them in the next frame.


> [...]
>> -    /* TODO: spectral extension coordinates */
>> +    /* spectral extension coordinates */
>> +    if (s->spx_in_use) {
>> +        for (ch = 1; ch <= fbw_channels; ch++) {
>> +            if (s->channel_in_spx[ch]) {
>> +                if (s->first_spx_coords[ch] || get_bits1(gbc)) {
>> +                    int bin, spx_blend;
>> +                    int master_spx_coord;
> 
>> +                    s->first_spx_coords[ch] = 0;
> 
> if i understand correctly, first_spx_coords could be replaced by checking
> if some of the fields below has been initialized already

This doesn't only apply to the first block.  If channel_in_spx is false
in a block, then first_spx_coords is reset to 1 for that channel.  That
way a bit can be saved if channel_in_spx is true in a subsequent block
of the same frame.  One of the other variables could be reused for this
same purpose, but it would be much less clear.. (e.g.
s->spx_coords[ch][0] = -1).  I like it better with its own variable though.

> 
> 
>> +                    spx_blend = get_bits(gbc, 5) << 18;
>> +                    master_spx_coord = get_bits(gbc, 2) * 3;
>> +                    bin = s->spx_start_freq;
>> +                    for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
>> +                        int spx_coord_exp, spx_coord_mant;
>>  
>> +                        /* calculate blending factors */
>> +                        int bandsize = s->spx_band_sizes[bnd];
>> +                        int nratio = (((bin + (bandsize >> 1)) << 23) / s->spx_end_freq) - spx_blend;
>> +                        nratio = av_clip(nratio, 0, INT24_MAX);
> 
>> +                        s->spx_noise_blend [ch][bnd] = ff_sqrt((            nratio) << 8) * M_SQRT_POW2_15;
>> +                        s->spx_signal_blend[ch][bnd] = ff_sqrt((INT24_MAX - nratio) << 8) * M_SQRT_POW2_15;
>> +                        s->spx_noise_blend[ch][bnd] = (s->spx_noise_blend[ch][bnd] * 14529495) >> 23;
> 
> vertical align

fixed.

> 
> [...]
>> @@ -939,8 +1021,14 @@
>>      if (channel_mode == AC3_CHMODE_STEREO) {
>>          if ((s->eac3 && !blk) || get_bits1(gbc)) {
>>              s->num_rematrixing_bands = 4;
>> -            if(cpl_in_use && s->start_freq[CPL_CH] <= 61)
>> +            if (cpl_in_use) {
>> +                if (s->start_freq[CPL_CH] <= 61)
>>                  s->num_rematrixing_bands -= 1 + (s->start_freq[CPL_CH] == 37);
>> +            } else if (s->spx_in_use) {
>> +                if (s->spx_start_freq <= 61)
>> +                    s->num_rematrixing_bands -= 1 + (s->spx_start_freq <= 37) +
>> +                                                    (s->spx_start_freq <= 25);
>> +            }
>>              for(bnd=0; bnd<s->num_rematrixing_bands; bnd++)
>>                  s->rematrixing_flags[bnd] = get_bits1(gbc);
>>          } else if (!blk) {
> 
> ok, i think

I went through this very carefully to make sure it was correct for only
coupling, only spectral extension, or both.

>>  
>> +    ff_eac3_apply_spectral_extension(s);
>> +
> 
> shouldnt this be under if(spx_in_use) ?
> maybe its irrelevant speedwise though, i dunno ...

fixed.

> 
>>      /* apply scaling to coefficients (headroom, dynrng) */
>>      for(ch=1; ch<=s->channels; ch++) {
>>          float gain = s->mul_bias / 4194304.0f;
>> Index: libavcodec/ac3dec.h
>> ===================================================================
>> --- libavcodec/ac3dec.h	(revision 15818)
>> +++ libavcodec/ac3dec.h	(working copy)
>>  
>> +#define INT24_MIN -8388608
>> +#define INT24_MAX  8388607
> 
> fine unless this conflicts with something from a standard header

I didn't think it did, but after checking C99, I found I was wrong.  I
have changed it to MIN_INT24 and MAX_INT24.


>> +#define M_SQRT_POW2_15 181
> 
> what does the 15 mean?

Aurel is right, sqrt(pow(2,15)). This was just the first name that came
to mind that relates to how it is used in the code (sqrt of 2 in signed
16-bit fixed-point). I'm open to other suggestions.

> [...]
> 
>> +/**
>> + * Table E.25: Spectral Extension Attenuation Table
>> + * 24-bit fixed-point version of the floating-point table in the specification.
>> + * ff_eac3_spx_atten_tab[code][bin]=lrint(pow(1<<(bin+1),(code+1)/-15.0)*(1<<23));
>> + */
> 
> does the table in the spec only specify 24bit or is this rounded somehow?
> 

The spec uses floating point values to 9 decimal places.


New patch attached.

Thanks,
Justin


-------------- next part --------------
A non-text attachment was scrubbed...
Name: eac3_spx_2.diff
Type: text/x-patch
Size: 18286 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20081203/40d870d0/attachment.bin>



More information about the ffmpeg-devel mailing list