[Home]

Summary:ASTERISK-04624: [patch] ast_tvdiff_ms is buggy due to different truncation effect of gcc for negative and positive numbers
Reporter:Steve Davies . (stevedavies)Labels:
Date Opened:2005-07-19 16:05:57Date Closed:2008-01-15 15:41:56.000-0600
Priority:MinorRegression?No
Status:Closed/CompleteComponents:Core/General
Versions:Frequency of
Occurrence
Related
Issues:
Environment:Attachments:( 0) 20050719__tvdiff.diff.txt
Description:Last year, when I was working on fixing up the old iax2 jitter buffer, I sent Mark patches for chan_iax2, chan_sip and others that fixed an issue with the way timevals were subtracted.

Before, this was often seen in the code:

 ms = (tva->tv_sec - tvb->tv_sec) * 1000 +
  (tva->tv_usec - tvb->tv_usec) / 1000;

But this code isn't quite right, it introduces a 1ms "jitter" into the calculated ms.  This is because of the fact that gcc rounds integer divisions towards 0.

(See, for example http://gcc.activeventure.org/Integers-implementation.html)

This rounding approach introduces a "lopsidedness" into this expression depending on whether the tva usec is bigger or smaller than the tvb.


Consider these two examples:
 tva = 2.500001, tvb = 2.000000  -> precise diff is 500.001ms, calc comes out as 500ms
 tva = 2.500000, tvb = 1.999999  -> precise diff is 500.001ms, calc comes out as 501ms

(Work example 2 in detail:
 (tva.tvsec - tvb.tvsec) *1000 = 1000
 (tva.tvusec - tvb.tvusec) = -499999
 -499999 / 1000 = -499 by the truncation to zero rule
 1000 - 499 = 501)

You might say "so what" - there has to be some sort of rounding or truncation as we got from usec to ms.  But the problem is the lopsidedeness.  Two timestamps can be changing perfectly in step with example the same difference between them, but the calculated difference in ms will "glitch" back and forth by 1ms as the "phase" of the usec parts of the two timestamps changes.

I saw this effect in chan_iax2, and it took many hours of digging to eventually figure out where it was coming from.

That's why this code pattern in chan_iax2.c and elsewhere was replaced by:

ms = (tva->tv_sec - tvb->tv_sec) * 1000 +
  (1000000 + tva->tv_usec - tvb->tv_usec) / 1000 - 1000;

This ensures that the usec substraction always has a positive result before the / 1000 and removes the inconsistency.  Now our two examples both come out as 500msec.

Now the code in ast_tvdiff_ms doesn't handle this right:

 int ast_tvdiff_ms(struct timeval end, struct timeval start),
 {
       return ((end.tv_sec - start.tv_sec) * 1000) + ((end.tv_usec - start.tv_usec) / 1000);
 }

So ast_tvdiff_ms should be changed to follow the style that was used in chan_iax2.c

Regards,
Steve Davies
Comments:By: Kevin P. Fleming (kpfleming) 2005-07-19 19:21:37

Committed to CVS HEAD with minor mods (added back the extra parens so readers can easily see how the expressions break down, and a comment including your reasoning why the code needs to be this way). Thanks!
By: Digium Subversion (svnbot) 2008-01-15 15:41:56.000-0600

Repository: asterisk
Revision: 6166

U   trunk/include/asterisk/time.h

------------------------------------------------------------------------
r6166 | kpfleming | 2008-01-15 15:41:56 -0600 (Tue, 15 Jan 2008) | 2 lines

restore proper difference calculation (bug ASTERISK-4624)

------------------------------------------------------------------------

http://svn.digium.com/view/asterisk?view=rev&revision=6166