[FFmpeg-devel] [PATCH] E-AC-3 decoder, round 3
Justin Ruggles
justin.ruggles
Thu Aug 28 02:36:03 CEST 2008
Michael Niedermayer wrote:
> On Sun, Aug 24, 2008 at 11:58:23AM -0400, Justin Ruggles wrote:
>> 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.
>>>>>>>>>>
>>>>>>>>>> -Justin
>>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>> @@ -533,10 +547,27 @@
>>>>>>>>>> }
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +static void get_transform_coeffs_ch(AC3DecodeContext *s, int blk, int ch,
>>>>>>>>>> + mant_groups *m)
>>>>>>>>>> +{
>>>>>>>>>> + if (!s->channel_uses_aht[ch]) {
>>>>>>>>>> + ac3_get_transform_coeffs_ch(s, ch, m);
>>>>>>>>>> + } else {
>>>>>>>>>> + /* if AHT is used, mantissas for all blocks are encoded in the first
>>>>>>>>>> + block of the frame. */
>>>>>>>>>> + int bin;
>>>>>>>>>> + if (!blk)
>>>>>>>>>> + ff_eac3_get_transform_coeffs_aht_ch(s, ch);
>>>>>>>>> am i blind? or where is this function, i cannot find it in this patch
>>>>>>>>> nor in svn
>>>>>>>> oops! I forgot to svn add eac3dec.c. I have attached the whole file
>>>>>>>> here. It would be applied in the same commit with the rest of these
>>>>>>>> changes (minus the part you said to commit separately).
>>>>>>> does any of the changes i ok-ed depend on eac3dec.c ?
>>>>>>> if not you could commit them and resubmit what is left + eac3dec.c
>>>>>> Well, sort of. I could apply all the parts OKed so far, but I would
>>>>>> have to comment out the 2 calls to functions which are in eac3dec.c and
>>>>>> leave out the part which actually detects the frame as being E-AC-3.
>>>>> fine
>>>>> every part commited moves us a step closer to full EAC3 support
>>>> done. new patch attached.
>> Updated patch attached with fixes as suggested in ffmpeg-soc mailing list.
>>
>> Thanks,
>> Justin
>>
>
>> Index: libavcodec/ac3dec.c
>> ===================================================================
>> --- libavcodec/ac3dec.c (revision 14945)
>> +++ libavcodec/ac3dec.c (working copy)
>> @@ -314,12 +314,12 @@
>> s->first_cpl_leak = 0;
>> s->dba_syntax = 1;
>> s->skip_syntax = 1;
>> + memset(s->cpl_strategy_exists, 0, sizeof(s->cpl_strategy_exists));
>> memset(s->channel_uses_aht, 0, sizeof(s->channel_uses_aht));
>> return ac3_parse_header(s);
>> } else {
>> - /*s->eac3 = 1;
>> - return ff_eac3_parse_header(s);*/
>> - return AC3_PARSE_ERROR_BSID;
>> + s->eac3 = 1;
>> + return ff_eac3_parse_header(s);
>> }
>> }
>>
>
> ok
I will not apply this part until all parts required for working E-AC-3
are committed.
>
>> @@ -443,7 +443,7 @@
>> * Get the transform coefficients for a particular channel
>> * reference: Section 7.3 Quantization and Decoding of Mantissas
>> */
>> -static void get_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, mant_groups *m)
>> +static void ac3_get_transform_coeffs_ch(AC3DecodeContext *s, int ch_index, mant_groups *m)
>> {
>> GetBitContext *gbc = &s->gbc;
>> int i, gcode, tbap, start, end;
>> @@ -548,7 +548,6 @@
>> }
>> }
>>
>> -#if 0
>> static void get_transform_coeffs_ch(AC3DecodeContext *s, int blk, int ch,
>> mant_groups *m)
>> {
>> @@ -565,12 +564,11 @@
>> }
>> }
>> }
>> -#endif
>
> IMHO its messy to call get_transform_coeffs_ch() for every block but
> then ignore all but the first.
> but i see no alternative so ok
I see your point. I could have a separate function for AHT mantissa
decoding and store the integer transform coeffs for all blocks in the
context instead of just the current block. I don't think that would be
a cleaner solution though.
applied.
>
>>
>> /**
>> * Get the transform coefficients.
>> */
>> -static void get_transform_coeffs(AC3DecodeContext *s)
>> +static void get_transform_coeffs(AC3DecodeContext *s, int blk)
>
> id name it decode_... but thats nitpicking, feel free to ignore me ...
fixed.
>
>> {
>> int ch, end;
>> int got_cplchan = 0;
>> @@ -580,12 +578,12 @@
>>
>> for (ch = 1; ch <= s->channels; ch++) {
>> /* transform coefficients for full-bandwidth channel */
>> - get_transform_coeffs_ch(s, ch, &m);
>> + get_transform_coeffs_ch(s, blk, ch, &m);
>> /* tranform coefficients for coupling channel come right after the
>> coefficients for the first coupled channel*/
>> if (s->channel_in_cpl[ch]) {
>> if (!got_cplchan) {
>> - get_transform_coeffs_ch(s, CPL_CH, &m);
>> + get_transform_coeffs_ch(s, blk, CPL_CH, &m);
>> calc_transform_coeffs_cpl(s);
>> got_cplchan = 1;
>> }
>
> ok
applied.
>
>> @@ -773,7 +771,7 @@
>> /* TODO: spectral extension coordinates */
>>
>> /* coupling strategy */
>> - if (get_bits1(gbc)) {
>> + if (s->cpl_strategy_exists[blk] || (!s->eac3 && get_bits1(gbc))) {
>
> if(s->eac3 ? s->cpl_strategy_exists[blk] : get_bits1(gbc))
>
> is clearer IMHO, this also may be usefull to simplify other similar
> cases
> note if above works then the init of cpl_strategy_exists may become
> unneeded for AC3
fixed for next patch.
>
>> memset(bit_alloc_stages, 3, AC3_MAX_CHANNELS);
>> if (!s->eac3)
>> s->cpl_in_use[blk] = get_bits1(gbc);
>
>> @@ -824,6 +822,9 @@
>> s->cpl_band_struct[bnd] = get_bits1(gbc);
>> }
>> } else if (!blk) {
>> + memcpy(s->cpl_band_struct,
>> + &ff_eac3_default_cpl_band_struct[cpl_begin_freq+1],
>> + s->num_cpl_subbands-1);
>> }
>> s->cpl_band_struct[s->num_cpl_subbands-1] = 0;
>>
>
> ok
applied.
>
>> @@ -856,7 +857,17 @@
>>
>> for (ch = 1; ch <= fbw_channels; ch++) {
>> if (s->channel_in_cpl[ch]) {
>> - if (get_bits1(gbc)) {
>> + int new_cpl_coords = 0;
>> +
>> + /* determine if coupling coordinates are new or reused */
>> + if (s->eac3 && s->first_cpl_coords[ch]) {
>> + new_cpl_coords = 1;
>> + s->first_cpl_coords[ch] = 0;
>> + } else {
>> + new_cpl_coords = get_bits1(gbc);
>> + }
>> +
>> + if (new_cpl_coords) {
>
> if(s->eac3 ? s->first_cpl_coords[ch] : get_bits1(gbc)){
> s->first_cpl_coords[ch] = 0;
fixed for next patch.
> [...]
>> @@ -898,10 +912,9 @@
>> }
>>
>> /* exponent strategies for each channel */
>> - s->exp_strategy[blk][CPL_CH] = EXP_REUSE;
>> - s->exp_strategy[blk][s->lfe_ch] = EXP_REUSE;
>> for (ch = !cpl_in_use; ch <= s->channels; ch++) {
>> - s->exp_strategy[blk][ch] = get_bits(gbc, 2 - (ch == s->lfe_ch));
>> + if (!s->eac3)
>> + s->exp_strategy[blk][ch] = get_bits(gbc, 2 - (ch == s->lfe_ch));
>> if(s->exp_strategy[blk][ch] != EXP_REUSE)
>> bit_alloc_stages[ch] = 3;
>> }
>
> ok
applied.
>
> [...]
>> @@ -1075,7 +1113,7 @@
>>
>> /* unpack the transform coefficients
>> this also uncouples channels if coupling is in use. */
>> - get_transform_coeffs(s);
>> + get_transform_coeffs(s, blk);
>>
>> /* TODO: generate enhanced coupling coordinates and uncouple */
>>
>
> ok
applied.
>
>> Index: libavcodec/ac3dec.h
>> ===================================================================
>> --- libavcodec/ac3dec.h (revision 14945)
>> +++ libavcodec/ac3dec.h (working copy)
>> @@ -82,7 +82,7 @@
>> int phase_flags[18]; ///< phase flags (phsflg)
>> int num_cpl_subbands; ///< number of coupling sub bands (ncplsubnd)
>> int num_cpl_bands; ///< number of coupling bands (ncplbnd)
>> - int cpl_band_struct[18]; ///< coupling band structure (cplbndstrc)
>> + uint8_t cpl_band_struct[18]; ///< coupling band structure (cplbndstrc)
>> int firstchincpl; ///< first channel in coupling
>> int first_cpl_coords[AC3_MAX_CHANNELS]; ///< first coupling coordinates states (firstcplcos)
>> int cpl_coords[AC3_MAX_CHANNELS][18]; ///< coupling coordinates (cplco)
>> @@ -168,4 +168,16 @@
>> ///@}
>> } AC3DecodeContext;
>>
>> +/**
>> + * Parse the E-AC-3 frame header.
>> + * This parses both the bit stream info and audio frame header.
>> + */
>> +int ff_eac3_parse_header(AC3DecodeContext *s);
>> +
>> +/**
>> + * Decode mantissas in a single channel for the entire frame.
>> + * This is used when AHT mode is enabled.
>> + */
>> +void ff_eac3_get_transform_coeffs_aht_ch(AC3DecodeContext *s, int ch);
>> +
>> #endif /* FFMPEG_AC3DEC_H */
>
> ok
applied.
>
>
>> Index: libavcodec/Makefile
>> ===================================================================
>> --- libavcodec/Makefile (revision 14945)
>> +++ libavcodec/Makefile (working copy)
>> @@ -27,7 +27,7 @@
>>
>> OBJS-$(CONFIG_AAC_DECODER) += aac.o aactab.o mdct.o fft.o
>> OBJS-$(CONFIG_AASC_DECODER) += aasc.o
>> -OBJS-$(CONFIG_AC3_DECODER) += ac3dec.o ac3tab.o ac3dec_data.o ac3.o mdct.o fft.o
>> +OBJS-$(CONFIG_AC3_DECODER) += eac3dec.o ac3dec.o ac3tab.o ac3dec_data.o ac3.o mdct.o fft.o
>> OBJS-$(CONFIG_AC3_ENCODER) += ac3enc.o ac3tab.o ac3.o
>> OBJS-$(CONFIG_ALAC_DECODER) += alac.o
>> OBJS-$(CONFIG_ALAC_ENCODER) += alacenc.o lpc.o
>
>> Index: libavcodec/eac3dec.c
>> ===================================================================
>> --- libavcodec/eac3dec.c (revision 14945)
>> +++ libavcodec/eac3dec.c (working copy)
>
>> @@ -78,3 +78,409 @@
>> pre_mant[4] = even1 - odd1;
>> pre_mant[5] = even0 - odd0;
>> }
>> +
>> +void ff_eac3_get_transform_coeffs_aht_ch(AC3DecodeContext *s, int ch)
>
> i think decode is a better word than get, this probably applies
> to a few other functions.
yes. fixed.
>
>> +{
>> + 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);
>> + if (group_gain > 26) {
>> + av_log(s->avctx, AV_LOG_WARNING, "GAQ gain value out-of-range\n");
>> + group_gain = 26;
>> + }
>> + 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];
>
> group_gain is not the gain of the group, its a index into a table of gain
> triples. it seems the name is misleading
i guess gaq_gain_group_code would be more accurate, but that's long.
how about group_code?
>
>
>> + gc = 0;
>> + }
>> + }
>> + }
>> + }
>> +
>> + gs=0;
>> + for (bin = s->start_freq[ch]; bin < s->end_freq[ch]; bin++) {
>
>> + int hebap = s->bap[ch][bin];
>
> what does hebap mean?
> ive by now figured out bap are band types, and iam not sure why i didnt
> suggest yet that they could be renamed to band_type[][]
> but i have no faint clue why they are called hebap here instead of bap ...
For AC-3 in general, bap means "bit allocation pointer". I think hebap
means "high-efficiency bit allocation pointer" and does the same thing
as bap, but is used only for AHT mode (same purpose, different table).
It tells both the quantization type and level. The bit allocation
process does not make any decision on the type, only the level. I
assume that different types of quantization are used because they are
particularly suited to certain levels.
I like keeping it as bap, but something like quant_level would work too.
>
>> + 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;
>
> ff_eac3_vq_hebap is a VQ tables of coefficient mantissas if i understand
> correctly, why is this not reflected in its name?
> ff_eac3_mantissa_vq
> ff_eac3_coeff_mantissa_vq
> seem better names to me
fixed.
>
>> + }
>> + } 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;
>> +
>> + for (blk = 0; blk < 6; blk++) {
>> + int mant = get_sbits(gbc, gbits);
>> + if (mant == -(1 << (gbits-1))) {
>> + /* large mantissa */
>> + int 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];
>
> isnt the first of the 2 missing a >>8 correction? or did i miss it?
I missed it. fixed for next patch.
>
> [...]
>> +
>> +int ff_eac3_parse_header(AC3DecodeContext *s)
>> +{
>> + int i, blk, ch;
>
>> + int ac3_exponent_strategy, parse_aht_info, parse_spx_atten_data;
>
> can you explain the difference between _info and _data variables?
Not much difference. Somewhere along the gradient between "information
about how to read the real data later" and "here is some data now".
>
> [..]
>> + /* skip mixing configuration information */
>> + if (get_bits1(gbc)) {
>
>> + if (s->num_blocks == 1) {
>> + skip_bits(gbc, 5);
>> + } else {
>> + for (blk = 0; blk < s->num_blocks; blk++) {
>> + if (get_bits1(gbc)) {
>> + skip_bits(gbc, 5);
>> + }
>> + }
>> + }
>
> for (blk = 0; blk < s->num_blocks; blk++) {
> if (s->num_blocks == 1 || get_bits1(gbc)) {
> skip_bits(gbc, 5);
> }
> }
fixed for next patch.
>
> [...]
>> +
>> + /* additional bitstream info */
>> + if (get_bits1(gbc)) {
>> + int addbsil = get_bits(gbc, 6);
>> + for (i = 0; i < addbsil + 1; i++) {
>> + skip_bits(gbc, 8); // skip additional bit stream info
>> + }
>> + }
>> +
>> + /* audio frame syntax flags, strategy data, and per-frame data */
>> +
>> + if (s->num_blocks == 6) {
>
>> + ac3_exponent_strategy = get_bits1(gbc);
>> + parse_aht_info = get_bits1(gbc);
>
> vertical align
fixed for next patch.
>
>> + } else {
>> + /* less than 6 blocks, so use AC-3-style exponent strategy syntax, and
>> + do not use AHT */
>> + ac3_exponent_strategy = 1;
>> + parse_aht_info = 0;
>> + }
>> +
>> + s->snr_offset_strategy = get_bits(gbc, 2);
>> + parse_transient_proc_info = get_bits1(gbc);
>> +
>> + s->block_switch_syntax = get_bits1(gbc);
>> + if (!s->block_switch_syntax)
>> + memset(s->block_switch, 0, sizeof(s->block_switch));
>> +
>> + s->dither_flag_syntax = get_bits1(gbc);
>> + if (!s->dither_flag_syntax) {
>
>> + s->dither_all = 1;
>
> is this variable really needed?
It is not. It only prevents from having to check all the flags again
when removing dithering.
>
> [...]
>> Index: libavcodec/ac3dec_data.c
>> ===================================================================
>> --- libavcodec/ac3dec_data.c (revision 14945)
>> +++ libavcodec/ac3dec_data.c (working copy)
>> @@ -87,18 +87,19 @@
>> /**
>> * Table E3.6, Gk=2 & Gk=4, B
>> * Large mantissa inverse quantization, negative mantissa remapping offsets
>> + * Table values from the spec are right-shifted by 8 to simplify calculations.
>> * ff_eac3_gaq_remap_3_4_b[hebap-8][Gk=2,4]
>> */
>> const int16_t ff_eac3_gaq_remap_2_4_b[9][2] = {
>> - { -5461, -1170},
>> - { -11703, -4915},
>> - { -14199, -6606},
>> - { -15327, -7412},
>> - { -15864, -7805},
>> - { -16126, -7999},
>> - { -16255, -8096},
>> - { -16320, -8144},
>> - { -16352, -8168}
>> + { -22, -5 },
>> + { -46, -20 },
>> + { -56, -26 },
>> + { -60, -29 },
>> + { -62, -31 },
>> + { -63, -32 },
>> + { -64, -32 },
>> + { -64, -32 },
>> + { -64, -32 },
>> };
>>
>
> ok, btw the table fits in int8_t now
applied.
Thanks,
Justin
More information about the ffmpeg-devel
mailing list