[FFmpeg-devel] [PATCH] increase AVStream pts_buffer size

[Baptiste Coudurier]

Michael Niedermayer wrote:
> On Mon, Aug 11, 2008 at 12:47:28PM -0700, Baptiste Coudurier wrote:
>> Hi Michael,
>>
>> Michael Niedermayer wrote:
>>> On Sat, Aug 09, 2008 at 06:28:47PM -0700, Baptiste Coudurier wrote:
>>>> Hi Michael,
>>>>
>>>> Michael Niedermayer wrote:
>>>>> On Sat, Aug 09, 2008 at 05:34:51PM -0700, Baptiste Coudurier wrote:
>>>>>> Baptiste Coudurier wrote:
>>>>>>> M?ns Rullg?rd wrote:
>>>>>>>> Baptiste Coudurier <baptiste.coudurier at smartjog.com> writes:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> H264 decoder can (now?) set has_b_frames to MAX_DELAYED_PIC_COUNT which
>>>>>>>>> is 16, need to port this to pts_buffer in AVStream struct.
>>>>>>>>>
>>>>>>>>> compute_pkt_files in utils.c:
>>>>>>>>> int delay = FFMAX(st->codec->has_b_frames, !!st->codec->max_b_frames);
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>> //calculate dts from pts
>>>>>>>>> if(pkt->pts != AV_NOPTS_VALUE && pkt->dts == AV_NOPTS_VALUE){
>>>>>>>>>     st->pts_buffer[0]= pkt->pts;
>>>>>>>>>     for(i=1; i<delay+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
>>>>>>>>>         st->pts_buffer[i]= (i-delay-1) * pkt->duration;
>>>>>>>>>     for(i=0; i<delay && st->pts_buffer[i] > st->pts_buffer[i+1]; i++)
>>>>>>>>>         FFSWAP(int64_t, st->pts_buffer[i], st->pts_buffer[i+1]);
>>>>>>>>>      pkt->dts= st->pts_buffer[0];
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> Patch attached.
>>>>>>>>>
>>>>>>>>> Index: libavformat/avformat.h
>>>>>>>>> ===================================================================
>>>>>>>>> --- libavformat/avformat.h	(revision 14513)
>>>>>>>>> +++ libavformat/avformat.h	(working copy)
>>>>>>>>> @@ -385,7 +385,7 @@
>>>>>>>>>  
>>>>>>>>>      int64_t nb_frames;                 ///< number of frames in this stream if known or 0
>>>>>>>>>  
>>>>>>>>> -#define MAX_REORDER_DELAY 4
>>>>>>>>> +#define MAX_REORDER_DELAY 16
>>>>>>>>>      int64_t pts_buffer[MAX_REORDER_DELAY+1];
>>>>>>>>>  
>>>>>>>>>      char *filename; /**< source filename of the stream */
>>>>>>>> This breaks ABI, so it needs a version bump.  Others can have an
>>>>>>>> opinion on how bad that is.
>>>>>>>>
>>>>>>> Right, we can avoid going too far in the mean time to avoid breaking
>>>>>>> ABI, it is not the right solution I believe but it should fix the bug,
>>>>>>> patch attached.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ------------------------------------------------------------------------
>>>>>>>
>>>>>>> Index: libavformat/utils.c
>>>>>>> ===================================================================
>>>>>>> --- libavformat/utils.c	(revision 14513)
>>>>>>> +++ libavformat/utils.c	(working copy)
>>>>>>> @@ -2472,7 +2472,7 @@
>>>>>>>      //calculate dts from pts
>>>>>>>      if(pkt->pts != AV_NOPTS_VALUE && pkt->dts == AV_NOPTS_VALUE){
>>>>>>>          st->pts_buffer[0]= pkt->pts;
>>>>>>> -        for(i=1; i<delay+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
>>>>>>> +        for(i=1; i<delay+1 && i < MAX_REORDER_DELAY+1 && st->pts_buffer[i] == AV_NOPTS_VALUE; i++)
>>>>>>>              st->pts_buffer[i]= (i-delay-1) * pkt->duration;
>>>>>>>          for(i=0; i<delay && st->pts_buffer[i] > st->pts_buffer[i+1]; i++)
>>>>>>>              FFSWAP(int64_t, st->pts_buffer[i], st->pts_buffer[i+1]);
>>>>>>>
>>>>>> Any comments ? This fixes a bug :>
>>>>> too fe hours each day ... :)
>>>>>
>>>>> i think there ae 2 issues here.
>>>>> first is that MAX_REORDER_DELAY is too small, that can be fixed adding a
>>>>> new pts_buffer at the end of AVStream that is bigger and putting the old
>>>>> one under #if VERSION<...
>>>>>
>>>> Thanks for review, patch attached.
>>> ok, but i would name the old pts_buffer2 and the new pts_buffer, this should
>>> make the diff smaller
>>>
>> Oh yeah, good idea, and both hunk can be applied separately.
> 
> patch to avformat.h is ok
> 
> for the other i think that 
> -if(pkt->pts != AV_NOPTS_VALUE){
> +if(pkt->pts != AV_NOPTS_VALUE && delay <= MAX_REORDER_DELAY){
> 
> would be better
> because the code will result in random timestamps otherwise.
> The reorder buffer would be smaller than what the codec needs.
> 

Ok, I was just thinking that some delay could be set would actually be
less according to pts (which can be the case now when decoder sets delay
to 16, right ?), though 16 is rather large, dunno about SVC though.

Patch updated.

-- 
Baptiste COUDURIER                              GnuPG Key Id: 0x5C1ABAAA
Smartjog USA Inc.                                http://www.smartjog.com
Key fingerprint                 8D77134D20CC9220201FC5DB0AC9325C5C1ABAAA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pts_buffer_increase3.patch
Type: text/x-diff
Size: 888 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20080811/e317aa7f/attachment.patch>