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

Ivan Schreter schreter
Sun Aug 9 11:45:17 CEST 2009


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.

As we parse only seq, ext, pict and slice start codes, I still feel the 
solution is good enough.

To correctly address that, one would probably have to pass information 
about "correctness" of the frame (whether picture start code was found 
in it) from low-level parser to the generic parser code, so that 
position and timestamp can be correctly assigned to the frame for which 
PTS was defined in PES packet header. But this will still not address 
the problem above, if more than one video (key) frame is in the same PES 
packet and first one actually commences with its seq start code already 
in previous packet. But for practical purposes, I think this can be 
safely ignored, as PES packets are rather small and video frames rather big.

 From H.222.0, 2.4.3.7:

"In the case of ISO/IEC 11172-2 video or ISO/IEC 14496-2 video, if a PTS 
is present in a PES packet header, it shall refer to the access unit 
containing the first picture start code that commences in this PES 
packet. A picture start code commences in a PES packet if the first byte 
of the picture start code is present in the PES packet."

and

"For ITU-T Rec. H.262 | ISO/IEC 13818-2 video, if a PTS is present in a 
PES packet header, it shall refer to the access unit containing the 
first picture start code that commences in this PES packet. A picture 
start code commences in a PES packet if the first byte of the picture 
start code is present in the PES packet."

Currently, if a PES packet contains the rest of the previous frame and 
start of a new one (its picture start code), the PTS will be assigned to 
this broken partial frame instead of the frame commencing in this PES 
packet.

If you have an idea how to correctly address this (how to pass the 
information from format parser to lavf utils and how to handle it 
there), that might be the solution.


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

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

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

BTW, I personally don't care much about MPEG-PS support, so I'm not 
doing this for myself.

Regards,

Ivan




More information about the ffmpeg-devel mailing list