Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(98)

Issue 9264013: Simplify shutdown of AudioDevice's audio thread. (Closed)

Created:
8 years, 11 months ago by tommi (sloooow) - chröme
Modified:
8 years, 11 months ago
Reviewers:
enal1, enal
CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org
Visibility:
Public.

Description

Simplify shutdown of AudioDevice's audio thread. This is a followup change for bug 107933 (see code review 9112029) that does the following: * Makes Stop() asynchronous. * Simplifies shutdown of AudioDevice's audio thread by always doing it from the IO thread. * Simplifies access to of most member variables (added documentation). * We no longer need synchronisation with the IO thread when shutting down the audio thread since we'll always shut it down from the IO thread. * We don't need a lock to guard the audio_thread_ variable (and others related ones), since they're only modifed on the IO thread when the audio thread is not running. I checked the implementations of AudioDevice::RenderCallback if this is OK and it should be fine. BUG=107933 TEST=Run media and content tests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=118810

Patch Set 1 #

Total comments: 2

Patch Set 2 : Removed incorrect call to ShutDownAudioThread from OnLowLatencyCreated #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -45 lines) Patch
M content/renderer/media/audio_device.h View 4 chunks +7 lines, -7 lines 0 comments Download
M content/renderer/media/audio_device.cc View 1 9 chunks +21 lines, -38 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
tommi (sloooow) - chröme
8 years, 11 months ago (2012-01-19 16:37:52 UTC) #1
enal1
https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (right): https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/audio_device.cc#newcode240 content/renderer/media/audio_device.cc:240: ShutDownAudioThread(); I don't fully understand the comment. Do you ...
8 years, 11 months ago (2012-01-20 23:09:45 UTC) #2
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (right): https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/audio_device.cc#newcode240 content/renderer/media/audio_device.cc:240: ShutDownAudioThread(); On 2012/01/20 23:09:46, enal1 wrote: > I don't ...
8 years, 11 months ago (2012-01-23 10:17:48 UTC) #3
enal1
8 years, 11 months ago (2012-01-23 21:56:23 UTC) #4
lgtm.

On 2012/01/23 10:17:48, tommi wrote:
>
https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/...
> File content/renderer/media/audio_device.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9264013/diff/1/content/renderer/media/...
> content/renderer/media/audio_device.cc:240: ShutDownAudioThread();
> On 2012/01/20 23:09:46, enal1 wrote:
> > I don't fully understand the comment. Do you mean "if there was shut down
task
> > posted, shut down now, and let the task that would be executed in the future
> to
> > shut down this stream, even if this stream has nothing to do with stream it
> was
> > supposed to stop?"
> > 
> > That is why I felt uneasy with making Stop() asynchronous...
> 
> You're right, I don't need to do this and I shouldn't.
> 
> (mostly repeating for my own sanity:)
> Since both Start() and Stop() are now serialized via the IO thread, we don't
> really have this problem.
> If Start() is called while a shutdown is taking place, then the actual
> "InitializeOnIOThread" task, won't run until the AudioDevice has been
completely
> shut down.  So, when we get the OnLowLatencyCreated notification, it can only
be
> for a new stream and if there was a previous one, it has already been shut
down
> correctly.
> 
> Thanks for catching this.  I removed this call.

Powered by Google App Engine
This is Rietveld 408576698