Re: [Opalvoip-devel] Bug in rtp.cxx

[Southanon R. <sra...@la...>]

Thanks Robert. You are exactly right. We are using a 64 bit compiler. 
Thank you very much for the update.

I had an additional question that I could not figure out while I was in 
this part of the code. Would you mind elaborating on the use of 
JitterRoundingGuardBits (4) and GetJitterTimeUnits (8)? I see that you 
are multiplying the difference in time by GetJitterTimeUnits. This seems 
to be the only deviation from the definition per RFC3550 Appendix 8. I 
did notice that you are later dividing this out when calling 
GetAvgJitterTime. However, I am unclear on the use of the 
JitterRoundingGuardBits. I notice you are shifting the jitterLevel in 
both the Average and the Max calculations but not the Remote 
calculation. Why?

     DWORD GetAvgJitterTime() const { return 
(jitterLevel>>JitterRoundingGuardBits)/GetJitterTimeUnits(); }
     DWORD GetMaxJitterTime() const { return 
(maximumJitterLevel>>JitterRoundingGuardBits)/GetJitterTimeUnits(); }
     DWORD GetJitterTimeOnRemote() const { return 
jitterLevelOnRemote/GetJitterTimeUnits(); }

Thank you in advance for your time.

Best Regards,
Southanon

On 2/15/13 5:08 PM, Robert Jongbloed wrote:
> As "variance" is a signed value the "unsigned wraparound" problem 
> should not occur. However, I can see in the log what you are saying.
>
> The only thing I can think of is you are using a 64 bit compiler and 
> "variance" is a 64 bit signed integer. Then, if the compiler does not 
> do the sign extension for the unsigned expression (diff - 
> lastTransitTime) then I can see how it could occur.
>
> I have changed the code to guarantee a positive number.
>
> *Robert Jongbloed*
> /OPAL/OpenH323/PTLib Architect and Co-founder./
> Commercial support at http://www.voxlucida.com.au
>
> On 16/02/2013 6:09 AM, Southanon Ratsamy wrote:
>> There seems to be a bug in rtp.cxx that causes the jitterLevel to 
>> jump up in some cases. The bug is seen when the difference in time 
>> goes from large to small to large again. This seems to be present in 
>> the opal 3.10.9 release.
>>
>> This is the line of the bug in rtp.cxx, function is 
>> Internal_OnReceiveData.
>>
>>         // As per RFC3550 Appendix 8
>>         diff *= GetJitterTimeUnits(); // Convert to timestamp units
>>         long variance = diff - lastTransitTime;
>>
>> Both "diff" and "lastTransitTime" are unsigned. If lastTransitTime is 
>> bigger than diff, this produces a very large unsigned number. Not the 
>> small negative number we are expecting.
>>
>> Here is sample output demonstrating what is happening:
>>
>>   0:11.293        RTP Jitter:0xe3494910 rtp.cxx(1346)  RTP     
>> Session 1, ssrc=3735373445, jitterLevel = 21
>>   0:11.318        RTP Jitter:0xe3494910 rtp.cxx(1305)  RTP     
>> Session 1, ssrc=3735373445, tick = 170:25:21.136, 
>> lastReceivedPacketTime = 170:25:21.110, diff = 26
>>   0:11.318        RTP Jitter:0xe3494910 rtp.cxx(1324)  RTP     
>> Session 1, ssrc=3735373445, diff = 208, lastTransitTime = 152, 
>> variance = 56, variance2 = 56
>>
>>   0:11.318        RTP Jitter:0xe3494910 rtp.cxx(1346)  RTP     
>> Session 1, ssrc=3735373445, jitterLevel = 76
>>   0:11.333        RTP Jitter:0xe3494910 rtp.cxx(1305)  RTP     
>> Session 1, ssrc=3735373445, tick = 170:25:21.151, 
>> lastReceivedPacketTime = 170:25:21.136, diff = 15
>>   0:11.333        RTP Jitter:0xe3494910 rtp.cxx(1324)  RTP     
>> Session 1, ssrc=3735373445, diff = 120, lastTransitTime = 208, 
>> variance = 4294967208, variance2 = -88
>>
>>   0:11.333        RTP Jitter:0xe3494910 rtp.cxx(1346)  RTP     
>> Session 1, ssrc=3735373445, jitterLevel = 4294967279
>>   0:11.353        RTP Jitter:0xe3494910 rtp.cxx(1305)  RTP     
>> Session 1, ssrc=3735373445, tick = 170:25:21.170, 
>> lastReceivedPacketTime = 170:25:21.151, diff = 19
>>   0:11.353        RTP Jitter:0xe3494910 rtp.cxx(1324)  RTP     
>> Session 1, ssrc=3735373445, diff = 152, lastTransitTime = 120, 
>> variance = 32, variance2 = 32
>>
>> Notice the jump in the variance value when diff is smaller than 
>> lastTransitTime. variance2 is the same calculation with 
>> lastTransitTime type casted to a long. Since this variance is part of 
>> the jitterLevel calculation, this subsequently causes the jitterLevel 
>> to jump in value dramatically.
>>
>> Best Regards,
>> Southanon
>>
>>
>> ------------------------------------------------------------------------------
>> The Go Parallel Website, sponsored by Intel - in partnership with Geeknet,
>> is your hub for all things parallel software development, from weekly thought
>> leadership blogs to news, videos, case studies, tutorials, tech docs,
>> whitepapers, evaluation guides, and opinion stories. Check out the most
>> recent posts - join the conversation now.http://goparallel.sourceforge.net/
>>
>>
>> _______________________________________________
>> Opalvoip-devel mailing list
>> Opa...@li...
>> https://lists.sourceforge.net/lists/listinfo/opalvoip-devel
>