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

Michael Niedermayer michaelni
Tue Jul 8 04:15:17 CEST 2008


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 / ...


> 
> >> @@ -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.


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


[..]
> >>          const enum Codebook cb[][64], const float sf[][64], float * coef) {
> >>      const uint16_t * offsets = ics->swb_offset;
> >> @@ -1276,6 +1326,10 @@
> >>
> >>  /**
> >>   * Decode an individual_channel_stream payload; reference: table 4.44.
> >> + *
> >> + * @param   common_window   Channels have independent [0], or share [1], Individual Channel Stream information.
> >
> >> + * @param   scale_flag
> >
> > yes, what did you want to say here? :)
> 
> I'll add a comment but this variable will be unused until scalable AAC
> is implemented. I was going to remove it but as it may not be
> redundant in the future I wasn't so sure. Opinions?

Well either remove it, or clearly mark it as not being used yet.
its hard to know in a 160k patch what is supposed to be finished and what
is unfinished ...


> 
> >> + * @return  Returns error status.
> >>   */
> >>  static int decode_ics(AACContext * ac, GetBitContext * gb, int common_window, int scale_flag, SingleChannelElement * sce) {
> >>      int icoeffs[1024];
> >> @@ -1326,6 +1380,9 @@
> >>      return 0;
> >>  }
> >>
> >> +/**
> >> + * Mid/Side stereo decoding; reference: 4.6.8.1.3.
> >> + */
> >>  static void ms_tool(AACContext * ac, ChannelElement * cpe) {
> >>      const MidSideStereo * ms = &cpe->ms;
> >>      const IndividualChannelStream * ics = &cpe->ch[0].ics;
> >> @@ -1353,7 +1410,9 @@
> >>      }
> >>  }
> >>
> >> -
> >> +/**
> >> + * intensity stereo decoding; reference: 4.6.8.2.3.
> >> + */
> >>  static void intensity_tool(AACContext * ac, ChannelElement * cpe) {
> >
> > above 2 are not very informative
> 
> Again, they add references to the spec. If you don't consider that
> sufficient reason for the comment then I can remove them.

I dont mind the references but i have a bad feeling about the numbers
being constant enough ...



> 
> >>      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 :)


[...]
> >> + *
> >> + * @param   decode  1 if tool is used normally, 0 if tool is used in LTP
> >
> > hmm
> 
> I didn't find any useful documentation of this variable or its
> semantics in the spec so, again, I did my best.

Iam not complaining about your efforts just that the resulting doxy is not
enough to make much sense of this variable


> 
> >> +/**
> >> + * tns_filter_tool wrapper to make interface consistent.
> >> + */
> >
> > yes and i hate this function :)
> 
> I could remove all these wrappers if you like. I don't really see the
> point in them either.

that would be great

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

Thouse who are best at talking, realize last or never when they are wrong.
-------------- 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/52c1946d/attachment.pgp>



More information about the ffmpeg-devel mailing list