[FFmpeg-devel] [PATCH] Fix MPEG-TS seek and frame positions in general

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sun, Mar 01, 2009 at 07:16:50PM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> On Sun, Mar 01, 2009 at 05:58:30PM +0100, Ivan Schreter wrote:
>>>   
>>>       
>>>> Michael Niedermayer wrote:
>>>>     
>>>>         
>>>>> On Sun, Mar 01, 2009 at 02:31:27PM +0100, Ivan Schreter wrote:
>>>>>   
>>>>> [...]
>>>>>       
>>>>>           
>>>>>> Why do we need the array with several timestamps? I'd say the code can be 
>>>>>> simplified to (names possibly suboptimal):
>>>>>>
>>>>>> 1) set cur_pts, cur_dts, cur_pos to pts/dts/pos of currently-arriving 
>>>>>> (packet) buffer,
>>>>>> 2) if frame_pts, frame_dts and frame_pos is unset, set them to cur_pts, 
>>>>>> cur_dts and cur_pos,
>>>>>> 3) unset next_pts, next_dts and next_pos,
>>>>>> 4) call actual parser,
>>>>>> 5) if the parser returns a buffer, frame_pts, frame_dts and frame_pos are 
>>>>>> actual pts/dts/pos for the parsed buffer, so return them in pts, dts and 
>>>>>> pos,
>>>>>> 6) reset frame_pts, frame_dts and frame_pos to next_pts, next_dts and 
>>>>>> next_pos,
>>>>>> 7) reset next_pts, next_dts and next_pos to unset.
>>>>>>     
>>>>>>         
>>>>>>             
>>>>> this will not work, not at all
>>>>> example
>>>>> packet1 50% of frame1 pts=0 (iam just writing one timestamp due to lazyness)
>>>>> (1) cur_pts=0
>>>>> (2) frame_pts=0
>>>>> (3) next_pts=-
>>>>> (45) parser says nothing yet
>>>>> (6) frame_pts=-
>>>>> (7) next_pts=-
>>>>> packet2 50% of frame1 pts=-
>>>>> (1) cur_pts=-
>>>>> (2) frame_pts=-
>>>>> (3) next_pts=-
>>>>> (45) parser returns the packet, correct pts is 0 but you return nothing
>>>>>
>>>>> this is just the most trivial case, a frame split in 2 packets and detected
>>>>> in the second
>>>>>   
>>>>>       
>>>>>           
>>>> Uhm, I think this is a misunderstanding of (2). For packet 2, (2) will 
>>>> NOT set frame_pts to (unset) cur_pts, but leave it set.
>>>>     
>>>>         
>>> frame_pts has been already wiped out in (6) of the previous packet.
>>> (2) is setting it to unset because it is unset already. its a NOP
>>>
>>>
>>>   
>>>       
>> Sorry I messed it up, 6 and 7 were meant to be executed also only if 
>> buffer returned, as:
>>
>> 1) set cur_pts, cur_dts, cur_pos to pts/dts/pos of currently-arriving 
>> (packet) buffer,
>> 2) if frame_pts, frame_dts and frame_pos is unset, set them to cur_pts, 
>> cur_dts and cur_pos,
>> 3) unset next_pts, next_dts and next_pos,
>> 4) call actual parser,
>> 5) if the parser returns a buffer:
>>    5a) frame_pts, frame_dts and frame_pos are 
>>        actual pts/dts/pos for the parsed buffer, so return them in
>>        pts, dts and pos,
>>    5b) reset frame_pts, frame_dts and frame_pos to next_pts,
>>        next_dts and next_pos,
>>    5c) reset next_pts, next_dts and next_pos to unset.
>>     
>
>
> packet1 50% of frame1 pts=0
> (1) cur_pts=0
> (2) frame_pts=0
> (3) next_pts=-
> (45) parser says nothing yet
> packet2 50% of frame1 pts=-
> (1) cur_pts=-
> (2) frame_pts=0
> (3) next_pts=-
> (45) parser says nothing yet (it has a complete frame but doesnt know yet)
> packet3 50% of frame2 pts=1
> (1) cur_pts=1
> (2) frame_pts=0
> (3) next_pts=-
> (45) parser returns packet correctly with pts0
> (5b) frame_pts=-
> at this point you have lost the timestamp of the next packet
>
>   
No, because when processing packet3, the codec parser finds out frame1 
was complete and calls something like ff_next_frame_found(). Thus, 
next_pts will be set to cur_pts == 1. After setting s->pts to frame_pts 
(== 0), frame_pts will be set to next_pts == 1, so timestamp is NOT 
lost. Next call of the parser will find frame_pts set to 1, so 
everything works as expected. Also today, if a codec parser handles such 
case, it has to tell the generic parser via ff_fetch_timestamp, it just 
has to tell it differently.

I just implemented the method as an experiment. In principle it works, 
but it has some issues, if there are some stray packets before actual 
frame packet, which get accounted for the frame before real first packet 
with dts/pts comes in (actually, I'm afraid that as of today, we have a 
similar problem). Since you already implemented a change in parser 
regarding packets without pts/dts, I'm putting it on the shelf and will 
update my original framepos patch based on your change.

Regards,

Ivan