[FFmpeg-devel] [PATCH] Make parser not favor packets with pts/dts (and related fixes)

[Ivan Schreter]

Michael Niedermayer wrote:
> On Wed, Mar 04, 2009 at 11:40:01PM +0100, Ivan Schreter wrote:
>   
>> Michael Niedermayer wrote:
>>     
>>> On Wed, Mar 04, 2009 at 09:13:54PM +0100, Ivan Schreter wrote:
>>>   
>>>       
>>>> Hi,
>>>>
>>>> Michael Niedermayer wrote:
>>>>     
>>>>         
>>>>> patch below does make the parser treat packets equal, <put some joke 
>>>>> mixing
>>>>> human and packet rights here>
>>>>>
>>>>> [...]
>>>>> this should make it easier to feed pos in the parser as well
>>>>> [...]
>>>>>   
>>>>>       
>>>>>           
>>>> Attached patches add support for storing packet position alongside dts/pts 
>>>> in lavc and using it in lavf to determine correct frame position (provided 
>>>> cur_pkt.pos is set correctly). They are prerequisite for seeking changes, 
>>>> which rely on having AVPacket.pos set correctly.
>>>>
>>>> lavf patch causes regression in seek test (attached as well), since 
>>>> positions are corrected. PTS/DTS is the same, so it seems perfectly OK.
>>>>
>>>>     
>>>>         
>>>   
>>>       
>>>> BTW, when committing changes, should the regression change go together in 
>>>> one commit with the code which causes the regression, or separately?
>>>>     
>>>>         
>>> regression changes should be in the comment that causes them to change so that
>>> one can check out any revission and has a working "make test"
>>>
>>>   
>>>       
>> It was also my idea how it should work :-)
>>
>>     
>>> [..]
>>>   
>>>       
>>>>                  got_packet:
>>>>                      pkt->duration = 0;
>>>>                      pkt->stream_index = st->index;
>>>>                      pkt->pts = st->parser->pts;
>>>>                      pkt->dts = st->parser->dts;
>>>> +                    if (st->parser->pos != AV_NOPTS_VALUE)
>>>> +                        pkt->pos = st->parser->pos;
>>>> +                    else
>>>> +                        pkt->pos = st->cur_pkt.pos;
>>>>     
>>>>         
>>> is this still needed?
>>> if so why?
>>>   
>>>       
>> Actually not anymore.
>>
>> So this hunk should be OK:
>>
>> @@ -967,12 +968,12 @@
>>
>>                  /* return packet if any */
>>                  if (pkt->size) {
>> -                    pkt->pos = st->cur_pkt.pos;              // Isn't 
>> quite accurate butclose.
>>                  got_packet:
>>                      pkt->duration = 0;
>>                      pkt->stream_index = st->index;
>>                      pkt->pts = st->parser->pts;
>>                      pkt->dts = st->parser->dts;
>> +                    pkt->pos = st->parser->pos;
>>                      pkt->destruct = av_destruct_packet_nofree;
>>                      compute_pkt_fields(s, st, st->parser, pkt);
>>
>> Any other comments or shall I commit?
>>     
>
> i think the rest is ok
>   
Applied.

BTW, are there any seek tests on FATE? If so, possibly they will also 
need regression update (and also after I commit seeking keyframe patches 
after review later)...

Regards,

Ivan