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

[Justin Ruggles]

Michael Niedermayer wrote:
> On Wed, Dec 03, 2008 at 10:41:59PM -0500, Justin Ruggles wrote:
>> 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
> [...]
>>>> @@ -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.
> 
> what about:
>     /* block start information */
>     if (s->num_blocks > 1 && get_bits1(gbc)) {
>         /* reference: Section E2.3.2.27
>            nblkstrtbits = (numblks - 1) * (4 + ceiling(log2(words_per_frame)))
>            The spec does not say what this data is or what it's used for.
>            It is likely the offset of each block within the frame. */
>         int block_start_bits = (s->num_blocks-1) * (4 + av_log2(s->frame_size-2));
>         skip_bits(gbc, block_start_bits);
>     }
> ?
> 
> with this it should be very well possible to decode blocks after a damaged
> block, or am i missing something?
> some bitstream dependancy between blocks that makes that impossible?
> or are these block offsets never stored?
> also above allows to do error checking, that is to check that the bitstream
> pointer matches the stored offset after each block.
> our mp3 decoder also decodes all "blocks" that are undamaged and out mpeg2
> decoder also decodes all slices that are undamaged not discarding thw whole
> frame or GOP after an error
> 
> besides this should be skip_bits_long() instead of skip_bits()

Out of curiosity, I will check to see if any of the E-AC-3 samples I
have actually provide the block start information.

Ainc AC-3 can optionally reuse quite a bit of information between blocks
(E-AC-3 even more so), this still would not help in many cases.  In
fact, nearly every single field can be reused from the previous block.
After a quick search through the spec, the only field I could find which
will always be in every single block and not reused from previous blocks
in both AC-3 and E-AC-3 is the frequency coeff mantissas.

I will change that to skip_bits_long().

> [...]
>>>>      /* 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.
> 
> why not use (1<<23) and (1<<23)-1 ? seems clearer than MIN/MAX_INT24

Ok. I'll change it.

>>> [...]
>>>
>>>> +/**
>>>> + * 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.
> 
> why do you round to 24 bits instead of 32?

Good point. It can be 32 bits. I'll change it.

New patch not attached, but will be sent soon.

Thanks,
Justin