[FFmpeg-devel] [PATCH] Workaround for MPEG-TS crashes

[Michael Niedermayer]

On Mon, Sep 14, 2009 at 10:27:43PM -0700, Baptiste Coudurier wrote:
> On 09/14/2009 07:43 PM, Michael Niedermayer wrote:
>> On Tue, Sep 15, 2009 at 04:27:32AM +0200, Michael Niedermayer wrote:
>>> On Mon, Sep 14, 2009 at 06:45:36PM -0700, Baptiste Coudurier wrote:
>>>> Hi Michael,
>>>>
>>>> On 09/14/2009 06:14 PM, Michael Niedermayer wrote:
>>>>> On Mon, Sep 14, 2009 at 11:52:39AM -0700, Baptiste Coudurier wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 09/14/2009 10:25 AM, Dan Dennedy wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> On Sun, Sep 13, 2009 at 3:00 AM, Ivan Schreter<schreter at gmx.net>
>>>>>>> wrote:
>>>>>>>> Hi Baptiste,
>>>>>>>>
>>>>>>>> several people reported crashes when working with MPEG-TS files. I
>>>>>>>> suppose,
>>>>>>>> those files are either really or subtly broken and MPEG-TS format
>>>>>>>> handler
>>>>>>>> doesn't handle them gracefully.
>>>>>>>>
>>>>>>>> With this patch, the crashes in the samples I have seem to be gone:
>>>>>>>>
>>>>>>>
>>>>>>> In addition, I ran into some small negative lengths passed to memcpy:
>>>>>>>
>>>>>>> Index: libavformat/mpegts.c
>>>>>>> ===================================================================
>>>>>>> --- libavformat/mpegts.c        (revision 19804)
>>>>>>> +++ libavformat/mpegts.c        (working copy)
>>>>>>> @@ -915,10 +915,12 @@
>>>>>>>                 len = PES_START_SIZE - pes->data_index;
>>>>>>>                 if (len>    buf_size)
>>>>>>>                     len = buf_size;
>>>>>>> -            memcpy(pes->header + pes->data_index, p, len);
>>>>>>> -            pes->data_index += len;
>>>>>>> -            p += len;
>>>>>>> -            buf_size -= len;
>>>>>>> +            if (len>    0) {
>>>>>>> +                memcpy(pes->header + pes->data_index, p, len);
>>>>>>> +                pes->data_index += len;
>>>>>>> +                p += len;
>>>>>>> +                buf_size -= len;
>>>>>>> +            }
>>>>>>
>>>>>> Well, len<   0 is an error IMHO. I'd like to have a sample though,
>>>>>> this should not happen.
>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>                 if (pes->data_index == PES_START_SIZE) {
>>>>>>>                     /* we got all the PES or section header. We can 
>>>>>>> now
>>>>>>>                        decide */
>>>>>>>
>>>>>>>> However, it's IMHO just a workaround and the real root cause of the
>>>>>>>> problem
>>>>>>>> should be found. At this time, pes->buffer is NULL, but data for PES
>>>>>>>> packet
>>>>>>>> are coming in =>    crash.
>>>>>>>>
>>>>>>>> Crashing sample is here:
>>>>>>>> http://dennedy.org/~ddennedy/dvgrab-2009.03.28_19-07-22.m2t
>>>>>>>> Playable sample is here:
>>>>>>>> http://dennedy.org/~ddennedy/dvgrab-2009.03.28_19-06-41.m2t
>>>>>>>>
>>>>>>>> Both samples seem to be seriously broken at the beginning. They
>>>>>>>> originated
>>>>>>>> from a HDV-capable camcorder (MPEG-TS containing MPEG-2 video).
>>>>>>>
>>>>>>> Last night, after some serious testing and soul searching, I
>>>>>>> discovered some file system corruption or bad file transfers when
>>>>>>> archiving this material to external HDD. There were some HDV test
>>>>>>> samples that were failing that I _know_ had worked well in the past. 
>>>>>>> I
>>>>>>> had a backup of those on my network, and when I tested them over the
>>>>>>> network, they worked fine! An e2fsck on that disk did indeed find 
>>>>>>> some
>>>>>>> problems that the "automatic repair" option could not fix.
>>>>>>>
>>>>>>> So, the samples above are definitely not to be considered playable,
>>>>>>> but I can confirm older versions of ffmpeg (0.5) at least did not
>>>>>>> segfault on this corrupt input. With these two changes, it is much
>>>>>>> more stable.
>>>>>>>
>>>>>>
>>>>>> These 2 files can be played if MAX_RESYNC_SIZE is increased to 65536.
>>>>>> Note that mplayer reads them.
>>>>>>
>>>>>> Is 65k reasonable ?
>>>>>
>>>>> i didnt follow this thread but if i understand the code correctly then
>>>>> UINT64_MAX seems more reasonable to me ;)
>>>>> whats the point in not continuing to search for a packet but failing
>>>>> fatally, which i think is what the code would do?
>>>>
>>>> Avoiding reading 100MB on a slow link ?
>>
>> btw, forgot to ask
>> mpeg-TS over a slow link?
>>
>> is mpeg-ts used on anything where 64k is not just a fraction of a second?
>>
>
> Well, mpeg-ts is a pretty nice transport format IMHO, I can easily see it 
> used to transport live streams on the net :)

it has quite a bit of overhead that seems to make it a bad choice to me
if you have a slow link ...

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Observe your enemies, for they first find out your faults. -- Antisthenes
-------------- 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/20090915/bcc47e88/attachment.pgp>