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

[Michael Niedermayer]

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.


> 
> If it puts your mind as ease (re: codebooks), they look up values from
> different ranges in pow2sf_tab[], maybe you could consider that the
> differing codebooks? :)
> 
> >> >> @@ -1042,6 +1058,13 @@
> >> >>
> >> >>  /**
> >> >>   * Decode scale_factor_data; reference: table 4.47.
> >> >> + *
> >> >> + * @param   mix_gain    Channel gain (not used by AAC bitstream).
> >> >> + * @param   global_gain first scalefactor value as scalefactors are differentially coded
> >> >> + * @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
> >> >> + * @param   sf          array of scalefactors or intensity stereo positions used for a window group's scalefactor band
> >> >> + * @return  Returns error status.
> >> >>   */
> >> >>  static int decode_scale_factor_data(AACContext * ac, GetBitContext * gb, float mix_gain, unsigned int global_gain,
> >> >>          IndividualChannelStream * ics, const enum Codebook cb[][64], const int cb_run_end[][64], float sf[][64]) {
> >> >
> >> >> @@ -1100,6 +1123,9 @@
> >> >>      return 0;
> >> >>  }
> >> >>
> >> >> +/**
> >> >> + * Decode pulse data; reference: table 4.7.
> >> >> + */
> >> >>  static void decode_pulse_data(AACContext * ac, GetBitContext * gb, Pulse * pulse) {
> >> >>      int i;
> >> >
> >> > redundant
> >>
> >> It adds a reference to the spec. I don't think that's redundant. Maybe you do.
> >
> > Let us prey that the numbers dont change in the next revision of the spec.
> 
> I see your point. We shall see.
> 
> >> > 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.


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



[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have never wished to cater to the crowd; for what I know they do not
approve, and what they approve I do not know. -- Epicurus
-------------- 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/c6b3445b/attachment.pgp>