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

[Michael Niedermayer]

On Sat, Feb 07, 2009 at 01:50:44AM +0100, Ivan Schreter wrote:
> Hi,
>
> I am working on getting files from my AVCHD camcorder fully supported by 
> ffmpeg. I noticed that MPEG-TS seeking is broken. Unfortunately, it has not 
> been trivial to fix. Here is a set of 7 patches, which address the issue.
>

> Patch #1: trivial patch to move av_read_frame_flush() to internal.h (needed 
> later).

ok if the function is needed from outside utils.c (it currently is not,
iff a patch is approved that needs this then this change is ok)


>
> Patch #2: let mpegts format decoder return correct packet positions by 
> keeping track of position of first TS packet of a PES packet in 
> MpegTSPESFilter.

not maintained by me


>
> Patch #3: trivial patch to handle decoding delays >1 correctly. Some 
> decoders (namely h264) set has_b_frames to >1 after seek, but only for 
> explicitly 1 is checked here. Can be applied independently of the rest of 
> the patches.

both hunks are wrong, you are attempting to apply mpeg2 semantics to h264
this is incorrect.


>
> Patch #4: make av_read_frame fill packet position correctly by keeping 
> track of the position of first packet of a frame in AVStream.cur_pos.

see comemnts inline


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

not maintained by me

>
> Patch #6: optimization of seeking by keeping track of min/max position and 
> timestamp on AVStream instead of computing them over and over again. Can be 
> applied independently of the rest of the patches.

rejected, there is a cache already. in the code calling av_gen_search()

also note that the seek API is likely going to be changed as it has some
issues (see the related thread about it)

[...]
> Index: libavformat/avformat.h
> ===================================================================
> --- libavformat/avformat.h	(revision 17012)
> +++ libavformat/avformat.h	(working copy)
> @@ -491,6 +491,7 @@
>      AVMetadata *metadata;
>  
>      /* av_read_frame() support */
> +    int64_t cur_pos;    /**< frame position in the stream */
>      const uint8_t *cur_ptr;
>      int cur_len;
>      AVPacket cur_pkt;

you cant add any field in the middle of a public struct, this breaks ABI


[...]
> @@ -944,12 +948,14 @@
>  
>                  /* return packet if any */
>                  if (pkt->size) {
> -                    pkt->pos = st->cur_pkt.pos;              // Isn't quite accurate but close.
>                  got_packet:
>                      pkt->duration = 0;
>                      pkt->stream_index = st->index;
>                      pkt->pts = st->parser->pts;
>                      pkt->dts = st->parser->dts;
> +                    pkt->pos = st->cur_pos;
> +                    /* reset stream position, next read will fill it again */
> +                    st->cur_pos = -1;
>                      pkt->destruct = av_destruct_packet_nofree;
>                      compute_pkt_fields(s, st, st->parser, pkt);
>  

This looks suspicious as the pos is set at a different place in the code

[...]


-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Republics decline into democracies and democracies degenerate into
despotisms. -- Aristotle
-------------- 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/20090208/d4ca396e/attachment.pgp>