[FFmpeg-devel] [PATCH] add E-AC-3 support to AC-3 decoder

Justin Ruggles justinruggles
Tue Jun 17 23:36:38 CEST 2008


Michael Niedermayer wrote:
> On Sat, Jun 07, 2008 at 10:30:31AM -0400, Justin Ruggles wrote:
>> Hi,
>>
>> Here is a patch set to incrementally add support for E-AC-3 to the AC-3
>> decoder.  There are 32 total patches.  I'm just attaching them all in
>> this email instead of doing the git-send-email thing.
>>
> [...]
>> +
>> +#include "avcodec.h"
>> +#include "ac3.h"
>> +#include "ac3_parser.h"
>> +#include "ac3dec.h"
>> +#include "ac3dec_data.h"
>> +
>> +/** Channel gain adaptive quantization mode */
>> +typedef enum {
>> +    EAC3_GAQ_NO =0,
>> +    EAC3_GAQ_12,
>> +    EAC3_GAQ_14,
>> +    EAC3_GAQ_124
>> +} EAC3GaqMode;
>> +
>> +#define EAC3_SR_CODE_REDUCED  3
>> +
>> +static int idct_cos_tab[6][5];
>> +
>> +static int gaq_ungroup_tab[32][3];
>> +
> 
>> +void ff_eac3_log_missing_feature(AVCodecContext *avctx, const char *log){
>> +    av_log(avctx, AV_LOG_ERROR, "%s is not implemented. If you want to help, "
>> +            "update your FFmpeg version to the newest one from SVN. If the "
>> +            "problem still occurs, it means that your file has extension "
>> +            "which has not been tested due to a lack of samples exhibiting "
>> +            "this feature. Upload a sample of the audio from this file to "
>> +            "ftp://upload.mplayerhq.hu/incoming and contact the ffmpeg-devel "
>> +            "mailing list.\n", log);
>> +}
> 
> I think this should be in utils.c it could be used by non ac3 as well

That's fine.  Should the text be changed then?  For E-AC3 the reason for
unimplemented features is pretty much solely due to lack of samples.
For unimplemented features in other codecs, that may not be the case.  I
just want to be sure this text is okay before inviting people to upload
samples which we may already have plenty of, but that we have simply not
implemented for whatever reason.

>> +        for (i = 1; i < 6; i++) {
>> +            tmp += ((int64_t)idct_cos_tab[blk][i-1] * (int64_t)s->pre_mantissa[i][ch][bin]) >> 23;
>> +        }
>> +        s->fixed_coeffs[ch][bin] = tmp >> s->dexps[ch][bin];
>> +    }
>> +}
> 
> there are symmetries in the idct, this brute force solution is a little
> umm ...

Ok.  I'll see if I can come up with a faster non-brute-force implementation.

>> +    // initialize ungrouping table for 1.67-bit GAQ gain codes
>> +    for(i=0; i<32; i++) {
>> +        gaq_ungroup_tab[i][0] = i / 9;
>> +        gaq_ungroup_tab[i][1] = (i % 9) / 3;
>> +        gaq_ungroup_tab[i][2] = i % 3;
>> +    }
> 
> i guess theres no way to factorize this and b1_mantissas related code without
> loosing speed?

Maybe.  I'll see what I can come up with.

> 
> [...]
>> @@ -496,6 +496,20 @@ static void get_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, mant_grou
>>      }
>>  }
>>  
>> +static void get_transform_coeffs_ch(AC3DecodeContext *s, int blk, int ch,
>> +                                    mant_groups *m)
>> +{
> 
>> +    if (!s->eac3 || !s->channel_uses_aht[ch]) {
>> +        ac3_get_transform_coeffs_ch(s, ch, m);
>> +    } else if (s->channel_uses_aht[ch] == 1) {
>> +        ff_eac3_get_transform_coeffs_aht_ch(s, ch);
>> +        s->channel_uses_aht[ch] = -1; /* AHT info for this frame has been read - do not read again */
>> +    }
>> +    if (s->eac3 && s->channel_uses_aht[ch]) {
>> +        ff_eac3_idct_transform_coeffs_ch(s, ch, blk);
>> +    }
> 
> if (s->channel_uses_aht[ch]) {
>     if (s->channel_uses_aht[ch] == 1){
>         ff_eac3_get_transform_coeffs_aht_ch(s, ch);
>         s->channel_uses_aht[ch] = -1; /* AHT info for this frame has been read - do not read again */
>     }
>     ff_eac3_idct_transform_coeffs_ch(s, ch, blk);
> }else
>     ac3_get_transform_coeffs_ch(s, ch, m);
> 
> And i assume something like blk==0 isnt valid to avoid the 
> s->channel_uses_aht[ch] = -1
> ?

Yeah, that is a bit simpler.  Also, I never really questioned that use
of -1 to signal that coeffs had been read from the bitstream.  It's
straight from the spec.  But after looking into it, it has to be only in
block 0 because AHT applies to all 6 blocks if it's turned on for that
channel, so I'll change the code accordingly.

All the other review comments are fairly straightforward and will be
addressed.

Thanks!

-Justin




More information about the ffmpeg-devel mailing list