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

[Michael Niedermayer]

On Wed, Aug 27, 2008 at 08:36:03PM -0400, Justin Ruggles wrote:
> 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:
[...]
> > 
> >> +{
> >> +    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?

ok



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

ok, keep  it

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

And what is the point?
Setting it with:
        s->dither_all = 1;
        for (ch = 1; ch <= fbw_channels; ch++) {
            s->dither_flag[ch] = get_bits1(gbc);
            if(!s->dither_flag[ch])
                s->dither_all = 0;
        }
does a check per fbw_channels
to avoid a check per fbw_channels later in remove_dithering()
?

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

Democracy is the form of government in which you can choose your dictator
-------------- 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/20080828/179e969a/attachment.pgp>