[asterisk-dev] [Code Review] Fix transcode_via_sln and increase utility of PLC

Tue May 18 16:19:46 CDT 2010

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

The one thing I don't see reflected in this patch is displaying the currently configured value in the output of a CLI command.  I was thinking "core show settings" would be appropriate, at least for trunk, unless you can think of something more appropriate.

/trunk/include/asterisk/channel.h
<https://reviewboard.asterisk.org/r/622/#comment4305>

    I think there is a _private.h where functions like this live.  They're prototypes that need to be in a header, but are not intended to be part of the public API.

- Russell

On 2010-05-18 11:08:01, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/622/
> -----------------------------------------------------------
> 
> (Updated 2010-05-18 11:08:01)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> The problem here is a bit complex, so try to bear with me...
> 
> It was noticed by a Digium customer that generic PLC (as configured in
> codecs.conf) did not appear to actually be having any sort of benefit when
> packet loss was introduced on an RTP stream. I reproduced this issue myself
> by streaming a file across an RTP stream and dropping approx. 5% of the
> RTP packets. I saw no real difference between when PLC was enabled or disabled
> when using wireshark to analyze the RTP streams.
> 
> After analyzing what was going on, it became clear that one of the problems
> faced was that when running my tests, the translation paths were being set
> up in such a way that PLC could not possibly work as expected. To illustrate,
> if packets are lost on channel A's read stream, then we expect that PLC will
> be applied to channel B's write stream. The problem is that generic PLC can
> only be done when there is a translation path that moves from some codec to
> SLINEAR. When I would run my tests, I found that every single time, read
> and write translation paths would be set up on channel A instead of channel
> B. There appeared to be no real way to predict which channel the translation
> paths would be set up on.
> 
> This is where Kevin swooped in to let me know about the transcode_via_sln
> option in asterisk.conf. It is supposed to work by placing a read translation
> path on both channels from the channel's rawreadformat to SLINEAR. It also
> will place a write translation path on both channels from SLINEAR to the
> channel's rawwriteformat. Using this option allows one to predictably set up
> translation paths on all channels. There are two problems with this, though.
> First and foremost, the transcode_via_sln option did not appear to be working
> properly when I was placing a SIP call between two endpoints which did not
> share any common formats. Second, even if this option were to work, for PLC
> to be applied, there had to be a write translation path that would go from
> some format to SLINEAR. It would not work properly if the starting format
> of translation was SLINEAR.
> 
> The one-line change presented in this review request in chan_sip.c fixed the
> first issue for me. The problem was that in sip_request_call, the
> jointcapability of the outbound channel was being set to the format passed to
> sip_request_call. This is nativeformats of the inbound channel. Because of this,
> when ast_channel_make_compatible was called by app_dial, both channels already
> had compatibly read and write formats. Thus, no translation path was set up at
> the time. My change is to set the jointcapability of the sip_pvt created during
> sip_request_call to the intersection of the inbound channel's nativeformats and
> the configured peer capability that we determined during the earlier call to
> create_addr. Doing this got the translation paths set up as expected when using
> transcode_via_sln.
> 
> The changes presented in channel.c fixed the second issue for me. First and
> foremost, when Asterisk is started, we'll read codecs.conf to see the value of
> the genericplc option. If this option is set, and ast_write is called for a
> frame with no data, then we will attempt to fill in the missing samples for
> the frame. The implementation uses a channel datastore for maintaining the
> PLC state and for creating a buffer to store PLC samples in. Even when we
> receive a frame with data, we'll call plc_rx so that the PLC state will have
> knowledge of the previous voice frame, which it can use as a basis for when
> it comes time to actually do a PLC fill-in.
> 
> So, reviewers, now I ask for your help. First off, there's the one line change
> in chan_sip that I have put in. Is it right? By my logic it seems correct, but
> I'm sure someone can tell me why it is not going to work. This is probably the
> change I'm least concerned about, though. What concerns me much more is the
> set of changes in channel.c. First off, am I even doing it right? When I run
> tests, I can clearly see that when PLC is activated, I see a significant increase
> in RTP traffic where I would expect it to be. However, in my humble opinion, the
> audio sounds kind of crappy whenever the PLC fill-in is done. It sounds worse to
> me than when no PLC is used at all. I need someone to review the logic I have used
> to be sure that I'm not misusing anything. As far as I can see my pointer arithmetic
> is correct, and my use of AST_FRIENDLY_OFFSET should be correct as well, but I'm
> sure someone can point out somewhere where I've done something incorrectly.
> 
> As I was writing this review request up, I decided to give the code a test run under
> valgrind, and I find that for some reason, calls to plc_rx are causing some invalid
> reads. Apparently I'm reading past the end of a buffer somehow. I'll have to dig around
> a bit to see why that is the case. If it's obvious to someone reviewing, speak up!
> 
> Finally, I have one other proposal that is not reflected in my code review. Since
> without transcode_via_sln set, one cannot predict or control where a translation
> path will be up, it seems to me that the current practice of using PLC only when
> transcoding to SLINEAR is not useful. I recommend that once it has been determined
> that the method used in this code review is correct and works as expected, then
> the code in translate.c that invokes PLC should be removed.
> 
> 
> Diffs
> -----
> 
>   /trunk/channels/chan_sip.c 263766 
>   /trunk/include/asterisk/channel.h 263766 
>   /trunk/main/channel.c 263766 
>   /trunk/main/loader.c 263766 
> 
> Diff: https://reviewboard.asterisk.org/r/622/diff
> 
> 
> Testing
> -------
> 
> I ran a test where I would place a call from my Polycom phone through one Asterisk box
> to a second one. The second box would play back the demo-congrats file. I modified the
> RTP code in the second box to drop approximately 5 percent of the outgoing packets. The
> audio between the two Asterisk boxes was GSM. The audio between the Polycom phone and
> Asterisk was ulaw.
> 
> I used wireshark on the first Asterisk box to analyze the RTP streams. I noticed an increase
> from an average of 2650 packets on the path from the first Asterisk box to the Polycom to
> an average of 2786 packets when PLC was enabled.
> 
> 
> Thanks,
> 
> Mark
> 
>