[FFmpeg-cvslog] r25931 - trunk/libavformat/asfdec.c

[Reimar Döffinger]

On 14 dec 2010, at 16:20, Michael Niedermayer <michaelni at gmx.at> wrote:

> On Sun, Dec 12, 2010 at 04:09:48PM +0100, Reimar D?ffinger wrote:
>> On Sun, Dec 12, 2010 at 03:13:04PM +0100, Aurelien Jacobs wrote:
>>> On Sat, Dec 11, 2010 at 10:41:47PM +0100, reimar wrote:
>>>> Author: reimar
>>>> Date: Sat Dec 11 22:41:47 2010
>>>> New Revision: 25931
>>>> 
>>>> Log:
>>>> Return an error when get_buffer reads none or only partial data instead
>>>> of returning packets with uninitialized data.
>>>> Returning partial packets as for other demuxers is problematice due to
>>>> packet scrambling and thus is not done.
>>> 
>>> This (or the previous commit ?) broke FATE.
>> 
>> No that one.
>> What happened before:
>> ==4939== Use of uninitialised value of size 8
>> ==4939==    at 0x7B05FD: ff_wma_run_level_decode (get_bits.h:611)
>                                                    ^^^^^^^^^^
> i doubt wma code is in there
> 

No, but the real issue/where that uninitialized data comes from isn't in the wma code either.

> 
>> ==4939==    by 0x7B5F06: decode_frame (wmaprodec.c:852)
>> ==4939==    by 0x7B72B8: decode_packet (wmaprodec.c:1529)
>> ==4939==    by 0x74A94E: avcodec_decode_audio3 (utils.c:671)
>> ==4939==    by 0x4327EF: output_packet (ffmpeg.c:1498)
>> ==4939==    by 0x434387: T.657 (ffmpeg.c:2620)
>> ==4939==    by 0x4352E2: main (ffmpeg.c:4330)
>> ==4939== 
>> [wmapro @ 0x66c0c90] frame[113] would have to skip -660 bits
>> Error while decoding stream #0.0
>> 
>> (why did our valgrind test config not catch that??)
>> 
>> After it changes to this:
>> stddev:    0.00 PSNR:157.31 MAXDIFF:    1 bytes:  2752512/  2506752
>> 
>> Which means quite a few samples (AFAICT some even sounding ok) are lost,
>> but no more invalid read.
>> Ok to update the reference file using dd (which makes sure only a few
>> samples will be lost but nothing changes)?
>> dd if=latin_192_mulitchannel_cut.pcm of=latin_192_mulitchannel_cut.pcm2 bs=4096 count=612
> 
> I dont think this is the solution or at least not the whole solution
> what is reading and using this byte, surely nothing should use data outside
> an array. Read yes but use no.
> 
> Id like to understand why and what is happening instead of just randomly
> changing fate references

Sorry, I considered it "obvious". The ASF demuxer did not check the get_buffer return value.
Thus for the last packet it did not realise only part of the data was read and the last part
of the buffer was uninitialized. Then it shaked the whole thing and passed it on to the decoder.
You can hardly blame the decoder for reading uninitialized data when it gets a buffer to
decode where there's uninitialized data right in the middle.
To avoid this issue, the demuxer now throws away any such incomplete packets.
I didn't double-check much that this is what really happens, but I didn't see any reason to doubt
this...