[FFmpeg-devel] [PATCH] Fix failure in av_read_frame on timestamp rollover

[Stephen Dredge i]

On 06/10/2010 07:11 PM, Michael Niedermayer wrote:
> On Thu, Jun 10, 2010 at 12:19:19PM +1000, Stephen Dredge i wrote:
>    
>> On 06/10/2010 03:25 AM, Michael Niedermayer wrote:
>>      
>>> On Wed, Jun 09, 2010 at 03:02:52PM +1000, Stephen Dredge i wrote:
>>>
>>>        
>>>> av_read_frame can fail on some (TS) files at timestamp rollover, reading
>>>> to
>>>> eof before returning a frame.
>>>>
>>>> The attached patch adds some utility functions for rollover aware
>>>> timestamp
>>>> comparison  and uses them in av_read_frame to avoid problems at timestamp
>>>> rollover.
>>>>
>>>> The utility functions provide normal comparisons if pts_wrap_bits is 0 or
>>>>
>>>>          
>>>>> = 63 so behaviour on formats without timestamp wrapping should be
>>>>>
>>>>>            
>>>> unaffected.
>>>>
>>>> -- 
>>>>     Stephen Dredge                        sdredge at tpg.com.au
>>>> _______________________________________________________________
>>>>    System Administrator
>>>>    +61 2 9850 0979
>>>>
>>>>     TPG Internet
>>>>    www.tpg.com.au
>>>>
>>>>
>>>>
>>>>
>>>>          
>>>
>>>        
>>>>    libavformat/utils.c     |    9 ++++---
>>>>    libavutil/mathematics.c |   59
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    libavutil/mathematics.h |   25 ++++++++++++++++++++
>>>>    3 files changed, 89 insertions(+), 4 deletions(-)
>>>> a5af48c8203ac3261e2c383073a131c64ca0d294  rollover_fix.patch
>>>> diff --git a/libavformat/utils.c b/libavformat/utils.c
>>>> index 6365f3e..02ffce1 100644
>>>> --- a/libavformat/utils.c
>>>> +++ b/libavformat/utils.c
>>>> @@ -1152,18 +1152,19 @@ int av_read_frame(AVFormatContext *s, AVPacket
>>>> *pkt)
>>>>        AVPacketList *pktl;
>>>>        int eof=0;
>>>>        const int genpts= s->flags&   AVFMT_FLAG_GENPTS;
>>>> +    int wrap_bits;	
>>>>
>>>>        for(;;){
>>>>            pktl = s->packet_buffer;
>>>>            if (pktl) {
>>>>                AVPacket *next_pkt=&pktl->pkt;
>>>> -
>>>> +            wrap_bits =
>>>> s->streams[next_pkt->stream_index]->pts_wrap_bits;		
>>>>                if(genpts&&   next_pkt->dts != AV_NOPTS_VALUE){
>>>>                    while(pktl&&   next_pkt->pts == AV_NOPTS_VALUE){
>>>>                        if(   pktl->pkt.stream_index ==
>>>> next_pkt->stream_index
>>>> -&&   next_pkt->dts<   pktl->pkt.dts
>>>>
>>>>          
>>>
>>>        
>>>> -&&   pktl->pkt.pts != pktl->pkt.dts //not b frame
>>>>
>>>>          
>>> a != check should
>>>
>>>        
>> dts can be negative and pts positive for the same time (compute_pkt_fields
>> can emit negative dts), so a mod comparison is required.
>>      
>>> [...]
>>>
>>>        
>>>> +int64_t av_mod_wrap_bits(int64_t x, int wrap_bits){
>>>> +	int64_t result;
>>>> +	int64_t modulo;
>>>> +	if(!wrap_bits || wrap_bits>= 63) {
>>>> +		return x;
>>>> +	}
>>>> +	modulo = 1LL<<   wrap_bits;
>>>> +	result = x % modulo;
>>>> +	if(result<   0) {
>>>> +		result += modulo;
>>>> +	}
>>>> +	return result;
>>>> +}
>>>> +
>>>> +int av_gt_mod_wrap_bits(int64_t x, int64_t y, int wrap_bits){
>>>> +	int64_t modulo;
>>>> +	int64_t result;
>>>> +	if(!wrap_bits || wrap_bits>= 63) {
>>>> +		return x>y?1:0;
>>>> +	}	
>>>> +	modulo = 1LL<<   wrap_bits;
>>>> +	result = av_mod_wrap_bits(x - y, wrap_bits);
>>>> +	if(result<   modulo/2) {
>>>> +		return 1;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>>
>>>>          
>>> this is a mess, ive added a much simpler av_compare_mod() that could be
>>> used
>>>
>>>        
>> Agreed it's clunky. I was going for clarity and simplicity of use over
>> elegance.
>>
>> av_compare_mod is not much help here. It can't be used for negative numbers
>>      
> a&(m-1) makes an argument positiv
>    
(Looking Closer True it does, and the function does work correctly for 
-ve input, the arg type should probably reflect this.

> but maybe the code messing with dts could be changed too, i dont know
>    
It's obviously a hack which could be eliminated from avformats point of 
view but it's probable there are apps which have come to rely on this
behaviour.
>    
>> or where pts_wrap_bits = 64.
>>      
> i dont see that problem
>    
Shifts greater than the width of the type are undefined and compiler 
dependant AFAIK. Certainly the version of gcc I'm using doesn't produce 
the expected output.

So you couldn't do something like av_compare_mod(a, b, 1 << wrap_bits) 
without testing wrap_bits first -painful.

New patch attached. Slight changes to av_compare_mod, use av_compare_mod 
for mod comparison in utils.c






> [...]
>    
>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at mplayerhq.hu
> https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel


-- 
   Stephen Dredge                        sdredge at tpg.com.au
_______________________________________________________________
  System Administrator
  +61 2 9850 0979

   TPG Internet
  www.tpg.com.au



-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: rollover_fix2.patch
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100611/3d72d5de/attachment.txt>