Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1830933002:
Making AudioThread::Thread restartable *** DRAFT *** (Closed)
Description*** This CL has been merged into https://codereview.chromium.org/1703473002/ ***
Making AudioThread::Thread restartable *** DRAFT ***
Based on Henrik's CL https://codereview.chromium.org/1703473002/
I changed audio_device_thread.cc and added some comments to audio_output_device.cc.
It's a compilable sketch, not a tested implementaion. Actually, it won't work,
because AudioOutoutDevice needs some fixes (I commented on that).
We need to cleanup AOD, and probably convert AudioDeviceThread into an interface.
(I'm not sure an extra lock it adds is needed.)
Actually, can we probably invest some time in AOD/AID refactoring? They share
quite a lot of the same code which is copy-pasted. Also, it would be nice
to get rid of "stopping hack" and probably even make AOD/AID non-refcounted if
we can clarify all the object lifetime stuff.
BUG=
Patch Set 1 #
Total comments: 55
Messages
Total messages: 14 (4 generated)
olka@chromium.org changed reviewers: + grunell@chromium.org, tommi@chromium.org
Hej, I sketched some draft for "restartable" based on Henrik's CL, using condition instead of event (see CL description). Could you take a look if you see any races/other problems, or they are hidden well enough? :) Thanks, Olga
One critical comment about the lock and condition variable. You can basically skip the other comments for now. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:83: ResumeAction action_; Is this action to perform, or ongoing, or something else? (Add comment.) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:97: // AudioDeviceThread implementation TODO(olka): get rid of it. It just adds one Nit: Start TODO comment on new line. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: base::AutoLock auto_lock(thread_lock_); If we decide to keep this layer/class, I wonder if we can use require this object be called on the same thread? Does the AudioOutputDevice call them on the same thread? https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:123: thread_->Play(callback, socket); Play() then Stop() could be called on different threads, and if Stop() is executed first, we need a if (thread_.get()) here. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:171: // NB: Stop() can be called any time, even before Start(), because it's What does NB mean? https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:246: action_condition_.Wait(); We have the lock when waiting here, so we won't be able to signal in Play() etc. since we take the lock there. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.h:111: // Not sure if we need this extra class and extra lock? Yeah, does the below motivation hold? It seems pretty old, it refers to SimpleThread, but we use PlatformThread, so maybe the need for wrapping is obsolete.
Henrik's questions answered. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:83: ResumeAction action_; On 2016/03/25 09:24:01, Henrik Grunell wrote: > Is this action to perform, or ongoing, or something else? (Add comment.) How would you call it? https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:97: // AudioDeviceThread implementation TODO(olka): get rid of it. It just adds one On 2016/03/25 09:24:01, Henrik Grunell wrote: > Nit: Start TODO comment on new line. Oh c'mon, this is a draft! :) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: base::AutoLock auto_lock(thread_lock_); On 2016/03/25 09:24:01, Henrik Grunell wrote: > If we decide to keep this layer/class, I wonder if we can use require this > object be called on the same thread? Does the AudioOutputDevice call them on the > same thread? I think you know better since you digged into AOD already. My understanding is that as of now, Pause() and Stop() can be called from a different thread, and Pause() should be fixed. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:123: thread_->Play(callback, socket); On 2016/03/25 09:24:01, Henrik Grunell wrote: > Play() then Stop() could be called on different threads, and if Stop() is > executed first, we need a > if (thread_.get()) > here. Good catch! This part is inherited from the original CL though, and I did not touch it, since I hope we are getting rig of the whole class. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:171: // NB: Stop() can be called any time, even before Start(), because it's On 2016/03/25 09:24:01, Henrik Grunell wrote: > What does NB mean? https://en.wikipedia.org/wiki/Nota_bene :) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:246: action_condition_.Wait(); On 2016/03/25 09:24:00, Henrik Grunell wrote: > We have the lock when waiting here, so we won't be able to signal in Play() etc. > since we take the lock there. We are fine: condition variable releases the lock on wait and re-acquires it when condition is signaled.
Looked through AudioDeviceThread for races. Looks good! For AudioDeviceThread, before doing any bigger refactorings we should take into account that the old IPC will be moved to mojo fairly soon and threading for those messages will change. Let's discuss how to move forward on the remaining fixes needed in AudioOutputDevice. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:81: base::Lock action_lock_; // Protects everything below. |socket_| is not protected by the lock, but only accessed on audio thread, or ShutDown() on control thread, which is fine. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:83: ResumeAction action_; On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > Is this action to perform, or ongoing, or something else? (Add comment.) > > How would you call it? Sorry for the quick first comment. When I just read this code, I didn't understand it from the declaration only. Let's get back to this detail later. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:97: // AudioDeviceThread implementation TODO(olka): get rid of it. It just adds one On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > Nit: Start TODO comment on new line. > > Oh c'mon, this is a draft! :) Ah, well, OK then. :) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: base::AutoLock auto_lock(thread_lock_); On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > If we decide to keep this layer/class, I wonder if we can use require this > > object be called on the same thread? Does the AudioOutputDevice call them on > the > > same thread? > > I think you know better since you digged into AOD already. My understanding is > that as of now, Pause() and Stop() can be called from a different thread, and > Pause() should be fixed. Yeah, it does all on the IO thread, but the Stop() hack thing. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:123: thread_->Play(callback, socket); On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > Play() then Stop() could be called on different threads, and if Stop() is > > executed first, we need a > > if (thread_.get()) > > here. > > Good catch! This part is inherited from the original CL though, and I did not > touch it, since I hope we are getting rig of the whole class. OK, sg. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:246: action_condition_.Wait(); On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:00, Henrik Grunell wrote: > > We have the lock when waiting here, so we won't be able to signal in Play() > etc. > > since we take the lock there. > > We are fine: condition variable releases the lock on wait and re-acquires it > when condition is signaled. Oops, sorry, I was too fast reviewing this. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.h:47: // 2. Control thread (may be the main render thread or another thread). Hmm, "Start(), Stop(), Play(), Pause(), SetVolume() must be called on the same thread." But there are no thread checks. What about adding that?
Description was changed from ========== Making AudioThread::Thread restartable *** DRAFT *** Based on Henrik's CL https://codereview.chromium.org/1703473002/ I changed audio_device_thread.cc and added some comments to audio_output_device.cc. It's a compilable sketch, not a tested implementaion. Actually, it won't work, because AudioOutoutDevice needs some fixes (I commented on that). We need to cleanup AOD, and probably convert AudioDeviceThread into an interface. (I'm not sure an extra lock it adds is needed.) Actually, can we probably invest some time in AOD/AID refactoring? They share quite a lot of the same code which is copy-pasted. Also, it would be nice to get rid of "stopping hack" and probably even make AOD/AID non-refcounted if we can clarify all the object lifetime stuff. BUG= ========== to ========== Making AudioThread::Thread restartable *** DRAFT *** Based on Henrik's CL https://codereview.chromium.org/1703473002/ I changed audio_device_thread.cc and added some comments to audio_output_device.cc. It's a compilable sketch, not a tested implementaion. Actually, it won't work, because AudioOutoutDevice needs some fixes (I commented on that). We need to cleanup AOD, and probably convert AudioDeviceThread into an interface. (I'm not sure an extra lock it adds is needed.) Actually, can we probably invest some time in AOD/AID refactoring? They share quite a lot of the same code which is copy-pasted. Also, it would be nice to get rid of "stopping hack" and probably even make AOD/AID non-refcounted if we can clarify all the object lifetime stuff. BUG= ==========
olka@chromium.org changed reviewers: + guidou@chromium.org
guidou@, could you take a look? (see the description, it's a draft!). Specifically, we are very interested in what you think of AudioDeviceThread::Thread, and what the problem may be with making AOD::Stop() to post on IO thread instead of synchronous execution.
https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:81: base::Lock action_lock_; // Protects everything below. On 2016/03/31 09:01:36, Henrik Grunell wrote: > |socket_| is not protected by the lock, but only accessed on audio thread, or > ShutDown() on control thread, which is fine. |socket_| IS protected by the lock. It can be reset in ThreadMain, this is done under the lock, as well as Stop() shuts it down under the lock, to avoid a race. All rest of socket access in threadMain is not under the lock, since the only race we need to avoid is related to shutdown. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:178: ; remove ;
https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:87: // Reset on client threaid. shutdown on client thread, reset in thread main.
https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:59: enum ResumeAction { kActionPause, kActionPlay, kActionStop }; It seems to me that this represents a state instead of an action. What about calling this enum simply State and the values kStatePaused, kStatePlaying and kStateStopped? https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:68: // Runs the loop that reads from the socket, called in ThreadMain(), s/,/. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:82: base::ConditionVariable action_condition_; Can you use a WaitableEvent instead of a ConditionVariable? My understanding is that the ConditionVariable has worse performance on Windows, although that might have changed. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:83: ResumeAction action_; On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > Is this action to perform, or ongoing, or something else? (Add comment.) > > How would you call it? I would call it state_ :) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:102: AudioDeviceThread::~AudioDeviceThread() { DCHECK(!thread_.get()); } Should action_ have a particular value at destruction time? https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:171: // NB: Stop() can be called any time, even before Start(), because it's On 2016/03/29 08:49:17, o1ka wrote: > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > What does NB mean? > > https://en.wikipedia.org/wiki/Nota_bene :) Probably better to remove the NB :) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.h:97: // Releases the thread from wait mode so that it starts running with the given Nit: Release is perhaps not the most familiar term for this. What about something like "Puts the thread in running mode" or something like that. Note that you use this wording in the documentation of Pause(). https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:82: DCHECK(!callback_) << "Calling Initialize() twice?"; Why should Initialize by async? A common idiom is AOD::Initialize(); AOD::Start(); That will no longer work if Initialize is asynchronous since the AOD might not be inizialized yet when AOD::Start() is called. Also, the DCHECK(!callback_) will not work if the intent was to prevent dual calls to Initialize(). And you would need a lock to protect callback_ https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:91: // to fix object lifetime issues. Probably we can do it in a cleaner way. The purpose of mandatory Stop() is to make sure you clean up resources on the browser side. For example, if you never call AOD::Start(), the browser will keep authorization data related to the AOD. That data is removed with Stop() (also with Start(), but if you call Start() then you have to call Stop() to remove the stream on the browser). https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:276: // Stop(). In most cases, the thread will already be stopped. now you call Pause() instead of Stop()
Some comments I forgot to post on Thursday, plus a new one. Main things are that AOD::Stop() is still mandatory since it cleans up things on browser side, and that when AOD::Stop() returns there must be no more callbacks. I'm looking at these two things. Caching authorization requires a bit of work on the browser side, so I think that should go in a separate future CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:81: base::Lock action_lock_; // Protects everything below. On 2016/03/31 10:10:56, o1ka wrote: > On 2016/03/31 09:01:36, Henrik Grunell wrote: > > |socket_| is not protected by the lock, but only accessed on audio thread, or > > ShutDown() on control thread, which is fine. > > |socket_| IS protected by the lock. It can be reset in ThreadMain, this is done > under the lock, as well as Stop() shuts it down under the lock, to avoid a race. > All rest of socket access in threadMain is not under the lock, since the only > race we need to avoid is related to shutdown. Ah, right. But does it seems that we could call Play() then Pause(), and maybe |socket_| isn't initialized in between. We'd dereference nullptr in Pause(). We need to add a condition for doing Shutdown(). https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:170: void AudioDeviceThread::Thread::Stop(base::MessageLoop* loop_for_join) { We must guarantee that after Stop() or Pause() have returned, the callback will not be run. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:82: DCHECK(!callback_) << "Calling Initialize() twice?"; On 2016/04/01 10:17:02, Guido Urdaneta wrote: > Why should Initialize by async? > > A common idiom is > AOD::Initialize(); > AOD::Start(); > > That will no longer work if Initialize is asynchronous since the AOD might not > be inizialized yet when AOD::Start() is called. > > Also, the DCHECK(!callback_) will not work if the intent was to prevent dual > calls to Initialize(). And you would need a lock to protect callback_ Initialize() and Start() are serialized, so we're fine. They should only post on IO thread, nothing else. The DCHECK here and in Start() must be removed. (Or moved to IO thread function counterpart.) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:91: // to fix object lifetime issues. Probably we can do it in a cleaner way. On 2016/04/01 10:17:01, Guido Urdaneta wrote: > The purpose of mandatory Stop() is to make sure you clean up resources on the > browser side. For example, if you never call AOD::Start(), the browser will keep > authorization data related to the AOD. That data is removed with Stop() (also > with Start(), but if you call Start() then you have to call Stop() to remove the > stream on the browser). I wonder if we can post a task on the IO thread in the dtor to clean this up. This should work if the data is guaranteed to be cleaned up when the IPC channel goes away. (Needed since the IO thread may have gone away when the dtor is run.) https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:116: // on IO thread only. The problem here is that after Stop() returns, the callback can go away, this is the reason for stopping the audio thread right away in Stop(), and having |stopping_hack_|. We must ensure the callback isn't called or that it outlives AOD (see comment in OnStreamCreated()). olka@ and I discussed offline to remove ref counting on AOD, similar work is currently done for AudioInputDevice.
I've addressed all comments here in CL https://codereview.chromium.org/1703473002/. Note that all further reviewing is done in that CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:59: enum ResumeAction { kActionPause, kActionPlay, kActionStop }; On 2016/04/01 10:17:01, Guido Urdaneta wrote: > It seems to me that this represents a state instead of an action. > What about calling this enum simply State and the values kStatePaused, > kStatePlaying and kStateStopped? Agree. Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:68: // Runs the loop that reads from the socket, called in ThreadMain(), On 2016/04/01 10:17:01, Guido Urdaneta wrote: > s/,/. Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:81: base::Lock action_lock_; // Protects everything below. On 2016/04/04 08:08:59, Henrik Grunell wrote: > On 2016/03/31 10:10:56, o1ka wrote: > > On 2016/03/31 09:01:36, Henrik Grunell wrote: > > > |socket_| is not protected by the lock, but only accessed on audio thread, > or > > > ShutDown() on control thread, which is fine. > > > > |socket_| IS protected by the lock. It can be reset in ThreadMain, this is > done > > under the lock, as well as Stop() shuts it down under the lock, to avoid a > race. > > All rest of socket access in threadMain is not under the lock, since the only > > race we need to avoid is related to shutdown. > > Ah, right. But does it seems that we could call Play() then Pause(), and maybe > |socket_| isn't initialized in between. We'd dereference nullptr in Pause(). We > need to add a condition for doing Shutdown(). Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:82: base::ConditionVariable action_condition_; On 2016/04/01 10:17:01, Guido Urdaneta wrote: > Can you use a WaitableEvent instead of a ConditionVariable? > My understanding is that the ConditionVariable has worse performance on Windows, > although that might have changed. This simplifies the locking and state checking in ThreadMain(). https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:83: ResumeAction action_; On 2016/04/01 10:17:01, Guido Urdaneta wrote: > On 2016/03/29 08:49:17, o1ka wrote: > > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > > Is this action to perform, or ongoing, or something else? (Add comment.) > > > > How would you call it? > > I would call it state_ :) Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:87: // Reset on client threaid. On 2016/03/31 11:27:15, o1ka wrote: > shutdown on client thread, reset in thread main. Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:102: AudioDeviceThread::~AudioDeviceThread() { DCHECK(!thread_.get()); } On 2016/04/01 10:17:01, Guido Urdaneta wrote: > Should action_ have a particular value at destruction time? Good point. (Although in ADT::Thread::~Thread().) Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: base::AutoLock auto_lock(thread_lock_); On 2016/03/31 09:01:36, Henrik Grunell wrote: > On 2016/03/29 08:49:17, o1ka wrote: > > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > > If we decide to keep this layer/class, I wonder if we can use require this > > > object be called on the same thread? Does the AudioOutputDevice call them on > > the > > > same thread? > > > > I think you know better since you digged into AOD already. My understanding is > > that as of now, Pause() and Stop() can be called from a different thread, and > > Pause() should be fixed. > > Yeah, it does all on the IO thread, but the Stop() hack thing. There is actually thread checkers in ADT::Thread::Start(), Play() (renamed to Resume() in other CL) and Pause(). That should be enough. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:123: thread_->Play(callback, socket); On 2016/03/31 09:01:36, Henrik Grunell wrote: > On 2016/03/29 08:49:17, o1ka wrote: > > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > > Play() then Stop() could be called on different threads, and if Stop() is > > > executed first, we need a > > > if (thread_.get()) > > > here. > > > > Good catch! This part is inherited from the original CL though, and I did not > > touch it, since I hope we are getting rig of the whole class. > > OK, sg. Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:170: void AudioDeviceThread::Thread::Stop(base::MessageLoop* loop_for_join) { On 2016/04/04 08:08:59, Henrik Grunell wrote: > We must guarantee that after Stop() or Pause() have returned, the callback will > not be run. This is not part of the contract anymore. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:171: // NB: Stop() can be called any time, even before Start(), because it's On 2016/04/01 10:17:01, Guido Urdaneta wrote: > On 2016/03/29 08:49:17, o1ka wrote: > > On 2016/03/25 09:24:01, Henrik Grunell wrote: > > > What does NB mean? > > > > https://en.wikipedia.org/wiki/Nota_bene :) > > Probably better to remove the NB :) Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:178: ; On 2016/03/31 10:10:56, o1ka wrote: > remove ; Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.h (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.h:97: // Releases the thread from wait mode so that it starts running with the given On 2016/04/01 10:17:01, Guido Urdaneta wrote: > Nit: Release is perhaps not the most familiar term for this. > What about something like "Puts the thread in running mode" or something like > that. Note that you use this wording in the documentation of Pause(). Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.h:111: // Not sure if we need this extra class and extra lock? On 2016/03/25 09:24:01, Henrik Grunell wrote: > Yeah, does the below motivation hold? It seems pretty old, it refers to > SimpleThread, but we use PlatformThread, so maybe the need for wrapping is > obsolete. Changed to TODO in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.cc (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:116: // on IO thread only. On 2016/04/04 08:08:59, Henrik Grunell wrote: > The problem here is that after Stop() returns, the callback can go away, this is > the reason for stopping the audio thread right away in Stop(), and having > |stopping_hack_|. We must ensure the callback isn't called or that it outlives > AOD (see comment in OnStreamCreated()). > > olka@ and I discussed offline to remove ref counting on AOD, similar work is > currently done for AudioInputDevice. This has been addressed in the other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.cc:276: // Stop(). In most cases, the thread will already be stopped. On 2016/04/01 10:17:01, Guido Urdaneta wrote: > now you call Pause() instead of Stop() Done in other CL. https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... File media/audio/audio_output_device.h (right): https://codereview.chromium.org/1830933002/diff/1/media/audio/audio_output_de... media/audio/audio_output_device.h:47: // 2. Control thread (may be the main render thread or another thread). On 2016/03/31 09:01:37, Henrik Grunell wrote: > Hmm, "Start(), Stop(), Play(), Pause(), SetVolume() must be called on the same > thread." But there are no thread checks. What about adding that? This comment is obsolete. For example, Pause() is called in the Render() callback function in the mixer. Removed in other CL.
Description was changed from ========== Making AudioThread::Thread restartable *** DRAFT *** Based on Henrik's CL https://codereview.chromium.org/1703473002/ I changed audio_device_thread.cc and added some comments to audio_output_device.cc. It's a compilable sketch, not a tested implementaion. Actually, it won't work, because AudioOutoutDevice needs some fixes (I commented on that). We need to cleanup AOD, and probably convert AudioDeviceThread into an interface. (I'm not sure an extra lock it adds is needed.) Actually, can we probably invest some time in AOD/AID refactoring? They share quite a lot of the same code which is copy-pasted. Also, it would be nice to get rid of "stopping hack" and probably even make AOD/AID non-refcounted if we can clarify all the object lifetime stuff. BUG= ========== to ========== *** This CL has been merged into https://codereview.chromium.org/1703473002/ *** Making AudioThread::Thread restartable *** DRAFT *** Based on Henrik's CL https://codereview.chromium.org/1703473002/ I changed audio_device_thread.cc and added some comments to audio_output_device.cc. It's a compilable sketch, not a tested implementaion. Actually, it won't work, because AudioOutoutDevice needs some fixes (I commented on that). We need to cleanup AOD, and probably convert AudioDeviceThread into an interface. (I'm not sure an extra lock it adds is needed.) Actually, can we probably invest some time in AOD/AID refactoring? They share quite a lot of the same code which is copy-pasted. Also, it would be nice to get rid of "stopping hack" and probably even make AOD/AID non-refcounted if we can clarify all the object lifetime stuff. BUG= ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
