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

[Michael Niedermayer]

On Sun, Aug 09, 2009 at 11:45:17AM +0200, Ivan Schreter wrote:
> Hi Michael,
>
> Michael Niedermayer wrote:
>> 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
>>   
> I know about this. I didn't come up with a solution that would also address 
> this and I'm afraid there is no good solution to this.
>
> OTOH, for real-world files which someone wants to play or edit, I doubt the 
> video size and time base will change in one file. And if someone has such 
> files, it will be with current implementation even more broken on seeking.
>
> The problem of handling seq start code on seek is just academical - it is 
> NOT present in all frames and seeking to a frame which doesn't have seq 
> start code might still produce garbled results. The same holds for ext 
> start code, which is also parsed by MPEG video parser.

you may consider it academical but it is not
heres another example
1. seek
2. stream copy

even with a single width/height this will loose the seq header in the case
above rendering part of the file unplayable

and no i dont care if it just happens rarely, it can happen and its not the
correct behavior in the "what the user expects" sense


[...]
>>>>> 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
>>   
> Seeking one frame earlier might work, though I find it being an ugly 
> workaround.
>
> I don't understand what you mean with "keep track of how many bytes need to 
> be skipped", but it also sounds like a workaround.

if you have a PES packet with 3 frames, seeking to the one you really want
by skiping 0-2 frames seems to make sense on its own, i wouldnt call it a
workaround


>
>>
>>   
>>> 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
>>   
> Yes, but as an iterim solution it works.

for h264 the interim solution does not seem to work (raw h264 amongth others)


>
>>> 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
>>   
> Then help me please find a correct solution.

i really dont see where the problem comes from
you do serach for a keyframe somehow already, so you must at that point know
where in the PES packet the frame is, skip whats before (minus seq headers
and such) from the parser output, that seems to be all thats needed assuming
the keyframe choosen is the correct one in the new seeking semantics
but maybe i miss something ...

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

Frequently ignored awnser#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- 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/20090810/dfbe6235/attachment.pgp>