ABANDONED: Remove the default receive channel in WVoE.

This CL is being replaced with:
https://codereview.webrtc.org/1386653002/
https://codereview.webrtc.org/1388723002/
https://codereview.webrtc.org/1388733002/
https://codereview.webrtc.org/1385893002/

BUG=webrtc:4690

[the sun, 2015-09-24 11:55:16]

solenberg@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[the sun, 2015-09-24 11:55:17]

Tests still need fixing, but I'd like to hear if you have any initial thoughts
on this?

Highlights:
1. No special handling for the "default" receive channel. Makes the code a lot
simpler.
2. If there are no receive streams configured programmatically, we'll accept
traffic from any SSRC and set up a receive stream for it. That should be the
same as before.
3. GetStats() won't return anything unless there is an active receive stream.
4. WebRtcVoiceMediaChannel::SetOptions() will configure receive per-channel
receive side processing for all streams, not just the default one.

[the sun, 2015-09-30 15:27:00]

Fixed remaining unit tests; PTAL.

[pthatcher1, 2015-10-02 02:33:31]

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1495: if
(engine()->voe()->processing()->SetRxAgcStatus(
Are sure we want to set this for all receive channels?

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:152: bool ValidateStreamParams(const
StreamParams& sp) {
static?

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1516: bool
WebRtcVoiceMediaChannel::SetRecvOptions(int channel_id) {
I think this method would be cleaner if you passed in the options with the id,
and then the calls sites passed in options_.

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1603: }
Can you move this into a separate static helper method?

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2319: // Use the same SSRC as our default
(send) channel, so the RTCP reports are
"default (send) channel" or "default send channel"?

At least be consistent between here and below.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1472:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
This is called in the following methods:

WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel
WebRtcVoiceMediaChannel::SetSendParameters
WebRtcVoiceMediaChannel::SetAudioSend

Can you put thread checks in those as well?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1496: SetRecvOptions(voe_channel());
Why do we have to call SetRecvOptions with the default channel which is not only
used for sending?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1518: bool
WebRtcVoiceMediaChannel::SetRecvOptions(int channel_id) {
Can you please pass option into here as well?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1600: // Verify no codecs have the same
payload type.
Can you put this in its own helper function?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1834:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
This is called by 

WebRtcVoiceMediaChannel::SetSendParameters
WebRtcVoiceMediaChannel::AddSendStream

Can you also add checks for those?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1906:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
This is called by WebRtcVoiceMediaChannel::SetRecvParameters

Can you also add a thread check there?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2275: RemoveRecvStream(ssrc);
Why remove it and re-add it?  Why not just keep using it as-is?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2341: // Turn off all supported codecs.
I like this in a helper method.  But instead "ResetRecvCodecs(channel)", it
should perhaps go along with the setting of the payload types in a
SetRecvCodecs(recv_channel_id, recv_codecs).

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2359: voe_codec.pltype = codec.id;
Why did you remove .rate = 0?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2506: // Collect channels to scale output
volume for (ssrc == 0 means all channels).
This "ssrc == 0 means all" was just a GTP thing.  It's no longer used by anyone
like that, so just remove it.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2617: default_recv_channel_id_ =
receive_channels_[ssrc]->channel();
Why not just call GetRecvChannelNum?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2669: int channel =
GetReceiveChannelNum(ssrc);
Should this be recv_channel_id to be consistent?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2950:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
This is called by WebRtcVoiceEngine::CallbackOnError, so
WebRtcVoiceEngine::CallbackOnError will blow up if it's called on a different
thread.  Are we sure it will only be called on the worker thread?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2999: int
WebRtcVoiceMediaChannel::GetReceiveChannelNum(uint32 ssrc) const {
Can you rename this GetReceiveChannelId?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3172:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
This is called by  WebRtcVoiceMediaChannel::SetRecvCodecs, so can you put a
thread check there?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3173: for (const auto& codec :
new_codecs) {
I prefer having the (non-auto) type on for loops

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:186: int voe_channel() const { return
voe_channel_; }
Can you rename this default_channel_id() or default_send_channel_id()?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:308: const int voe_channel_;
Can you rename this default_channel_id_ or default_send_channel_id_?

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:325: int default_recv_channel_id_;
Why do we store the default_recv_channel_id_ when we can just call
GetRecvChannelNum at any time go get the same value?

[the sun, 2015-10-02 11:34:20]

Thanks for taking the time to dig into this Peter. The code for sure is quite
involved.

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1495: if
(engine()->voe()->processing()->SetRxAgcStatus(
On 2015/10/02 02:33:30, pthatcher1 wrote:
> Are sure we want to set this for all receive channels?

No, but it seems really weird that we don't. I'm asking ajm@ about it.

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:152: bool ValidateStreamParams(const
StreamParams& sp) {
On 2015/10/02 02:33:30, pthatcher1 wrote:
> static?

Nope, it's in an anonymous namespace.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Namespaces

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1516: bool
WebRtcVoiceMediaChannel::SetRecvOptions(int channel_id) {
On 2015/10/02 02:33:30, pthatcher1 wrote:
> I think this method would be cleaner if you passed in the options with the id,
> and then the calls sites passed in options_.

Done.

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1603: }
On 2015/10/02 02:33:30, pthatcher1 wrote:
> Can you move this into a separate static helper method?

Done.

https://codereview.webrtc.org/1315903004/diff/240001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2319: // Use the same SSRC as our default
(send) channel, so the RTCP reports are
On 2015/10/02 02:33:30, pthatcher1 wrote:
> "default (send) channel" or "default send channel"?
> 
> At least be consistent between here and below.

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1472:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/02 02:33:31, pthatcher1 wrote:
> This is called in the following methods:
> 
> WebRtcVoiceMediaChannel::WebRtcVoiceMediaChannel
> WebRtcVoiceMediaChannel::SetSendParameters
> WebRtcVoiceMediaChannel::SetAudioSend
> 
> Can you put thread checks in those as well?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1496: SetRecvOptions(voe_channel());
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Why do we have to call SetRecvOptions with the default channel which is not
only
> used for sending?

Sorry; debugging left overs.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1518: bool
WebRtcVoiceMediaChannel::SetRecvOptions(int channel_id) {
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Can you please pass option into here as well? 

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1600: // Verify no codecs have the same
payload type.
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Can you put this in its own helper function?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1834:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/02 02:33:31, pthatcher1 wrote:
> This is called by 
> 
> WebRtcVoiceMediaChannel::SetSendParameters
> WebRtcVoiceMediaChannel::AddSendStream
> 
> Can you also add checks for those?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:1906:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/02 02:33:31, pthatcher1 wrote:
> This is called by WebRtcVoiceMediaChannel::SetRecvParameters
> 
> Can you also add a thread check there?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2275: RemoveRecvStream(ssrc);
On 2015/10/02 02:33:30, pthatcher1 wrote:
> Why remove it and re-add it?  Why not just keep using it as-is?

Because we need to recreate the AudioReceiveStream anyway, when reconfiguring.
Going forward, there won't anything left to reconfigure here.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2341: // Turn off all supported codecs.
On 2015/10/02 02:33:30, pthatcher1 wrote:
> I like this in a helper method.  But instead "ResetRecvCodecs(channel)", it
> should perhaps go along with the setting of the payload types in a
> SetRecvCodecs(recv_channel_id, recv_codecs).

Personally I have a problem with "helper" methods. To me, a function signals
reuse; that the code in it is actually used in multiple places. The caller and
reader expects a well defined interface and associated tests (in this case I'd
need to make the function part of the public WVoE interface in order to do
that). I don't create additional classes just to hide a piece of code from one
call site either.

I don't have a problem with long, messy functions, as long as they are divided
up into logical sections with clearly scope variables. When refactoring it
actually makes it easier, since I don't have to look all over the place for who
may be using a function, just to realize: "oh, just in this one place?".

Yes, this is your cue to disagree... :)

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2359: voe_codec.pltype = codec.id;
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Why did you remove .rate = 0?

It said in a comment: "// Needed to make GetRecPayloadType work for ISAC"
I'm not using GetRecPayloadType anymore.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2506: // Collect channels to scale output
volume for (ssrc == 0 means all channels).
On 2015/10/02 02:33:31, pthatcher1 wrote:
> This "ssrc == 0 means all" was just a GTP thing.  It's no longer used by
anyone
> like that, so just remove it.

But we still need to support "ssrc == 0 means default channel, if available"?
Just looking up the channel from ssrc causes
PeerConnectionInterfaceTest.ReceiveFireFoxOffer to fail.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2617: default_recv_channel_id_ =
receive_channels_[ssrc]->channel();
On 2015/10/02 02:33:30, pthatcher1 wrote:
> Why not just call GetRecvChannelNum?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2669: int channel =
GetReceiveChannelNum(ssrc);
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Should this be recv_channel_id to be consistent?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2950:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/02 02:33:31, pthatcher1 wrote:
> This is called by WebRtcVoiceEngine::CallbackOnError, so
> WebRtcVoiceEngine::CallbackOnError will blow up if it's called on a different
> thread.  Are we sure it will only be called on the worker thread?

Good point. Actually, AFAICT VoE only calls CallbackOnError() with -1 for the
channel argument, so I've removed this code instead.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2999: int
WebRtcVoiceMediaChannel::GetReceiveChannelNum(uint32 ssrc) const {
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Can you rename this GetReceiveChannelId?

Yes, also renamed GetSendChannelNum() similarly.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3172:
RTC_DCHECK(thread_checker_.CalledOnValidThread());
On 2015/10/02 02:33:30, pthatcher1 wrote:
> This is called by  WebRtcVoiceMediaChannel::SetRecvCodecs, so can you put a
> thread check there?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:3173: for (const auto& codec :
new_codecs) {
On 2015/10/02 02:33:30, pthatcher1 wrote:
> I prefer having the (non-auto) type on for loops

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
File talk/media/webrtc/webrtcvoiceengine.h (right):

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:186: int voe_channel() const { return
voe_channel_; }
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Can you rename this default_channel_id() or default_send_channel_id()?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:308: const int voe_channel_;
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Can you rename this default_channel_id_ or default_send_channel_id_?

Done.

https://codereview.webrtc.org/1315903004/diff/270001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:325: int default_recv_channel_id_;
On 2015/10/02 02:33:31, pthatcher1 wrote:
> Why do we store the default_recv_channel_id_ when we can just call
> GetRecvChannelNum at any time go get the same value?

You're right; I was using it as a flag anyway.

[the sun, 2015-10-02 12:10:38]

solenberg@webrtc.org changed reviewers:
+ andrew@webrtc.org

[the sun, 2015-10-02 12:10:39]

Andrew, can you explain why receive side AGC was only enabled for the default
receive channel?

(see line 1496 of webrtcvoiceengine.cc)

[Andrew MacDonald, 2015-10-02 17:02:36]

andrew@webrtc.org changed reviewers:
+ tomasl@google.com

[Andrew MacDonald, 2015-10-02 17:04:01]

On 2015/10/02 12:10:39, the sun wrote:
> Andrew, can you explain why receive side AGC was only enabled for the default
> receive channel?
> 
> (see line 1496 of webrtcvoiceengine.cc)

No. IIRC this was originally desired for PSTN users that wouldn't otherwise have
gone through the webrtc AGC. It seems to me you'd want to configure that for
each channel.

Tomas, do you have any idea if this is still a needed feature? I can see the
value in it, but we should be permitting configuration consistent with the
expected use case :)

[the sun, 2015-10-05 14:11:23]

solenberg@webrtc.org changed reviewers:
- andrew@webrtc.org, tomasl@google.com