[FFmpeg-devel] [PATCH] correct MPEG video parser not to return incomplete frames
Thu Sep 10 05:57:41 CEST 2009
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
>>>> how unlikely it would cause a problem in mpeg video, it will cause
>>>> 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
>> 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
>>>> invalid without this being detectable the decoder or parser would have
>>>> 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
>> 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
> * 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,
>>>>>> has to be fixed _first_. I am against adding new features on top of
>>>>> 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
>>> Man, I'm trying to help! Instead of bashing, make a proposal how to do
>> iam not sure how to implement all the stuff down to the finest detail
>> 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
>> 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
>> 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
> 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
>> writing a functioning implementation does take time, you seem to expect
>> 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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel