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

[Ivan Schreter]

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.

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

In code around hunk #2, the if block computing timestamps doesn't get 
executed at all with delay==2, which produces wrong timestamp. With my 
patch, the timestamps produced were correct. But I admit, I don't 
understand the code there fully.

>> 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. Thus, every seek determines the first and last 
ts again and again. Or did I miss something? I believe not, since it 
does it at runtime again and again.

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.

I still believe, this way it is correct.

Regards,

Ivan