[FFmpeg-devel] [PATCH] Fix MPEG-TS seek and frame positions in general
Sun Mar 1 16:21:01 CET 2009
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
> 6) reset frame_pts, frame_dts and frame_pos to next_pts, next_dts and
> 7) reset next_pts, next_dts and next_pos to unset.
this will not work, not at all
packet1 50% of frame1 pts=0 (iam just writing one timestamp due to lazyness)
(45) parser says nothing yet
packet2 50% of frame1 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
Size: 189 bytes
Desc: Digital signature
More information about the ffmpeg-devel