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

[Michael Niedermayer]

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 ?



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

i want a grep '_tool()' aac* to fail [except in comments refering to the spec]


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

hmm, i need to think more about this, but at least the code should be
consistent, calling one id and then another is not good.

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

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- 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/20080708/3eb0b63b/attachment.pgp>