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

[Ivan Schreter]

Michael Niedermayer wrote:
> On Sun, Feb 08, 2009 at 07:40:42PM +0100, Ivan Schreter wrote:
>   
>> 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
>
>   
Hm, the description is now somewhat clearer... So one could infer, in 
the case of MPEG-2 IPBBPBB... stream, this should be set to 2? What is 
the semantics in H.264?

>>>> 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.
>
>   
OK, but I'm not going to fix this (performance) problem. Someone else 
has to.

>>>  
>>>       
>>>> 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
>>> [...]
>>>       
>> I still believe, this way it is correct.
>>     
>
> maybe it is so, ill look more at it in the next iteration of the patch
>   
So here we go, patch #4 attached.

Note: the position will be correct only if demuxer's read_packet also 
returns correct position in the packet (patch #2 does this for MPEG-TS, 
some other demuxers seem to work already correctly).

Regards,

Ivan

-------------- next part --------------
A non-text attachment was scrubbed...
Name: seek_4_frame_pos.patch
Type: text/x-patch
Size: 2709 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090209/058f3566/attachment.bin>