Prevent a voice channel from sending data before a renderer is set.

Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.

R=pthatcher@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/1a018dcda39691c7cb91ef524003482944bc8960
Cr-Commit-Position: refs/heads/master@{#11918}

[Taylor Brandstetter, 2016-02-26 21:45:05]

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

[Taylor Brandstetter, 2016-02-26 21:45:06]

PTAL. I'm not sure if I'm going about this exactly the right way, but the issue
I'm trying to fix is that if you have an RtpSender without a track, an SSRC is
put in the local description, and this is all it takes for audio to be sent (for
native apps; doesn't affect Chrome).

The intent is that no packets are sent until you set a track on the sender,
which will call SetAudioSend with a renderer linking the track to the send
stream.

[Taylor Brandstetter, 2016-03-02 21:53:52]

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

[Taylor Brandstetter, 2016-03-02 21:53:53]

Adding Peter in case he has some thoughts.

https://codereview.webrtc.org/1741933002/diff/1/webrtc/media/engine/webrtcvoi...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1741933002/diff/1/webrtc/media/engine/webrtcvoi...
webrtc/media/engine/webrtcvoiceengine.cc:1954: return ChangeSend(channel,
desired_send_);
I changed this from desired_send_ to send_, because it seems like if the
MediaChannel is "paused" and you add a new channel, it should start out paused
as well.

[pthatcher1, 2016-03-02 22:18:40]

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1200: void
StartRendering(AudioRenderer* renderer) {
Since the AudioRenderer is really acting as a source, can we call this source_,
SetSource(), ClearSource(), and HasSource() instead of IsRendering,
StartRendering, StopRendering?

Call it rendering is very confusing in this context because it makes it sound
like audio going from the network to local playout, when in fact it is audio
from a local source going out to the network.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1814: send_ = send;
Why do we have desired_send_ and send_?  Can't we just use desired_send_?

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1841: if
(!UpdateChannelSendState(kv.first, send)) {
This requires a lookup for every entry while we're doing an iteration.  A better
way to do this would be to have two UpdateChannelSendState methods: one that
does the ssrc => channel lookup for you and one that allows you to pass in the
already known channel.  The first can call the second.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1861: if (send == SEND_MICROPHONE &&
it->second->IsRendering()) {
Yeah, this would be a lot easier to read:

send == SEND_MICROPHONE && it->second->HasSource()

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1890: // state of the voe::Channel.
And this would be a lot more clear as well: 

"If the source was set or cleared we may need to update the sending state"

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2113: AudioRenderer* renderer) {
Here, too, calling it SetSource, would be a lot nicer.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:409:
rtc::scoped_ptr<FakeAudioRenderer> fake_renderer_;
fake_source would be more clear here as well.

[Taylor Brandstetter, 2016-03-03 00:45:03]

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1200: void
StartRendering(AudioRenderer* renderer) {
On 2016/03/02 22:18:40, pthatcher1 wrote:
> Since the AudioRenderer is really acting as a source, can we call this
source_,
> SetSource(), ClearSource(), and HasSource() instead of IsRendering,
> StartRendering, StopRendering?
> 
> Call it rendering is very confusing in this context because it makes it sound
> like audio going from the network to local playout, when in fact it is audio
> from a local source going out to the network.

I agree. I renamed it in a new patch, though it does change quite a few files.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1814: send_ = send;
On 2016/03/02 22:18:40, pthatcher1 wrote:
> Why do we have desired_send_ and send_?  Can't we just use desired_send_?

For PauseSend/ResumeSend below. Though if Hangouts isn't using this, I don't
know who is, so maybe we can remove it.

Oh, I found one other place it's used. If the last stream is removed, "send_"
goes to SEND_NOTHING while desired_send_ stays at SEND_MICROPHONE. This has the
effect that when SetSend is called after a stream is added again,
engine()->ApplyOptions gets called. I think this could be accomplished by just
calling ApplyOptions when the first stream is added though, if this is really
the desired behavior.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1841: if
(!UpdateChannelSendState(kv.first, send)) {
On 2016/03/02 22:18:39, pthatcher1 wrote:
> This requires a lookup for every entry while we're doing an iteration.  A
better
> way to do this would be to have two UpdateChannelSendState methods: one that
> does the ssrc => channel lookup for you and one that allows you to pass in the
> already known channel.  The first can call the second.

Just as an optimization, you mean? I really don't think this lookup should be
that expensive, and having one method is simpler than having two.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1861: if (send == SEND_MICROPHONE &&
it->second->IsRendering()) {
On 2016/03/02 22:18:39, pthatcher1 wrote:
> Yeah, this would be a lot easier to read:
> 
> send == SEND_MICROPHONE && it->second->HasSource() 

Done.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1890: // state of the voe::Channel.
On 2016/03/02 22:18:40, pthatcher1 wrote:
> And this would be a lot more clear as well: 
> 
> "If the source was set or cleared we may need to update the sending state"

Done.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:2113: AudioRenderer* renderer) {
On 2016/03/02 22:18:39, pthatcher1 wrote:
> Here, too, calling it SetSource, would be a lot nicer.

Done.

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/20001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:409:
rtc::scoped_ptr<FakeAudioRenderer> fake_renderer_;
On 2016/03/02 22:18:40, pthatcher1 wrote:
> fake_source would be more clear here as well.

Done.

[the sun, 2016-03-03 15:15:25]

Nice change!

My biggest concern is that cricket::AudioSource is too close to
webrtc::AudioSourceInterface. WDYT?

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
File webrtc/media/base/audiosource.h (right):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
webrtc/media/base/audiosource.h:11: #ifndef WEBRTC_MEDIA_BASE_AUDIOSOURCE_H_
I like this change because I've always found this name confusing.

However, the change means we now have webrtc::AudioSourceInterface and
cricket::AudioSource, which is a bit confusing. Especially given that this class
should likely be a pure interface and following naming conventions it should
then be called cricket::AudioSourceInterface.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...

One way to mitigate any confusion would be to call the MediaSourceInterface
descendant AudioMediaSourceInterface (or AudioMediaSource, since it is not
purely abstract); video would have to follow. Another way would be to be more
explicit about the use of namespaces; the border between cricket and webrtc is a
bit blurry.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
webrtc/media/base/audiosource.h:39: virtual void SetSink(Sink* sink) {}
= 0

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1981: }
Note that this had the effect of setting send_ = SEND_NOTHING, so that the next
new stream added won't automatically be enabled for sending. Am I missing the
logic where that behavior is maintained, or is there a reason for changing it
which eludes me?

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1818: if (!send_streams_.empty() &&
send == SEND_MICROPHONE) {
nit: can we consistently check "send == SEND_..." first in these expressions?

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1834: SendFlags send) {
You could remove the SendFlags enum while you're at it. See:
https://codereview.webrtc.org/1499903002/

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1959: // and we have at least one
stream.
nit: at least -> exactly

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:409:
rtc::scoped_ptr<FakeAudioSource> fake_source_;
No need for a scoped_ptr/unique_ptr here.

[the sun, 2016-03-03 15:15:26]

Nice change!

My biggest concern is that cricket::AudioSource is too close to
webrtc::AudioSourceInterface. WDYT?

[Taylor Brandstetter, 2016-03-03 21:44:26]

Description was changed from

==========
Prevent a voice channel from sending data before a renderer is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a renderer on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a renderer shouldn't send data.
==========

to

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.
==========

[Taylor Brandstetter, 2016-03-04 15:45:14]

Description was changed from

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.
==========

to

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.
==========

[Taylor Brandstetter, 2016-03-04 16:06:56]

I addressed all your comments (aside from renaming AudioSource, I'm still
thinking about that). This includes merging your CL that removes SendFlags and
moves logic to WebRtcAudioSendStream, which simplified things a lot.

One change this introduces (carried over from your CL) is that ApplyOptions will
be called if SetSend(true) is called with 0 send streams. Is this ok? What's the
reason for reapplying the options in the first place?

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
File webrtc/media/base/audiosource.h (right):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
webrtc/media/base/audiosource.h:11: #ifndef WEBRTC_MEDIA_BASE_AUDIOSOURCE_H_
On 2016/03/03 15:15:24, the sun wrote:
> I like this change because I've always found this name confusing.
> 
> However, the change means we now have webrtc::AudioSourceInterface and
> cricket::AudioSource, which is a bit confusing. Especially given that this
class
> should likely be a pure interface and following naming conventions it should
> then be called cricket::AudioSourceInterface.
> 
> See:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> 
> One way to mitigate any confusion would be to call the MediaSourceInterface
> descendant AudioMediaSourceInterface (or AudioMediaSource, since it is not
> purely abstract); video would have to follow. Another way would be to be more
> explicit about the use of namespaces; the border between cricket and webrtc is
a
> bit blurry.

I wish we could just get rid of this class and use AudioSourceInterface
everywhere. But what this class provides is a separate "OnClose" method which is
protected by a critical section in the implementation, and we probably can't get
rid of that. Maybe the solution is to add OnClose to AudioSinkInterface, and
when it's implemented everywhere, switch over to that. But for now, I'll try to
think of a better name.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/base/audioso...
webrtc/media/base/audiosource.h:39: virtual void SetSink(Sink* sink) {}
On 2016/03/03 15:15:24, the sun wrote:
> = 0

Done.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (left):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1981: }
On 2016/03/03 15:15:25, the sun wrote:
> Note that this had the effect of setting send_ = SEND_NOTHING, so that the
next
> new stream added won't automatically be enabled for sending. Am I missing the
> logic where that behavior is maintained, or is there a reason for changing it
> which eludes me?

Actually, the new stream was created with desired_send_, so as far as I can tell
the only effect of ChangeSend(SEND_NOTHING) is that the next call of
SetSend(SEND_MICROPHONE) (while there are streams) will call ApplyOptions. I
tried to retain this logic by calling ApplyOptions when the first send stream is
created, though I removed it when merging your CL.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1818: if (!send_streams_.empty() &&
send == SEND_MICROPHONE) {
On 2016/03/03 15:15:25, the sun wrote:
> nit: can we consistently check "send == SEND_..." first in these expressions?

Done.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1834: SendFlags send) {
On 2016/03/03 15:15:25, the sun wrote:
> You could remove the SendFlags enum while you're at it. See:
> https://codereview.webrtc.org/1499903002/

Done.

https://codereview.webrtc.org/1741933002/diff/80001/webrtc/media/engine/webrt...
webrtc/media/engine/webrtcvoiceengine.cc:1959: // and we have at least one
stream.
On 2016/03/03 15:15:25, the sun wrote:
> nit: at least -> exactly

Done.

[pthatcher1, 2016-03-05 01:55:09]

I think this looks really good.  But about the name of "AudioSource":

We had a very similar thing happen recently with VideoCapturer.  We wanted to
change the name to VideoSourceInterface (because it's a source and an
interface), but there was already a webrtc::VideoSoureInterface that was an
implementation detail of VideoTracks.

So what we did was:
1.  Rename VideoCapturer to VideoSourceInterface, which is in the cricket::
namespace, even though there is already one in the webrtc:: namespace.
2.  Rename the one in the webrtc:: namespace to VideoTrackSourceInterface.


Similarly, I think the thing you have here as AudioSource should be
AudioSourceInterface (because it's an interface) and we should rename the
existing webrtc::AudioSourceInterface to webrtc::AudioTrackSourceInterface.

Just like with VideoSourceinterface, I think we can live temporarily with there
being two, one in each namespace.  Getting this audio bug fixed is important
enough to land this CL and fix the name of the AudioTrackSourceInterface later.

[the sun, 2016-03-07 15:49:22]

On 2016/03/05 01:55:09, pthatcher1 wrote:
> I think this looks really good.  But about the name of "AudioSource":
> 
> We had a very similar thing happen recently with VideoCapturer.  We wanted to
> change the name to VideoSourceInterface (because it's a source and an
> interface), but there was already a webrtc::VideoSoureInterface that was an
> implementation detail of VideoTracks.
> 
> So what we did was:
> 1.  Rename VideoCapturer to VideoSourceInterface, which is in the cricket::
> namespace, even though there is already one in the webrtc:: namespace.
> 2.  Rename the one in the webrtc:: namespace to VideoTrackSourceInterface.
> 
> 
> Similarly, I think the thing you have here as AudioSource should be
> AudioSourceInterface (because it's an interface) and we should rename the
> existing webrtc::AudioSourceInterface to webrtc::AudioTrackSourceInterface.
> 
> Just like with VideoSourceinterface, I think we can live temporarily with
there
> being two, one in each namespace.  Getting this audio bug fixed is important
> enough to land this CL and fix the name of the AudioTrackSourceInterface
later.

sgtm

[the sun, 2016-03-07 16:35:15]

FYI: Not through reviewing yet; will continue in ~4 hours.

https://codereview.webrtc.org/1741933002/diff/140001/webrtc/api/webrtcsession...
File webrtc/api/webrtcsession_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/140001/webrtc/api/webrtcsession...
webrtc/api/webrtcsession_unittest.cc:311: cricket::AudioSource::Sink* sink()
const { return sink_; }
almost unrelated: is it possible to make the result a const* ?

[the sun, 2016-03-07 21:24:54]

Great job on this! Just a few more things.

As to your question about re-applying the options, well, the design here is in
need of refactoring. All current options are global attributes on the VoE, yet
options can be set per MediaChannel. So in theory each MC can have its own
settings, and the strategy to deal with that is to pick the last options set on
a WVoMC, or the options from the last WVoMC that was enabled for sending
(presumably because most, but not all, options apply to the send side).

Step by step...

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:102: RTC_DCHECK_EQ(0, error);
I don't think a DCHECK is appropriate here. There are many reasons why
VoEBase::StartSend() could fail - it enables the ADM for recording! So it should
be treated as a legitimate run-time condition, not a logic error. Thus, to
maintain the current logic, add a conditional log line here, just like the one
in pc/channel.cc which will never be triggered anyway, following this CL.

(same in Stop())

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:899: virtual bool SetSend(bool send) = 0;
Since WVoMC always returns true, change return type to void.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1197: bool HasSource() const {
Only used internally; make private

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1254: source_ = nullptr;
UpdateSendState() ?

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1831: if (send_ == send) {
Add "|| send_streams_.empty()" to maintain current logic.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:408:
rtc::scoped_ptr<FakeAudioSource> fake_source_;
No need for scoped_ptr here.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1466: LOG(LS_ERROR) << "Failed to SetSend " << send << " on
voice channel";
SetSend() always returns true now. This will never happen. Log line should be
moved into AudioSendStream::Start()/Stop()

[Taylor Brandstetter, 2016-03-08 00:00:31]

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.cc:102: RTC_DCHECK_EQ(0, error);
On 2016/03/07 21:24:53, the sun wrote:
> I don't think a DCHECK is appropriate here. There are many reasons why
> VoEBase::StartSend() could fail - it enables the ADM for recording! So it
should
> be treated as a legitimate run-time condition, not a logic error. Thus, to
> maintain the current logic, add a conditional log line here, just like the one
> in pc/channel.cc which will never be triggered anyway, following this CL.
> 
> (same in Stop())

Done.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/base/mediac...
File webrtc/media/base/mediachannel.h (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/base/mediac...
webrtc/media/base/mediachannel.h:899: virtual bool SetSend(bool send) = 0;
On 2016/03/07 21:24:53, the sun wrote:
> Since WVoMC always returns true, change return type to void.

Done.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1197: bool HasSource() const {
On 2016/03/07 21:24:54, the sun wrote:
> Only used internally; make private

I removed the method since it's only called in one place now.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1254: source_ = nullptr;
On 2016/03/07 21:24:53, the sun wrote:
> UpdateSendState() ?

Good catch.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1831: if (send_ == send) {
On 2016/03/07 21:24:53, the sun wrote:
> Add "|| send_streams_.empty()" to maintain current logic.

Done.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine_unittest.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine_unittest.cc:408:
rtc::scoped_ptr<FakeAudioSource> fake_source_;
On 2016/03/07 21:24:54, the sun wrote:
> No need for scoped_ptr here.

Done.

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/pc/channel.cc
File webrtc/pc/channel.cc (right):

https://codereview.webrtc.org/1741933002/diff/160001/webrtc/pc/channel.cc#new...
webrtc/pc/channel.cc:1466: LOG(LS_ERROR) << "Failed to SetSend " << send << " on
voice channel";
On 2016/03/07 21:24:54, the sun wrote:
> SetSend() always returns true now. This will never happen. Log line should be
> moved into AudioSendStream::Start()/Stop()

Done.

[Taylor Brandstetter, 2016-03-08 02:09:14]

I ended up adding a "need_to_apply_options_" flag, so that ApplyOptions is
called in the exact same places as before. It's not pretty, but I don't think
it's any less pretty than "send_" and "desired_send_", and at least its meaning
is clear. We can clean it up in a later CL if desired.

[the sun, 2016-03-08 10:54:12]

LGTM, except please remove the need_to_apply_options_ flag. I'd rather change
the behavior slightly than (re-)introduce that logic.

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1827: // Apply channel specific options
when channel is enabled for sending.
Uhm, right, this becomes a bit messy. I'd rather change the logic slightly than
having to add the extra logic. Please change the impl back to:

void WebRtcVoiceMediaChannel::SetSend(bool send) {
  if (send_ == send) {
    return;
  }

  // Apply channel specific options when channel is enabled for sending.
  if (send) {
    engine()->ApplyOptions(options_);
  }

  // Change the settings on each send channel.
  for (auto& kv : send_streams_) {
    kv.second->SetSend(send);
  }

  send_ = send;
}

And remove the need_to_apply_options_ flag. Terribly sorry about the
inconvenience.

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1970: need_to_apply_options_ = true;
Likewise, change back to:
SetSend(false);

Sorry.

[Taylor Brandstetter, 2016-03-08 16:24:42]

No need to be sorry; thanks for the thorough review.

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
File webrtc/media/engine/webrtcvoiceengine.cc (right):

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1827: // Apply channel specific options
when channel is enabled for sending.
On 2016/03/08 10:54:12, the sun wrote:
> Uhm, right, this becomes a bit messy. I'd rather change the logic slightly
than
> having to add the extra logic. Please change the impl back to:
> 
> void WebRtcVoiceMediaChannel::SetSend(bool send) {
>   if (send_ == send) {
>     return;
>   }
> 
>   // Apply channel specific options when channel is enabled for sending.
>   if (send) {
>     engine()->ApplyOptions(options_);
>   }
> 
>   // Change the settings on each send channel.
>   for (auto& kv : send_streams_) {
>     kv.second->SetSend(send);
>   }
> 
>   send_ = send;
> }
> 
> And remove the need_to_apply_options_ flag. Terribly sorry about the
> inconvenience.

Done.

https://codereview.webrtc.org/1741933002/diff/200001/webrtc/media/engine/webr...
webrtc/media/engine/webrtcvoiceengine.cc:1970: need_to_apply_options_ = true;
On 2016/03/08 10:54:12, the sun wrote:
> Likewise, change back to:
> SetSend(false);
> 
> Sorry.

Done.

[Taylor Brandstetter, 2016-03-08 16:25:32]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-03-08 16:25:33]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1741933002/#ps220001 (title:
"Removing "need_to_apply_options_" in favor of simpler, slightly changed
logic.")

[commit-bot: I haz the power, 2016-03-08 16:25:41]

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

[commit-bot: I haz the power, 2016-03-08 16:26:52]

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

[commit-bot: I haz the power, 2016-03-08 16:26:53]

Try jobs failed on following builders:
  android_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x64_dbg...)
  android_compile_x86_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_compile_x86_dbg...)
  android_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_dbg/builds/9348)
  android_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_gn_rel/builds/9436)
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/5784)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/5776)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/8114)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/8029)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/13307)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/11940)
  linux_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/13057)
  linux_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_compile_dbg/build...)
  linux_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_gn_rel/builds/9213)
  linux_tsan2 on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/10458)
  linux_ubsan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/343)
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  mac_asan on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/12971)
  mac_compile_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_dbg/builds/...)
  mac_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_dbg/builds/1430)
  mac_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_gn_rel/builds/7661)
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/13343)
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3978)

[Taylor Brandstetter, 2016-03-08 16:29:31]

The CQ bit was checked by deadbeef@webrtc.org

[Taylor Brandstetter, 2016-03-08 16:29:31]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1741933002/#ps240001 (title:
"Merge with master")

[commit-bot: I haz the power, 2016-03-08 16:29:41]

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

[commit-bot: I haz the power, 2016-03-08 16:47:30]

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

[commit-bot: I haz the power, 2016-03-08 16:47:31]

Try jobs failed on following builders:
  presubmit on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/3981)

[pthatcher1, 2016-03-08 17:46:51]

lgtm

Just random comments/thoughts.  Although, you probably want to avoid breaking
the Chrome FYI bots, perhaps by having leaving an audiorenderer.h with an empty
AudioRenderer type.

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audior...
File webrtc/media/base/audiorenderer.h (left):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audior...
webrtc/media/base/audiorenderer.h:19: class AudioRenderer {
We should make sure this doesn't break the Chrome FYI bots because of this:

//src/content/renderer/media/webrtc/webrtc_local_audio_track_adapter.h

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
File webrtc/media/base/audiosource.h (right):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
webrtc/media/base/audiosource.h:23: class Sink {
In the CL renaming this, we might as well rename AudioSourceInterface::Sink to
AudioSinkInterface as well, to match the AudioSourceInterface and
AudioSinkInterface.

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/fakeme...
File webrtc/media/base/fakemediaengine.h (right):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/fakeme...
webrtc/media/base/fakemediaengine.h:256: virtual void SetSend(bool send) {
set_sending(send); }
I like having it not be flags.

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/engine/fake...
File webrtc/media/engine/fakewebrtccall.h (right):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/engine/fake...
webrtc/media/engine/fakewebrtccall.h:47: bool IsSending() const { return
sending_; }
sending() might be a better name here.

[Taylor Brandstetter, 2016-03-08 20:37:52]

Description was changed from

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.
==========

to

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.

R=pthatcher@webrtc.org, solenberg@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1a018dcda39691c7cb91ef524...
==========

[Taylor Brandstetter, 2016-03-08 20:37:54]

Committed patchset #15 (id:280001) manually as
1a018dcda39691c7cb91ef524003482944bc8960 (presubmit successful).

[commit-bot: I haz the power, 2016-03-08 20:37:55]

Description was changed from

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.

R=pthatcher@webrtc.org, solenberg@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1a018dcda39691c7cb91ef524...
==========

to

==========
Prevent a voice channel from sending data before a source is set.

At the top level, setting a track on an RtpSender is equivalent to
setting a source (previously called a renderer)
on a voice send stream. An RtpSender without a track
is not supposed to send data (not even muted data), so a send stream without
a source shouldn't send data.

Also replacing SendFlags with a boolean and implementing "Start"
and "Stop" methods on AudioSendStream, which was planned anyway
and simplifies this CL.

R=pthatcher@webrtc.org, solenberg@webrtc.org

Committed: https://crrev.com/1a018dcda39691c7cb91ef524003482944bc8960
Cr-Commit-Position: refs/heads/master@{#11918}
==========

[commit-bot: I haz the power, 2016-03-08 20:37:56]

Patchset 15 (id:??) landed as
https://crrev.com/1a018dcda39691c7cb91ef524003482944bc8960
Cr-Commit-Position: refs/heads/master@{#11918}

[the sun, 2016-03-08 20:48:40]

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
File webrtc/media/base/audiosource.h (right):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
webrtc/media/base/audiosource.h:23: class Sink {
On 2016/03/08 17:46:51, pthatcher1 wrote:
> In the CL renaming this, we might as well rename AudioSourceInterface::Sink to
> AudioSinkInterface as well, to match the AudioSourceInterface and
> AudioSinkInterface.

I think that may be confusing. We'd likely be better off adding an OnClose()
method to webrtc::AudioSinkInterface and start to use that in more places.

[pthatcher1, 2016-03-08 21:19:18]

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
File webrtc/media/base/audiosource.h (right):

https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
webrtc/media/base/audiosource.h:23: class Sink {
On 2016/03/08 20:48:40, the sun wrote:
> On 2016/03/08 17:46:51, pthatcher1 wrote:
> > In the CL renaming this, we might as well rename AudioSourceInterface::Sink
to
> > AudioSinkInterface as well, to match the AudioSourceInterface and
> > AudioSinkInterface.
> 
> I think that may be confusing. We'd likely be better off adding an OnClose()
> method to webrtc::AudioSinkInterface and start to use that in more places.

There's already a webrtc::AudioSinkInterface?  Yikes.  Well, we should
definitely merge them in some way, if we can.

[Taylor Brandstetter, 2016-03-08 21:26:21]

On 2016/03/08 21:19:18, pthatcher1 wrote:
>
https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
> File webrtc/media/base/audiosource.h (right):
> 
>
https://codereview.webrtc.org/1741933002/diff/240001/webrtc/media/base/audios...
> webrtc/media/base/audiosource.h:23: class Sink {
> On 2016/03/08 20:48:40, the sun wrote:
> > On 2016/03/08 17:46:51, pthatcher1 wrote:
> > > In the CL renaming this, we might as well rename
AudioSourceInterface::Sink
> to
> > > AudioSinkInterface as well, to match the AudioSourceInterface and
> > > AudioSinkInterface.
> > 
> > I think that may be confusing. We'd likely be better off adding an OnClose()
> > method to webrtc::AudioSinkInterface and start to use that in more places.
> 
> There's already a webrtc::AudioSinkInterface?  Yikes.  Well, we should
> definitely merge them in some way, if we can.

Yeah. In comment #11 I mentioned this. The only thing missing from the other
AudioSourceInterface is "OnClose" so we should just add it and converge the two.