[asterisk-dev] [Code Review] rtp timestamp to timeval calculation fix

[Kevin Fleming]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/468/#review1382
-----------------------------------------------------------

Ship it!


This looks like exactly the right fix. Nice work.

- Kevin


On 2010-01-20 14:34:43, David Vossel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/468/
> -----------------------------------------------------------
> 
> (Updated 2010-01-20 14:34:43)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> ------Problem
> The rtp timestamp field, from best I understand, starts at a marked value and increments by the number of samples contained in each packet.  When we receive a rtp timestamp we attempt to convert that value into a timeval to keep up with the actual time the timestamp represents.  This value is very important because it ends up representing the ast_frame's delivery timeval.  The delivery timeval is used during ast_translate to predict time the next frame should arrive.  If rtp incorrectly calculates the frame's delivery timeval, ast_translate will attempt to compensate for the new delivery time by adding the difference between what it expects and what it received to the translated frame's delivery time.
> 
> So, in ast_translate if the incoming frame's delivery time is 20ms above our expected next incoming time, the next outgoing delivery time has 20ms added to it.
> 
> Right now the conversion between the rtp timestamp to timeval appears to only be accurate for 8000khz audio.  The problem is primarily in how we calculate the tv_usec portion of the timeval.
> 
> -----Current method of timestamp to timeval calculation
> Ideally the sec and usec result from all of these calculations should be the same since they represent the same length of audio.
> 
>     --8000 kHz Example
>         timestamp = 1422080 (8888 packets, 160 samples, 20ms ulaw)
>         rate = 8000;
> 
>         sec = timestamp / rate
>         usec = (timestamp % rate) * 125
>         result is sec: 177 usec: 760000
> 
>     --16000 kHz example
>         timestamp = 2844160 (8888 packets, 320 samples, 20ms siren7)
>         rate = 16000;
> 
>         sec = timestamp / rate
>         usec = (timestamp % rate) * 125
>         result is sec: 177 usec: 1520000
> 
>     --32000 kHz example
>         timestamp = 5688320 (8888 packets, 640 samples, 20ms siren14)
>         rate = 16000;
> 
>         sec = timestamp / rate
>         usec = (timestamp % rate) * 125
>         result is sec: 177 usec: 3040000
> 
> The usec value is incorrect for both the 16000kHz and 32000kHz calculation.  This results in 16000kHz delivery looking like it contains twice as much audio than it does, and 32000kHz containing 4 times as much.  In fact, the usec values for 16000kHz and 32000kHz are even above the max usec value allowed.  We attempt to account for this error by using the sanitize_tv function.
> 
> -----New method of timestamp to timeval calculation
> 
>     -- Method 1: modifying current method.
>         sec = timestamp / rate
>         usec = (((timestamp % rate) / (rate / 8000))) * 125
> 
>     -- Method 2: use time.h api.
>         timeval = ast_samp2tv(timestamp, rate);
> 
> 
> Method 2 is the one this patch currently implements.  Unless the rtp timestamp does not always reflect the number of samples incremented, I don't see any reason not to use this method.  Both methods have been tested.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_rtp_asterisk.c 241620 
> 
> Diff: https://reviewboard.asterisk.org/r/468/diff
> 
> 
> Testing
> -------
> 
> siren7 to siren14 translation now works.  siren7 to ulaw works, siren14 to ulaw works.
> 
> 
> Thanks,
> 
> David
> 
>