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

Robert Swain robert.swain
Tue Jul 8 14:28:07 CEST 2008


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

[...]
>> >> > and what does the _data postfix say?
>> >> > could you explain what a decode_pulses() would do different?
>> >>
>> >> All the decode_*_data() are parsing functions I believe. They could be
>> >> renamed parse_*() but most are called *_data() in the spec so maybe
>> >> the decode_ could be dropped. What would you prefer?
>> >
>> > I think i prefer decode_pulses() at least untill someone comes up with
>> > a better name
>>
>> OK. Do you want them all renaming?
>
> yes, id like to see _data() disapear.

OK. I'll do this. Are there any of the other *_tool() to which you
take particular distaste? Do you want all *_tool() renaming as well?

> [...]
>> >> >>      const IndividualChannelStream * ics = &cpe->ch[1].ics;
>> >> >>      SingleChannelElement * sce1 = &cpe->ch[1];
>> >> >> @@ -1382,6 +1441,9 @@
>> >> >>
>> >> >>  /**
>> >> >>   * Decode a channel_pair_element; reference: table 4.4.
>> >> >> + *
>> >> >> + * @param   id  identifies the instance of a syntax element
>> >> >> + * @return  Returns error status.
>> >> >>   */
>> >> >>  static int decode_cpe(AACContext * ac, GetBitContext * gb, int id) {
>> >> >>      int i;
>> >> >
>> >> > iam as smart as without the comment (i could RTFS but that defeats the puprose
>> >> > of the doxy)
>> >>
>> >> Maybe you are but I thought clarification of 'id' was needed.
>> >
>> > yes, what i meant is that i still do not know what id is :)
>>
>> There are multiple syntax elements within a frame (they are the ID_*)
>> and they can occur more than once within a frame. This variable
>> identifies the instance of a syntax element within a frame. Does that
>> make sense? Would adding "within a frame" to the end help the
>> situation at all?
>
> no, because this isnt true
> id=5 is not neccarrily the 5th element of its kind within a frame.
> and the calling code of decode_cpe() calls the id parameter tag
> and another different variable id.

In the spec it's called element_instance_tag hence 'tag' when calling
decode_cpe(). The variable is incorrectly named 'id' within
decode_cpe(). I could rename both 'element_instance_tag' if you think
this is an acceptable name. If not, what do you suggest as an
alternative?

Regards,
Rob




More information about the ffmpeg-devel mailing list