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

Fri Apr 23 13:56:52 CDT 2010

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

I am in total agreeance that the generic PLC logic in translate.c should completely go away.  It doesn't do what we'd expect it to do and is just an ugly place for it to live in my opinion.  As far as your architecture goes, this feels right to me and I don't see anything wrong with it.  My only concerns are with the use of the plc functions.

/trunk/main/channel.c
<https://reviewboard.asterisk.org/r/622/#comment4112>

    plc_fillin returns the number of samples synthesized, this should probably be used to determine the frame's samples and datalen.  I just don't know enough about this function to know if we are guaranteed the fillin or not.

    From what I gathered plc_rx should always be passed the frame's audio data regardless if plc_fillin was used or not.  I know there are some issues with this in your testing (terrible audio quality), but from the vague documentation I read it appeared to me that this is supposed to be what smoothed out the fillin frames... It probably needs to have that previous rx history to mix in the interpolated fillin data, which we've already discussed and I know you know.  But if this can't be achieved I really question why we would ever even want to use this type of PLC to begin with.

- David

On 2010-04-22 16:37:18, Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/622/
> -----------------------------------------------------------
> 
> (Updated 2010-04-22 16:37:18)
> 
> 
> 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 257809 
>   /trunk/main/channel.c 257809 
> 
> 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
> 
>