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

Ivan Schreter schreter
Sun Aug 30 15:46:33 CEST 2009

Michael Niedermayer wrote:
> On Sat, Aug 22, 2009 at 10:07:48AM +0200, Ivan Schreter wrote:
>> Hi Michael,
>> Michael Niedermayer wrote:
>>> [...]
>>>>> i really dont see where the problem comes from
>>>>> you do serach for a keyframe somehow already, so you must at that point 
>>>>> know
>>>>> where in the PES packet the frame is, skip whats before (minus seq 
>>>>> headers
>>>>> and such) from the parser output, that seems to be all thats needed 
>>>>> assuming
>>>>> the keyframe choosen is the correct one in the new seeking semantics
>>>>> but maybe i miss something ...
>>>> That's exactly what I do!
>>>> But the problem is:
>>>>    * I find the PES packet where my frame starts and return position of
>>>>      this frame.
>>>>    * I reposition the file pointer to this PES packet.
>>>>    * Parser is reset.
>>>>    * First frame read from the parser is the remainder of the previous
>>>>      frame (starting somewhere hundred PES packets earlier).
>>>>    * Parser assigns PTS/DTS of the frame actually starting in this PES
>>>>      packet to this broken frame with remainder of the previous frame.
>>>>    * Second frame read from the parser is the key frame, but timing
>>>>      info is wrong - PTS/DTS contains AV_NOPS_VALUE and cannot be
>>>>      computed correctly.
>>>> Strictly speaking, the frame stream coming out of the parser is as 
>>>> decodable as ever, but since timestamps are messed up, it's impossible to 
>>>> correctly play it from this position.
>>>> Unfortunately, I don't have any chance to detect that whatever I find 
>>>> before first picture start code in the PES packet is not a frame, except 
>>>> by checking whether there is a picture start code and a slice in there.
>>>> PTS/DTS in the PES packet is associated by lavf/utils.c with first frame 
>>>> returned, which starts in this PES packet. Since our partial frame 
>>>> "starts" in this PES packet, PTS/DTS is associated with it instead of 
>>>> with the actually starting frame in this PES packet.
>>>> So, when I simply skip the broken frame at the beginning, first frame 
>>>> read by the parser will be the frame which really starts in the PES 
>>>> packet and it gets the proper timing info associated.
>>>> When decoding the file from the beginning, we find the proper beginning 
>>>> of the frame whose last part is in my PES packet and thus don't steal 
>>>> PTS/DTS of the frame really starting in the PES packet.
>>>> I hope I explained it somewhat clearly now...
>>> i understand your explanation but i dont understand the problem
>>> the parser, after an reset misassociates the first timestamp to something
>>> that is not a frame and i guess it also is not detect as a frame.
>>> That is a bug, it has to be fixed (and not by discarding this partial
>>> frame)
>> Uhm, as you say, the parser misassociates the timestamp to something that 
>> is not a frame. It will be then refused by the decoder, since it doesn't 
>> contain minimum necessary information (picture start code and slice start 
>> code).
>> So is something missing picture start code and slice start code in your 
>> opinion a partial frame or is it something not a frame?
> it can be both, this depends on what else is in there
> if you look at mpeg4 (a)sp then its possible to even redundantly store
> various things from the picture headers in the slice headers
True. We are talking MPEG-1 video and MPEG-2 video here, though.

> [...]
>> OK, it would be maybe possible to get the additional information about 
>> where in PES packet the frame starts and handle it in generic parser. BUT: 
> its certainly possible
In MPEG-1 and MPEG-2 video it is indeed possible to detect, where the 
next frame starts, but this is done by searching for picture start code 
and slice start code. There is no additional information for frame start 

>> This won't work as well. Namely, this would only fix the problem with first 
>> stream encountered after a seek. If there is a PES packet for stream 1 
>> followed by a PES packet for stream 2, which has exactly the same problem 
>> (rest of previous frame without picture start code and slice start code and 
>> start of a new frame and contains PTS/DTS), then the problem would simply 
>> happen in stream 2.
> AVStream.initial_bytes_to_discard
> and that amount is discarded after the parser
A possiblity, yes. But who will fill it and when?

I actually find your suggestion of fixing the misassociation of the 
timestamp to the frame first (at the end) much better. So let's 
concentrate on that one.

> [...]
>> you'd have to keep the information per-stream. 
> thats what i meant to suggest
OK, that was not clear to me. Then, your suggestion makes sense, of course.

 > [...]
> 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.
    * Otherwise, just note down in parser context to fetch the timestamp
      next time the parser is called, so it's associated with the next

Do you think this would be the right direction how to solve it?



More information about the ffmpeg-devel mailing list