On 03 Jul 2014, at 19:45, Matthew Jordan <mjor...@digium.com> wrote:
> On Wed, Jul 2, 2014 at 4:58 AM, Olle E. Johansson <o...@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) You should never run an Asterisk jitterbuffer in a bridged call. But it is of course possible. > (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. This is when writing, after jitter buffers and all I guess. Just before writing to the network. > > 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? :-) > Everything started working, the gateway that had problems started happily relaying auio. As stated in a separate e-mail, we've run a lot of different tests in our testbed and have no issues. We have *not* tested stuff like reframing from 20 to 30 ms which seems to use the same flag. /O -- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev