[FFmpeg-devel] [PATCH] AAC Decoder round 3

[Michael Niedermayer]

On Wed, Jul 09, 2008 at 09:52:41PM +0100, Robert Swain wrote:
> 2008/7/9 Robert Swain <robert.swain at gmail.com>:
> > 2008/7/8 Michael Niedermayer <michaelni at gmx.at>:
> >> On Tue, Jul 08, 2008 at 01:28:07PM +0100, Robert Swain wrote:
> >>> 2008/7/8 Michael Niedermayer <michaelni at gmx.at>:
> >>> > On Tue, Jul 08, 2008 at 06:31:36AM +0100, Robert Swain wrote:
> >>> >> 2008/7/8 Michael Niedermayer <michaelni at gmx.at>:
> >>> >> > On Tue, Jul 08, 2008 at 01:25:52AM +0100, Robert Swain wrote:
> >>> >> > [...]
> >>> >> >> >> @@ -1008,6 +1020,10 @@
> >>> >> >> >>
> >>> >> >> >>  /**
> >>> >> >> >>   * Decode section_data payload; reference: table 4.46.
> >>> >> >> >> + *
> >>> >> >> >> + * @param   cb          array of the codebook used for a window group's scalefactor band
> >>> >> >> >> + * @param   cb_run_end  array of the last scalefactor band of a codebook run for a window group's scalefactor band
> >>> >> >> >> + * @return  Returns error status.
> >>> >> >> >>   */
> >>> >> >> >>  static int decode_section_data(AACContext * ac, GetBitContext * gb, IndividualChannelStream * ics, enum Codebook cb[][64], int cb_run_end[][64]) {
> >>> >> >> >>      int g;
> >>> >> >> >
> >>> >> >> > this one is only confusing, it begins at the use of the term "code book"
> >>> >> >> > Could you explain how these variables fit any definition of code book?
> >>> >> >>
> >>> >> >> Hmm, I think cb[][] should be renamed cb_type[][]. Otherwise I see no
> >>> >> >> problem as they relate to Huffman codebooks. Any further confusion?
> >>> >> >
> >>> >> > yes, i still dont see what this variable has in common with a code book.
> >>> >> > or code book type.
> >>> >> > IIRC it is the coding mode used to code each coeff/band. A little like the
> >>> >> > 4MV vs. 1MV intra vs inter macroblock types.
> >>> >> > just that here its things like NOISE / INTENSITY / ...
> >>> >>
> >>> >> I see what you mean, as they use the same Huffman table to be decoded,
> >>> >> they aren't indicating different Huffman codebooks but rather
> >>> >> different methods of coding the different variable types.
> >>> >>
> >>> >> I guess you'll need to complain to ISO that the variable and constant
> >>> >> names have the wrong semantics as they're called codebooks throughout
> >>> >> the spec and the *_HCB are used too.
> >>> >
> >>> > Iam complaining to you because your implementation uses the word codebook
> >>> > as well
> >>> > i could complain to ISO but that would be futile.
> >>> >
> >>> > Just because ISO uses bad terminology doesnt mean we should copy it.
> >>>
> >>> Fair enough. Do you have any suggestions for a more appropriate name?
> >>> scalefactor_or_intensity_stereo_position_coding_method[][]
> >>
> >> band_type ?
> >
> > What do you think of the attached patch? I'll re-indent to vertically
> > align and so on after this or something similar is committed of
> > course. :)
> 
> See attached. Updated to current code.

I think this change went a little too far.

the aac_codebook_vector* things are fine IMHO as they are
They could be renamed to vector_quantizer something maybe but thats seperate

The rest of the patch is probably ok

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20080709/22cb2bff/attachment.pgp>