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

[Baptiste Coudurier]

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 :)

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