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

[Stephen Dredge i]

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 or where pts_wrap_bits = 64.

The code could be cleaned up in a quite few places and potential bugs 
eliminated if there was a set of mod math functions to use,
instead of adding code to catch wraps at every point where timestamp 
math or comparisons are made.

I'm open to suggestions.
> [...]
>
>    
>
>
> _______________________________________________
> 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