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

[Michael Niedermayer]

On Sun, Mar 01, 2009 at 02:31:27PM +0100, Ivan Schreter wrote:
> 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.

the idea was of course to store all and not just when dts/pts is set.
I see this causes problems with the late fetch_timestamp use in mpeg1/2
though, not sure about this but this is a bug IMHO


[...]
>
> 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.

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

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

While the State exists there can be no freedom; when there is freedom there
will be no State. -- Vladimir Lenin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090301/f6447134/attachment.pgp>