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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Mon, Feb 16, 2009 at 06:14:01PM -0800, Baptiste Coudurier wrote:
>   
>> Hi,
>>
>> Ivan Schreter wrote:
>>     
>>> Ivan Schreter wrote:
>>>       
>>>> Ivan Schreter wrote:
>>>>   
>>>>         
>>>>> Ivan Schreter wrote:
>>>>>   
>>>>>     
>>>>>           
>>>>>> [...]
>>>>>> Patch #5: mpegts seek only searches for a PES packet with correct pid 
>>>>>> and existing DTS and relies on this being a key frame. This is not 
>>>>>> even the case in the test file. Read frames via av_read_frame until a 
>>>>>> key frame is found and return position/timestamp of this frame. This 
>>>>>> needs #1 to reset the packet reader after reading the frames, so 
>>>>>> further reads work correctly, #2 to have correct positions from mpegts 
>>>>>> and #4 to have exact position of the packet for later file seek.
>>>>>>     
>>>>>>             
>> If I understand correctly, this seems to be a general issue with
>> read_timestamp which does not honor the key frame request.
>>     
>
> originally read_timestamp() was supposed to always saerch for a keyframe
> and only return its pos&timestamp. If it did that seeking would work alot
> more reliable ...
> but this had a flaw i did not fully realize originally, namely that
> doing it that way needs O(keyframe_distance * log filesize)
> while first searching for a timestamp and then secondly for a keyframe
> needs O(packet_distance * log filesize + keyframe_distance)
> so because its better and faster read_timestamp() ignores keyframes
> sadly the 2nd step (keyframe search) is missing ...
>
>   
So you mean, the second step of searching for key frame should be done 
in generic way in utils.c? I was experimenting with it a bit.

I made a simple routine av_find_keyframe() which consumes read_timestamp 
function to position the stream to a defined position and then finds the 
position and DTS of the key frame using av_read_frame() calls. Then, 
I've replaced all read_timestamp calls with this. It does work, but I 
didn't get it clean yet, so seek regression doesn't work yet (it hangs 
for one format). I suppose, the reason is some bug in handling border case.

Shall I clean it up and prepare a patch for that?

Anyway, even if finding keyframe is done in generic way, seeking will 
work in O(key_frame_distance * log filesize). But I don't see this as a 
major problem. At least it will work :-)

Further, as a prerequisite we still need to correct frame position as 
returned from av_read_frame (discussed in another subthread), but I 
didn't get to clean it up yet.

> also for the new seek API we need to not only find one keyframe of one
> stream but one keyframe for each active stream and in both directions
> then from this decide where to seek to. Sounds nasty but shouldnt be
> because it doesnt cost to consider more streams ...
>
>   
When do you plan to introduce working new seeking API? Anyway, at least 
correct positions from av_read_frame() and from containers are a 
prerequisite here as well...

Regards,

Ivan