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

[Michael Niedermayer]

On Sun, Sep 06, 2009 at 05:33:34PM +0200, Ivan Schreter wrote:
> Michael Niedermayer wrote:
>> [...]
>>>> Also the mpeg demuxer returns random trash prior to the first frame, 
>>>> this IS
>>>> a bug in your seeking code. Fix it please! And before you start arguing 
>>>> about
>>>> how unlikely it would cause a problem in mpeg video, it will cause 
>>>> problems
>>>> in mpeg audio as well.
>>>>         
>>> I don't understand what random trash is returned from MPEG demuxer prior 
>>> to first frame. Also, I don't understand why the seeking code should be 
>>> causing this, since it positions the file to start reading at the packet 
>>> with start of a frame. Old seeking code didn't do it differently...
>>>
>>> How can I reproduce the problem? With which sample file?
>>>     
>>
>> any mpeg-ps file created with ffmpeg
>>   
>
> Okaaay... My seeking patches were only applied to mpeg-TS (transport 
> stream). I did no change to mpeg-PS (program stream) yet. So it's 
> DEFINITELY not a bug in my seeking code, as it's not even active there! I 
> wanted to fix the parser first, before activating it there.

the issue applies to TS as much as PS i just havnt checked if TS files
generated by ffmpeg could trigger it, i think that at least in the past
ffmegs TS muxer did not support putting frames over PES packet boundaries


[...]

>> To make it even more clear what the problem is, look at this example, the 
>> seeking code seeks to packet 5, with the expectation that the decoder
>> will decode the first frame in there but there is a startcode emulation
>> in the previous partial frame causing the decoder not only to return noise
>> but also to miss the first complete frame. The only way for the decoder to
>> avoid this is to drop the first few frames but if it does, it starts 
>> decoding
>> later than when the seeking code expects.
>> Its the seeking code that should start a little earlier and discard data
>> to make sure synchronization of frame boundaries did happen
>>   
>
> OK. But the question is, how much earlier? Theoretically, we could have 
> such a bad file, where we can possibly never synchronize, or am I mistaken? 

i belive that you are correct. Its pretty unlikely unless such a file is
created intentionally though


> I'll have to look in the standard for details.
>
>> its what i suggested already previously for fixing the video corner cases
>>   
>
> To which thread/post are you referring?
>
>>>> The goal of the seeking code is to be exact, if now the first frame can 
>>>> be
>>>> invalid without this being detectable the decoder or parser would have 
>>>> to
>>>> discard it and this would break exactness
>>>>         
>>> I agree with you. But again, the parser has to somehow detect the broken 
>>> non-frame. And you disagree with detection of non-frame by detecting 
>>> missing picture start code and slice start code (concretely, for MPEG 
>>> video). So how am I supposed to detect such non-frame, so I can 
>>> initialize AVStream.initial_bytes_to_discard?
>>>     
>>
>> you start a frame or 2 earlier and throw them away, with each the 
>> probability
>> of being off drops
>>   
>
> This basically answers my question above. So you just want to reduce the 
> probability. I.e., I could do it in this way in the ff_gen_syncpoint_search 
> routine:
>
>    * Back off to position X.
>    * Start reading frames from this position and for each stream count
>      number of parsed frames.
>    * When at least 2 frames have been read, only then actually consider
>      the frame for further processing (keyframe detection, timestamp
>      comparison and the like).
>    * Exception: We are already at position 0 in the file (then, it's
>      equivalent starting playing from the beginning, so no frames
>      should be ignored).

this sounds approximately what i had in mind, yes


>
>
> Further, the parser needs to be enhanced to somehow store in-packet 
> position of the frame, so AVStream.initial_bytes_to_discard can be 
> implemented. I suppose, frame start offset in packet can be handled in 
> similar manner as cur_frame_pos. Right?

iam not sure if this is needed, but if it is then yes


>
>>
>>   
>>>> [...]
>>>>         
>>>>>> [...]
>>>>>> IIRC our parser misassociates a timestamp in the partial frame case, 
>>>>>> this
>>>>>> has to be fixed _first_. I am against adding new features on top of 
>>>>>> known
>>>>>> bugs.
>>>>>>                   
>>>>> I looked at the code in mpegvideo_parse() and 
>>>>> ff_mpeg1_find_frame_end(). To me, it seems like 
>>>>> ff_mpeg1_find_frame_end() misassociates the timestamp. Namely, the 
>>>>> timestamp is fetched there, but actually the _previous_ frame (or in 
>>>>> this case, a non-frame) is returned from the parsing routine.
>>>>>
>>>>> I'm not sure how to fix this, though. Possibly, instead of fetching the 
>>>>> timestamp in ff_mpeg1_find_frame_end(), it should be done in this way:
>>>>>
>>>>>             
>>>>         
>>>>>    * If the frame starts at offset 0 in the PES packet AND we don't
>>>>>      have anything in the buffer, then fetch the timestamp.
>>>>>             
>>>> sounds like another one of your not always working hacks
>>>>         
>>> Grrrr....
>>>
>>> Man, I'm trying to help! Instead of bashing, make a proposal how to do 
>>> it!
>>>     
>>
>> iam not sure how to implement all the stuff down to the finest detail 
>> without
>> new issues poping up, but iam sure you take this too serious ...
>>
>> the first partial frame has no pic startcode, it cant otherwise it would 
>> be
>> the correct target for the timestamp so  why is our parser associateing 
>> the timestamo with it? ive not had time to look at the actual code more
>> carefully sadly ...
>> The timestamp association rules are quite clear in mpeg, the parser really
>> should not need any black magic to get it right even if it doesnt know if 
>> the
>> first part is a complete frame or not.
>>   
>
> As I understand it, if a packet contains some data with a start code in 
> there, the data before startcode is part of previous frame and the frame 
> with its data starting at the first startcode should get the timestamp 
> specified in this packet.
>
> MPEG video parser fetches the timestamp when finding the new start code. 
> Data before the start code are added to the buffer containing previous 
> frame. This previous frame is returned in this call of the parser, since it 
> has been completely read. But the parser's timestamp is already fetched. So 
> the previous frame will get the timestamp. Next frame just beginning in the 
> packet will not get the correct timestamp.
>
> If the start code is at position 0 in the packet, then the frame will get 
> the correct timestamp.
>
> I suppose, timestamp fetching must be instead postponed by remembering the 
> position of the start code in the file and fetching the timestamp when the 
> buffer is completed instead. I.e., instead of fetching the timestamp in 
> ff_mpeg1_find_frame_end() we'd just store the position on parser context 
> there and fetch the timestamp in mpegvideo_parse() associated with this 
> position just before returning the new frame back. Would you agree with 
> this?
>

> I'm just unsure, how long the association of timestamps with positions is 
> kept in lavf. Possibly, this buffer overflows before the packet is 
> completely constructed, so the timestamp has to be fetched earlier (in the 
> next parser call after returning previous frame).

the buffer can overflow quite quickly in theory, the timestamps thus must
be processed pretty much immedeatly, handling them at the end of a video
frame is not guranteed to work

also the issue with startcode emulation doesnt exist for mpeg1/2 video
just for some audio codecs 


>
> My first patch for mpegvideo_parser.c was incorrect, since it only corrects 
> associating the first timestamp of the first frame by discarding the 
> non-frame at the beginning of the packet. But the remaining frames will 
> also get wrong timestamps, if I see it correctly.
>
>>
>>   
>>> I'm again at the point of simply giving up and investing my limited time 
>>> into something else, where people value the contribution a bit more...
>>>     
>>
>> your contribution is valued but implementing this is not a trivial task 
>> and
>> writing a functioning implementation does take time, you seem to expect 
>> that
>> a quick implementation would fully work
>>   
>
> No. I'm just expecting constructive criticism instead of bashing.
>
> I simply find comments like "sounds like another one of your not always 
> working hacks" or "is complete nonsense" insulting and inappropriate, 
> especially if no constructive criticism follows.

its sometimes hard to provide constructive criticism when one has little
time and the issue at hand is complex.

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20090910/0d73e7d1/attachment.pgp>