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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sat, Feb 28, 2009 at 10:08:46PM +0100, Ivan Schreter wrote:
>   
>> [...]
>> What do you think?
>>     
>
> your code looks broken
> pos has to be handled like pts/dts in the parser so that it is assigned to
> the correct packet you arent doing that so my assumtation is your code
> does not work except for cases where you are lucky enough that frames are
> not split over packets 
>   
Mhm... OK, I took your advice and handle it exactly like pts/dts now 
(patches attached).

It seems like the positions are now corrected and each seek returns 
packets with some well-defined position. I just have to find out how to 
integrate it with my other seeking changes (but that's another story).

Updated patches + seek regression changes attached.

Before you ask about this:

                     pkt->pts = st->parser->pts;
                     pkt->dts = st->parser->dts;
+                    if (st->parser->pos != AV_NOPTS_VALUE)
+                        pkt->pos = st->parser->pos;
+                    else
+                        pkt->pos = st->cur_pkt.pos;
                     pkt->destruct = av_destruct_packet_nofree;
                     compute_pkt_fields(s, st, st->parser, pkt);

some formats (e.g., mp2) don't provide dts/pts at all, so in that case, 
we also don't store position at all (since it will be stored only for 
packets having dts/pts). Therefore the fallback to position of the 
current packet.

Alternatively, it might make sense to consider positions independent of 
timestamps in the parser and store them always and add something like 
ff_fetch_position() for positions. I don't know, though, how big the 
reorder buffer must be in that case.

My original patch let the parser implementation decide about when the 
packet starts and store then-current position in internal context to 
return it after parsing complete. If I understand the code in 
av_parser_parse() and ff_fetch_timestamp() correctly, it will anyway 
fetch timestamps (and thus position) at exactly next call of 
av_parser_parse() after the frame is completely parsed.

The only exception is when a frame starts in the middle of the buffer 
(e.g., new startcode in middle of mpeg packet in transport stream) and 
thus we need the timestamp/position of this packet for the next frame. 
In current implementation, this is achieved by explicit call of 
ff_fetch_timestamp() in mpegvideo_parser. But this could be done as 
easily by assigning frame start timestamp to just processed frame and 
copying current packet timestamp to frame start timestamp. We do not 
need an array for that...

We seem to have just two exceptions: mpegvideo_parser and dvbsub_parser.

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.

If a specialized parser (e.g., mpegvideo) detects a start code within 
current buffer, it will set next_pts, next_dts and next_pos to cur_pts, 
cur_dts and cur_pos, thus making sure this next frame will get assigned 
correct pts/dts/pos.

The whole code storing and comparing some funny offsets, which don't 
match actual file offsets might be dumped then.

Is there something that escapes me? If not, I'd propose to redo it this 
way. It's much simpler to understand it than the current code.

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: avcodec_framepos.patch
Type: text/x-patch
Size: 5567 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/99352518/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: avformat_framepos.patch
Type: text/x-patch
Size: 2974 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/99352518/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: regress_framepos.patch
Type: text/x-patch
Size: 23104 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/99352518/attachment-0002.bin>