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

[Ivan Schreter]

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.

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

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:

After the seek, you are positioned to a packet containing some video 
stream data. There is a bit from the previous frame, but PTS/DTS are 
related to the new frame. You don't know whether the first part of the 
packet belongs to a previous frame or whether it is a complete frame 
(access unit). But you do know, it can only be a complete frame, if it 
contains picture start code _and_ a slice.

Current code will simply take the rest of previous frame as a complete 
frame and return it back, thus associating PTS/DTS of the packet with 
this broken frame. Then, parser will read the next frame (correct one), 
but associate AV_NOPTS_VALUE for PTS/DTS of that frame, since it is the 
second frame starting in the same packet (and thus timestamps have to be 
calculated). Wrong timestamps will be calculated for this frame. This 
breaks seeking and CANNOT be addressed in the decoder (it's not the 
proper place).

Regards,

Ivan