[FFmpeg-devel] AAC decoder round 6

[Robert Swain]

2008/8/10 Michael Niedermayer <michaelni at gmx.at>:
> On Sun, Aug 10, 2008 at 11:11:21PM +0100, Robert Swain wrote:
>> 2008/8/10 Michael Niedermayer <michaelni at gmx.at>:
> [...]
>> >> > [...]
>> >> >> >> +    }
>> >> >> >> +    domain = get_bits1(gb);
>> >> >> >> +
>> >> >> >> +    if (is_indep_coup) {
>> >> >> >> +        coup->coupling_point = AFTER_IMDCT;
>> >> >> >> +    } else if(domain) {
>> >> >> >> +        coup->coupling_point = BETWEEN_TNS_AND_IMDCT;
>> >> >> >> +    } else
>> >> >> >> +        coup->coupling_point = BEFORE_TNS;
>> >> >> >
>> >> >> > :/ i thought the bits were at least stored together, what morons designed
>> >> >> > this!?
>> >> >> >
>> >> >> > coup->coupling_point= get_bits1(gb);
>> >> >> > ...
>> >> >> > coup->coupling_point+= 2*get_bits1(gb);
>> >> >> >
>> >> >> > still is a possibility though that is cleaner
>> >> >> > btw, what is the 4th possibility for?
>> >> >>
>> >> >> is_indep_coup = 1 overrides whatever domain is so the 4th case is
>> >> >> redundant. Though theoretically, domain must be 1 if is_indep_coup is
>> >> >> 1. is_indep_coup = 1 and domain = 0 is theoretically invalid.
>> >> >>
>> >> >> coup->coupling_point = 2*get_bits1(gb);
>> >> >> ...
>> >> >> coup->coupling_point += get_bits1(gb) && !coup->coupling_point;
>> >> >> ?
>> >> >
>> >> > hmm, this seems unneccesarily complex
>> >> > and the invalid value should be checked for with av_log&return -1
>> >>
>> >> It's not invalid in that it should be checked and cause an error. It's
>> >> just that the value of domain is ignored when is_indep_coup is set. I
>> >
>> > Where does the spec say this?
>> > If the spec says that one bit switches between doing it before and after A
>> > and says the second switches before and after B then the order of A and B
>> > implicate which combination is invalid.
>>
>> ind_sw_cce_flag
>> one bit indicating whether the coupled target syntax element is an
>> independently switched (1) or a dependently switched (0).
>>
>> cc_domain
>> one bit indicating whether the coupling is performed before (0) or after (1)
>> the TNS decoding of the coupled target channels
>>
>> In 4.6.8.3.3 Decoding process (4.6.8.3 is Coupling channel) it states that:
>>
>> "the independently switched CCE must be decoded all the way to the
>> time domain (i.e. including the synthesis filterbank) before it is
>> scaled and added onto the various SCE and CPE channels that it is
>> coupled to in the case that window state does not match."
>>
>> "A dependently switched CCE, on the other hand, must have a window
>> state that matches all of the target SCE and CPE channels that it is
>> coupled onto as determined by the list of cc_l and cc_r elements. In
>> this case, the CCE only needs to be decoded as far as the frequency
>> domain and then scaled as directed by the gain list before it is added
>> to the target SCE or CPE channels."
>>
>> So, the spec seems to suggest that one can couple an independently
>> switched CCE in the frequency domain in the case that the window state
>> matches. In these cases, I suppose the cc_domain variable would be of
>> consequence but otherwise, when coupling in the time domain for
>> independently switched CCEs, cc_domain should really always be 1 as
>> coupling would always be conducted after TNS decoding.
>
>> Can all
>> encoders be trusted to write the correct value? :)
>
> who cares? its a bug that is VERY easy to debug as long as the "illegal"
> value is checked for. Its much better than silently doing something with
> it that may or may not be what the encoder expects. (the artifacts would
> be much harder to debug)

Fair enough. See attached. I added another memset 0 to be safe and I
think the same should be added in the decode_ics() return case. Do you
agree?

Regards,
Rob