Remove the default send channel in WVoE.

Remove the default send channel in WVoE.

BUG=webrtc:4690
Committed: https://crrev.com/0a617e22a46d476abcaaa081cc90300d335da9f9
Cr-Commit-Position: refs/heads/master@{#10344}

[the sun, 2015-10-01 08:40:26]

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

[pthatcher1, 2015-10-01 19:57:56]

This looks very good, except one thing I worry about:

We won't send RTCP (as a sender) until AddSendStream is called, and
AddSendStream won't be called unless "a=ssrc" is in the local description.  And
while it *should* always be in the local description, there's no guarantee that
it would be.

I think I'm OK with that, it's just a risk I think we should be aware of, and
maybe comment on in the code.

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:1309: int
WebRtcVoiceEngine::CreateVoiceChannel() {
Can we rename this CreateVoeChannel?

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:2099: int
WebRtcVoiceMediaChannel::CreateChannel() {
Again, can this be called CreateVoeChannel?

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:2130: if (-1 != GetSendChannelNum(ssrc))
{
"-1 != X" here and "X == -1" below.  Please be consistent.  And given how
everything in the rest of the file is "X != -1", I'd go with that here.

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:2151: // (with a codec requires multiple
SSRC(s)).
What does "with a codec requires multiple SSRC(s)" mean?

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:2638: }
The only thing that ever used ssrc=0 here was GTP, and we don't have to worry
about it anymore.

Plus, if we did support MuteStream(0) to mean "mute the default one", then we'd
have to worry about what happens when you call MuteStream(0) and then
SendAddStream().  But since no one calls MuteStream(0), I vote we just remove
support for it.

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine.cc:2963: int
WebRtcVoiceMediaChannel::GetSendChannelNum(uint32 ssrc) const {
Can you rename this to GetSendChannelBySsrc?

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.chromium.org/1364643003/diff/60001/talk/media/webrtc/webrt...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3079: // TODO(solenberg): !!!!!!
You're going to fix this, right?

[the sun, 2015-10-13 15:07:03]

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:1309: int
WebRtcVoiceEngine::CreateVoiceChannel() {
On 2015/10/01 19:57:56, pthatcher1 wrote:
> Can we rename this CreateVoeChannel?

Done.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2099: int
WebRtcVoiceMediaChannel::CreateChannel() {
On 2015/10/01 19:57:56, pthatcher1 wrote:
> Again, can this be called CreateVoeChannel?

Done.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2130: if (-1 != GetSendChannelNum(ssrc))
{
On 2015/10/01 19:57:56, pthatcher1 wrote:
> "-1 != X" here and "X == -1" below.  Please be consistent.  And given how
> everything in the rest of the file is "X != -1", I'd go with that here.

Done.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2151: // (with a codec requires multiple
SSRC(s)).
On 2015/10/01 19:57:56, pthatcher1 wrote:
> What does "with a codec requires multiple SSRC(s)" mean?

We don't support any audio codecs that require multiple SSRCs.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2638: }
On 2015/10/01 19:57:56, pthatcher1 wrote:
> The only thing that ever used ssrc=0 here was GTP, and we don't have to worry
> about it anymore.
> 
> Plus, if we did support MuteStream(0) to mean "mute the default one", then
we'd
> have to worry about what happens when you call MuteStream(0) and then
> SendAddStream().  But since no one calls MuteStream(0), I vote we just remove
> support for it.

Done.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine.cc:2963: int
WebRtcVoiceMediaChannel::GetSendChannelNum(uint32 ssrc) const {
On 2015/10/01 19:57:56, pthatcher1 wrote:
> Can you rename this to GetSendChannelBySsrc?

Not going to do that right now.

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
File talk/media/webrtc/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1364643003/diff/60001/talk/media/webrtc/webrtcv...
talk/media/webrtc/webrtcvoiceengine_unittest.cc:3079: // TODO(solenberg): !!!!!!
On 2015/10/01 19:57:56, pthatcher1 wrote:
> You're going to fix this, right?

Oh yes.

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

https://codereview.webrtc.org/1364643003/diff/120001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2101:
engine()->voe()->base()->AssociateSendChannel(recv_channel, channel);
Q: Send+receive channels are associated, and recv channel local SSRCs are set,
when send/recv streams are added. Is there any reason the relationship needs to
be updated when the associated send channel is removed?

[pthatcher1, 2015-10-15 05:15:04]

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

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2053: return false;
This could use a LOG(LS_ERROR).

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2088: }
Why do we do "add and then remove if SetSendCodecs fails" instead of trying to
SetSendCodecs and then add only if it succeeds?  That's how all the other
settings before this work.  What's special about SetSendCodecs?

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2094: first_send_ssrc_ = ssrc;
Would it make sense to call this "receiver_reports_ssrc_" or "rtcp_rr_ssrc_" and
default to 1 instead of -1?

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

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:319: uint32_t first_send_ssrc_ = -1;
Can you comment more on what this means and when it is set?

[the sun, 2015-10-15 12:07:12]

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

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2053: return false;
On 2015/10/15 05:15:04, pthatcher1 wrote:
> This could use a LOG(LS_ERROR).

Added LOG(LS_INFO) at start of AddSendStream() instead. CreateVoEChannel()
already logs its errors; now it is evident in what context it fails.

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2088: }
On 2015/10/15 05:15:04, pthatcher1 wrote:
> Why do we do "add and then remove if SetSendCodecs fails" instead of trying to
> SetSendCodecs and then add only if it succeeds?  That's how all the other
> settings before this work.  What's special about SetSendCodecs?

Added comment.

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.cc:2094: first_send_ssrc_ = ssrc;
On 2015/10/15 05:15:04, pthatcher1 wrote:
> Would it make sense to call this "receiver_reports_ssrc_" or "rtcp_rr_ssrc_"
and
> default to 1 instead of -1?

Good idea, makes the logic clearer.

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

https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
talk/media/webrtc/webrtcvoiceengine.h:319: uint32_t first_send_ssrc_ = -1;
On 2015/10/15 05:15:04, pthatcher1 wrote:
> Can you comment more on what this means and when it is set?

Done.

[the sun, 2015-10-20 07:42:45]

On 2015/10/15 12:07:12, the sun wrote:
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvoiceengine.cc (right):
> 
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.cc:2053: return false;
> On 2015/10/15 05:15:04, pthatcher1 wrote:
> > This could use a LOG(LS_ERROR).
> 
> Added LOG(LS_INFO) at start of AddSendStream() instead. CreateVoEChannel()
> already logs its errors; now it is evident in what context it fails.
> 
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.cc:2088: }
> On 2015/10/15 05:15:04, pthatcher1 wrote:
> > Why do we do "add and then remove if SetSendCodecs fails" instead of trying
to
> > SetSendCodecs and then add only if it succeeds?  That's how all the other
> > settings before this work.  What's special about SetSendCodecs?
> 
> Added comment.
> 
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.cc:2094: first_send_ssrc_ = ssrc;
> On 2015/10/15 05:15:04, pthatcher1 wrote:
> > Would it make sense to call this "receiver_reports_ssrc_" or "rtcp_rr_ssrc_"
> and
> > default to 1 instead of -1?
> 
> Good idea, makes the logic clearer.
> 
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> File talk/media/webrtc/webrtcvoiceengine.h (right):
> 
>
https://codereview.webrtc.org/1364643003/diff/160001/talk/media/webrtc/webrtc...
> talk/media/webrtc/webrtcvoiceengine.h:319: uint32_t first_send_ssrc_ = -1;
> On 2015/10/15 05:15:04, pthatcher1 wrote:
> > Can you comment more on what this means and when it is set?
> 
> Done.

Ping....

[pthatcher1, 2015-10-20 18:15:28]

lgtm

[the sun, 2015-10-20 18:48:52]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2015-10-20 18:48:52]

The patchset sent to the CQ was uploaded after l-g-t-m from pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1364643003/#ps240001 (title:
"rebase")

[commit-bot: I haz the power, 2015-10-20 18:48:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364643003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364643003/240001

[commit-bot: I haz the power, 2015-10-20 20:49:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-20 20:49:27]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[the sun, 2015-10-20 21:02:46]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2015-10-20 21:03:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1364643003/240001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1364643003/240001

[commit-bot: I haz the power, 2015-10-20 22:49:43]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-10-20 22:49:56]

Patchset 13 (id:??) landed as
https://crrev.com/0a617e22a46d476abcaaa081cc90300d335da9f9
Cr-Commit-Position: refs/heads/master@{#10344}