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

Ivan Schreter schreter
Sun Aug 2 17:41:00 CEST 2009


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.

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.
    * If the buffer doesn't contain either one of them, the buffer
      doesn't contain complete frame. So it can be safely assumed, it is
      the tail of previous packet and must be ignored.

If the stream IS broken, then we at most skip one frame (provided the 
check will be done only in a fresh parser when reading the first frame, 
this is not yet in).

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

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

Regards,

Ivan




More information about the ffmpeg-devel mailing list