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

Michael Niedermayer michaelni
Mon Aug 25 00:07:56 CEST 2008


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


> @@ -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


>  
>  /**
>   * 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 ...


>  {
>      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


> @@ -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


>          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


> @@ -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;

[...]
> @@ -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


[...]
> @@ -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


> 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



> 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.


> +{
> +    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



> +                    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 ...


> +        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


> +            }
> +        } 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?


[...]
> +
> +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?


[..]
> +            /* 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);
    }
}


[...]
> +
> +    /* 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


> +    } 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?


[...]
> 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


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Why not whip the teacher when the pupil misbehaves? -- Diogenes of Sinope
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080825/2337e028/attachment.pgp>



More information about the ffmpeg-devel mailing list