[FFmpeg-devel] [PATCH] AAC Pulse data boundaries

[Robert Swain]

2008/9/15 Michael Niedermayer <michaelni at gmx.at>:
> On Mon, Sep 15, 2008 at 12:36:21AM +0100, Robert Swain wrote:
>> 2008/9/14 Alex Converse <alex.converse at gmail.com>:
>> > Currently the AAC pulse_data tool can try to apply pulses to offsets outside
>> > of the boundaries of the spectral coefficients. This leads to a crash
>> > (SIGSEGV) with a stream generated wit the following command:
>> > zzuf -s16 -r0.001:0.1 -b1193- <
>> > mpeg4audio-conformance/compressedMp4/al04_08.mp4 > fuzzed.mp4
>> >
>> > Attached is a patch to clip pulse positioning to 1023.
>>
>> The code changes look OK but is clipping the best solution to this or
>> should those pulses rather be discarded? Michael?
>>
>> It would seem to me that if there are multiple pulses, clipping them
>> could lead to multiple pulses being applied to position 1023. Is a
>> spike in the spectral data better than no spike or vice versa? My
>> guess is that ignoring incorrectly positioned pulses may be a better
>> option.
>
> Well as you ask :)
> Bitstream errors should cause the data until the next resync possibility
> to be discarded. It also should discard some of the pevious data as the
> bitstream error likely happened earlier.
> In practice that likely means replacing the current frame by silence, that
> is before the windowing. Or maybe to duplicate the last frame.

If it's possible to do something better than discard everything and
output silence, I think we should. Is this the concept of error
concealment whose errors I have seen in h.264 and MPEG-4 streams as I
recall?

> Just listen to a slightly damaged file to find out which solution sounds
> best. This really is the only way to be sure what is best ...

For this particular issue (as there can only be 5 pulses maximum
anyway...), my gut tells me that continuing but discarding the invalid
pulse positions would be best. I'll leave that to Alex to test, if he
wants. :)

> And btw, using get_bits(), a function with "side effects" as argument
> to a macro like FFMIN() is a bad idea.
> get_bits() > X ? get_bits() : X
> is likely not what you meant.

Indeed. Damn macros... :)

Rob