[FFmpeg-devel] [PATCH] correct MPEG video parser not to return incomplete frames

[Michael Niedermayer]

On Sun, Aug 02, 2009 at 05:41:00PM +0200, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> On Sun, Aug 02, 2009 at 05:11:55PM +0200, Ivan Schreter wrote:
>>   
>>> Hi,
>>>
>>> Michael Niedermayer wrote:
>>>     
>>>> On Sun, Aug 02, 2009 at 03:58:59PM +0200, Ivan Schreter wrote:
>>>> [...]
>>>>         
>>>>> @@ -146,7 +153,12 @@
>>>>>      /* we have a full frame : we just parse the first few MPEG headers
>>>>>         to have the full timing information. The time take by this
>>>>>         function should be negligible for uncorrupted streams */
>>>>> -    mpegvideo_extract_headers(s, avctx, buf, buf_size);
>>>>> +    if (mpegvideo_extract_headers(s, avctx, buf, buf_size) < 0) {
>>>>> +        /* garbled frame, ignore (possibly first read after seek) */
>>>>> +        *poutbuf = NULL;
>>>>> +        *poutbuf_size = 0;
>>>>> +        return next;
>>>>> +    }
>>>>>  #if 0
>>>>>      printf("pict_type=%d frame_rate=%0.3f repeat_pict=%d\n",
>>>>>             s->pict_type, (double)avctx->time_base.den / 
>>>>> avctx->time_base.num, s->repeat_pict);
>>>>>             
>>>> as already said elsewhere, its not the job of parsers to discard data
>>>> it would break decoders that are capable to decode damaged frames, like
>>>> our decoder.
>>>>         
>>> The problem is not in the decoder, but rather parser. Namely, associated 
>>> data (PTS, DTS, position, etc.) are passed incorrectly.
>>>     
>>
>> your patch _creates_ a new problem for the decoder
>>
>>
>>   
>>>> in addition to this your patch is VERY broken in a subtile and hard to
>>>> debug way, that is it will detect 99% of the incomplete frames but not
>>>> all from what it doesnt detect again most but not all would be fine,
>>>> that would be a nightmare to debug as it would occur so rarely and
>>>> require exact seeking to the problematic spot to reproduce ...
>>>> the issue and that might not be the only one is that there can be
>>>> things prior to the picture start code that can matter ...
>>>>         
>>> It is also not the intention to detect 100% of incomplete frames. The 
>>> intention is only to detect partial frame after the seek, so we don't 
>>> return nonsense back (which breaks seeking).
>>>     
>>
>> but it does not detect 100% of the partial frames after a seek
>>   
> Actually, it does a good job.

it does an incomplete job


>
> If the stream is NOT broken:
>
>    * If the buffer for the frame contains both picture start code and
>      slice, then the frame is complete and PTS/DTS belongs to this frame.

3rd try

PES pkt0       | PES pkt1                     
seq start code | picture start code | slice ..

PES pkt1 contains both but is NOT a complete frame and depending on what is
in the seq header and the previous seen seq headers the decoder can fail


[...]
>>> Possibly, the check could be made in a "fresh" parser only, so that we at 
>>> most lose very first broken frame of a stream and it would still correct 
>>> the seeking...
>>>
>>> Otherwise, how would you do it? The problem:
>>>     
>>
>> i dont know, my first thought would be just to discard the first frame
>> the parser outputs after seeking no matter if it looks valid or not.
>> Of course thats just a thought and may have issues as well i dont know
>>   
> That won't work at all (or in your own words, it is VERY broken). In seek, 
> we reposition the file pointer to the first PES packet containing data of 
> the key frame. 

no, we dont, thats maybe what your patches do but they arent approved, given
that its no big issue to reposition one frame earlier OR keep track of how
many byetes need to be skiped it doesnt seem that this is an issue


> This PES packet might contain the rest of previous frame. 
> Now, read_frame must read the key frame. If you discard the first frame, 
> either of this could happen:
>
>    * If the key frame begins at offset 0 of the PES packet, you discard
>      the very key frame you wanted to have.
>    * If the key frame begins at offset >0 of the PES packet, you
>      discard the invalid frame, BUT your key frame won't carry DTS/PTS
>      timestamps. Thus, it's possible to decode it, but the user won't
>      know what frame it is. It is equally bad.
>

> Therefore I believe my fix is a good one, when I add that check for a fresh 
> parser, so it will be only executed just after seek, in order not to cause 
> problems when decoding broken streams.

as said 3 times alraedy your code does not work reliable so we will have
to do something else


>
> Current situation is bad, since seeking of MPEG streams is completely 
> broken (not only due to this problem, but I'm working on the other problem 
> as well).

yes i know and agree of course about that one

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

If you really think that XML is the answer, then you definitly missunderstood
the question -- Attila Kinali
-------------- 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/20090803/7734f83f/attachment.pgp>