[asterisk-dev] Timestamps in RTP bridged calls

[Matthew Jordan]

On Wed, Jul 2, 2014 at 4:58 AM, Olle E. Johansson <oej at edvina.net> wrote:
> Related issue:
>
> https://issues.asterisk.org/jira/browse/ASTERISK-23142
>
>
> In the big jitterbuffer patch in 2006 ther was code that sets a flag on a AST_FRAME
> that it contains time stamp information. This is set on all incoming RTP audio frames.
>
> When sending RTP we reset the timestamp to the one in the frame if this flag is set.
>
> Now, if we have a call on hold this is dangerous.
>
> Alice calls Bob and he answers.
> -> we take the incoming TS and send out to Bob in the RTP stream
>
> Alice puts Bob on hold
>  -> we activate MOH and raise the TS with 160 for every RTP packet
>
> Alice puts Bob off hold
>  -> We get RTP from Alice with a new time stamp and reset ours
>
> This can lead to a big jump in time stamps and in our case lead to loss of audio.

If I'm following the scenario, this could be two different problems:
(1) A jump in the timestamp from Alice causes a jitterbuffer on either
Alice or Bob (depending on how it was put on a channel) to view the
audio stream as being skewed. The jitterbuffer should to be reset when
this occurs (and if it isn't, that's bad)
(2) The outbound stream faithfully replicating the timestamp from the
inbound side causes the jump in timestamps to be reflected across the
bridge, confusing Bob's UA.

Problem #1 shouldn't happen so long as the inbound channel driver
raises a HOLD, SRCUPDATE, or SRCCHANGE control frame. This should
cause the abstract jitterbuffer to resync itself to the new stream
(with its new timestamps). (There may be some issues with this in 11
with func_jitterbuffer, but getting a pcap illustrating this has been
problematic for the issues filed for it so far. I think we fixed it in
12 when things with the jitterbuffer got consolidated between the
framehook provided by the function and the jitterbuffer hooks in
bridging).

Looking at what you highlighted to be the offending code:

    if (ast_test_flag(frame, AST_FRFLAG_HAS_TIMING_INFO)) {
        rtp->lastts = frame->ts * rate;
    }

This code is old... as in, it goes back at least to r31159, which
means the reason for its inclusion is going to be hard to determine.
It may be that we thought at the time that reproducing the timestamps
from the inbound stream was a 'good thing' if it was available... I
don't know.

> I don't see any reason to change the TS like this in the outbound stream. It's an
> unrelated stream and we set the TS on different grounds.

I would think we would either (a) pass it through when in a p2p
bridge, or (b) generate it completely separately when we are in a core
bridge.

> We could shange the SSRC when this happens, but I already have systems complaining over the way Asterisk change SSRC every time we detect a problem, so I don't think it's a good idea (TM).

Agreed.

> I can understand why the jitter buffer for an incoming RTP stream needs to know if a
> stream has timing info, but I don't see a reason for us using the same timing on the
> outbound stream.

Depends on the what we mean by outbound stream :-)

In 1.8/11, the jitterbuffer provided by channel drivers and managed by
the bridging performed in ast_generic_bridge is put on the write side
of a channel - which could be construed as a part of the outbound
media stream. That means we don't de-jitter jittered audio coming off
of a channel, we de-jitter it on the channel that it is writing to in
a two party bridge (which is odd and strange, but that is what
happens). So as long as we mean 'outbound stream' to be the frame at
the point in time it is handed off to the RTP implementation, then
we're good; at that point, we're out of any jitter buffer. Up until
the frame is written to the RTP implementation, it needs timing
information.

Now, the jitterbuffer put on a channel by func_jitterbuffer is a
framehook, and is put on the read side of a channel (which makes a lot
more sense, since the media coming off of a channel is generally where
think of jitter coming from).

>
> Does anyone see any harm in deleting the piece of code that resets the timestamp,
> overriding the calculations for a new time stamp already done in the RTP write code?
>

I don't think so... what happened when you removed it? :-)


-- 
Matthew Jordan
Digium, Inc. | Engineering Manager
445 Jan Davis Drive NW - Huntsville, AL 35806 - USA
Check us out at: http://digium.com & http://asterisk.org