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

[Ivan Schreter]

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.
>
>   
Yes, read_timestamp will return position of some timestamp, not 
necessarily a key frame. mpegts by itself cannot know, if it is a key 
frame or not, only after actually parsing the frame found at the given 
position (and walking to a frame which IS a key frame).

Generic implementation would then in av_gen_search() call 
read_timestamp_and_find_keyframe() instead of just read_timestamp(), and 
this new function would call read_timestamp() and then walk the frames 
until key frame found. I.e., the code for key frame search which I 
implemented for mpegts.c would be moved to utils.c. Do I understand you 
correctly?

The only disadvantage is, if we have some formats which do return 
timestamp of a key frame, we'd do an unnecessary read_frame. But since 
seeking is IMHO not that time critical, this should be no big deal.

Shall I prepare a patch for that? Michael must also give his OK then, I 
suppose.

> I'm not sure if this code belongs in mpegts demuxer since mpegps also
> suffer from the same problem, using read_timestamps.
>   
Possibly. I didn't check mpegps, since I only want to process mpegts 
files :-)

> I think this could be handled in a generic way in av_seek_frame_binary.
> Maybe mpegts and mpegps demuxer could use AVFMT_GENERIC_INDEX to
> populate index entries with key flag set which will be set by parser, it
> seems current mpegps demuxer use index but consider all frames as key
> frames.
>   
Currently, it seems like the index is not populated for mpegts. Do you 
mean, setting this flag will do it then automagically? I'll try it.

Regards,

Ivan