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

[Robert Swain]

2008/7/9 Michael Niedermayer <michaelni at gmx.at>:
> On Wed, Jul 09, 2008 at 10:54:33PM +0100, Robert Swain wrote:
>> 2008/7/8 Michael Niedermayer <michaelni at gmx.at>:
>> > 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:
>>
>> >> > [...]
>> >> >> >> >>      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.
>>
>> The code consistently uses 'tag' now.
>>
>> RE: "id=5 is not neccarrily the 5th element of its kind within a frame"
>>
>> Indeed it isn't, but "identifies the instance of a syntax element"
>> doesn't state that id=n implies id=1..n-1 exist also.
>>
>> "syntax element instance identifier"? :)
>
> I still dont like it, if the stream contains 2 elements with the same
> identifer they no longer identify the instance. This may be disallowed
> by the spec but none the less it doesnt identify an instance in an
> corrupted stream then ...
>
> Anyway this is not a big thing, if we canot find a good way to describe
> it we as well can leave it, it doesnt make sense to hold the aac code up
> because of that ...

And of course we can change the comment at any time. I'll ready
another full submission before I go to sleep. On to round 4...

Rob