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

[Justin Ruggles]

Justin Ruggles wrote:
> Michael Niedermayer wrote:
>> On Sat, Jun 06, 2009 at 05:24:23PM -0400, Justin Ruggles wrote:
>>> Justin Ruggles wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Tue, Jun 02, 2009 at 09:19:23PM -0400, Justin Ruggles wrote:
>>>>> [...]
>>>>>> -    /* 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;
>>>>>> +                    float spx_blend;
>>>>>> +                    int master_spx_coord;
>>>>>> +                    s->first_spx_coords[ch] = 0;
>>>>>> +                    spx_blend = get_bits(gbc, 5) * 0.03125f;
>>>>>> +                    master_spx_coord = get_bits(gbc, 2) * 3;
>>>>>> +                    bin = s->spx_start_freq;
>>>>> an empty line in there somewhere would improve readability IMHO
>>>> fixed.
>>>>
>>>>> [...]
>>>>>> +                        /* decode spx coordinates */
>>>>>> +                        spx_coord_exp  = get_bits(gbc, 4);
>>>>>> +                        spx_coord_mant = get_bits(gbc, 2);
>>>>>> +                        if (spx_coord_exp == 15)
>>>>>> +                            spx_coord = spx_coord_mant * 8.0f;
>>>>>> +                        else
>>>>>> +                            spx_coord = (spx_coord_mant + 4) * 4.0f;
>>>>>> +                        spx_coord /= 1 << (spx_coord_exp + master_spx_coord);
>>>>> something based on the following would avoid the /
>>>>> spx_coord *= (1<<123) >> (spx_coord_exp + master_spx_coord)
>>>>>
>>>>> also *4 can be factored out of the if/else and into the factor above
>>>> fixed (I think).
>>>>
>>>>> [...]
>>>>>> @@ -66,6 +62,96 @@ typedef enum {
>>>>>>  
>>>>>>  #define EAC3_SR_CODE_REDUCED  3
>>>>>>  
>>>>>> +void ff_eac3_apply_spectral_extension(AC3DecodeContext *s)
>>>>>> +{
>>>>>> +    int bin, bnd, ch, i;
>>>>>> +    uint8_t wrapflag[SPX_MAX_BANDS]={0,}, num_copy_sections, copy_sizes[SPX_MAX_BANDS];
>>>>>> +    float rms_energy[SPX_MAX_BANDS];
>>>>>> +
>>>>>> +    /* Set copy index mapping table. Set wrap flags to apply a notch filter at
>>>>>> +       wrap points later on. */
>>>>>> +    wrapflag[0] = 1;
>>>>> double initialization
>>>> fixed.
>>>>
>>>>>> +    bin = s->spx_copy_start_freq;
>>>>>> +    num_copy_sections = 0;
>>>>>> +    for (bnd = 0; bnd < s->num_spx_bands; bnd++) {
>>>>>> +        int copysize;
>>>>>> +        int bandsize = s->spx_band_sizes[bnd];
>>>>>> +        if ((bin + bandsize) > s->spx_start_freq) {
>>>>> redundant ()
>>>> fixed.
>>>>
>>>> New patch attached.
>>>>
>>>> Thanks,
>>>> Justin
>>> oops.  empty patch...
>>>
>>>
>>> +            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;
>> iam not sure, but maybe the following is easier to make sense of:
>>
>> begf + 2 + (begf >= 6 ? begf-5 : 0);
> 
> Yes, that's fine too.
> 
>>> +            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;
>>> +            }
>> would it be possible not to write values in the context and validate them
>> afterwards?
>> doing this requires me to proof that the value is not used afterwards
>> <- more time for doing a review, more chances for bugs ...
>> als it would need to be documented so that noone mistakely uses invalid values
>> through a change in the code beliving the values in the contex would
>> be validted
>> its really easier to move checks before the storing in the context
> 
> ok, I will fix this.  I know they are not used, but I see your point.
> 
>> [...]
>>> +                float noise  = nscale * (int)av_lfg_get(&s->dith_state);
>> have you considered that int may be 32 and 64 bit on different platforms ?
> 
> No, I have not considered this.  Since the code expects 32-bit, I'll
> change it to int32_t.
> 
> Thanks for the review.  I'll send a new patch this weekend.

New patch attached.

Thanks,
Justin

-------------- next part --------------
A non-text attachment was scrubbed...
Name: eac3_spx_6.diff
Type: text/x-diff
Size: 20425 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090802/b79bc467/attachment.diff>