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

[Robert Swain]

2008/7/9 Michael Niedermayer <michaelni at gmx.at>:
> 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

Applied without the *codebook_vector and IS_CODEBOOK_UNSIGNED changes.

Rob