Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2076793002:
Cleanup AudioDeviceThread to reduce locking and unused features. (Closed)
|
Created:
4 years, 6 months ago by DaleCurtis Modified:
4 years, 5 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionCleanup AudioDeviceThread to reduce locking and unused features.
While reviewing some of the other AudioOutputDevice changes, I
noticed this class is exceedingly crufty. This removes an extra
layer and associated locking from the audio callback thread.
AudioInputDevice/AudioOutputDevice both always use synchronized
buffers now, so that option is no longer necessary. Removes the
silly ref-counting from AudioDeviceThread in favor of ownership
by an already ref counted class :)
Removes the Start/Stop API from AudioDeviceThread since it's
easier to just construct and destruct it.
BUG=612792
TEST=audio plays, all unittests still pass.
Committed: https://crrev.com/47f99faeb277c3fbd9eb709f803ce788b9c1e64f
Cr-Commit-Position: refs/heads/master@{#400734}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Remove silliness. #Patch Set 3 : Rebase. Relocate one-time use variables. #
Messages
Total messages: 32 (12 generated)
dalecurtis@chromium.org changed reviewers: + grunell@chromium.org, tommi@chromium.org
While reviewing grunell@'s CL this mess became apparent, cleaned up!
The CQ bit was checked by dalecurtis@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/2076793002/1
https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:42: CHECK(shared_memory_.memory()); should there be a [D]CHECK for memory() being nullptr at the beginning of the function? Henrik had a dcheck + comment that InitializeOnAudioThread wasn't guaranteed to be called on the same thread always, but I think it should be called exactly once (right?). https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: thread_handle_ = base::PlatformThreadHandle(); I think we'll need to fix this before checkin. Ideally, the handle should always be closed on the same thread. https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_input_dev... File media/audio/audio_input_device.cc (right): https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_input_dev... media/audio/audio_input_device.cc:187: base::AutoLock auto_lock_(audio_thread_lock_); auto_lock
https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:42: CHECK(shared_memory_.memory()); On 2016/06/16 at 22:19:22, tommi-chrömium wrote: > should there be a [D]CHECK for memory() being nullptr at the beginning of the function? > Henrik had a dcheck + comment that InitializeOnAudioThread wasn't guaranteed to be called on the same thread always, but I think it should be called exactly once (right?). It's called in the ThreadMain() below so it must be exactly once. I'll add the DCHECK. https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: thread_handle_ = base::PlatformThreadHandle(); On 2016/06/16 at 22:19:22, tommi-chrömium wrote: > I think we'll need to fix this before checkin. Ideally, the handle should always be closed on the same thread. What do you think about changing this to a shutdown_signal_.Wait()? And signaling during destruction before the Join?
https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: thread_handle_ = base::PlatformThreadHandle(); On 2016/06/16 22:24:59, DaleCurtis wrote: > On 2016/06/16 at 22:19:22, tommi-chrömium wrote: > > I think we'll need to fix this before checkin. Ideally, the handle should > always be closed on the same thread. > > What do you think about changing this to a shutdown_signal_.Wait()? And > signaling during destruction before the Join? Hmm... this is both the normal and abnormal exit path. How could we join the wrong thread during a later stop? Even though it has exited, how could we join the wrong thread? (Is it a pthread thing?) It does seem like we're missing a notification to the callback that the thread is exiting. If we had that, the owner of the thread could trigger the teardown by e.g. posting a task to the thread that actually owns this one. Possibly, the implementation of such a signal, could be done by moving ownership of the callback to the AudioDeviceThread and always delete the callback object before exiting the thread (i.e. the dtor would be the signal).
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. Kind of fixes an existing issue where if the thread exits early we may end up joining the wrong thread later... the fix could race, but is better than nothing. BUG=none TEST=audio plays, all unittests still pass. ========== to ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. BUG=none TEST=audio plays, all unittests still pass. ==========
https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... File media/audio/audio_device_thread.cc (right): https://codereview.chromium.org/2076793002/diff/1/media/audio/audio_device_th... media/audio/audio_device_thread.cc:106: thread_handle_ = base::PlatformThreadHandle(); On 2016/06/16 at 22:36:18, tommi-chrömium wrote: > On 2016/06/16 22:24:59, DaleCurtis wrote: > > On 2016/06/16 at 22:19:22, tommi-chrömium wrote: > > > I think we'll need to fix this before checkin. Ideally, the handle should > > always be closed on the same thread. > > > > What do you think about changing this to a shutdown_signal_.Wait()? And > > signaling during destruction before the Join? > > Hmm... this is both the normal and abnormal exit path. How could we join the wrong thread during a later stop? Even though it has exited, how could we join the wrong thread? (Is it a pthread thing?) > > It does seem like we're missing a notification to the callback that the thread is exiting. If we had that, the owner of the thread could trigger the teardown by e.g. posting a task to the thread that actually owns this one. > Possibly, the implementation of such a signal, could be done by moving ownership of the callback to the AudioDeviceThread and always delete the callback object before exiting the thread (i.e. the dtor would be the signal). Derp, I should actually do a bit more research before declaring something a bug. I was wrong, just because ThreadMain() exits, doesn't mean that the |thread_handle_| is invalid. Removed this silliness.
The CQ bit was checked by tommi@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/2076793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Lgtm. Great simplification. I hope everything continues to work :-)
Thanks! Me too :) Henrik, any comments?
lgtm This makes sense now when we won't re-use the thread (removing the extra layer would have made sense still). Thanks for doing this!
On 2016/06/20 07:39:27, Henrik Grunell wrote: > lgtm > > This makes sense now when we won't re-use the thread (removing the extra layer > would have made sense still). Thanks for doing this! I don't see any problems with this, afaict the only reason we cleared |callback_| in Stop() before was if we joined the thread on another message loop. But obviously that isn't used anymore. So nice clean-up!
The CQ bit was checked by dalecurtis@chromium.org
Thanks for the reviews!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076793002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grunell@chromium.org, tommi@chromium.org Link to the patchset: https://codereview.chromium.org/2076793002/#ps60001 (title: "Rebase. Relocate one-time use variables.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2076793002/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. BUG=none TEST=audio plays, all unittests still pass. ========== to ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. BUG=none TEST=audio plays, all unittests still pass. Committed: https://crrev.com/47f99faeb277c3fbd9eb709f803ce788b9c1e64f Cr-Commit-Position: refs/heads/master@{#400734} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/47f99faeb277c3fbd9eb709f803ce788b9c1e64f Cr-Commit-Position: refs/heads/master@{#400734}
Message was sent while issue was closed.
Description was changed from ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. BUG=none TEST=audio plays, all unittests still pass. Committed: https://crrev.com/47f99faeb277c3fbd9eb709f803ce788b9c1e64f Cr-Commit-Position: refs/heads/master@{#400734} ========== to ========== Cleanup AudioDeviceThread to reduce locking and unused features. While reviewing some of the other AudioOutputDevice changes, I noticed this class is exceedingly crufty. This removes an extra layer and associated locking from the audio callback thread. AudioInputDevice/AudioOutputDevice both always use synchronized buffers now, so that option is no longer necessary. Removes the silly ref-counting from AudioDeviceThread in favor of ownership by an already ref counted class :) Removes the Start/Stop API from AudioDeviceThread since it's easier to just construct and destruct it. BUG=612792 TEST=audio plays, all unittests still pass. Committed: https://crrev.com/47f99faeb277c3fbd9eb709f803ce788b9c1e64f Cr-Commit-Position: refs/heads/master@{#400734} ==========
Message was sent while issue was closed.
Just FYI: There is a bug for removing the extra class layer, added it to the description. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
