Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2941563002:
Enable new getUserMedia audio constraints algorithm behind a flag. (Closed)
|
Created:
3 years, 6 months ago by Guido Urdaneta Modified:
3 years, 5 months ago CC:
avayvod+watch_chromium.org, chfremer+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, extensions-reviews_chromium.org, feature-media-reviews_chromium.org, imcheng+watch_chromium.org, isheriff+watch_chromium.org, jam, jasonroberts+watch_google.com, mfoltz+watch_chromium.org, miu+watch_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, xjz+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable new getUserMedia audio constraints algorithm behind a flag.
BUG=657733
Review-Url: https://codereview.chromium.org/2941563002
Cr-Commit-Position: refs/heads/master@{#482273}
Committed: https://chromium.googlesource.com/chromium/src/+/860dd9ed86a08dbdd234f0d89f6e53a079a683b3
Patch Set 1 #
Total comments: 35
Patch Set 2 : address hbos comments #
Total comments: 11
Patch Set 3 : address miu@'s comments #
Total comments: 6
Patch Set 4 : address Devlin's comments #
Total comments: 2
Patch Set 5 : remove vertical space from DEPS #Depends on Patchset: Messages
Total messages: 66 (41 generated)
The CQ bit was checked by guidou@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.
Description was changed from ========== Enable new getUserMedia audio constraints algorithm behind a flag. BUG=657733 ========== to ========== Enable new getUserMedia audio constraints algorithm behind a flag. BUG=657733 ==========
guidou@chromium.org changed reviewers: + hbos@chromium.org, miu@chromium.org
Hi, PTAL
https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:86: properties->goog_noise_suppression = false; nit: disabled twice https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:508: DisableDefaultAudioProcessingProperties(&properties); Before, only echo cancellation was disabled and mirroring enabled. Now all of them are except mirroring, why? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:570: AudioProcessingProperties properties; goog_experimental_noise_suppression is not set to true and the test still passes, why? Does its value not matter or is there an assumption about the default being true? Can you set it anyway or DCHECK that it's true? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:556: settings.device_id(); What about ->audio.stream_source? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:595: return; Was this a drive-by fix? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:168: GetAudioInputCapabilitiesCallback client_callback) override { Can you do if old constraints NOTREACHED()? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:174: std::vector<::mojom::AudioInputDeviceCapabilitiesPtr> result; nit: Can you move this line to the top so that the pattern of doing "device = ...; device_id = ...; parameters = ...; result.push_back(...);" holds without a vector in the middle the first time? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:187: std::move(client_callback).Run(std::move(result)); Why are you moving before invoking Run? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:844: TEST_P(UserMediaClientImplTest, DefaultConstraintsPropagate) { Is it possible to get rid of the assumption about what the default values are? Like instantiate a default struct and copy the values, or use the inverse of them in the tests where non-default is desired. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:907: EXPECT_EQ(track_settings.max_frame_rate, 0.0); Can you document this at ::max_frame_rate unless already documented? Perhaps with a TODO to make it Optional. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source.cc (left): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source.cc:98: if (!audio_constraints.IsValid()) { Is the concept of "not valid" not applicable to properties? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (left): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:211: TEST_F(ProcessedLocalAudioSourceTest, FailToStartWithWrongConstraints) { Why is this test removed? If invalid constraints are ignored would the equivalent test be the same as unconstrained? Should't that succeed? https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:15: #include "content/renderer/media/mock_constraint_factory.h" nit: Here and perhaps other files: remove inclusion of mock_constraint_factory.h https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); Why is this needed if it wasn't before?
https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:86: properties->goog_noise_suppression = false; On 2017/06/16 17:27:14, hbos_chromium wrote: > nit: disabled twice Fixed. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:508: DisableDefaultAudioProcessingProperties(&properties); On 2017/06/16 17:27:14, hbos_chromium wrote: > Before, only echo cancellation was disabled and mirroring enabled. Now all of > them are except mirroring, why? The old test was disabling all audio-processing constraints via the old constraints algorithm by setting the echoCancellation constraint to false. Similarly, the old test was using constraints to enable audio mirroring. The new version of the test does not use constraints, but sets the values of properties explicitly since the MSAP no longer performs constraints processing. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:570: AudioProcessingProperties properties; On 2017/06/16 17:27:14, hbos_chromium wrote: > goog_experimental_noise_suppression is not set to true and the test still > passes, why? Does its value not matter or is there an assumption about the > default being true? Can you set it anyway or DCHECK that it's true? The default value is true on all platforms, so it's not necessary to set it to true. I guess the old test was originally written when the default value was false, or tried to make sure that it's true even if the default changes. I think relying on the default true value is better than explicitly setting since the test would fail if the default changes, which is what applications would do. This would bring the attention of the people making the change as to the consequences in the wild. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:556: settings.device_id(); On 2017/06/16 17:27:14, hbos_chromium wrote: > What about ->audio.stream_source? It's set by InitializeTrackControls(). https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:595: return; On 2017/06/16 17:27:14, hbos_chromium wrote: > Was this a drive-by fix? Yes. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:168: GetAudioInputCapabilitiesCallback client_callback) override { On 2017/06/16 17:27:14, hbos_chromium wrote: > Can you do if old constraints NOTREACHED()? Done. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:174: std::vector<::mojom::AudioInputDeviceCapabilitiesPtr> result; On 2017/06/16 17:27:14, hbos_chromium wrote: > nit: Can you move this line to the top so that the pattern of doing "device = > ...; device_id = ...; parameters = ...; result.push_back(...);" holds without a > vector in the middle the first time? Done. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:187: std::move(client_callback).Run(std::move(result)); On 2017/06/16 17:27:14, hbos_chromium wrote: > Why are you moving before invoking Run? It's a OnceCallback, so it can only be invoked on an rvalue reference. The idea is that since you moved it you cannot call it again, hence OnceCallback. Otherwise it doesn't compile. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:844: TEST_P(UserMediaClientImplTest, DefaultConstraintsPropagate) { On 2017/06/16 17:27:14, hbos_chromium wrote: > Is it possible to get rid of the assumption about what the default values are? > Like instantiate a default struct and copy the values, or use the inverse of > them in the tests where non-default is desired. Since this is testing default values, I think it's OK to leave it as it is. This way, the test will break if the default values change. That will gives a signal to the developer changing the defaults that stuff might break. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:907: EXPECT_EQ(track_settings.max_frame_rate, 0.0); On 2017/06/16 17:27:14, hbos_chromium wrote: > Can you document this at ::max_frame_rate unless already documented? Perhaps > with a TODO to make it Optional. Added comment in VideoAdapterSettings and filed http://crbug.com/734528. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source.cc (left): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source.cc:98: if (!audio_constraints.IsValid()) { On 2017/06/16 17:27:14, hbos_chromium wrote: > Is the concept of "not valid" not applicable to properties? No, it's not. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (left): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:211: TEST_F(ProcessedLocalAudioSourceTest, FailToStartWithWrongConstraints) { On 2017/06/16 17:27:14, hbos_chromium wrote: > Why is this test removed? If invalid constraints are ignored would the > equivalent test be the same as unconstrained? Should't that succeed? The concept of "wrong" constraints does not exist in the new algorithm the way it existed with the old one. If you set a width with the new algorithm it will be the same as unconstrained, so the corresponding test would be the same as the previous one (using default values for AudioProcessingProperties). The spec says that unsupported constraints should be ignored. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/webr... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:15: #include "content/renderer/media/mock_constraint_factory.h" On 2017/06/16 17:27:14, hbos_chromium wrote: > nit: Here and perhaps other files: remove inclusion of mock_constraint_factory.h Done. https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/16 17:27:14, hbos_chromium wrote: > Why is this needed if it wasn't before? Because the old code first asked for permission and then did constraints processing. The new code does constraints processing first and, if there are no devices, getUserMedia fails without asking for permission, as the spec says it should. This test relies on getUserMedia always asking for permission, which can be guaranteed with fake devices. Many bots have no real devices, so the test would fail there otherwise.
The CQ bit was checked by guidou@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.
lgtm https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... File content/renderer/media/media_stream_audio_processor_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:508: DisableDefaultAudioProcessingProperties(&properties); On 2017/06/19 11:56:48, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > Before, only echo cancellation was disabled and mirroring enabled. Now all of > > them are except mirroring, why? > > The old test was disabling all audio-processing constraints via the old > constraints algorithm by setting the echoCancellation constraint to false. > Similarly, the old test was using constraints to enable audio mirroring. > > The new version of the test does not use constraints, but sets the values of > properties explicitly since the MSAP no longer performs constraints processing. Acknowledged. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/medi... content/renderer/media/media_stream_audio_processor_unittest.cc:570: AudioProcessingProperties properties; On 2017/06/19 11:56:48, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > goog_experimental_noise_suppression is not set to true and the test still > > passes, why? Does its value not matter or is there an assumption about the > > default being true? Can you set it anyway or DCHECK that it's true? > > The default value is true on all platforms, so it's not necessary to set it to > true. > I guess the old test was originally written when the default value was false, or > tried to make sure that it's true even if the default changes. > I think relying on the default true value is better than explicitly setting > since the test would fail if the default changes, which is what applications > would do. This would bring the attention of the people making the change as to > the consequences in the wild. Acknowledged. I'd still like a comment and/or DCHECK so that the variable relevant to the test is not completely absent. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl.cc:556: settings.device_id(); On 2017/06/19 11:56:48, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > What about ->audio.stream_source? > > It's set by InitializeTrackControls(). Acknowledged. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:187: std::move(client_callback).Run(std::move(result)); On 2017/06/19 11:56:49, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > Why are you moving before invoking Run? > > It's a OnceCallback, so it can only be invoked on an rvalue reference. > The idea is that since you moved it you cannot call it again, hence > OnceCallback. > Otherwise it doesn't compile. Acknowledged. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:844: TEST_P(UserMediaClientImplTest, DefaultConstraintsPropagate) { On 2017/06/19 11:56:48, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > Is it possible to get rid of the assumption about what the default values are? > > Like instantiate a default struct and copy the values, or use the inverse of > > them in the tests where non-default is desired. > > Since this is testing default values, I think it's OK to leave it as it is. This > way, the test will break if the default values change. That will gives a signal > to the developer changing the defaults that stuff might break. Acknowledged. https://codereview.chromium.org/2941563002/diff/1/content/renderer/media/user... content/renderer/media/user_media_client_impl_unittest.cc:907: EXPECT_EQ(track_settings.max_frame_rate, 0.0); On 2017/06/19 11:56:48, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > Can you document this at ::max_frame_rate unless already documented? Perhaps > > with a TODO to make it Optional. > > Added comment in VideoAdapterSettings and filed http://crbug.com/734528. Acknowledged. https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/1/extensions/browser/guest_vi... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/19 11:56:49, Guido Urdaneta wrote: > On 2017/06/16 17:27:14, hbos_chromium wrote: > > Why is this needed if it wasn't before? > > Because the old code first asked for permission and then did constraints > processing. > The new code does constraints processing first and, if there are no devices, > getUserMedia fails without asking for permission, as the spec says it should. > This test relies on getUserMedia always asking for permission, which can be > guaranteed with fake devices. > Many bots have no real devices, so the test would fail there otherwise. Acknowledged.
Looks great. Mostly minor things. Comments on PS2: https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:506: if (!IsCurrentRequestInfo(user_media_request)) Can you write a comment here to clarify why these might not match at this point? Is the early return is safe (instead of some error-handling or logging)? Or, would DCHECK(IsCurrentRequestInfo(umr)) be more appropriate? https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:537: if (!IsCurrentRequestInfo(user_media_request)) ditto here https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:566: SetupVideoInput(user_media_request); Can this be moved to MaybeProcessNextRequestInfo()? It's a bit surprising for a SelectAudioSettings() method to call a SetupVideoInput() method. ;-) https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.h (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.h:129: const AudioCaptureSettings& AudioCaptureSettingsForCurrentRequest() const; Couple suggestions: 1. naming/style: Test-only methods should have the "XXXForTesting()" naming pattern. This allows presubmit scripts to warn developers when future code changes are made. 2. These methods both just call into |current_request_info_|. So, perhaps it'd be simpler to just provide one accessor here for that: UserMediaRequestInfo* current_request_info_for_testing() const { return current_request_info_.get(); } ...and then the helper you have in the test code would: auto* const request_info = user_media_client->current_request_info_for_testing(); ASSERT_TRUE(request_info); return request_info->audio_settings(); https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl_unittest.cc:376: if (GetParam()) { Great idea! :) https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:75: void DisableDefaultAudioProcessingProperties( Looks like this same code is in two places. I'd suggest making it a method in AudioProcessingProperties and let it be optimized-out (i.e., removed by the linker) in release builds.
The CQ bit was checked by guidou@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.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by guidou@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...
https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.cc (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:506: if (!IsCurrentRequestInfo(user_media_request)) On 2017/06/19 21:18:44, miu wrote: > Can you write a comment here to clarify why these might not match at this point? > Is the early return is safe (instead of some error-handling or logging)? Or, > would DCHECK(IsCurrentRequestInfo(umr)) be more appropriate? Nice catch. This function is always called synchronously while |current_request_info_| is being processed, so it shouldn't even take a WebUserMediaRequest parameter. Applied the same fix to SetupVideoInput and GetUserMediaRequestFailed. There are some other functions that are called asynchronously, in which case the check is necessary because the request might have been canceled. https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:537: if (!IsCurrentRequestInfo(user_media_request)) On 2017/06/19 21:18:44, miu wrote: > ditto here In this case the check is necessary. Added comment (here and in other places) explaining why the check is necessary. https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.cc:566: SetupVideoInput(user_media_request); On 2017/06/19 21:18:44, miu wrote: > Can this be moved to MaybeProcessNextRequestInfo()? It's a bit surprising for a > SelectAudioSettings() method to call a SetupVideoInput() method. ;-) The plan is to do it after we remove the old constraints processing code, so that the code is simpler. Right now we first set up audio and when we're done we go for video. The plan is to do it in parallel. There is a TODO on line 431 about this. https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/user_media_client_impl.h (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/user_media_client_impl.h:129: const AudioCaptureSettings& AudioCaptureSettingsForCurrentRequest() const; On 2017/06/19 21:18:44, miu wrote: > Couple suggestions: > > 1. naming/style: Test-only methods should have the "XXXForTesting()" naming > pattern. This allows presubmit scripts to warn developers when future code > changes are made. > > 2. These methods both just call into |current_request_info_|. So, perhaps it'd > be simpler to just provide one accessor here for that: > > UserMediaRequestInfo* current_request_info_for_testing() const { > return current_request_info_.get(); > } > > ...and then the helper you have in the test code would: > > auto* const request_info = > user_media_client->current_request_info_for_testing(); > ASSERT_TRUE(request_info); > return request_info->audio_settings(); Did not expose |current_request_info_| because its type is internal to user_media_client_impl.cc. I see little value in making that type public just for a unit test. Instead renamed the methods to XXXForTesting() as per your suggestion #1. https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... File content/renderer/media/webrtc/processed_local_audio_source_unittest.cc (right): https://codereview.chromium.org/2941563002/diff/20001/content/renderer/media/... content/renderer/media/webrtc/processed_local_audio_source_unittest.cc:75: void DisableDefaultAudioProcessingProperties( On 2017/06/19 21:18:44, miu wrote: > Looks like this same code is in two places. I'd suggest making it a method in > AudioProcessingProperties and let it be optimized-out (i.e., removed by the > linker) in release builds. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PS3 lgtm.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org Link to the patchset: https://codereview.chromium.org/2941563002/#ps60001 (title: "address miu@'s comments")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
guidou@chromium.org changed reviewers: + jochen@chromium.org, rdevlin.cronin@chromium.org
rdevlin.cronin@chromium.org: Please review changes in extensions/ jochen@chromium.org: Please review changes in content/public/common
https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); nit: maybe add a comment explaining why this is necessary? https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); Can we use the kUseFakeDeviceForMediaStream constant?
https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/21 13:14:33, Devlin wrote: > Can we use the kUseFakeDeviceForMediaStream constant? No, due to layering rules. https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/21 13:14:33, Devlin wrote: > nit: maybe add a comment explaining why this is necessary? will do in the next patchset.
https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/21 14:54:56, Guido Urdaneta wrote: > On 2017/06/21 13:14:33, Devlin wrote: > > Can we use the kUseFakeDeviceForMediaStream constant? > > No, due to layering rules. Using the value of a constant doesn't make it less of a layering violation. :) But since this is just a test, I'd be fine with adding a DEPS file to extensions/browser/guest_view/web_view with specific_include_rules = { ".*(test)\.(cc|h)$": [ "+media", # Tests need to be able to tweak media devices. ] }
https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... File extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc (right): https://codereview.chromium.org/2941563002/diff/60001/extensions/browser/gues... extensions/browser/guest_view/web_view/web_view_media_access_apitest.cc:87: command_line->AppendSwitch("use-fake-device-for-media-stream"); On 2017/06/21 19:21:26, Devlin wrote: > On 2017/06/21 14:54:56, Guido Urdaneta wrote: > > On 2017/06/21 13:14:33, Devlin wrote: > > > Can we use the kUseFakeDeviceForMediaStream constant? > > > > No, due to layering rules. > > Using the value of a constant doesn't make it less of a layering violation. :) > But since this is just a test, I'd be fine with adding a DEPS file to > extensions/browser/guest_view/web_view with > > specific_include_rules = { > ".*(test)\.(cc|h)$": [ > "+media", # Tests need to be able to tweak media devices. > ] > } Done. I thought using the string directly was OK too since that string is a public API available to users outside the source code. Didn't know about specific_include_rules, though :)
The CQ bit was checked by guidou@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.
extensions lgtm https://codereview.chromium.org/2941563002/diff/80001/extensions/browser/gues... File extensions/browser/guest_view/web_view/DEPS (right): https://codereview.chromium.org/2941563002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/web_view/DEPS:6: nit: remove this extra newline
https://codereview.chromium.org/2941563002/diff/80001/extensions/browser/gues... File extensions/browser/guest_view/web_view/DEPS (right): https://codereview.chromium.org/2941563002/diff/80001/extensions/browser/gues... extensions/browser/guest_view/web_view/DEPS:6: On 2017/06/22 13:14:20, Devlin wrote: > nit: remove this extra newline Done.
The CQ bit was checked by guidou@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...
guidou@chromium.org changed reviewers: - jochen@chromium.org
guidou@chromium.org changed reviewers: + avi@chromium.org
avi@chromium.org: can you take a look at content/public/common/content_features.* ?
lgtm
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 guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org, miu@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2941563002/#ps100001 (title: "remove vertical space from DEPS")
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
guidou@chromium.org changed reviewers: + dalecurtis@chromium.org
dalecurtis@: can you rs the dependency to media added for a test file?
guidou@chromium.org changed reviewers: + jochen@chromium.org - dalecurtis@chromium.org
jochen@: can you rs?
content/public/common lgtm
The CQ bit was checked by guidou@chromium.org
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": 100001, "attempt_start_ts": 1498484585123090,
"parent_rev": "acdc0882b00a4548e152cf43e36855b0b712a540", "commit_rev":
"860dd9ed86a08dbdd234f0d89f6e53a079a683b3"}
Message was sent while issue was closed.
Description was changed from ========== Enable new getUserMedia audio constraints algorithm behind a flag. BUG=657733 ========== to ========== Enable new getUserMedia audio constraints algorithm behind a flag. BUG=657733 Review-Url: https://codereview.chromium.org/2941563002 Cr-Commit-Position: refs/heads/master@{#482273} Committed: https://chromium.googlesource.com/chromium/src/+/860dd9ed86a08dbdd234f0d89f6e... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/chromium/src/+/860dd9ed86a08dbdd234f0d89f6e... |
