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

[Baptiste Coudurier]

On 12/14/2010 08:21 AM, Reimar D?ffinger wrote:
>
>
> 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...

I thought we decided that the partial packet had to be demuxed ?

That is why we have av_shrink_packet, av_grow_packet, and stuff like that...
Did somebody change his mind ?
That is I prefer discarding it.

-- 
Baptiste COUDURIER
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
FFmpeg maintainer                                  http://www.ffmpeg.org