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

[Michael Niedermayer]

On Sun, Feb 08, 2009 at 07:40:42PM +0100, Ivan Schreter wrote:
> Michael Niedermayer wrote:
> > On Sat, Feb 07, 2009 at 01:50:44AM +0100, Ivan Schreter wrote:
> >   
> > [...]
> >> 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.
> >   
> What is the semantics of has_b_frames? In doxygen docs, only value of 1 
> for 1 frame delay is documented. In H264, the decoder sets has_b_frames 
> to 1 (although we have 2 frame delay). After a seek, it suddenly becomes 
> 2 (correct). I didn't check exactly what value it has during whole 
> lifetime of the stream. In any case, there is a delay.

ive fixed the has_b_frames doxy
thanks for spotting this


> 
> In code around hunk #1, it is tested, if pts != dts in delay case. But 
> why check only delay==1? With higher delays, it probably should check as 
> well...

This check depends on presentation_delayed being set correctly, it is not
for h264.

[...]
> 
> >> 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()
> >
> >   
> Yes and no. There is index cache, if index is maintained, but this is 
> not the case for mpegts. 

so you know where the problem is, fix it there.
dont add hacks on top.


[...]

> 
> But anyway, this is not so important, it's just seek speed optimization.
> 
> 
> > 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
> >   
> OK, I'll correct it. Although in this particular case, the fields are 
> used only internally, thus it wouldn't break applications.
> 
> >
> > [...]
> >   
> >> @@ -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
> >   
> Other possibility would be to set it also at the other point before 
> goto, but why duplicate code? Original code would not set the position 
> at all for frames completely contained in one packet.
> 

> Anyway, the current implementation with goto into different if branch is 
> pretty nasty and IMHO should be refactored.

cleanup patches are always welcome just keep in mind if you call it
simplification and it doesnt decrease the number of lines its rejected.

and replacing a goto by half a page of code is as well, if OTOH you can
replace the goto by a loop that is not larger than this is welcome.


> 
> I still believe, this way it is correct.

maybe it is so, ill look more at it in the next iteration of the patch

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/802bbf29/attachment.pgp>