Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791203002:
Switching WebRtc private API from AudioManager to AudioSystem (Closed)
DescriptionSwitching WebRtc private API from AudioManager to AudioSystem.
Parent CLs:
* WebRTC Audio private API: removing WebRtcAudioPrivate(Set/Get)ActiveSinkFunction (https://codereview.chromium.org/2784563003/)
* Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one (https://codereview.chromium.org/2763383002/)
BUG=672468
Review-Url: https://codereview.chromium.org/2791203002
Cr-Commit-Position: refs/heads/master@{#466322}
Committed: https://chromium.googlesource.com/chromium/src/+/2629d55c9fa2e2587a25066c6ac2c45bef93b7dc
Patch Set 1 #Patch Set 2 : browser_tests fixed #
Total comments: 18
Patch Set 3 : Review comments addressed #
Total comments: 2
Patch Set 4 : Methods renamed #Patch Set 5 : rebase #Patch Set 6 : rebase #Patch Set 7 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 32 (18 generated)
Description was changed from ========== Switching WebRtc private API from AudioManager to AudioSystem BUG=672468 ========== to ========== Switching WebRtc private API from AudioManager to AudioSystem. Parent CLs: * WebRTC Audio private API: removing WebRtcAudioPrivate(Set/Get)ActiveSinkFunction (https://codereview.chromium.org/2784563003/) * Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one (https://codereview.chromium.org/2763383002/) BUG=672468 ==========
olka@chromium.org changed reviewers: + guidou@chromium.org, rdevlin.cronin@chromium.org
PTAL. It might be a bit confusing to review without a context of parent CLs, if so - could you take a look at threading and thread hops at least?
On 2017/04/03 12:05:08, o1ka wrote: > PTAL. It might be a bit confusing to review without a context of parent CLs, if > so - could you take a look at threading and thread hops at least? Ooops, please hold on - I need to fix tests.
On 2017/04/03 12:12:48, o1ka wrote: > On 2017/04/03 12:05:08, o1ka wrote: > > PTAL. It might be a bit confusing to review without a context of parent CLs, > if > > so - could you take a look at threading and thread hops at least? > > Ooops, please hold on - I need to fix tests. sg; reping when ready. :)
Tests fixed - PTAL now. A question: is there any specific reason for these functions to work on IO thread, except the consideration below? Consideration: AudioSystem can be accessed from both IO and UI threads. When audio moves to a spearate process, communication with it will happen on IO. Accessing AudioSystem on IO will reduce the amount of thread hops. So it makes sense to keep this code working on IO as it is now.
On 2017/04/05 09:13:34, o1ka wrote: > Tests fixed - PTAL now. > > A question: is there any specific reason for these functions to work on IO > thread, except the consideration below? > > Consideration: > AudioSystem can be accessed from both IO and UI threads. When audio moves to a > spearate process, communication with it will happen on IO. Accessing AudioSystem > on IO will reduce the amount of thread hops. So it makes sense to keep this code > working on IO as it is now. I don't think there's any reason beyond that, no. But, I'm not super familiar with this code.
https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (left): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:214: results_ = wap::GetSinks::Results::Create(results); I think this was indeed safe, if you wanted to avoid currying the vector in the callback. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:153: std::unique_ptr<SyncInfoVector> results = base::MakeUnique<SyncInfoVector>(); optional: I sometimes find it cleaner to do: auto results = base::MakeUnique<SyncInfoVector>(); The type can't be anything other than std::unique_ptr<T> using base::MakeUnique<T>, so auto just saves a bit of reading. :) Though maybe moot with below. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:212: for (const auto device : source_devices) { const auto& https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:56: namespace { nit: \n https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:66: [](base::Closure finished_callback, AudioDeviceDescriptions* result, nit: const base::Closure& https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:71: base ::Passed(run_loop.QuitClosure()), device_descriptions), eliminate rogue space between 'base' and '::'
https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:80: using SyncInfoVector = std::vector<api::webrtc_audio_private::SinkInfo>; "Sink"? https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:112: // IO thread: Enumertaes input devices. Enumerates.
Thanks! Addressed the comments. guidou@ - ping? https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (left): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:214: results_ = wap::GetSinks::Results::Create(results); On 2017/04/06 01:42:33, Devlin wrote: > I think this was indeed safe, if you wanted to avoid currying the vector in the > callback. Yes, it was safe. I just prefer the code which does not require 8-line comment explaining its safety (wherever it's possible and reasonable) :) Also, such code is more future-proof. I don't have a very strong opinion in this case though. Do you prefer me to revert it? https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:153: std::unique_ptr<SyncInfoVector> results = base::MakeUnique<SyncInfoVector>(); On 2017/04/06 01:42:33, Devlin wrote: > optional: I sometimes find it cleaner to do: > auto results = base::MakeUnique<SyncInfoVector>(); > > The type can't be anything other than std::unique_ptr<T> using > base::MakeUnique<T>, so auto just saves a bit of reading. :) > > Though maybe moot with below. Makes sense! Done. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:212: for (const auto device : source_devices) { On 2017/04/06 01:42:33, Devlin wrote: > const auto& Done. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:80: using SyncInfoVector = std::vector<api::webrtc_audio_private::SinkInfo>; On 2017/04/06 07:41:20, Max Morin wrote: > "Sink"? Done. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:112: // IO thread: Enumertaes input devices. On 2017/04/06 07:41:20, Max Morin wrote: > Enumerates. Done. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc (right): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:56: namespace { On 2017/04/06 01:42:33, Devlin wrote: > nit: \n Done. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:66: [](base::Closure finished_callback, AudioDeviceDescriptions* result, On 2017/04/06 01:42:33, Devlin wrote: > nit: const base::Closure& It takes the ownership. base::Closure is movable and is moved in below (l.71) https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_browsertest.cc:71: base ::Passed(run_loop.QuitClosure()), device_descriptions), On 2017/04/06 01:42:33, Devlin wrote: > eliminate rogue space between 'base' and '::' Done.
Nice refactoring! https://codereview.chromium.org/2791203002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (right): https://codereview.chromium.org/2791203002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:231: CalculateHMACAndReplyOnUIThread(const std::string& raw_sink_id) { Normally the OnXXThread prefix is used to indicate the thread where the function runs on. This one is called OnUIThread, but runs on IO thread. Please rename.
lgtm % nit https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (left): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:214: results_ = wap::GetSinks::Results::Create(results); On 2017/04/06 11:01:22, o1ka wrote: > On 2017/04/06 01:42:33, Devlin wrote: > > I think this was indeed safe, if you wanted to avoid currying the vector in > the > > callback. > > Yes, it was safe. I just prefer the code which does not require 8-line comment > explaining its safety (wherever it's possible and reasonable) :) Also, such code > is more future-proof. > I don't have a very strong opinion in this case though. > Do you prefer me to revert it? I don't have a strong preference. The only reason to prefer this would be because it lets us allocate the vector<SinkInfo> on the stack instead of the heap, which would be a bit faster. But this shouldn't be very performance-sensitive code (and already does a ton), so this should be fine. (And, as you say, not needing an 8 line comment is its own reward. :)) https://codereview.chromium.org/2791203002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h (right): https://codereview.chromium.org/2791203002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.h:113: void GetInputDeviceDescriptions(); As Guido mentioned, we often have the OnFooThread suffix here. That would obviate the need for the 'IO thread:' comments (so this would just become GetInputDeviceDescriptionsOnIOThread())
The CQ bit was checked by olka@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.
Thanks! Methods renamed - PTAL. https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc (left): https://codereview.chromium.org/2791203002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/webrtc_audio_private/webrtc_audio_private_api.cc:214: results_ = wap::GetSinks::Results::Create(results); On 2017/04/06 21:23:27, Devlin wrote: > On 2017/04/06 11:01:22, o1ka wrote: > > On 2017/04/06 01:42:33, Devlin wrote: > > > I think this was indeed safe, if you wanted to avoid currying the vector in > > the > > > callback. > > > > Yes, it was safe. I just prefer the code which does not require 8-line comment > > explaining its safety (wherever it's possible and reasonable) :) Also, such > code > > is more future-proof. > > I don't have a very strong opinion in this case though. > > Do you prefer me to revert it? > > I don't have a strong preference. The only reason to prefer this would be > because it lets us allocate the vector<SinkInfo> on the stack instead of the > heap, which would be a bit faster. But this shouldn't be very > performance-sensitive code (and already does a ton), so this should be fine. > (And, as you say, not needing an 8 line comment is its own reward. :)) Acknowledged.
lgtm
The CQ bit was checked by olka@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_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by olka@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 olka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, guidou@chromium.org Link to the patchset: https://codereview.chromium.org/2791203002/#ps120001 (title: "rebase")
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": 120001, "attempt_start_ts": 1492782240457380,
"parent_rev": "8041d6145bbd1f92ff8d5dea5264be9e59d8fc3e", "commit_rev":
"2629d55c9fa2e2587a25066c6ac2c45bef93b7dc"}
Message was sent while issue was closed.
Description was changed from ========== Switching WebRtc private API from AudioManager to AudioSystem. Parent CLs: * WebRTC Audio private API: removing WebRtcAudioPrivate(Set/Get)ActiveSinkFunction (https://codereview.chromium.org/2784563003/) * Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one (https://codereview.chromium.org/2763383002/) BUG=672468 ========== to ========== Switching WebRtc private API from AudioManager to AudioSystem. Parent CLs: * WebRTC Audio private API: removing WebRtcAudioPrivate(Set/Get)ActiveSinkFunction (https://codereview.chromium.org/2784563003/) * Switching AudioInputDeviceManager from using AudioManager interface to AudioSystem one (https://codereview.chromium.org/2763383002/) BUG=672468 Review-Url: https://codereview.chromium.org/2791203002 Cr-Commit-Position: refs/heads/master@{#466322} Committed: https://chromium.googlesource.com/chromium/src/+/2629d55c9fa2e2587a25066c6ac2... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2629d55c9fa2e2587a25066c6ac2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
