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

[Michael Niedermayer]

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

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- 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/20080710/eb76a030/attachment.pgp>