Detect AudioInputStream muting and propagate to MediaStreamAudioSource.

Detect AudioInputStream muting and propagate to MediaStreamAudioSource.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2919793002
Cr-Commit-Position: refs/heads/master@{#481184}
Committed: https://chromium.googlesource.com/chromium/src/+/5b2232658c703564b42eb768cb97c36e385cb7d1

[ossu-chromium, 2017-06-01 15:30:07]

Description was changed from

==========
Detect change in AudioInputStream muted state and propagate to JS.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=
==========

to

==========
Detect change in AudioInputStream muted state and propagate to JS.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ossu-chromium, 2017-06-01 15:56:58]

ossu@chromium.org changed reviewers:
+ maxmorin@chromium.org, olka@chromium.org

[ossu-chromium, 2017-06-01 15:57:00]

Hi Max, Olga,

Hoping you can spare some time to have a look at this.

This is how I think we should get the stream's muted status propagated from an
audio device all the way up to JS.
The changes cut through many, many files, but most of them only get an extra
callback and sends the information along.
I've outlined the steps below:

1. AudioInputController probes the stream's IsMuted property once every second.

2. AudioInputController::EventHandler gets a new callback, OnMuted(bool). If the
muted state has changed, AudioInputController notifies AudioInputRendererHost
through said callback.

3. A new IPC message for AudioInput needs to be created in audio_messages.h.
I've gone with:
   IPC_MESSAGE_CONTROL2(AudioInputMsg_NotifyStreamMuted, int /* stream id */,
bool /* is muted? */)

4. AudioInputMessageFilter::OnMessageReceived has this new message added to its
message map, calling OnStreamMuted. Its OnStreamMuted implementation calls
OnMuted on the correct AudioInputIPCDelegate for the stream.

5. AudioInputIPCDelegate now has an OnMuted callback added to it, implemented by
AudioInputDevice.

6. AudioCapturerSource::CaptureCallback now has an OnCaptureMuted callback added
to it.

7. AudioInputDevice::OnMuted calls its callback_->OnCaptureMuted with the new
muted info.

8. MediaStreamAudioSource gets a proteced SetMutedState member which calls
SetReadyState on its owner (a blink::WebMediaStreamSource). The call is actually
posted on a TaskRunner to get it onto the right thread first.

9. The implementation of the AudioSources calls this SetMutedState(is_muted) to
propagate the stream's muted state into blink.

10. To get an initially "muted" state to be recognized, I had to add a check to
WebKit's MediaStreamTrack. Otherwise, it would only pick up changes after the
track had been created.


I thought it'd be easier to put it up as a CL, rather than discuss the code
separately first. I've a couple of open questions:

A. AudioInputController shouldn't have to probe the input stream. It seems
possible the stream implementations could provide a callback when the muted
state has changed (though I haven't actually tried it). I'm considering this as
a follow-up.

B. The IPC stuff. I just went with OnStreamMuted for now, but maybe something
more flexible should be used instead? Whatever we choose, it needs to be part of
the upcoming mojofication as well.

Oh, and a couple of small things below.

Thanks!

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.cc:153: FROM_HERE,
base::Bind(&MediaStreamSource::SetSourceMuted, GetWeakPtr(),
I've put SetSourceMuted directly in MediaStreamSource for now. It's got similar
functionality to what's already in MediaStreamVideoSource, so maybe I should get
both implementations to call SetSourceMuted. Or I should just put this
functionality here in MediaStreamAudioSource instead?

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we
only go between Muted and Live, not from Ended.
This looks like it's already be taken care of downstream, but maybe a sanity
check here would be useful?

[Max Morin, 2017-06-02 09:56:03]

maxmorin@chromium.org changed reviewers:
+ guidou@chromium.org

[Max Morin, 2017-06-02 09:56:05]

Some random comments. Overall approach looks good.

Re A: event from the stream would be ideal, but maybe too much effort? Just make
sure IsMuted is properly implemented for ~all platforms.

Re B: I actually think I removed some message like "OnStreamStateChanged"
before, since it wasn't used :). I prefer having this simple OnMuted message. We
shouldn't generalize before we know we need to.

In addition to this, I have some questions on the spec
https://w3c.github.io/mediacapture-main/ and this CL. I notice that ReadyState
and muted are different things in the spec, but in MediaStreamTrack they're
kinda smushed together. Not sure what's up with that or if it's a problem. I'm
also not sure if we actually have to fire an event if the track is already muted
at construction, or if the js app is supposed to check the state of the track
after construction.

Would be nice to have someone with a better understanding of the spec have a
look at this CL, so +R Guido.

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.cc:153: FROM_HERE,
base::Bind(&MediaStreamSource::SetSourceMuted, GetWeakPtr(),
On 2017/06/01 15:56:59, ossu-chromium wrote:
> I've put SetSourceMuted directly in MediaStreamSource for now. It's got
similar
> functionality to what's already in MediaStreamVideoSource, so maybe I should
get
> both implementations to call SetSourceMuted. Or I should just put this
> functionality here in MediaStreamAudioSource instead?

I think someone with experience in this layer should look at this code. Judging
from the git log, maybe miu@?

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we
only go between Muted and Live, not from Ended.
On 2017/06/01 15:56:59, ossu-chromium wrote:
> This looks like it's already be taken care of downstream, but maybe a sanity
> check here would be useful?

Yes. Could you point out where this is taken care of?

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:361: check_muted_state_timer_.reset(new
base::RepeatingTimer());
If you use unique_ptr, most Chromium people prefer base::MakeUnique over raw new
calls for allocating memory. Up to you though.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:396: // TODO(ossu): Can't we just reset()
it here? Should stop automatically, IIUC.
Looking at the implementation, yes. Would be nice to have that documented in
timer.h if you want to use it, though.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:498: handler_->OnLog(this, "AIC::OnData:
microphone is muted!");
If you log it elsewhere, maybe remove this log?

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:678: // Don't log this here,
AudioInputRendererHost will log instead.
Does AIRH actually log this?

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.h:375: std::unique_ptr<base::RepeatingTimer>
check_muted_state_timer_;
Does this need to be allocated separately? Otherwise, consider using
base::Optional to save an allocation and improve memory locality.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_dev...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:229:
DCHECK(task_runner()->BelongsToCurrentThread());
I think you have to check |state_| here to ensure you're not accessing
|callback_| after it's freed (like in OnError).

https://codereview.chromium.org/2919793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
You seem to be calling a virtual method from the constructor which is usually
scary. Maybe you can make it final?

[ossu-chromium, 2017-06-02 10:48:00]

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_audio_source.cc:153: FROM_HERE,
base::Bind(&MediaStreamSource::SetSourceMuted, GetWeakPtr(),
On 2017/06/02 09:56:04, Max Morin wrote:
> On 2017/06/01 15:56:59, ossu-chromium wrote:
> > I've put SetSourceMuted directly in MediaStreamSource for now. It's got
> similar
> > functionality to what's already in MediaStreamVideoSource, so maybe I should
> get
> > both implementations to call SetSourceMuted. Or I should just put this
> > functionality here in MediaStreamAudioSource instead?
> 
> I think someone with experience in this layer should look at this code.
Judging
> from the git log, maybe miu@?

Sounds good. I'll add him!

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we
only go between Muted and Live, not from Ended.
On 2017/06/02 09:56:04, Max Morin wrote:
> On 2017/06/01 15:56:59, ossu-chromium wrote:
> > This looks like it's already be taken care of downstream, but maybe a sanity
> > check here would be useful?
> 
> Yes. Could you point out where this is taken care of?

MediaStreamTrack::SourceChangedState() checks if the current ReadyState is Ended
and just does nothing.
MediaStreamSource::SetReadyState also checks that the current ReadyState isn't
Ended before doing anything.
I could point to those in a comment. I'm wondering if a DCHECK here would be
good. We really shouldn't be sending mute/unmute notifications for a stream
that's ended, but maybe timing, threading and queuing sometimes would have that
happen? It would be sad if we crashed.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:396: // TODO(ossu): Can't we just reset()
it here? Should stop automatically, IIUC.
On 2017/06/02 09:56:04, Max Morin wrote:
> Looking at the implementation, yes. Would be nice to have that documented in
> timer.h if you want to use it, though.

The top of timer.h says:
"OneShotTimer and RepeatingTimer both cancel the timer when they go out of
scope, which makes it easy to ensure that you do not get called when your object
has gone out of scope."

But this is how we've used it elsewhere. Looking at it again, I'm not sure why
we recreate the timer at all. Can it not just be a member variable and
started/stopped on Create() and Close()?

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:498: handler_->OnLog(this, "AIC::OnData:
microphone is muted!");
On 2017/06/02 09:56:04, Max Morin wrote:
> If you log it elsewhere, maybe remove this log?

I think it's probably good to keep this one, since it's directly related to the
periodic audio power logging. Makes it that much easier to figure out why
there's no audio, rather than looking for the separate muted message.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:678: // Don't log this here,
AudioInputRendererHost will log instead.
On 2017/06/02 09:56:04, Max Morin wrote:
> Does AIRH actually log this?

Aww, shucks. It did until I removed that. I think that, since we're delegating
the logging to that class anyways, we might as well log this there directly,
rather than imposing a second PostTask etc. just to get the message out. Will
add it back in in AIRH::DoNotifyMutedState.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.h:375: std::unique_ptr<base::RepeatingTimer>
check_muted_state_timer_;
On 2017/06/02 09:56:05, Max Morin wrote:
> Does this need to be allocated separately? Otherwise, consider using
> base::Optional to save an allocation and improve memory locality.

I'll try to get it as a regular object instead, see if that works.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_dev...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:229:
DCHECK(task_runner()->BelongsToCurrentThread());
On 2017/06/02 09:56:05, Max Morin wrote:
> I think you have to check |state_| here to ensure you're not accessing
> |callback_| after it's freed (like in OnError).

Oh, you're probably right. I'll look into it.

https://codereview.chromium.org/2919793002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
On 2017/06/02 09:56:05, Max Morin wrote:
> You seem to be calling a virtual method from the constructor which is usually
> scary. Maybe you can make it final?

You're right. I actually only need to add a check for Muted and then call
component_->SetMuted(). The rest SourceChangedState will just never happen
(since the default ready_state_ is Live).

[ossu-chromium, 2017-06-02 10:48:23]

On 2017/06/02 09:56:05, Max Morin wrote:
> Some random comments. Overall approach looks good.

Cool!

> Re A: event from the stream would be ideal, but maybe too much effort? Just
make
> sure IsMuted is properly implemented for ~all platforms.

Right. That it isn't. I believe it might be implemented only for
PulseAudio and the low latency macOS backend. I'll add that
functionality in separate CLs, so as not to make this one incomprehensible

> Re B: I actually think I removed some message like "OnStreamStateChanged"
> before, since it wasn't used :). I prefer having this simple OnMuted message.
We
> shouldn't generalize before we know we need to.

Seeing as adding an extra IPC call was really quite quick, at least
programatically, I agree with that.
 
> In addition to this, I have some questions on the spec
> https://w3c.github.io/mediacapture-main/ and this CL. I notice that ReadyState
> and muted are different things in the spec, but in MediaStreamTrack they're
> kinda smushed together. Not sure what's up with that or if it's a problem. I'm
> also not sure if we actually have to fire an event if the track is already
muted
> at construction, or if the js app is supposed to check the state of the track
> after construction.

It seems to me that the changing the ReadyState is how mute is set
internally; I've just copied what's already being done for video. WRT
sending an event, the change to MediaStreamTrack.cpp is necessary for
the muted parameter to show "true". For some strange reason, the
SourceStateChange() callback sets the muted property on the
source. There's effectively no event being sent out externally by
this, since no-one has had the chance to subscribe to events yet.

> Would be nice to have someone with a better understanding of the spec have a
> look at this CL, so +R Guido.

Great!

[ossu-chromium, 2017-06-02 10:58:10]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 10:58:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 11:08:16]

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

[commit-bot: I haz the power, 2017-06-02 11:08:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ossu-chromium, 2017-06-02 12:50:20]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 12:50:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ossu-chromium, 2017-06-02 13:15:28]

Description was changed from

==========
Detect change in AudioInputStream muted state and propagate to JS.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Detect change in AudioInputStream muted state and propagate to JS.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[Guido Urdaneta, 2017-06-02 13:33:46]

PS2 came up while I was revieweing PS1. Take a look at these comments and I'll
continue with PS2.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:361: check_muted_state_timer_.reset(new
base::RepeatingTimer());
On 2017/06/02 09:56:04, Max Morin wrote:
> If you use unique_ptr, most Chromium people prefer base::MakeUnique over raw
new
> calls for allocating memory. Up to you though.

MakeUnique is strongly preferred over raw new, but you can alsop switch to a
regular object.

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:396: // TODO(ossu): Can't we just reset()
it here? Should stop automatically, IIUC.
On 2017/06/02 10:48:00, ossu-chromium wrote:
> On 2017/06/02 09:56:04, Max Morin wrote:
> > Looking at the implementation, yes. Would be nice to have that documented in
> > timer.h if you want to use it, though.
> 
> The top of timer.h says:
> "OneShotTimer and RepeatingTimer both cancel the timer when they go out of
> scope, which makes it easy to ensure that you do not get called when your
object
> has gone out of scope."
> 
> But this is how we've used it elsewhere. Looking at it again, I'm not sure why
> we recreate the timer at all. Can it not just be a member variable and
> started/stopped on Create() and Close()?

There are also many uses in the codebase that just call reset() without calling
Stop().

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.h:375: std::unique_ptr<base::RepeatingTimer>
check_muted_state_timer_;
On 2017/06/02 10:48:00, ossu-chromium wrote:
> On 2017/06/02 09:56:05, Max Morin wrote:
> > Does this need to be allocated separately? Otherwise, consider using
> > base::Optional to save an allocation and improve memory locality.
> 
> I'll try to get it as a regular object instead, see if that works.

Using a regular object should suffice. From the documentation of timer.h:

// Sample RepeatingTimer usage:
//
//   class MyClass {
//    public:
//     void StartDoingStuff() {
//       timer_.Start(FROM_HERE, TimeDelta::FromSeconds(1),
//                    this, &MyClass::DoStuff);
//     }
//     void StopDoingStuff() {
//       timer_.Stop();
//     }
//    private:
//     void DoStuff() {
//       // This method is called every second to do stuff.
//       ...
//     }
//     base::RepeatingTimer timer_;
//   };

A code search shows both kinds of usage.

[Guido Urdaneta, 2017-06-02 13:45:34]

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
Perhaps you should postpone the blink work to a follow-up CL because it requires
a little more work than this.
The spec says the only valid ready states are live and ended, but this change
will introduce a "muted" ready state which does not exist in the spec.
Can you introduce a test (possibly browser test, but maybe layout test) to check
that muting changes the muted attribute, but keeps the readyState as "live"?

[ossu-chromium, 2017-06-02 13:53:20]

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
On 2017/06/02 13:45:34, Guido Urdaneta wrote:
> Perhaps you should postpone the blink work to a follow-up CL because it
requires
> a little more work than this.
> The spec says the only valid ready states are live and ended, but this change
> will introduce a "muted" ready state which does not exist in the spec.
> Can you introduce a test (possibly browser test, but maybe layout test) to
check
> that muting changes the muted attribute, but keeps the readyState as "live"?

Sounds good. I believe this should work as intended, since I've copied the blink
integration from the video side, but it really should be tested properly. The
rest of the code can land without this change, as well. It just won't pick up
the initial change.

[ossu-chromium, 2017-06-02 14:07:21]

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
On 2017/06/02 13:53:19, ossu-chromium wrote:
> On 2017/06/02 13:45:34, Guido Urdaneta wrote:
> > Perhaps you should postpone the blink work to a follow-up CL because it
> requires
> > a little more work than this.
> > The spec says the only valid ready states are live and ended, but this
change
> > will introduce a "muted" ready state which does not exist in the spec.
> > Can you introduce a test (possibly browser test, but maybe layout test) to
> check
> > that muting changes the muted attribute, but keeps the readyState as "live"?
> 
> Sounds good. I believe this should work as intended, since I've copied the
blink
> integration from the video side, but it really should be tested properly. The
> rest of the code can land without this change, as well. It just won't pick up
> the initial change.

Just checked: readyState goes to "muted" with this CL. Though the spec only has
"live" and "ended", MediaStreamSource in blink has three states (and did before
my change), so it's quite possible I've misunderstood how they're supposed to
interact. It's also possible blink should report "live" even though the source
goes to muted. I'll keep the part setting the ReadyState out of this CL as well
for now, so this CL only covers the propagation up into MediaStreamAudioSource.

[Guido Urdaneta, 2017-06-02 14:13:21]

Can you also add a test to verify that the event propagates to the relevant
layers?

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
media/audio/audio_input_controller.cc:201: DCHECK(!audio_callback_);
DCHECK that the timer has no value?

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
media/audio/audio_input_controller.cc:674: // Don't log this here,
AudioInputRendererHost will log instead.
remove obsolete comment

[Guido Urdaneta, 2017-06-02 14:16:40]

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right):

https://codereview.chromium.org/2919793002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:78:
SourceChangedState();
On 2017/06/02 14:07:21, ossu-chromium wrote:
> On 2017/06/02 13:53:19, ossu-chromium wrote:
> > On 2017/06/02 13:45:34, Guido Urdaneta wrote:
> > > Perhaps you should postpone the blink work to a follow-up CL because it
> > requires
> > > a little more work than this.
> > > The spec says the only valid ready states are live and ended, but this
> change
> > > will introduce a "muted" ready state which does not exist in the spec.
> > > Can you introduce a test (possibly browser test, but maybe layout test) to
> > check
> > > that muting changes the muted attribute, but keeps the readyState as
"live"?
> > 
> > Sounds good. I believe this should work as intended, since I've copied the
> blink
> > integration from the video side, but it really should be tested properly.
The
> > rest of the code can land without this change, as well. It just won't pick
up
> > the initial change.
> 
> Just checked: readyState goes to "muted" with this CL. Though the spec only
has
> "live" and "ended", MediaStreamSource in blink has three states (and did
before
> my change), so it's quite possible I've misunderstood how they're supposed to
> interact. It's also possible blink should report "live" even though the source
> goes to muted. I'll keep the part setting the ReadyState out of this CL as
well
> for now, so this CL only covers the propagation up into
MediaStreamAudioSource.

MediaStreamSource was probably designed for an older version of the spec that
included "muted" as a readyState.
It can be made spec-compliant without many changes, but it's probably better to
do that in a follow-up CL.

[ossu-chromium, 2017-06-02 14:38:33]

Description was changed from

==========
Detect change in AudioInputStream muted state and propagate to JS.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Detect AudioInputStream muting and propagate to MediaStreamAudioSource.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[ossu-chromium, 2017-06-02 15:00:06]

On 2017/06/02 14:16:40, Guido Urdaneta wrote:
> MediaStreamSource was probably designed for an older version of the spec that
> included "muted" as a readyState.
> It can be made spec-compliant without many changes, but it's probably better
to
> do that in a follow-up CL.

Ah, ok. I've put up a second CL here:
https://codereview.chromium.org/2922733002/
It currently contains only the changes I removed from this one, as a jumping-off
point.

[ossu-chromium, 2017-06-02 15:32:45]

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
media/audio/audio_input_controller.cc:201: DCHECK(!audio_callback_);
On 2017/06/02 14:13:21, Guido Urdaneta wrote:
> DCHECK that the timer has no value?

Done.

https://codereview.chromium.org/2919793002/diff/20001/media/audio/audio_input...
media/audio/audio_input_controller.cc:674: // Don't log this here,
AudioInputRendererHost will log instead.
On 2017/06/02 14:13:21, Guido Urdaneta wrote:
> remove obsolete comment

No, I added it so that anyone scouring this code, wondering why this isn't
logged, will know that it is, just not right here. I've rephrased it to try to
make it clearer.

[o1ka, 2017-06-02 16:27:27]

Neat! lgtm %comments below

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.cc:365: this,
&AudioInputController::CheckMutedState);
Add AudioInputController unit tests checking that OnMuted is called correctly in
various cases?
(now the code is pretty straightforward, but we want to be able to catch
regressions)

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.cc:676: handler_->OnMuted(this, is_muted_);
So all the clients should assume that by default it's unmuted, and the first (if
any) OnMuted call will be with "false", right?
Document it as a part of the contract for both browser and renderer side
interfaces?

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.h:376: // The timer needs to be destroyed on
the correct thread, so we can't leave
nit: empty line before the comment

[Max Morin, 2017-06-05 07:01:38]

LGTM.

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
File content/renderer/media/media_stream_source.cc (right):

https://codereview.chromium.org/2919793002/diff/1/content/renderer/media/medi...
content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we
only go between Muted and Live, not from Ended.
On 2017/06/02 10:47:59, ossu-chromium wrote:
> On 2017/06/02 09:56:04, Max Morin wrote:
> > On 2017/06/01 15:56:59, ossu-chromium wrote:
> > > This looks like it's already be taken care of downstream, but maybe a
sanity
> > > check here would be useful?
> > 
> > Yes. Could you point out where this is taken care of?
> 
> MediaStreamTrack::SourceChangedState() checks if the current ReadyState is
Ended
> and just does nothing.
> MediaStreamSource::SetReadyState also checks that the current ReadyState isn't
> Ended before doing anything.
> I could point to those in a comment. I'm wondering if a DCHECK here would be
> good. We really shouldn't be sending mute/unmute notifications for a stream
> that's ended, but maybe timing, threading and queuing sometimes would have
that
> happen? It would be sad if we crashed.

Right, feels better without a DCHECK.

[Guido Urdaneta, 2017-06-05 13:34:14]

the code looks good to me, will wait for the tests

[ossu-chromium, 2017-06-07 17:27:06]

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.cc:365: this,
&AudioInputController::CheckMutedState);
On 2017/06/02 16:27:26, o1ka wrote:
> Add AudioInputController unit tests checking that OnMuted is called correctly
in
> various cases?
> (now the code is pretty straightforward, but we want to be able to catch
> regressions)

Sure. Should help verify the solution if I go one step further in a future CL
and have the state be listened for, rather than polled for.

There's now also a browser test in the next CL that ensures the JavaScript state
gets set correctly when the AudioInputStream muted state changes.

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.cc:676: handler_->OnMuted(this, is_muted_);
On 2017/06/02 16:27:27, o1ka wrote:
> So all the clients should assume that by default it's unmuted, and the first
(if
> any) OnMuted call will be with "false", right?
> Document it as a part of the contract for both browser and renderer side
> interfaces?

Should document this, yes. The first OnMuted call will be OnMuted(true), though.

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/40001/media/audio/audio_input...
media/audio/audio_input_controller.h:376: // The timer needs to be destroyed on
the correct thread, so we can't leave
On 2017/06/02 16:27:27, o1ka wrote:
> nit: empty line before the comment

Will fix!

[ossu-chromium, 2017-06-14 09:20:36]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 09:20:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-14 09:23:42]

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

[commit-bot: I haz the power, 2017-06-14 09:23:44]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[ossu-chromium, 2017-06-14 13:38:40]

Patchset #4 (id:60001) has been deleted

[ossu-chromium, 2017-06-14 15:17:53]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 15:18:11]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ossu-chromium, 2017-06-14 16:38:28]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-14 16:38:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ossu-chromium, 2017-06-14 16:40:04]

Moved FakeAudioStream::SetGlobalState from a couple of CLs down-the-line to this
one, so that I could implement AudioInputController unit tests.

[ossu-chromium, 2017-06-14 17:19:43]

ossu@chromium.org changed reviewers:
+ hubbe@chromium.org

[ossu-chromium, 2017-06-14 17:19:44]

Adding hubbe for owner review.

PTAL. I'm adding a callback to AudioInputController to (eventually) let the
JavaScript code be notified of when an input is muted on the system level.

https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
File content/renderer/media/media_stream_audio_source.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
content/renderer/media/media_stream_audio_source.cc:151: // TODO(ossu):
Propagate this muted state into blink.
This is done in the final CL of the set.

[hubbe, 2017-06-14 18:30:22]

On 2017/06/14 17:19:44, ossu-chromium wrote:
> Adding hubbe for owner review.
> 
> PTAL. I'm adding a callback to AudioInputController to (eventually) let the
> JavaScript code be notified of when an input is muted on the system level.
> 
>
https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
> File content/renderer/media/media_stream_audio_source.cc (right):
> 
>
https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
> content/renderer/media/media_stream_audio_source.cc:151: // TODO(ossu):
> Propagate this muted state into blink.
> This is done in the final CL of the set.

Hmm, what files do you want me to review?
It might go faster if you find a reviewer who is more active in the audio
directory though.

[commit-bot: I haz the power, 2017-06-14 19:01:10]

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

[commit-bot: I haz the power, 2017-06-14 19:01:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[ossu-chromium, 2017-06-15 16:38:13]

On 2017/06/14 18:30:22, hubbe wrote:
> On 2017/06/14 17:19:44, ossu-chromium wrote:
> > Adding hubbe for owner review.
> > 
> > PTAL. I'm adding a callback to AudioInputController to (eventually) let the
> > JavaScript code be notified of when an input is muted on the system level.
> > 
> >
>
https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
> > File content/renderer/media/media_stream_audio_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/2919793002/diff/100001/content/renderer/media...
> > content/renderer/media/media_stream_audio_source.cc:151: // TODO(ossu):
> > Propagate this muted state into blink.
> > This is done in the final CL of the set.
> 
> Hmm, what files do you want me to review?
> It might go faster if you find a reviewer who is more active in the audio
> directory though.

I was thinking media/base and media/audio. I'm not sure who to pick that'd be
more active in audio/. Someone mentioned miu@, but he's OOO. If nothing else, I
guess you could have a look at the change in media/base. :)

[ossu-chromium, 2017-06-15 16:41:29]

ossu@chromium.org changed reviewers:
+ bbudge@chromium.org, dcheng@chromium.org

[ossu-chromium, 2017-06-15 16:41:30]

Adding bbudge for pepper changes (just simple updates to callback interfaces).

Adding dcheng for common/media/audio_messages.h

[ossu-chromium, 2017-06-15 16:44:06]

ossu@chromium.org changed reviewers:
+ dalecurtis@chromium.org
- hubbe@chromium.org

[ossu-chromium, 2017-06-15 16:44:07]

Adding dalecurtis for media/ reviews after a tip from a coworker. :)

[DaleCurtis, 2017-06-15 18:44:01]

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:361: CheckMutedState();
Instead of using a repeating timer like this why not just check the muted state
during OnData() and post a task to the handler if it's muted? We already do that
for power level checks.

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.h:383: base::Optional<base::RepeatingTimer>
check_muted_state_timer_;
Drop Optional in favor of just IsRunning().

[dcheng, 2017-06-17 08:50:02]

ipc lgtm

https://codereview.chromium.org/2919793002/diff/100001/media/audio/fake_audio...
File media/audio/fake_audio_input_stream.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/fake_audio...
media/audio/fake_audio_input_stream.cc:89: return
base::subtle::NoBarrier_Load(&g_fake_input_streams_are_muted) != 0;
Is it necessary to use atomics here?

The code would be easier to understand without it.

[Guido Urdaneta, 2017-06-19 09:46:30]

c/r/m and c/b/rh/m lgtm

[bbudge, 2017-06-19 13:42:52]

content/renderer/pepper lgtm

[ossu-chromium, 2017-06-20 11:25:45]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 11:26:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 12:15:52]

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

[commit-bot: I haz the power, 2017-06-20 12:15:54]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[ossu-chromium, 2017-06-20 12:25:15]

Rewrote tests to work on Mac and made the timer a regular member again.

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:361: CheckMutedState();
On 2017/06/15 18:44:00, DaleCurtis wrote:
> Instead of using a repeating timer like this why not just check the muted
state
> during OnData() and post a task to the handler if it's muted? We already do
that
> for power level checks.

From looking at the implementations of IsMuted, they seem to be making calls
that would result in IPC (not Chrome IPC) and waiting for callbacks. I don't
want to put that kind of code in the audio input callback and risk introducing
latency issues. The call already there only happens once every 15 seconds, which
probably isn't often enough for proper feedback.

Once this chain of CLs has landed, I'd like to have a look at whether these
events could be listened for, instead of polling. If so, I'd be able to get rid
of this timer.

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.h (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.h:383: base::Optional<base::RepeatingTimer>
check_muted_state_timer_;
On 2017/06/15 18:44:00, DaleCurtis wrote:
> Drop Optional in favor of just IsRunning().

I was quite sure I was unable to get the timer to work properly as a regular
member: I was hitting a thread check in the destructor. I've not been able to
replicate it now, though, so I'll put up a version which avoids the Optional,
see if one of the bots find anything.

[ossu-chromium, 2017-06-20 12:26:28]

ossu@chromium.org changed reviewers:
+ tommi@chromium.org

[ossu-chromium, 2017-06-20 12:26:29]

Adding tommi@ for (essentially) rubber-stamping
content/browser/speech/speech_recognizer_impl.h - it's only a no-op event
handler implementation added.

[ossu-chromium, 2017-06-20 14:29:28]

The CQ bit was checked by ossu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 14:29:39]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-06-20 17:08:10]

FWIW, I think it'd be fine to just poll this during OnData and PostTask just
like audio power levels. It's a minor detail either way though, so defer to
olka@, maxmorin@ and you. media/ lgtm

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:361: CheckMutedState();
On 2017/06/20 at 12:25:15, ossu-chromium wrote:
> On 2017/06/15 18:44:00, DaleCurtis wrote:
> > Instead of using a repeating timer like this why not just check the muted
state
> > during OnData() and post a task to the handler if it's muted? We already do
that
> > for power level checks.
> 
> From looking at the implementations of IsMuted, they seem to be making calls
that would result in IPC (not Chrome IPC) and waiting for callbacks. I don't
want to put that kind of code in the audio input callback and risk introducing
latency issues. The call already there only happens once every 15 seconds, which
probably isn't often enough for proper feedback.
> 
> Once this chain of CLs has landed, I'd like to have a look at whether these
events could be listened for, instead of polling. If so, I'd be able to get rid
of this timer.

Which implementation are you worried about? I briefly checked the Windows/Mac
ones and they consist of the same type of calls we make elsewhere in the input
drivers.

[commit-bot: I haz the power, 2017-06-20 18:59:12]

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

[commit-bot: I haz the power, 2017-06-20 18:59:14]

Dry run: This issue passed the CQ dry run.

[tommi (sloooow) - chröme, 2017-06-21 11:57:15]

Rs lgtm

[ossu-chromium, 2017-06-21 12:51:47]

On 2017/06/20 17:08:10, DaleCurtis wrote:
> FWIW, I think it'd be fine to just poll this during OnData and PostTask just
> like audio power levels. It's a minor detail either way though, so defer to
> olka@, maxmorin@ and you. media/ lgtm

Hmm, yeah, posting a new task from OnData would get rid of any possible waits
introduced by the IsMuted implementation from the audio callback thread. So on
that respect, the solutions are probably equivalent. I'll keep this as-is for
now and possibly revisit a bit further down the line.

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2919793002/diff/100001/media/audio/audio_inpu...
media/audio/audio_input_controller.cc:361: CheckMutedState();
On 2017/06/20 17:08:09, DaleCurtis wrote:
> On 2017/06/20 at 12:25:15, ossu-chromium wrote:
> > On 2017/06/15 18:44:00, DaleCurtis wrote:
> > > Instead of using a repeating timer like this why not just check the muted
> state
> > > during OnData() and post a task to the handler if it's muted? We already
do
> that
> > > for power level checks.
> > 
> > From looking at the implementations of IsMuted, they seem to be making calls
> that would result in IPC (not Chrome IPC) and waiting for callbacks. I don't
> want to put that kind of code in the audio input callback and risk introducing
> latency issues. The call already there only happens once every 15 seconds,
which
> probably isn't often enough for proper feedback.
> > 
> > Once this chain of CLs has landed, I'd like to have a look at whether these
> events could be listened for, instead of polling. If so, I'd be able to get
rid
> of this timer.
> 
> Which implementation are you worried about? I briefly checked the Windows/Mac
> ones and they consist of the same type of calls we make elsewhere in the input
> drivers.

I'm mostly concerned about the PulseAudio implementation, which does lock the
PulseAudio mainloop, issues an operation and waits for it to complete. I'd guess
that the Mac implementation results in at least one mach ipc call to coreaudiod.
Window seems to be using COM.

It's probably not that much of an issue once every 15 seconds, but I'd like to
avoid it within the callback if at all possible.

[ossu-chromium, 2017-06-21 12:51:56]

The CQ bit was checked by ossu@chromium.org

[ossu-chromium, 2017-06-21 12:51:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
maxmorin@chromium.org, olka@chromium.org, bbudge@chromium.org,
guidou@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2919793002/#ps140001
(title: "Reworked test again, to make tsan2 happy.")

[commit-bot: I haz the power, 2017-06-21 12:52:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-21 12:57:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498049516961250,
"parent_rev": "f81fb7a57311120997cf080cb6349560c2c42270", "commit_rev":
"5b2232658c703564b42eb768cb97c36e385cb7d1"}

[commit-bot: I haz the power, 2017-06-21 12:57:54]

Description was changed from

==========
Detect AudioInputStream muting and propagate to MediaStreamAudioSource.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Detect AudioInputStream muting and propagate to MediaStreamAudioSource.

Also adds a check in WebKit's MediaStreamTrack for if the source's
state is not initially Live.

BUG=chromium:729002
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2919793002
Cr-Commit-Position: refs/heads/master@{#481184}
Committed:
https://chromium.googlesource.com/chromium/src/+/5b2232658c703564b42eb768cb97...
==========

[commit-bot: I haz the power, 2017-06-21 12:57:57]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/5b2232658c703564b42eb768cb97...