Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1703473002:
Make AudioOutputDevice restartable and reinitializable (Closed)
|
Created:
4 years, 10 months ago by Henrik Grunell Modified:
3 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, miu+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, vanellope-cl_google.com Base URL:
https://chromium.googlesource.com/chromium/src.git@new_mixing Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake AudioOutputDevice restartable and reinitializable.
* Changing the render callbacks to pass through AOD, only clearing the callback in Stop() and moving stopping of the audio thread to the IO thread. This serializes Initialize(), Start() and Stop() while ensuring no callbacks after Stop(). It also removed the stopping hack flag.
* Device authorization is not touched in this CL, meaning it will reauthorize each restart.
Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes.
BUG=607916
Patch Set 1 #
Total comments: 19
Patch Set 2 : Minor CR fixes. #Patch Set 3 : Changes from olka@, adding callback lock, removing stopping hack, rebase. #
Total comments: 3
Patch Set 4 : Fixes, cleaning, and updated unit test. #Patch Set 5 : git cl format #
Total comments: 36
Patch Set 6 : Code review (olka@) #Patch Set 7 : Fixed use after free - clearing ADT callback and protect it with lock. #Patch Set 8 : Fixed comments in olka@'s CL. Updated some comments. #
Total comments: 28
Patch Set 9 : Code review (guidou@). #
Total comments: 18
Patch Set 10 : Code review (olka@) + added more unit test cases. #Patch Set 11 : Cleaning. #Patch Set 12 : Code review (tommi@). #Patch Set 13 : Rebase. #Patch Set 14 : Added missing include. #
Total comments: 28
Patch Set 15 : Code review (tommi@). #
Total comments: 14
Patch Set 16 : Code review (dalecurtis@). #Patch Set 17 : No thread re-use. Also rebase. #
Total comments: 13
Patch Set 18 : Code review fixes (dalecurtis@). #Patch Set 19 : Back to stopping hack. #
Total comments: 6
Patch Set 20 : Code review (dalecurtis@). #
Total comments: 6
Messages
Total messages: 55 (13 generated)
Description was changed from ========== Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have StartRun() and StopRun() functions, where StartRun() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use this. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG= ========== to ========== *** Work in progress *** The state in ADT is not ideal. I'd like to remove it and only rely on the event, but the case when thread run is exited because of a socket failure is problematic. ----- Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have StartRun() and StopRun() functions, where StartRun() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use this. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG= ==========
grunell@chromium.org changed reviewers: + olka@chromium.org, tommi@chromium.org
Description was changed from ========== *** Work in progress *** The state in ADT is not ideal. I'd like to remove it and only rely on the event, but the case when thread run is exited because of a socket failure is problematic. ----- Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have StartRun() and StopRun() functions, where StartRun() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use this. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG= ========== to ========== *** Work in progress *** * The state in ADT is not ideal. I'd like to remove it and only rely on the event, but the case when thread run is exited because of a socket failure is problematic. * Debug code to be removed. ----- Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have StartRun() and StopRun() functions, where StartRun() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use this. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG= ==========
I'd like input on how to handle the synchronization in AudioDeviceThread. I wonder if it would be a problem to wait for the event if the socket fails? Then the thread can be stopped or restarted.
Took a quick look but we should probably discuss this f2f to get a clearer understanding https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:125: base::AutoLock auto_lock(thread_lock_); This might be off topic but... can we make it so that Start* and Stop* methods are called on the same thread? I.e. that thread_ is always accessed on the same thread and therefore we don't need a lock for it? As is, we're holding a lock while we wait for a thread to terminate and I'm wondering if we still need it. I seem to be the one to blame for this btw, and back in 2012 I wrote this: "Additionally, I'm adding a lock to guard the thread_ variable in AudioDeviceThread. Before I was relying on external synchronization in the AudioDevice and AudioInputDevice state machine. However, it doesn't hurt to go belt and suspenders although it might not look cool ;)" https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:126: CHECK(thread_.get()); nit: no need for these sort of checks since we deref thread_ anyway. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:178: DCHECK(state_ == IDLE || state_ == RUNNING); Can/do we update the state_ always on the same thread? https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:254: static_assert(sizeof(size_t) == sizeof(pthread_t), "This won't work"); fyi - pthread_t doesn't exist on some platforms and it doesn't always reflect the same thing. platform_thread.h should help. check out the difference between PlatformThreadId and PlatformThreadRef. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:349: base::AutoLock auto_lock(state_lock_); this can be a wait too https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:364: // |state_| == RUNNING and the event is signaled. This reads like a bug to me actually. The event should be auto resetting and the race described in the TODO exists only because we're trying to reset manually. What's the difference between Start and StartRun? https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:380: base::AutoLock auto_lock(callback_lock_); any chance we can get away with not using this lock? (socket_ always touched on the same thread)
Yes, let's talk in person. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:125: base::AutoLock auto_lock(thread_lock_); On 2016/02/16 11:25:29, tommi-chromium wrote: > This might be off topic but... > > can we make it so that Start* and Stop* methods are called on the same thread? > I.e. that thread_ is always accessed on the same thread and therefore we don't > need a lock for it? > > As is, we're holding a lock while we wait for a thread to terminate and I'm > wondering if we still need it. > I seem to be the one to blame for this btw, and back in 2012 I wrote this: > > "Additionally, I'm adding a lock to guard the thread_ variable in > AudioDeviceThread. > Before I was relying on external synchronization in the AudioDevice and > AudioInputDevice state machine. However, it doesn't hurt to go belt and > suspenders although it might not look cool ;)" :) Yeah, I can add a thread checker instead, let's see if it blows up. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:126: CHECK(thread_.get()); On 2016/02/16 11:25:29, tommi-chromium wrote: > nit: no need for these sort of checks since we deref thread_ anyway. Ineed. Done. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:178: DCHECK(state_ == IDLE || state_ == RUNNING); On 2016/02/16 11:25:29, tommi-chromium wrote: > Can/do we update the state_ always on the same thread? Yes, if we add a thread checker and are sure it's called on the same thread (I think it should be). Then it's updated only on this thread. And only read on the audio thread. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:254: static_assert(sizeof(size_t) == sizeof(pthread_t), "This won't work"); On 2016/02/16 11:25:29, tommi-chromium wrote: > fyi - pthread_t doesn't exist on some platforms and it doesn't always reflect > the same thing. platform_thread.h should help. check out the difference between > PlatformThreadId and PlatformThreadRef. Yeah, this was just for verifying my assumption in the debug printouts. Removed. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:349: base::AutoLock auto_lock(state_lock_); On 2016/02/16 11:25:29, tommi-chromium wrote: > this can be a wait too Wait for the lock you mean? https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:364: // |state_| == RUNNING and the event is signaled. On 2016/02/16 11:25:29, tommi-chromium wrote: > This reads like a bug to me actually. The event should be auto resetting and > the race described in the TODO exists only because we're trying to reset > manually. Yes, that's what I want to do, but the case when the socket fails without us stopping may be problematic. > > What's the difference between Start and StartRun? Start creates the thread, but it will wait here above until StartRun is called. We could make Start not wait too, I'm leaning against that but waited for input first. :) https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:380: base::AutoLock auto_lock(callback_lock_); On 2016/02/16 11:25:29, tommi-chromium wrote: > any chance we can get away with not using this lock? (socket_ always touched on > the same thread) |callback_| needs protection. (And |new_socket_handle_|.)
Just a couple of comments in addition to our offline discussion. Let's check with Tommi an idea of having a flag "socket shut down" + a lock guarding it and the socket. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:88: base::WaitableEvent event_; Can we name it more descriptive, like 'wait_for_command', or 'start_or_stop', or something like that? It would make the code more verbal. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:182: socket_->Shutdown(); Is it safe to shutdown a socket twice? https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:251: // TODO: Handle case when Stop() is called before we get here. For this you can probably just place if(MaybeWait()) before Run() in the loop https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:341: bool AudioDeviceThread::Thread::MaybeWait() { I would call it "Continue" or "Resume" or "SyncAndResume" or something like this to explain its return value meaning. https://codereview.chromium.org/1703473002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:349: base::AutoLock auto_lock(state_lock_); If state modifications are done by this thread only, than lock is not needed, otherwise looks like a race is possible between setting a state and making a decision on 'to wait or not to wait'.
Description was changed from ========== *** Work in progress *** * The state in ADT is not ideal. I'd like to remove it and only rely on the event, but the case when thread run is exited because of a socket failure is problematic. * Debug code to be removed. ----- Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have StartRun() and StopRun() functions, where StartRun() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use this. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG= ========== to ========== *** DRAFT *** Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have Play() and Pause() functions, where Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ==========
Here's the new approach for ensuring no callbacks after Stop() when making AudioOutputDevice restartable. Also created a bug. The CL is still a draft with some things to sort out, but the change to route callbacks through AOD which will gate them to ensure the above should be possible to see here. The new patch set merges back Olga's forked CL. https://codereview.chromium.org/1703473002/diff/40001/media/audio/audio_devic... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1703473002/diff/40001/media/audio/audio_devic... media/audio/audio_device_thread.h:99: void Play(AudioDeviceThread::Callback* callback, For output, we could now set the callback once in the constructor since AudioOutputDevice is always the callback. TODO: Verify OK for input as well and make that change. https://codereview.chromium.org/1703473002/diff/40001/media/audio/audio_devic... media/audio/audio_device_thread.h:110: // Not sure if we need this extra class and extra lock? This was Olga's addition.
grunell@chromium.org changed reviewers: + guidou@chromium.org
Description was changed from ========== *** DRAFT *** Make AudioOutputDevice restartable. * Changed AudioDeviceThread to have Play() and Pause() functions, where Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ========== to ========== Make AudioOutputDevice restartable and reinitializable. * Changed AudioDeviceThread to have Play() and Pause() functions, where Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ==========
Ptal now. Some more unit tests will be added. https://codereview.chromium.org/1703473002/diff/40001/media/audio/audio_devic... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1703473002/diff/40001/media/audio/audio_devic... media/audio/audio_device_thread.h:99: void Play(AudioDeviceThread::Callback* callback, On 2016/04/29 14:34:55, Henrik Grunell wrote: > For output, we could now set the callback once in the constructor since > AudioOutputDevice is always the callback. > > TODO: Verify OK for input as well and make that change. Actually, it's not possible - this is the callback wrapper that also contains parameters etc.
Haven't found any "restartable" problems so far, need to take another look tomorrow. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:97: // AudioDeviceThread implementation TODO(olka): get rid of it. It just adds one You've got a comment on it in the header I think. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:123: thread_->Resume(callback, socket); if (thread_.get())? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:211: DCHECK(action_ != kActionPlay) << "Resume a non-paused thread?"; (action_ == kActionPause) for clarity? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:229: DCHECK(action_ != kActionPause) << "Pause a paused thread?"; (action_ == kActionPlay)? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:288: size_t bytes_read = socket_->Receive(&pending_data, sizeof(pending_data)); Add a comment that both socket_ and callback_ are modified in ThreadMain only, so it's safe to access it here? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:86: // clearing it in Stop(). Note that after this it is possible to get stale Could you explain here how exactly it can happen? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:87: // callbacks from previous runs (i.e. from before Stop() was called earlier). Taking into account current usage model, we are not expecting callback to be changed during AOD lifetime, right? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:134: // Clear the callback synchronously to ensure no callbacks after returning. This is nice. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:183: : 0; Shouldn't we zero-out the bus? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:213: base::AutoLock auto_lock(callback_lock_); It's just AOD::OnRenderError() https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:285: did_receive_auth_.Reset(); Add a comment why it's needed? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:310: base::AutoLock auto_lock(callback_lock_); AOD::OnRenderError() https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:365: base::AutoLock auto_lock(callback_lock_); ditto https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.h:166: AudioRendererSink::RenderCallback* callback_; Probably store it as atomic word => get rid of the lock? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.h:200: std::unique_ptr<AudioOutputDevice::AudioThreadCallback> audio_callback_; Could we probably rename "callback_" into "render_callback_" and "audio_callback_" into "audio_thread_callback_", or something like this, to improve readability? WDYT? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:307: StopAudioDevice(); Here and below: this does not look like a natural usage sequence. Remove SetDevice() call from constructor instead? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:342: // than the task runner given to it. Comment is a bit unclear. GetOutputDeviceInfo must not be called on which task runner? https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:346: Probably you should initialize device_info_ with some garbage here, to make sure it's really updated?
Patch set 6 addresses Olga's comments. Still todo: add more test cases. Also I'll go through the comments in Olga's CL that was merged into this. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:97: // AudioDeviceThread implementation TODO(olka): get rid of it. It just adds one On 2016/05/03 15:47:32, o1ka wrote: > You've got a comment on it in the header I think. Yes. Removed the whole comment here actually. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:123: thread_->Resume(callback, socket); On 2016/05/03 15:47:32, o1ka wrote: > if (thread_.get())? Haha, yes, this I commented on in your CL with changes but it wasn't changed. I'll go through all those comments and make sure they're addressed. Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:211: DCHECK(action_ != kActionPlay) << "Resume a non-paused thread?"; On 2016/05/03 15:47:32, o1ka wrote: > (action_ == kActionPause) for clarity? Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:229: DCHECK(action_ != kActionPause) << "Pause a paused thread?"; On 2016/05/03 15:47:32, o1ka wrote: > (action_ == kActionPlay)? Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_devic... media/audio/audio_device_thread.cc:288: size_t bytes_read = socket_->Receive(&pending_data, sizeof(pending_data)); On 2016/05/03 15:47:32, o1ka wrote: > Add a comment that both socket_ and callback_ are modified in ThreadMain only, > so it's safe to access it here? Done. I put it before the loop as a general comment for that part. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:86: // clearing it in Stop(). Note that after this it is possible to get stale On 2016/05/03 15:47:32, o1ka wrote: > Could you explain here how exactly it can happen? Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:87: // callbacks from previous runs (i.e. from before Stop() was called earlier). On 2016/05/03 15:47:32, o1ka wrote: > Taking into account current usage model, we are not expecting callback to be > changed during AOD lifetime, right? Yes, that's right, but we allow it. I suspect you were thinking of checking/enforcing it? I don't think we should make that change now (if we want to at all). https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:134: // Clear the callback synchronously to ensure no callbacks after returning. On 2016/05/03 15:47:32, o1ka wrote: > This is nice. Thanks. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:183: : 0; On 2016/05/03 15:47:32, o1ka wrote: > Shouldn't we zero-out the bus? That would not be correct, we should return the number of frames filled, if we zero out we have actually filled it, literally speaking at least. It should in any way be enough to say "I filled 0 frames". https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:213: base::AutoLock auto_lock(callback_lock_); On 2016/05/03 15:47:32, o1ka wrote: > It's just AOD::OnRenderError() Of course, thanks. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:285: did_receive_auth_.Reset(); On 2016/05/03 15:47:32, o1ka wrote: > Add a comment why it's needed? Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:310: base::AutoLock auto_lock(callback_lock_); On 2016/05/03 15:47:32, o1ka wrote: > AOD::OnRenderError() Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.cc:365: base::AutoLock auto_lock(callback_lock_); On 2016/05/03 15:47:32, o1ka wrote: > ditto Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.h:166: AudioRendererSink::RenderCallback* callback_; On 2016/05/03 15:47:32, o1ka wrote: > Probably store it as atomic word => get rid of the lock? We need to hold on to a lock while calling e.g. Render() so that the callee can't call Stop() and then expect no callbacks, while we could actually be executing one at that time. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device.h:200: std::unique_ptr<AudioOutputDevice::AudioThreadCallback> audio_callback_; On 2016/05/03 15:47:32, o1ka wrote: > Could we probably rename "callback_" into "render_callback_" and > "audio_callback_" into "audio_thread_callback_", or something like this, to > improve readability? WDYT? Good idea. Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... File media/audio/audio_output_device_unittest.cc (right): https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:307: StopAudioDevice(); On 2016/05/03 15:47:32, o1ka wrote: > Here and below: this does not look like a natural usage sequence. Remove > SetDevice() call from constructor instead? Agree. A general note on this file is that I wasn't not done with it yet, so it could have been rough in places. Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:342: // than the task runner given to it. On 2016/05/03 15:47:32, o1ka wrote: > Comment is a bit unclear. GetOutputDeviceInfo must not be called on which task > runner? Done. https://codereview.chromium.org/1703473002/diff/80001/media/audio/audio_outpu... media/audio/audio_output_device_unittest.cc:346: On 2016/05/03 15:47:32, o1ka wrote: > Probably you should initialize device_info_ with some garbage here, to make sure > it's really updated? Good point. Done.
Patch set 7 fixes a use-after-free error. Patch set 8 addresses comments in olka@'s CL, see https://codereview.chromium.org/1830933002/#msg13. Ptal.
just a few minor comments. Have to do another round. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:71: // Start(), Resume(), Pause() must be serialized; Stop() can be called any It's not clear (to me at least) why this comment is directly related to this variable. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:159: // This reference will be released when the thread exists. drive-by comment: s/exists/exits ? https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:279: Should there be a thread_checker or similar here? https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... media/audio/audio_output_device.cc:98: DCHECK(!render_callback_) << "Calling Initialize() twice?"; Isn't the purpose of the CL to make AOD reinitializable? Perhaps the logging message should be changed. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... media/audio/audio_output_device.cc:126: #if !defined(NDEBUG) Is using NDEBUG allowed?
https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:71: // Start(), Resume(), Pause() must be serialized; Stop() can be called any On 2016/05/09 13:15:49, Guido Urdaneta wrote: > It's not clear (to me at least) why this comment is directly related to this > variable. Clarified. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:159: // This reference will be released when the thread exists. On 2016/05/09 13:15:49, Guido Urdaneta wrote: > drive-by comment: s/exists/exits ? Yep. Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:279: On 2016/05/09 13:15:49, Guido Urdaneta wrote: > Should there be a thread_checker or similar here? That's not necessary, it's the audio thread run function only called from ThreadMain. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... media/audio/audio_output_device.cc:98: DCHECK(!render_callback_) << "Calling Initialize() twice?"; On 2016/05/09 13:15:50, Guido Urdaneta wrote: > Isn't the purpose of the CL to make AOD reinitializable? > Perhaps the logging message should be changed. Updated comment. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_outp... media/audio/audio_output_device.cc:126: #if !defined(NDEBUG) On 2016/05/09 13:15:50, Guido Urdaneta wrote: > Is using NDEBUG allowed? Afaik.
Nice, looking forward to updated tests. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (left): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:69: // AudioDeviceThread implementation Put the comment back? https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:46: // Must be called. .. before destruction? (It just sounds more complete to me, easier for brains :) ) https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:72: // Stop() can be called any thread. nit: on/from any thread https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:148: DCHECK_EQ(state_, kStateStopped); nit: DCHECK_EQ(state_, kStateStopped) << "Thread must be stopped before destruction"; (a bit more verbal) https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:174: DCHECK(state_ != kStateStopped) << "Calling Stop() on a stopped thread?"; Why DCHECK? Can it be a problem? I think we allow to stop a device multiple times, all Stop() calls but one are just no-ops. (We can add DCHECKs there that if kStateStopped, then everything is stopped properly though) https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:182: // Must be done under lock to avoid race wuth Start(). nit: wuth -> with https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:297: base::AutoLock auto_lock(state_lock_); Could you add a comment why we should hold a lock during render call and cannot use atomic pointer here instead? https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... media/audio/audio_output_device.cc:285: audio_thread_.Pause(); It seems that calling Stop() on a stopped AOD is allowed (and it used to be). But here is will result in pausing a paused thread, which is not allowed by current implementation. We probably need to add a test for that and make sure it works. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... media/audio/audio_output_device.h:161: base::Lock callback_lock_; Probably add a comment that nothing can be done under this lock but access to the callback, and specifically, not thread operations are allowed under it?
https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (left): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:69: // AudioDeviceThread implementation On 2016/05/10 13:04:53, o1ka wrote: > Put the comment back? Done. (Although I don't think it or the one for AudioDeviceThread::Thread implementation add readability. But I'm OK with either.) https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:46: // Must be called. On 2016/05/10 13:04:53, o1ka wrote: > .. before destruction? (It just sounds more complete to me, easier for brains :) > ) Done. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:72: // Stop() can be called any thread. On 2016/05/10 13:04:53, o1ka wrote: > nit: on/from any thread Done. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:148: DCHECK_EQ(state_, kStateStopped); On 2016/05/10 13:04:53, o1ka wrote: > nit: DCHECK_EQ(state_, kStateStopped) << "Thread must be stopped before > destruction"; > (a bit more verbal) Done. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:174: DCHECK(state_ != kStateStopped) << "Calling Stop() on a stopped thread?"; On 2016/05/10 13:04:53, o1ka wrote: > Why DCHECK? Can it be a problem? I think we allow to stop a device multiple > times, all Stop() calls but one are just no-ops. > (We can add DCHECKs there that if kStateStopped, then everything is stopped > properly though) Multiple calls to AudioDeviceThread::Stop() is allowed. But it will only call AudioDeviceThread::Thread::Stop() the first time, then AudioDeviceThread::Thread will be deleted. So the DCHECK is a correct assertion. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:182: // Must be done under lock to avoid race wuth Start(). On 2016/05/10 13:04:53, o1ka wrote: > nit: wuth -> with Done. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_devi... media/audio/audio_device_thread.cc:297: base::AutoLock auto_lock(state_lock_); On 2016/05/10 13:04:53, o1ka wrote: > Could you add a comment why we should hold a lock during render call and cannot > use atomic pointer here instead? Done. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... media/audio/audio_output_device.cc:285: audio_thread_.Pause(); On 2016/05/10 13:04:53, o1ka wrote: > It seems that calling Stop() on a stopped AOD is allowed (and it used to be). > But here is will result in pausing a paused thread, which is not allowed by > current implementation. > > We probably need to add a test for that and make sure it works. Great catch. I'll added such a test and only Pause() if |audio_thread_callback_| is non-null, which is equivalent to not being paused. https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/160001/media/audio/audio_outp... media/audio/audio_output_device.h:161: base::Lock callback_lock_; On 2016/05/10 13:04:54, o1ka wrote: > Probably add a comment that nothing can be done under this lock but access to > the callback, and specifically, not thread operations are allowed under it? Done.
just noticed that I have a few comments sitting in draft. Sending them out now and then catch up on the latest patches. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:59: enum State { kStatePaused, kStatePlaying, kStateStopped }; Intentionally not using MACRO_STYLE? https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:71: // Start(), Resume(), Pause() must be serialized; Stop() can be called any serialized and required to be called on the same thread aren't exactly the same things (serialized can still execute on multiple threads by using locking). Should this be "must be called on the same thread"? And then "any moment" can still mean on one thread. I think you mean "on any thread". hmm.. actually, maybe I'm misunderstanding the comment completely... does this refer to the calling sequence perhaps? https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:78: base::Lock state_lock_; // Protects everything below. nit: Could just call it lock_ now that it doesn't only protect the state. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:83: base::PlatformThreadHandle thread_; nit: do you mind renaming this variable to thread_handle_ ? It can be confusing to read code in the file and look for where this variable is used and then see the code in AudioDeviceThread that uses a variable with the same name. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:168: // Note: Stop() can be called any time, even before Start(), because it's nit: called at any time https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:182: // Must be done under lock to avoid race wuth Start(). with https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: state_condition_.Wait(); Can you add a comment that this releases the state_lock_ while waiting? https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:270: } // while nit: remove the comment. It looks like it's a part of the previous comment. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:282: // |socket_| is only modified in ThreadMain, so it is safe to access here. nit: // |socket_| is only modified on this thread, so we don't need a lock to access it here.
https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:59: enum State { kStatePaused, kStatePlaying, kStateStopped }; On 2016/05/11 14:12:34, tommi-chrömium wrote: > Intentionally not using MACRO_STYLE? No. Actually Olga's code and I missed that. Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:71: // Start(), Resume(), Pause() must be serialized; Stop() can be called any On 2016/05/11 14:12:34, tommi-chrömium wrote: > serialized and required to be called on the same thread aren't exactly the same > things (serialized can still execute on multiple threads by using locking). > Should this be "must be called on the same thread"? > > And then "any moment" can still mean on one thread. I think you mean "on any > thread". > > hmm.. actually, maybe I'm misunderstanding the comment completely... does this > refer to the calling sequence perhaps? This comment is very much a draft. :) I already clarified it so your comments have been addressed. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:78: base::Lock state_lock_; // Protects everything below. On 2016/05/11 14:12:34, tommi-chrömium wrote: > nit: Could just call it lock_ now that it doesn't only protect the state. Agree, done. I also changed the name of the |state_condition_| to "exit_paused_state_". https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:83: base::PlatformThreadHandle thread_; On 2016/05/11 14:12:34, tommi-chrömium wrote: > nit: do you mind renaming this variable to thread_handle_ ? It can be confusing > to read code in the file and look for where this variable is used and then see > the code in AudioDeviceThread that uses a variable with the same name. Sure, done. Also changed name of the local variable in Stop() below. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:168: // Note: Stop() can be called any time, even before Start(), because it's On 2016/05/11 14:12:34, tommi-chrömium wrote: > nit: called at any time Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:182: // Must be done under lock to avoid race wuth Start(). On 2016/05/11 14:12:34, tommi-chrömium wrote: > with Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: state_condition_.Wait(); On 2016/05/11 14:12:34, tommi-chrömium wrote: > Can you add a comment that this releases the state_lock_ while waiting? Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:270: } // while On 2016/05/11 14:12:34, tommi-chrömium wrote: > nit: remove the comment. It looks like it's a part of the previous comment. Done. https://codereview.chromium.org/1703473002/diff/140001/media/audio/audio_devi... media/audio/audio_device_thread.cc:282: // |socket_| is only modified in ThreadMain, so it is safe to access here. On 2016/05/11 14:12:34, tommi-chrömium wrote: > nit: // |socket_| is only modified on this thread, so we don't need a lock to > access it here. Done.
Tommi, can you take a good look at if there's concurrency issues? Since Olga also has written code in this CL, she's (admittedly) somewhat biased.
https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:40: // called to actually start running it. Can we rename the method to StartSuspended? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:57: void Pause(); Suspend() (to match with Resume()) https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:86: // Reset in ThreadMain(). Shutdown on client thread. what does 'client thread' mean? is it the thread_checker_ thread? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:89: // Set on client thead. thread https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:104: base::AutoLock auto_lock(thread_lock_); are Start()/Stop()/Resume()/Pause() not always called on the same thread? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:106: thread_ = new AudioDeviceThread::Thread(thread_name, synchronized_buffers); if thread_name and synchronize_buffers will always be the same value for a given AudioDeviceThread instance, maybe thread_ doesn't need to be a pointer and can just be initialized in the constructor? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:145: thread_checker_.DetachFromThread(); why this detach? From what I can tell, Start() is called immediately after construction, so the ctor will be running on that thread. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:172: // allowed to be executed on a different thread. Is that something we can fix/avoid? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:178: DCHECK(state_ != STATE_STOPPED) << "Calling Stop() on a stopped thread?"; nit: DCHECK_NE https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:211: DCHECK(state_ == STATE_PAUSED) << "Resume a non-paused thread?"; nit: DCHECK_EQ https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:219: exit_paused_state_.Signal(); any chance that this may already be signaled and we could get out of sync? (or is that simply ok if it happens?) https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:235: socket_->Shutdown(); nit: would it be worth it to swap the socket to a local variable and do the shutdown outside of the lock? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.h:83: void Start(const char* thread_name, bool synchronized_buffers); Can the thread be started using different names each time it's started? Just wondering if the name shouldn't be supplied in the ctor. Also, may synchronized_buffers be of a different value for different runs of the same instance? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_outp... media/audio/audio_output_device.cc:405: audio_thread_.Start("AudioOutputDevice", true); since these params always appear to be the same value, I'd prefer to inject them via the ctor of audio_thread_ and have the corresponding member variables in the implementation, const.
https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:40: // called to actually start running it. On 2016/05/14 11:06:28, tommi-chrömium wrote: > Can we rename the method to StartSuspended? Good idea. Done. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:57: void Pause(); On 2016/05/14 11:06:28, tommi-chrömium wrote: > Suspend() (to match with Resume()) Also good. Done. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:86: // Reset in ThreadMain(). Shutdown on client thread. On 2016/05/14 11:06:27, tommi-chrömium wrote: > what does 'client thread' mean? is it the thread_checker_ thread? It's actually any other thread since Stop() may be called on any thread. Changed. (Yes, "client thread" is the |thread_checker_| thread.) https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:89: // Set on client thead. On 2016/05/14 11:06:27, tommi-chrömium wrote: > thread I removed this comment, I don't think it adds anything. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:104: base::AutoLock auto_lock(thread_lock_); On 2016/05/14 11:06:27, tommi-chrömium wrote: > are Start()/Stop()/Resume()/Pause() not always called on the same thread? Unfortunately no. AudioInputDevice still calls Stop() on another thread to stop it synchronously. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:106: thread_ = new AudioDeviceThread::Thread(thread_name, synchronized_buffers); On 2016/05/14 11:06:27, tommi-chrömium wrote: > if thread_name and synchronize_buffers will always be the same value for a given > AudioDeviceThread instance, maybe thread_ doesn't need to be a pointer and can > just be initialized in the constructor? It can be in the ctor. I'd like to not change that in this CL though, it changes enough stuff. :) Also, the ADT::Thread layer seems to not be useful anymore, so it should probably go away anyway. There's a todo for this. OK? https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:145: thread_checker_.DetachFromThread(); On 2016/05/14 11:06:27, tommi-chrömium wrote: > why this detach? From what I can tell, Start() is called immediately after > construction, so the ctor will be running on that thread. Indeed. removed. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:172: // allowed to be executed on a different thread. On 2016/05/14 11:06:28, tommi-chrömium wrote: > Is that something we can fix/avoid? AudioOutputDevice doesn't do this, but AudioInputDevice does. It would require fairly large and sensitive changes to fix and I don't want to do it here or now. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:178: DCHECK(state_ != STATE_STOPPED) << "Calling Stop() on a stopped thread?"; On 2016/05/14 11:06:28, tommi-chrömium wrote: > nit: DCHECK_NE Done. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:211: DCHECK(state_ == STATE_PAUSED) << "Resume a non-paused thread?"; On 2016/05/14 11:06:28, tommi-chrömium wrote: > nit: DCHECK_EQ Done. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:219: exit_paused_state_.Signal(); On 2016/05/14 11:06:27, tommi-chrömium wrote: > any chance that this may already be signaled and we could get out of sync? (or > is that simply ok if it happens?) You tell me. :D When we enter wait we have the lock and we simply wait until surely out of suspended state, so that should be fine. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.cc:235: socket_->Shutdown(); On 2016/05/14 11:06:27, tommi-chrömium wrote: > nit: would it be worth it to swap the socket to a local variable and do the > shutdown outside of the lock? Gut feeling says no. Apart from that, I haven't looked closely but it seems like we could possibly get concurrency related problems then. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_devi... media/audio/audio_device_thread.h:83: void Start(const char* thread_name, bool synchronized_buffers); On 2016/05/14 11:06:28, tommi-chrömium wrote: > Can the thread be started using different names each time it's started? > Just wondering if the name shouldn't be supplied in the ctor. > Also, may synchronized_buffers be of a different value for different runs of the > same instance? It can only be started once, and stopped once. Pause/Resume can be done several times. https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/250001/media/audio/audio_outp... media/audio/audio_output_device.cc:405: audio_thread_.Start("AudioOutputDevice", true); On 2016/05/14 11:06:28, tommi-chrömium wrote: > since these params always appear to be the same value, I'd prefer to inject them > via the ctor of audio_thread_ and have the corresponding member variables in the > implementation, const. Yep that's better. See other comment though.
Ping Tommi.
lgtm. Would be good to do a follow up change soon that removes the extra thread layer now that we don't need it, and initialize the thread object only once and the name+synchronized params.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:60: enum State { STATE_SUSPENDED, STATE_PLAYING, STATE_STOPPED }; Instead of adding STATE_ consider making this an "enum class" Also this should live down near the actual state_ variable. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:62: friend class base::RefCountedThreadSafe<AudioDeviceThread::Thread>; friend goes right under private: https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:120: if (thread_.get()) Drop .get() here and elsewhere. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:159: // This reference will be released when the thread exits. We should try to remove this in the future too; pretty sketchy. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { What's the point of doing this versus just restarting the thread each time?
Description was changed from ========== Make AudioOutputDevice restartable and reinitializable. * Changed AudioDeviceThread to have Play() and Pause() functions, where Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions. * Device authorization is not touched in this CL, meaning it will have to reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ========== to ========== Make AudioOutputDevice restartable and reinitializable. * Added Suspend() and Resume()to AudioDeviceThread. Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions, i.e. suspend the thread in Stop() and resume it in Start(). * Device authorization is not touched in this CL, meaning it will reauthorize each restart. The reason for the new functions Suspend() and Resume() is that clients assume render calls on the same thread, and we need to ensure this when restarting the AudioOutputDevice. This is done by suspending/resuming instead of starting a new thread. The alternative to somehow inform about a new thread is complicated and would require substantial refactoring. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ==========
Addressed Dale's comments. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:60: enum State { STATE_SUSPENDED, STATE_PLAYING, STATE_STOPPED }; On 2016/05/17 17:54:35, DaleCurtis wrote: > Instead of adding STATE_ consider making this an "enum class" Also this should > live down near the actual state_ variable. Good points, though can't use DCHECK_EQ etc. with enum class types. Both done. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:62: friend class base::RefCountedThreadSafe<AudioDeviceThread::Thread>; On 2016/05/17 17:54:35, DaleCurtis wrote: > friend goes right under private: Done. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:120: if (thread_.get()) On 2016/05/17 17:54:35, DaleCurtis wrote: > Drop .get() here and elsewhere. Done. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:159: // This reference will be released when the thread exits. On 2016/05/17 17:54:35, DaleCurtis wrote: > We should try to remove this in the future too; pretty sketchy. Agree, added TODO. https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { On 2016/05/17 17:54:35, DaleCurtis wrote: > What's the point of doing this versus just restarting the thread each time? The problem with spinning up a new thread each time is that in many places in the code we assume render calls on the same thread and have checks for this. It's complicated to inform/restart them to accept a new thread. I put this in the CL description too.
The CQ bit was checked by grunell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1703473002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1703473002/290001
On 2016/05/17 14:09:13, tommi-chrömium wrote: > lgtm. Would be good to do a follow up change soon that removes the extra thread > layer now that we don't need it, and initialize the thread object only once and > the name+synchronized params. Filed bug for that.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { On 2016/05/18 at 12:21:32, Henrik Grunell wrote: > On 2016/05/17 17:54:35, DaleCurtis wrote: > > What's the point of doing this versus just restarting the thread each time? > > The problem with spinning up a new thread each time is that in many places in the code we assume render calls on the same thread and have checks for this. It's complicated to inform/restart them to accept a new thread. > > I put this in the CL description too. I feel like overcomplicating our production code to workaround debug checks is not a great course of action. What do you think about introducing an AudioRendererSink::CurrentlyOnAudioRenderingThread() type helper method that we can replace all of these type checks with? It would simply be a tid comparison implemented by the underlying device.
https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { On 2016/05/18 17:20:43, DaleCurtis wrote: > On 2016/05/18 at 12:21:32, Henrik Grunell wrote: > > On 2016/05/17 17:54:35, DaleCurtis wrote: > > > What's the point of doing this versus just restarting the thread each time? > > > > The problem with spinning up a new thread each time is that in many places in > the code we assume render calls on the same thread and have checks for this. > It's complicated to inform/restart them to accept a new thread. > > > > I put this in the CL description too. > > I feel like overcomplicating our production code to workaround debug checks is > not a great course of action. What do you think about introducing an > AudioRendererSink::CurrentlyOnAudioRenderingThread() type helper method that we > can replace all of these type checks with? It would simply be a tid comparison > implemented by the underlying device. The problems is more specifically the components used in clients in the render callbacks in various places, also in blink iirc, that have thread checks. Those components don't know of AudioRendererSinks or who uses them. I did write down what dchecks we hit, but I couldn't find it now. So that option is unfortunately not viable.
https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { On 2016/05/19 at 13:03:11, Henrik Grunell wrote: > On 2016/05/18 17:20:43, DaleCurtis wrote: > > On 2016/05/18 at 12:21:32, Henrik Grunell wrote: > > > On 2016/05/17 17:54:35, DaleCurtis wrote: > > > > What's the point of doing this versus just restarting the thread each time? > > > > > > The problem with spinning up a new thread each time is that in many places in > > the code we assume render calls on the same thread and have checks for this. > > It's complicated to inform/restart them to accept a new thread. > > > > > > I put this in the CL description too. > > > > I feel like overcomplicating our production code to workaround debug checks is > > not a great course of action. What do you think about introducing an > > AudioRendererSink::CurrentlyOnAudioRenderingThread() type helper method that we > > can replace all of these type checks with? It would simply be a tid comparison > > implemented by the underlying device. > > The problems is more specifically the components used in clients in the render callbacks in various places, also in blink iirc, that have thread checks. Those components don't know of AudioRendererSinks or who uses them. I did write down what dchecks we hit, but I couldn't find it now. So that option is unfortunately not viable. I don't think that's a good enough reason to make this change instead of fixing those call sites; this adds quite a bit of extra complexity to an already complex, high-frequency path to workaround some debug checks. Can you elaborate on why the approach is not viable? It doesn't seem like it should be all that difficult to change how those debug checks work. Even though Blink doesn't know about sinks, it does know about blink::WebAudioDevice which knows about media::AudioRendererSink, so it's seems like it would just be a proxied call away.
https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1703473002/diff/270001/media/audio/audio_devi... media/audio/audio_device_thread.cc:248: while (state_ == STATE_SUSPENDED) { On 2016/05/19 20:41:00, DaleCurtis wrote: > On 2016/05/19 at 13:03:11, Henrik Grunell wrote: > > On 2016/05/18 17:20:43, DaleCurtis wrote: > > > On 2016/05/18 at 12:21:32, Henrik Grunell wrote: > > > > On 2016/05/17 17:54:35, DaleCurtis wrote: > > > > > What's the point of doing this versus just restarting the thread each > time? > > > > > > > > The problem with spinning up a new thread each time is that in many places > in > > > the code we assume render calls on the same thread and have checks for this. > > > It's complicated to inform/restart them to accept a new thread. > > > > > > > > I put this in the CL description too. > > > > > > I feel like overcomplicating our production code to workaround debug checks > is > > > not a great course of action. What do you think about introducing an > > > AudioRendererSink::CurrentlyOnAudioRenderingThread() type helper method that > we > > > can replace all of these type checks with? It would simply be a tid > comparison > > > implemented by the underlying device. > > > > The problems is more specifically the components used in clients in the render > callbacks in various places, also in blink iirc, that have thread checks. Those > components don't know of AudioRendererSinks or who uses them. I did write down > what dchecks we hit, but I couldn't find it now. So that option is unfortunately > not viable. > > I don't think that's a good enough reason to make this change instead of fixing > those call sites; this adds quite a bit of extra complexity to an already > complex, high-frequency path to workaround some debug checks. Can you elaborate > on why the approach is not viable? > > It doesn't seem like it should be all that difficult to change how those debug > checks work. Even though Blink doesn't know about sinks, it does know about > blink::WebAudioDevice which knows about media::AudioRendererSink, so it's seems > like it would just be a proxied call away. Actually, I have brought up this thread re-use approach for discussion before, and you approved of it then. I'll wake up that mail thread and you can take a look at that.
Removed the thread re-use. This CL will be followed by a CL that instead changes the thread checking in WebRTC to deal with thread switches when running. Ptal.
Description was changed from ========== Make AudioOutputDevice restartable and reinitializable. * Added Suspend() and Resume()to AudioDeviceThread. Resume() takes the socket and callback arguments instead of Start(). * Changes to AudioOutputDevice to use the new functions, i.e. suspend the thread in Stop() and resume it in Start(). * Device authorization is not touched in this CL, meaning it will reauthorize each restart. The reason for the new functions Suspend() and Resume() is that clients assume render calls on the same thread, and we need to ensure this when restarting the AudioOutputDevice. This is done by suspending/resuming instead of starting a new thread. The alternative to somehow inform about a new thread is complicated and would require substantial refactoring. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. The code written by myself in this CL and olka@ in https://codereview.chromium.org/1830933002. That CL was forked off this CL and merged back here in patch set 3, along with some other changes. BUG=607916 ========== to ========== Make AudioOutputDevice restartable and reinitializable. * Changing the render callbacks to pass through AOD, only clearing the callback in Stop() and moving stopping of the audio thread to the IO thread. This serializes Initialize(), Start() and Stop() while ensuring no callbacks after Stop(). It also removed the stopping hack flag. * Device authorization is not touched in this CL, meaning it will reauthorize each restart. Following this, it would probably be good to cache the device authorization. This requires mostly browser side changes. BUG=607916 ==========
https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:115: DCHECK(audio_thread_.IsStopped()); This won't be true with the code you have currently, since the OnCreated() may come in later and start the thread (you removed stopping_hack_) https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:128: base::AutoLock auto_lock(render_callback_lock_); This doesn't need a lock since it can only be read on the same thread Stop() is called on. It might be worth having a thread checker for these methods. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:285: // TODO BEFORE COMMIT: Do we need this? I think you still do unless we're okay spinning up an unused thread when the call finally comes through; a simple solution is the WeakPtr one mentioned below. Just invalidate the weakptr used for create such that the return is dropped. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:388: // We can receive OnStreamCreated() on the IO thread after the client has But aren't you still spinning up a new thread in this case? Instead of stopping hack it sounds like we want to just add an internal WeakPtrFactory and cancel vended pointers when Stop() is called? I think if you don't want to do that it might be worth keeping the stopping_hack_ for now. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.h:135: // media::AudioRendererSink::RenderCallback implementation. I don't understand why you have this? Isn't this the same as AudioOutputDevice::AudioThreadCallback::Process() ? I'm a fan of making an OnRenderError() method, but you don't need to inherit RenderCallback for this. The Render() method I would remove since you can do that in ::Process() (it's only called there).
Trying to answer the questions, and added a thread checker. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:115: DCHECK(audio_thread_.IsStopped()); On 2016/06/09 22:37:27, DaleCurtis wrote: > This won't be true with the code you have currently, since the OnCreated() may > come in later and start the thread (you removed stopping_hack_) This class is ref counted so all pending tasks will complete first. After StopOnIOThread() has run there will be no calls from the IPC implementation, so no OnXyz() functions will be called after that, thus when we are in the destructor we can assert that we are stopped. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:128: base::AutoLock auto_lock(render_callback_lock_); On 2016/06/09 22:37:27, DaleCurtis wrote: > This doesn't need a lock since it can only be read on the same thread Stop() is > called on. It might be worth having a thread checker for these methods. Done. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:285: // TODO BEFORE COMMIT: Do we need this? On 2016/06/09 22:37:27, DaleCurtis wrote: > I think you still do unless we're okay spinning up an unused thread when the > call finally comes through; a simple solution is the WeakPtr one mentioned > below. Just invalidate the weakptr used for create such that the return is > dropped. Kept as is. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:388: // We can receive OnStreamCreated() on the IO thread after the client has On 2016/06/09 22:37:27, DaleCurtis wrote: > But aren't you still spinning up a new thread in this case? Instead of stopping > hack it sounds like we want to just add an internal WeakPtrFactory and cancel > vended pointers when Stop() is called? I think if you don't want to do that it > might be worth keeping the stopping_hack_ for now. Yes, a thread will start without needing it, and later it will be stopped. With the stopping hack instead, what happens if we do Stop, Initialize, Start before we get here? It seems like a risk of getting into trouble in this already hacky implementation. To be safe, we would need a stopping hack/weak ptr per Start, was that what you meant? https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.h:135: // media::AudioRendererSink::RenderCallback implementation. On 2016/06/09 22:37:27, DaleCurtis wrote: > I don't understand why you have this? Isn't this the same as > AudioOutputDevice::AudioThreadCallback::Process() ? > > I'm a fan of making an OnRenderError() method, but you don't need to inherit > RenderCallback for this. The Render() method I would remove since you can do > that in ::Process() (it's only called there). The reason is to gate the callback here. After Stop() has returned we must guarantee that there is no more callbacks. And to be able to serialize the Initialize, Start and Stop, which they must be to make it restartable, the stopping hack must go away. This makes the actual stopping of the thread asynchronous and this is used to handle the new situation.
It's not clear why you're extending RenderCallback in this class. I understand you need to protect the callback with a lock and that's fine, but there's no reason that I can see for extending RenderCallback and having a Render() method. You can keep a OnRenderError() method which is guarded by a lock for convenience, but the Render() method should just be inlined into Process(). https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:388: // We can receive OnStreamCreated() on the IO thread after the client has On 2016/06/13 at 13:43:43, Henrik Grunell wrote: > On 2016/06/09 22:37:27, DaleCurtis wrote: > > But aren't you still spinning up a new thread in this case? Instead of stopping > > hack it sounds like we want to just add an internal WeakPtrFactory and cancel > > vended pointers when Stop() is called? I think if you don't want to do that it > > might be worth keeping the stopping_hack_ for now. > > Yes, a thread will start without needing it, and later it will be stopped. With the stopping hack instead, what happens if we do Stop, Initialize, Start before we get here? It seems like a risk of getting into trouble in this already hacky implementation. To be safe, we would need a stopping hack/weak ptr per Start, was that what you meant? Ah, I forgot this is an interface, we can't use a WeakPtr because of that. Since you now clear the callback on stop, why don't we just reject all OnStreamCreated() calls when callback is null? https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.h:135: // media::AudioRendererSink::RenderCallback implementation. On 2016/06/13 at 13:43:43, Henrik Grunell wrote: > On 2016/06/09 22:37:27, DaleCurtis wrote: > > I don't understand why you have this? Isn't this the same as > > AudioOutputDevice::AudioThreadCallback::Process() ? > > > > I'm a fan of making an OnRenderError() method, but you don't need to inherit > > RenderCallback for this. The Render() method I would remove since you can do > > that in ::Process() (it's only called there). > > The reason is to gate the callback here. After Stop() has returned we must guarantee that there is no more callbacks. And to be able to serialize the Initialize, Start and Stop, which they must be to make it restartable, the stopping hack must go away. This makes the actual stopping of the thread asynchronous and this is used to handle the new situation. I'm sorry I don't understand. I don't see any reason why AudioOutputDevice needs to implement the RenderCallback interface. You just need to protect callback_ with a lock.
Ah right, you mean the Callback wrapper Process(). Yep, that makes sense to deal with it there. I need yet another lock there though. It's unfortunate to add a lock at all for the Process() or Render() call, but I don't see another way. https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/310001/media/audio/audio_outp... media/audio/audio_output_device.cc:388: // We can receive OnStreamCreated() on the IO thread after the client has On 2016/06/13 18:52:32, DaleCurtis wrote: > On 2016/06/13 at 13:43:43, Henrik Grunell wrote: > > On 2016/06/09 22:37:27, DaleCurtis wrote: > > > But aren't you still spinning up a new thread in this case? Instead of > stopping > > > hack it sounds like we want to just add an internal WeakPtrFactory and > cancel > > > vended pointers when Stop() is called? I think if you don't want to do that > it > > > might be worth keeping the stopping_hack_ for now. > > > > Yes, a thread will start without needing it, and later it will be stopped. > With the stopping hack instead, what happens if we do Stop, Initialize, Start > before we get here? It seems like a risk of getting into trouble in this already > hacky implementation. To be safe, we would need a stopping hack/weak ptr per > Start, was that what you meant? > > Ah, I forgot this is an interface, we can't use a WeakPtr because of that. Since > you now clear the callback on stop, why don't we just reject all > OnStreamCreated() calls when callback is null? That's a good idea, I'll do that.
On 2016/06/13 at 20:12:20, grunell wrote: > Ah right, you mean the Callback wrapper Process(). Yep, that makes sense to deal with it there. I need yet another lock there though. > > It's unfortunate to add a lock at all for the Process() or Render() call, but I don't see another way. Yes, unless you keep the stopping hack I don't think there's another way to do this. Do you really need to change that hack in this CL? I don't think you do, so you might be making this riskier than you need to.
On 2016/06/13 20:39:11, DaleCurtis wrote: > On 2016/06/13 at 20:12:20, grunell wrote: > > Ah right, you mean the Callback wrapper Process(). Yep, that makes sense to > deal with it there. I need yet another lock there though. > > > > It's unfortunate to add a lock at all for the Process() or Render() call, but > I don't see another way. > > Yes, unless you keep the stopping hack I don't think there's another way to do > this. Do you really need to change that hack in this CL? I don't think you do, > so you might be making this riskier than you need to. I'll take another round looking at using the stopping hack tomorrow.
Patchset #19 (id:350001) has been deleted
And now back to stopping hack... So, I haven't found any issues with this. Please review thoroughly.
Patchset #19 (id:370001) has been deleted
I think there is even more you can revert out of this patch set to reduce your risk. https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:85: thread_checker_.DetachFromThread(); Needs a more specific name in light of your other change for thread checking. https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:96: base::AutoLock auto_lock(lock_); Locking shouldn't be necessary here, right? The thread should be stopped at this point. I don't think locking the callback is required at all anymore since you went back to the stopping hack. Ditto for ReportRenderError. The thread is either running or not, when running it always has a callback. https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:120: { Again, shouldn't be necessary, the thread is stopped here (and should be dchecked as such). Also I don't think you should change this value here, the task will be posted after ShutdownOnIOThread() so by the time it goes through the right value should be set for stopping_hack.
https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:85: thread_checker_.DetachFromThread(); On 2016/06/15 16:51:41, DaleCurtis wrote: > Needs a more specific name in light of your other change for thread checking. Done. https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:96: base::AutoLock auto_lock(lock_); On 2016/06/15 16:51:42, DaleCurtis wrote: > Locking shouldn't be necessary here, right? The thread should be stopped at this > point. I don't think locking the callback is required at all anymore since you > went back to the stopping hack. Ditto for ReportRenderError. The thread is > either running or not, when running it always has a callback. |render_callback_| isn't only coupled with the thread. ReportRenderError is called on the IO thread. If we execute that and get a Stop() call (where it's set to null) it could dereference null. The old code worked without locks since we never cleared the callback. https://codereview.chromium.org/1703473002/diff/390001/media/audio/audio_outp... media/audio/audio_output_device.cc:120: { On 2016/06/15 16:51:41, DaleCurtis wrote: > Again, shouldn't be necessary, the thread is stopped here (and should be > dchecked as such). Also I don't think you should change this value here, the > task will be posted after ShutdownOnIOThread() so by the time it goes through > the right value should be set for stopping_hack. Right, it's cleared in StoponIOThread. That should be enough. Removed.
https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:95: { Drop the lock? DCHECK(!thread.started()) ? https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:284: base::AutoLock auto_lock_(lock_); I'm worried this can deadlock now. You're waiting for the render thread to stop, but something on the render thread might be waiting for the lock.
https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:95: { On 2016/06/16 16:39:14, DaleCurtis wrote: > Drop the lock? DCHECK(!thread.started()) ? SG, I'll do that tomorrow. https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:284: base::AutoLock auto_lock_(lock_); On 2016/06/16 16:39:14, DaleCurtis wrote: > I'm worried this can deadlock now. You're waiting for the render thread to stop, > but something on the render thread might be waiting for the lock. This lock is never grabbed on the audio render thread. Only the control (typically main render) thread and IO thread. Do you find a concrete case? I'll add a thread check in ReportRenderError, and maybe change name. It should only be called on IO thread.
Your CL inspired me to do this https://codereview.chromium.org/2076793002 -- which might clean some things up for you in this one. https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:142: void AudioOutputDevice::Pause() { Note: This is called from the Render thread by RendererWebAudioDeviceImpl. So you should probably say something explicit about the fact that it's okay to call Play()/Pause() from any thread, but Start()/Stop() must be called from the same thread... https://codereview.chromium.org/1703473002/diff/410001/media/audio/audio_outp... media/audio/audio_output_device.cc:284: base::AutoLock auto_lock_(lock_); On 2016/06/16 at 20:23:00, Henrik Grunell wrote: > On 2016/06/16 16:39:14, DaleCurtis wrote: > > I'm worried this can deadlock now. You're waiting for the render thread to stop, > > but something on the render thread might be waiting for the lock. > > This lock is never grabbed on the audio render thread. Only the control (typically main render) thread and IO thread. Do you find a concrete case? I'll add a thread check in ReportRenderError, and maybe change name. It should only be called on IO thread. No, I had forgotten we're not using your old patch that added a lock there. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
