Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163473002:
[Chromoting] Audio playback won't work after switching default playback device (Closed)
Description[Chromoting] Audio playback won't work after switching default playback device
We can only capture audio for the default device now. So we need to update audio
components after switching a default playback device in Windows.
BUG=628814
Committed: https://crrev.com/b40182aceb9f9ab588a296ace041360fa40bac88
Cr-Commit-Position: refs/heads/master@{#407390}
Patch Set 1 #
Total comments: 24
Patch Set 2 : Resolve review comments #
Total comments: 7
Patch Set 3 : Resolve review comments #Patch Set 4 : Resolve review comments #
Total comments: 9
Patch Set 5 : Resolve review comments #Patch Set 6 : Resolve review comments #
Total comments: 4
Patch Set 7 : Resolve review comments #
Total comments: 2
Patch Set 8 : Resolve review comments #
Messages
Total messages: 62 (44 generated)
The CQ bit was checked by zijiehe@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...
Description was changed from ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 ========== to ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 ==========
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, sergeyu@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:43: class AudioCapturerWin::MMNotificationClient : public IMMNotificationClient { Do we need this as a separate class? Maybe just implement IMMNotificationClient interface in AudioCapturerWin? https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:55: return S_OK; } here and below: formatting https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( Which thread is this called on? https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:77: EDataFlow flow, ERole role, LPCWSTR pwstrDefaultDevice) { One argument per line please git cl format https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:96: notificator_(new MMNotificationClient(this)), I don't think that |notificator_| is the right name for this variable. Call it mm_notification_client_ https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:115: bool AudioCapturerWin::Initialize() { Call this ResetAndInitialize() as it doesn't just initialize. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:131: hr = mm_device_enumerator_->RegisterEndpointNotificationCallback( Do we also need to unregister the callback when calling mm_device_enumerator_.Release() above or when the capturer is deleted? https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:277: bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) { Move this before initialize, so the order of methods matches the order in the header. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:288: capture_timer_.reset(new base::RepeatingTimer()); nit: move this below, next to the capture_timer_->Start() call. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.h:76: bool default_audio_device_changed_; = false;
The CQ bit was checked by zijiehe@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.
https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:43: class AudioCapturerWin::MMNotificationClient : public IMMNotificationClient { On 2016/07/18 23:09:24, Sergey Ulanov wrote: > Do we need this as a separate class? Maybe just implement IMMNotificationClient > interface in AudioCapturerWin? Then AudioCapturerWin will export a set of unrelated functions. And the most important thing is, there are AddRef and Release function, which are too common and easily to conflict with our code base. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:55: return S_OK; } On 2016/07/18 23:09:24, Sergey Ulanov wrote: > here and below: formatting Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( On 2016/07/18 23:09:24, Sergey Ulanov wrote: > Which thread is this called on? Totally no idea, no document about it on MSDN. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:77: EDataFlow flow, ERole role, LPCWSTR pwstrDefaultDevice) { On 2016/07/18 23:09:24, Sergey Ulanov wrote: > One argument per line please > git cl format Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:96: notificator_(new MMNotificationClient(this)), On 2016/07/18 23:09:24, Sergey Ulanov wrote: > I don't think that |notificator_| is the right name for this variable. Call it > mm_notification_client_ Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:115: bool AudioCapturerWin::Initialize() { On 2016/07/18 23:09:24, Sergey Ulanov wrote: > Call this ResetAndInitialize() as it doesn't just initialize. Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:131: hr = mm_device_enumerator_->RegisterEndpointNotificationCallback( On 2016/07/18 23:09:24, Sergey Ulanov wrote: > Do we also need to unregister the callback when calling > mm_device_enumerator_.Release() above or when the capturer is deleted? Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:277: bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) { On 2016/07/18 23:09:24, Sergey Ulanov wrote: > Move this before initialize, so the order of methods matches the order in the > header. Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:288: capture_timer_.reset(new base::RepeatingTimer()); On 2016/07/18 23:09:24, Sergey Ulanov wrote: > nit: move this below, next to the capture_timer_->Start() call. Done. https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.h:76: bool default_audio_device_changed_; On 2016/07/18 23:09:24, Sergey Ulanov wrote: > = false; Done.
https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( On 2016/07/19 02:36:40, Hzj_jie wrote: > On 2016/07/18 23:09:24, Sergey Ulanov wrote: > > Which thread is this called on? > > Totally no idea, no document about it on MSDN. According to comments in audio/win/audio_device_listener_win.h it's called from system's media thread. I.e. it's not the audio thread that's used for AudioCapturer and so you need to add synchronization. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:77: : owner_(owner) { nit: Please either inline all methods in MMNotificationClient, or none. It looks like all methods are short, so I'd prefer inlining them all. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.h:35: // default_audio_device_changed_ in OnDefaultDeviceChanged function. nit: s/default_audio_device_changed_/|default_audio_device_changed_|/ s/OnDefaultDeviceChanged function/OnDefaultDeviceChanged()/ https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.h:37: friend class MMNotificationClient; if you move default_audio_device_changed_ to MMNotificationClient (with a get-and-reset access method) then it won't need to be a friend.
The CQ bit was checked by zijiehe@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.
https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( On 2016/07/20 17:28:34, Sergey Ulanov wrote: > On 2016/07/19 02:36:40, Hzj_jie wrote: > > On 2016/07/18 23:09:24, Sergey Ulanov wrote: > > > Which thread is this called on? > > > > Totally no idea, no document about it on MSDN. > > According to comments in audio/win/audio_device_listener_win.h it's called from > system's media thread. I.e. it's not the audio thread that's used for > AudioCapturer and so you need to add synchronization. Assignment of a bool value is atomic, so the worst case is we miss the true value in several DoCapture calls after default_audio_device_changed_ has been set to true. Considering we execute DoCapture function 33 times per second, this does not have significant impact. Note, default_audio_device_changed_ will only be set to false once we got a true value before, so we won't loss a single false-to-true flip. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:77: : owner_(owner) { On 2016/07/20 17:28:34, Sergey Ulanov wrote: > nit: Please either inline all methods in MMNotificationClient, or none. It looks > like all methods are short, so I'd prefer inlining them all. Oh, I thought only functions with only return statement can be inlined. No matter, I will inline them all. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.h (right): https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.h:35: // default_audio_device_changed_ in OnDefaultDeviceChanged function. On 2016/07/20 17:28:34, Sergey Ulanov wrote: > nit: > s/default_audio_device_changed_/|default_audio_device_changed_|/ > s/OnDefaultDeviceChanged function/OnDefaultDeviceChanged()/ Since default_audio_device_changed_ has been moved to MMNotificationClient, I updated the comment. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.h:37: friend class MMNotificationClient; On 2016/07/20 17:28:34, Sergey Ulanov wrote: > if you move default_audio_device_changed_ to MMNotificationClient (with a > get-and-reset access method) then it won't need to be a friend. Done.
https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( On 2016/07/20 20:43:14, Hzj_jie wrote: > On 2016/07/20 17:28:34, Sergey Ulanov wrote: > > On 2016/07/19 02:36:40, Hzj_jie wrote: > > > On 2016/07/18 23:09:24, Sergey Ulanov wrote: > > > > Which thread is this called on? > > > > > > Totally no idea, no document about it on MSDN. > > > > According to comments in audio/win/audio_device_listener_win.h it's called > from > > system's media thread. I.e. it's not the audio thread that's used for > > AudioCapturer and so you need to add synchronization. > > Assignment of a bool value is atomic, so the worst case is we miss the true > value in several DoCapture calls after default_audio_device_changed_ has been > set to true. No types are atomic in C++ by default. You cannot assume that this will always be the behavior you get. Also TSan will detect this as an issue. > Considering we execute DoCapture function 33 times per second, this > does not have significant impact. > > Note, default_audio_device_changed_ will only be set to false once we got a true > value before, so we won't loss a single false-to-true flip. When you have two threads writing the same variable without any synchronization you cannot make any assumption about how it will work in C++. It's undefined behavior, which means the compiler is allowed generate code that would crash, hang, or make itself a sandwich when it's executed. Please add a lock which is locked whenever the flag is accessed. https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/20001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:77: : owner_(owner) { On 2016/07/20 20:43:14, Hzj_jie wrote: > On 2016/07/20 17:28:34, Sergey Ulanov wrote: > > nit: Please either inline all methods in MMNotificationClient, or none. It > looks > > like all methods are short, so I'd prefer inlining them all. > > Oh, I thought only functions with only return statement can be inlined. That's for when you define class in a header, which doesn't apply here. > No > matter, I will inline them all.
The CQ bit was checked by zijiehe@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/2163473002/diff/1/remoting/host/audio_capture... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/1/remoting/host/audio_capture... remoting/host/audio_capturer_win.cc:76: HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged( On 2016/07/21 18:54:01, Sergey Ulanov wrote: > On 2016/07/20 20:43:14, Hzj_jie wrote: > > On 2016/07/20 17:28:34, Sergey Ulanov wrote: > > > On 2016/07/19 02:36:40, Hzj_jie wrote: > > > > On 2016/07/18 23:09:24, Sergey Ulanov wrote: > > > > > Which thread is this called on? > > > > > > > > Totally no idea, no document about it on MSDN. > > > > > > According to comments in audio/win/audio_device_listener_win.h it's called > > from > > > system's media thread. I.e. it's not the audio thread that's used for > > > AudioCapturer and so you need to add synchronization. > > > > Assignment of a bool value is atomic, so the worst case is we miss the true > > value in several DoCapture calls after default_audio_device_changed_ has been > > set to true. > > No types are atomic in C++ by default. You cannot assume that this will always > be the behavior you get. Also TSan will detect this as an issue. > > > Considering we execute DoCapture function 33 times per second, this > > does not have significant impact. > > > > Note, default_audio_device_changed_ will only be set to false once we got a > true > > value before, so we won't loss a single false-to-true flip. > > When you have two threads writing the same variable without any synchronization > you cannot make any assumption about how it will work in C++. It's undefined > behavior, which means the compiler is allowed generate code that would crash, > hang, or make itself a sandwich when it's executed. > Please add a lock which is locked whenever the flag is accessed. Done. Emmm, I believe the way I am using here is definitely troubleless.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:46: MMNotificationClient(AudioCapturerWin* owner) owner_ is not needed anymore. Remove it? https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:102: bool default_audio_device_changed_ = false; // GUARDED_BY(lock_) replace GUARDED_BY with a comment (e.g. "|lock_| must be locked when accessing |default_audio_device_changed_|"). We don't have GUARDED_BY macro in chromium. https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:368: !ResetAndInitialize()) { This expression is hard to read: it's not obvious that you reinitialize when the flag is set. Please replace with two nested if statements. https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:369: // Initialization failed, we should wait for next DoCapture call. When DoCapture() is called again this code won't call ResetAndInitialize() again and will try to procede in uninitialized state. So I think you need another check for that case if (devices_changed || !initialized_) { if (!ResetAndInitialize()) { :( return; } }
The CQ bit was checked by zijiehe@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.
https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:46: MMNotificationClient(AudioCapturerWin* owner) On 2016/07/22 17:38:37, Sergey Ulanov wrote: > owner_ is not needed anymore. Remove it? Yep, sorry. https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:102: bool default_audio_device_changed_ = false; // GUARDED_BY(lock_) On 2016/07/22 17:38:37, Sergey Ulanov wrote: > replace GUARDED_BY with a comment (e.g. "|lock_| must be locked when accessing > |default_audio_device_changed_|"). We don't have GUARDED_BY macro in chromium. Done. https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:368: !ResetAndInitialize()) { On 2016/07/22 17:38:37, Sergey Ulanov wrote: > This expression is hard to read: it's not obvious that you reinitialize when the > flag is set. Please replace with two nested if statements. Done. https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:369: // Initialization failed, we should wait for next DoCapture call. On 2016/07/22 17:38:37, Sergey Ulanov wrote: > When DoCapture() is called again this code won't call ResetAndInitialize() again > and will try to procede in uninitialized state. So I think you need another > check for that case > if (devices_changed || !initialized_) { > if (!ResetAndInitialize()) { > :( > return; > } > } No, SetDefaultAudioDeviceChanged in line 370 will reset the state if ResetAndInitialize failed. So we won't need an extra initialized_ variable.
https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... remoting/host/audio_capturer_win.cc:369: // Initialization failed, we should wait for next DoCapture call. On 2016/07/22 21:03:11, Hzj_jie wrote: > On 2016/07/22 17:38:37, Sergey Ulanov wrote: > > When DoCapture() is called again this code won't call ResetAndInitialize() > again > > and will try to procede in uninitialized state. So I think you need another > > check for that case > > if (devices_changed || !initialized_) { > > if (!ResetAndInitialize()) { > > :( > > return; > > } > > } > > No, SetDefaultAudioDeviceChanged in line 370 will reset the state if > ResetAndInitialize failed. So we won't need an extra initialized_ variable. I see. It seems error-prone to piggy-back on that flag for reinitialization. Add a separate flag for this case in AudioCapturerWin?
The CQ bit was checked by zijiehe@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...
Patchset #6 (id:100001) has been deleted
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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 zijiehe@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.
On 2016/07/22 21:29:27, Sergey Ulanov wrote: > https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... > File remoting/host/audio_capturer_win.cc (right): > > https://codereview.chromium.org/2163473002/diff/60001/remoting/host/audio_cap... > remoting/host/audio_capturer_win.cc:369: // Initialization failed, we should > wait for next DoCapture call. > On 2016/07/22 21:03:11, Hzj_jie wrote: > > On 2016/07/22 17:38:37, Sergey Ulanov wrote: > > > When DoCapture() is called again this code won't call ResetAndInitialize() > > again > > > and will try to procede in uninitialized state. So I think you need another > > > check for that case > > > if (devices_changed || !initialized_) { > > > if (!ResetAndInitialize()) { > > > :( > > > return; > > > } > > > } > > > > No, SetDefaultAudioDeviceChanged in line 370 will reset the state if > > ResetAndInitialize failed. So we won't need an extra initialized_ variable. > > I see. It seems error-prone to piggy-back on that flag for reinitialization. Add > a separate flag for this case in AudioCapturerWin? Instead of a new flag, I would prefer to use the state of existing ComPtrs to decide whether we need to initialize again. If one more flag is added, we may need to take care of inconsistency between this flag and the real internal state.
https://codereview.chromium.org/2163473002/diff/120001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:294: bool AudioCapturerWin::Initialized() const { nit: call it is_initialized()? https://codereview.chromium.org/2163473002/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:299: audio_volume_; The way ResetAndInitialize() is implemented I'm not sure this will always work correctly. Particularly if mm_device_->Activate() fails then it leaves all these objects. Maybe replace ResetAndInitialize with the following bool ResetAndInitialize() { Reset(); if (!Initialize()) { Reset(); return false; } return true; } This would guarantee that the object is always left in clean state if initialization fails.
The CQ bit was checked by zijiehe@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/2163473002/diff/120001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:294: bool AudioCapturerWin::Initialized() const { On 2016/07/22 23:38:05, Sergey Ulanov wrote: > nit: call it is_initialized()? Done. https://codereview.chromium.org/2163473002/diff/120001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:299: audio_volume_; On 2016/07/22 23:38:05, Sergey Ulanov wrote: > The way ResetAndInitialize() is implemented I'm not sure this will always work > correctly. Particularly if mm_device_->Activate() fails then it leaves all these > objects. > Maybe replace ResetAndInitialize with the following > > bool ResetAndInitialize() { > Reset(); > if (!Initialize()) { > Reset(); > return false; > } > return true; > } > > This would guarantee that the object is always left in clean state if > initialization fails. Indeed, no, the last statement mm_device->Activate creates audio_volume_ instance, well, the instance in audio_volume_. But yes, it's a very good point, some random change later may easily break this assumption silently. I have added an Initialize() and Deinitialize() function, and executes them in ResetAndInitialize() function.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2163473002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:307: return wave_format_ex_.get() && nit: now it's enough to check only one of these instead of all of them
The CQ bit was checked by zijiehe@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/2163473002/diff/140001/remoting/host/audio_ca... File remoting/host/audio_capturer_win.cc (right): https://codereview.chromium.org/2163473002/diff/140001/remoting/host/audio_ca... remoting/host/audio_capturer_win.cc:307: return wave_format_ex_.get() && On 2016/07/23 06:36:08, Sergey Ulanov wrote: > nit: now it's enough to check only one of these instead of all of them Done.
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 zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org Link to the patchset: https://codereview.chromium.org/2163473002/#ps160001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 ========== to ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 ========== to ========== [Chromoting] Audio playback won't work after switching default playback device We can only capture audio for the default device now. So we need to update audio components after switching a default playback device in Windows. BUG=628814 Committed: https://crrev.com/b40182aceb9f9ab588a296ace041360fa40bac88 Cr-Commit-Position: refs/heads/master@{#407390} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/b40182aceb9f9ab588a296ace041360fa40bac88 Cr-Commit-Position: refs/heads/master@{#407390} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
