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

[Michael Niedermayer]

On Mon, Feb 09, 2009 at 01:35:58AM +0100, Ivan Schreter wrote:
> 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? 

no for mpeg2 it will be set to 1 99% of the time, and that can be 
IBPBPBBBBBBBBBBPBBPIIIPPPBBPPIIPIBBBBI if the encoder likes
or just IPPPPPPPPPPPPPPPPPP

low delay mpeg2 have it set to 0 and they do not contain B frames

h264 can set it to any value between 0 and 15 or 16 or so
and any of that can contain any frame type


> What is the 
> semantics in H.264?
[...]
>
>>>>        
>>>>> 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.

ive looked more at it and it looks very wrong
the newly added variable is a duplicate of cur_pkt.pos and you override
the use of cur_pkt.pos at some places with it.
i suspect that moving got_packet: up one line has the same effect as your
rather bloated patch

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

Freedom in capitalist society always remains about the same as it was in
ancient Greek republics: Freedom for slave owners. -- Vladimir Lenin
-------------- 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/20090210/0df6016d/attachment.pgp>