[FFmpeg-devel] [PATCH] AAC Decoder round 3
Robert Swain
robert.swain
Tue Jul 8 07:31:36 CEST 2008
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.
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?
> [..]
>> >> 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 ...
Indeed. I only just noticed it when making these comments. I'll mark it.
>> >> + * @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 ...
OK.
>> >> 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?
> [...]
>> >> + *
>> >> + * @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
OK. I expect that without reading and understanding TNS and its
implications in LTP, I won't be able to elaborate or clarify. If I can
improve it, I will.
>> >> +/**
>> >> + * 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
Then it shall be done soon.
Rob
More information about the ffmpeg-devel
mailing list