[FFmpeg-devel] [PATCH] E-AC-3 decoder, round 3

[Justin Ruggles]

Hi,

Michael Niedermayer wrote:
> On Tue, Aug 19, 2008 at 09:16:24PM -0400, Justin Ruggles wrote:
>> Michael Niedermayer wrote:
>>> On Tue, Aug 19, 2008 at 07:43:31PM -0400, Justin Ruggles wrote:
>>>> Michael Niedermayer wrote:
>>>>> On Tue, Aug 19, 2008 at 06:54:35PM -0400, Justin Ruggles wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> Michael Niedermayer wrote:
>>>>>>> On Sun, Aug 17, 2008 at 07:30:26PM -0400, Justin Ruggles wrote:
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Here is a new patch to complete support for E-AC-3 decoding within the
>>>>>>>> current AC-3 decoder.  It will be followed up by a cosmetic commit to
>>>>>>>> indent and align.
>>>>>>>>
>>[...]

I will apply the OKed parts soon and send a new patch, but first I have
a few questions.

>>      /* coupling strategy */
>> -    if (get_bits1(gbc)) {
>> +    if (!s->eac3)
>> +        s->cpl_strategy_exists[blk] = get_bits1(gbc);
>> +    if (s->cpl_strategy_exists[blk]) {
> 
> wouldnt 
> if(s->cpl_strategy_exists[blk] || get_bits1(gbc)){
> 
> work as well?

Not exactly. The 1 bit is only read for normal AC-3. For E-AC-3 it is
always in the header. I can still simplify it though by initializing it
to zero for normal AC-3.

> 
>> +
>> +void ff_eac3_get_transform_coeffs_aht_ch(AC3DecodeContext *s, int ch)
>> +{
>> +    int bin, blk, gs;
>> +    int end_bap, gaq_mode;
>> +    GetBitContext *gbc = &s->gbc;
>> +    int gaq_gain[AC3_MAX_COEFS];
>> +
>> +    gaq_mode = get_bits(gbc, 2);
>> +    end_bap = (gaq_mode < 2) ? 12 : 17;
>> +
>> +    /* if GAQ gain is used, decode gain codes for bins with hebap between
>> +       8 and end_bap */
>> +    gs = 0;
>> +    if (gaq_mode == EAC3_GAQ_12 || gaq_mode == EAC3_GAQ_14) {
>> +        /* read 1-bit GAQ gain codes */
>> +        for (bin = s->start_freq[ch]; bin < s->end_freq[ch]; bin++) {
>> +            if (s->bap[ch][bin] > 7 && s->bap[ch][bin] < end_bap)
>> +                gaq_gain[gs++] = get_bits1(gbc) << (gaq_mode-1);
>> +        }
>> +    } else if (gaq_mode == EAC3_GAQ_124) {
>> +        /* read 1.67-bit GAQ gain codes (3 codes in 5 bits) */
>> +        int gc = 2;
>> +        for (bin = s->start_freq[ch]; bin < s->end_freq[ch]; bin++) {
>> +            if (s->bap[ch][bin] > 7 && s->bap[ch][bin] < end_bap) {
>> +                if (gc++ == 2) {
>> +                    int group_gain = get_bits(gbc, 5);
>> +                    gaq_gain[gs++] = ff_ac3_ungroup_3_in_5_bits_tab[group_gain][0];
>> +                    gaq_gain[gs++] = ff_ac3_ungroup_3_in_5_bits_tab[group_gain][1];
>> +                    gaq_gain[gs++] = ff_ac3_ungroup_3_in_5_bits_tab[group_gain][2];
>> +                    gc = 0;
>> +                }
>> +            }
>> +        }
>> +    }
>> +
>> +    gs=0;
>> +    for (bin = s->start_freq[ch]; bin < s->end_freq[ch]; bin++) {
>> +        int hebap = s->bap[ch][bin];
>> +        int bits = ff_eac3_bits_vs_hebap[hebap];
>> +        if (!hebap) {
>> +            /* zero-mantissa dithering */
>> +            for (blk = 0; blk < 6; blk++) {
>> +                s->pre_mantissa[ch][bin][blk] = (av_lfg_get(&s->dith_state) & 0x7FFFFF) - 0x400000;
>> +            }
>> +        } else if (hebap < 8) {
>> +            /* Vector Quantization */
>> +            int v = get_bits(gbc, bits);
>> +            for (blk = 0; blk < 6; blk++) {
>> +                s->pre_mantissa[ch][bin][blk] = ff_eac3_vq_hebap[hebap][v][blk] << 8;
>> +            }
>> +        } else {
>> +            /* Gain Adaptive Quantization */
>> +            int gbits, log_gain;
>> +            if (gaq_mode != EAC3_GAQ_NO && hebap < end_bap) {
>> +                log_gain = gaq_gain[gs++];
>> +            } else {
>> +                log_gain = 0;
>> +            }
> 
>> +            gbits = bits - log_gain;
> 
> this is maybe missing a check for gbits<=0, iam not certain it can happen
> but i think it can

going strictly by the spec, it will not happen, but to ensure this i
will need to add a check for the 3-in-5-bit group code to be less than
27 to keep the gaq_gain values within their valid range of 0 to 2.

> 
>> +
>> +            for (blk = 0; blk < 6; blk++) {
>> +                int mant = get_sbits(gbc, gbits);
>> +                if (mant == -(1 << (gbits-1))) {
>> +                    /* large mantissa */
>> +                    int64_t b;
>> +                    mant = get_sbits(gbc, bits-2+log_gain) << (26-log_gain-bits);
>> +                    /* remap mantissa value to correct for asymmetric quantization */
>> +                    if (mant >= 0)
>> +                        b = 32768 >> log_gain;
>> +                    else
>> +                        b = ff_eac3_gaq_remap_2_4_b[hebap-8][log_gain-1];
>> +                    mant += ((int64_t)ff_eac3_gaq_remap_2_4_a[hebap-8][log_gain-1] * mant + b) >> 15;
> 
> this can be done without a 64 bit multiple i think

I can see how it works for the other one, but not for this one.  In the
other case it works out well because mant will always have zeros for at
least the low 8 bits.  For this case, I don't see where avoiding the
64-bit multiply is possible without loss of precision.

> 
>> +                } else {
>> +                    /* small mantissa, no GAQ, or Gk=1 */
>> +                    mant <<= 24 - bits;
>> +                    if (!log_gain) {
>> +                        /* remap mantissa value for no GAQ or Gk=1 */
>> +                        mant += ((int64_t)ff_eac3_gaq_remap_1[hebap-8] * mant) >> 15;
>> +                    }
> 
> mant <<= 24 - bits;
> if (!log_gain) {
>     /* remap mantissa value for no GAQ or Gk=1 */
>     mant = (ff_eac3_gaq_remap_1[hebap-8] * (mant>>8)) >> 7;
> }

Did you mean to change the "+=" to "="? I don't see how it would work
that way.  I do see the point though about splitting the shift to avoid
64-bit multiply and will change it in the next patch.

Thanks,
Justin