Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2922733002:
Propagate muted state from MediaStreamAudioSource into JS. (Closed)
|
Created:
3 years, 6 months ago by ossu-chromium Modified:
3 years, 5 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, imcheng+watch_chromium.org, feature-media-reviews_chromium.org, posciak+watch_chromium.org, chfremer+watch_chromium.org, blink-reviews, jam, avayvod+watch_chromium.org, tommyw+watchlist_chromium.org, darin-cc_chromium.org, jasonroberts+watch_google.com, haraken, xjz+watch_chromium.org, mfoltz+watch_chromium.org, isheriff+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPropagate muted state from MediaStreamAudioSource into JS.
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/2922733002
Cr-Commit-Position: refs/heads/master@{#482609}
Committed: https://chromium.googlesource.com/chromium/src/+/d998fd8a158eab8a17162049277c1f37aab1cf42
Patch Set 1 #Patch Set 2 : Initial stab at getting mute to not affect visible readyState. #Patch Set 3 : Added browser tests for AudioInputStream::IsMuted propagating to JS. #
Total comments: 1
Patch Set 4 : Updated WebKit MediaStreamTrack layout test to not expect readyState=muted #
Total comments: 4
Patch Set 5 : Also updated MediaStreamTrack-expected.txt. #
Total comments: 1
Patch Set 6 : SetMutedState -> SetGlobalMutedState. Also rebased. #Patch Set 7 : Rebase: FakeAudioInputStream changes moved into earlier CL #
Total comments: 7
Patch Set 8 : Rebase #Patch Set 9 : Made SetMuted call unconditional. Rebased. #Messages
Total messages: 50 (38 generated)
ossu@chromium.org changed reviewers: + guidou@chromium.org
Did a minimal, viable change to not have MediaStreamTrack.readyState change to "muted" in JS. Ideally, the ready states we use in blink shouldn't contain muted at all, but I think that's a farther-reaching change. I'd be willing to do that, though, if that's the way we want to go. I'll also happily add a test for this, but I'm not quite sure where to start. I'll look into it next week.
The change looks good to me. Will wait for the test.
Description was changed from ========== Propagate muted state from MediaStreamAudioSource into JS. BUG=chromium:729002 ========== to ========== Propagate muted state from MediaStreamAudioSource into JS. 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 ==========
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Tests added. Put them in with the WebRTC getUserMedia browser tests. It seemed like the best fit. https://codereview.chromium.org/2922733002/diff/40001/media/audio/fake_audio_... File media/audio/fake_audio_input_stream.h (right): https://codereview.chromium.org/2922733002/diff/40001/media/audio/fake_audio_... media/audio/fake_audio_input_stream.h:59: static void SetMutedState(bool is_muted); Initially, I thought having SetMutedState be static was a bit hacky. Considering we want to be able to set the muted state _before_ any stream is constructed, to check initial conditions, I now think it's as good as can be.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Looks good in general. I have a couple of nits and a question. Also, it looks like your change breaks the MediaStreamTrack.html LayoutTest, but it should be easy to fix. https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_source.cc (right): https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we only go between Muted and Live, not from Ended. nit: Add a crbug.com reference? https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... File media/audio/fake_audio_input_stream.h (right): https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... media/audio/fake_audio_input_stream.h:59: static void SetMutedState(bool is_muted); nit: Change the name to SetGlobalMutedState or similar to make intent clearer. You could also have the global variable represent the initial/default muted state, and provide a nonstatic SetMutedState method to change it per stream. This would allow future code to manupulate the muted state of different streams independently. WDYT?
On 2017/06/08 14:15:06, Guido Urdaneta wrote: > Looks good in general. I have a couple of nits and a question. > Also, it looks like your change breaks the MediaStreamTrack.html LayoutTest, but > it should be easy to fix. Yep! Seems I updated the test, but not the expected outcome of the test, so it failed slightly differently. Running another test on a new patch set now to try it. Will address the other issues separately. https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... File content/renderer/media/media_stream_source.cc (right): https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... 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/08 14:15:06, Guido Urdaneta wrote: > nit: Add a http://crbug.com reference? Naah, I think I should just fix it before submitting. Since downstream is handling it properly, I think I should either point that out in the comment, or do a check of Owner().GetReadyState() just in case. https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... File media/audio/fake_audio_input_stream.h (right): https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... media/audio/fake_audio_input_stream.h:59: static void SetMutedState(bool is_muted); On 2017/06/08 14:15:06, Guido Urdaneta wrote: > nit: Change the name to SetGlobalMutedState or similar to make intent clearer. Yes, that makes sense. > You could also have the global variable represent the initial/default muted > state, and provide a nonstatic SetMutedState method to change it per stream. > This would allow future code to manupulate the muted state of different streams > independently. WDYT? I could do that, but I haven't the first clue on how to get the AudioInputStream for a specific Track or something in JavaScript. The current usage also vibes quite well with how this is implemented in real life, where there's a global volume/mute setting for the microphone, for example. So.. I'd rather avoid doing that here unless there's pressing need for that functionality. :)
On 2017/06/08 14:52:08, ossu-chromium wrote: > On 2017/06/08 14:15:06, Guido Urdaneta wrote: > > Looks good in general. I have a couple of nits and a question. > > Also, it looks like your change breaks the MediaStreamTrack.html LayoutTest, > but > > it should be easy to fix. > > Yep! Seems I updated the test, but not the expected outcome of the test, so it > failed slightly differently. Running another test on a new patch set now to try > it. Will address the other issues separately. > > https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... > File content/renderer/media/media_stream_source.cc (right): > > https://codereview.chromium.org/2922733002/diff/60001/content/renderer/media/... > 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/08 14:15:06, Guido Urdaneta wrote: > > nit: Add a http://crbug.com reference? > > Naah, I think I should just fix it before submitting. Since downstream is > handling it properly, I think I should either point that out in the comment, or > do a check of Owner().GetReadyState() just in case. > > https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... > File media/audio/fake_audio_input_stream.h (right): > > https://codereview.chromium.org/2922733002/diff/60001/media/audio/fake_audio_... > media/audio/fake_audio_input_stream.h:59: static void SetMutedState(bool > is_muted); > On 2017/06/08 14:15:06, Guido Urdaneta wrote: > > nit: Change the name to SetGlobalMutedState or similar to make intent clearer. > > Yes, that makes sense. > > > You could also have the global variable represent the initial/default muted > > state, and provide a nonstatic SetMutedState method to change it per stream. > > This would allow future code to manupulate the muted state of different > streams > > independently. WDYT? > > I could do that, but I haven't the first clue on how to get the AudioInputStream > for a specific Track or something in JavaScript. The current usage also vibes > quite well with how this is implemented in real life, where there's a global > volume/mute setting for the microphone, for example. So.. I'd rather avoid doing > that here unless there's pressing need for that functionality. :) SGTM. Let's wait until we actually need it before that. lgtm % renaming of SetMutedState.
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Propagate muted state from MediaStreamAudioSource into JS. 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 ========== Propagate muted state from MediaStreamAudioSource into JS. 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.org changed reviewers: + miu@chromium.org
Hi miu,
Could you have a look, especially on the media_stream{,_audio}_source stuff? I
also believe you're an owner for the rest of the files I'm missing owner lgtm
on. :)
https://codereview.chromium.org/2922733002/diff/80001/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.cc (right):
https://codereview.chromium.org/2922733002/diff/80001/content/renderer/media/...
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?
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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_...)
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
BTW--I'm not an owner for the blink code. You'll have to find another reviewer for that. :) Comments on PS7: https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... File content/renderer/media/media_stream_source.cc (right): https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we only go between Muted and Live, not from Ended. Instead of this TODO, why not: DCHECK_NE(readyState(), kReadyStateEnded); https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:77: if (ready_state_ == MediaStreamSource::kReadyStateMuted) This code assumes MediaStreamComponent::muted_ is always false at this point. However, this is not guaranteed (see the MSC ctor). Suggest making this an unconditional call: component_->SetMuted(ready_state_ == MediaStreamSource::kReadyStateMuted); https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:209: ready_state_ = MediaStreamSource::kReadyStateEnded; Assigning to |ready_state_| here could be dangerous: What prevents a future call to SourceChangedState() from changing it to something else?
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
Patchset #9 (id:180001) has been deleted
On 2017/06/19 19:40:38, miu wrote: > BTW--I'm not an owner for the blink code. You'll have to find another reviewer > for that. :) Alright. It looks like guidou@ has that covered. :) https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... File content/renderer/media/media_stream_source.cc (right): https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... 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/19 19:40:37, miu wrote: > Instead of this TODO, why not: > > DCHECK_NE(readyState(), kReadyStateEnded); > I considered that but am a bit worried that we may hit such a DCHECK if the muted state changes just as the stream ends. The blink code seems to already discard any readyState changes if it's already at ended. I'm considering just making a note of that in a comment here, instead. https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp (right): https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:77: if (ready_state_ == MediaStreamSource::kReadyStateMuted) On 2017/06/19 19:40:37, miu wrote: > This code assumes MediaStreamComponent::muted_ is always false at this point. > However, this is not guaranteed (see the MSC ctor). > > Suggest making this an unconditional call: > > component_->SetMuted(ready_state_ == MediaStreamSource::kReadyStateMuted); From what I could gather, that constructor is (un?)fortunately never called with muted set to anything but false. However, I think your suggestion makes a lot of sense. Will change! https://codereview.chromium.org/2922733002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaStreamTrack.cpp:209: ready_state_ = MediaStreamSource::kReadyStateEnded; On 2017/06/19 19:40:37, miu wrote: > Assigning to |ready_state_| here could be dangerous: What prevents a future call > to SourceChangedState() from changing it to something else? There's an if (Ended()) clause in SourceChangedState as well, which would stop any future changes once we get to ended.
The CQ bit was checked by ossu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PS9 lgtm. https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... File content/renderer/media/media_stream_source.cc (right): https://codereview.chromium.org/2922733002/diff/140001/content/renderer/media... content/renderer/media/media_stream_source.cc:31: // TODO(ossu): Check that we only go between Muted and Live, not from Ended. Nice! No TODO here anymore. :)
The CQ bit was checked by ossu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org Link to the patchset: https://codereview.chromium.org/2922733002/#ps200001 (title: "Made SetMuted call unconditional. Rebased.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1498568210903030,
"parent_rev": "d700da7019ad568e243d9d451a3ba2513d448a5d", "commit_rev":
"d998fd8a158eab8a17162049277c1f37aab1cf42"}
Message was sent while issue was closed.
Description was changed from ========== Propagate muted state from MediaStreamAudioSource into JS. 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 ========== Propagate muted state from MediaStreamAudioSource into JS. 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/2922733002 Cr-Commit-Position: refs/heads/master@{#482609} Committed: https://chromium.googlesource.com/chromium/src/+/d998fd8a158eab8a17162049277c... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/d998fd8a158eab8a17162049277c... |
