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

Unified Diff: media/audio/mac/audio_output_mac.cc

Issue 7888055: Add a lock around getting and setting of source_ to avoid possible compiler reorderings. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src
Patch Set: rebased Created 9 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « media/audio/mac/audio_output_mac.h ('k') | no next file » | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: media/audio/mac/audio_output_mac.cc
diff --git a/media/audio/mac/audio_output_mac.cc b/media/audio/mac/audio_output_mac.cc
index a81dae01107c0dd8ae315fae14ea4a56d1ace726..02e8c6354dd7479197d5974400bc4568fa679da6 100644
--- a/media/audio/mac/audio_output_mac.cc
+++ b/media/audio/mac/audio_output_mac.cc
@@ -91,7 +91,7 @@ void PCMQueueOutAudioOutputStream::HandleError(OSStatus err) {
// source_ can be set to NULL from another thread. We need to cache its
// pointer while we operate here. Note that does not mean that the source
// has been destroyed.
- AudioSourceCallback* source = source_;
+ AudioSourceCallback* source = GetSource();
if (source)
source->OnError(this, static_cast<int>(err));
NOTREACHED() << "error code " << err;
@@ -157,7 +157,7 @@ bool PCMQueueOutAudioOutputStream::Open() {
// Create the actual queue object and let the OS use its own thread to
// run its CFRunLoop.
err = AudioQueueNewOutput(&format_, RenderCallback, this, NULL,
- kCFRunLoopCommonModes, 0, &audio_queue_);
+ kCFRunLoopCommonModes, 0, &audio_queue_);
if (err != noErr) {
HandleError(err);
return false;
@@ -314,7 +314,8 @@ void PCMQueueOutAudioOutputStream::Close() {
void PCMQueueOutAudioOutputStream::Stop() {
// We request a synchronous stop, so the next call can take some time. In
// the windows implementation we block here as well.
- source_ = NULL;
+ SetSource(NULL);
+
// We set the source to null to signal to the data queueing thread it can stop
// queueing data, however at most one callback might still be in flight which
// could attempt to enqueue right after the next call. Rather that trying to
@@ -378,10 +379,10 @@ bool PCMQueueOutAudioOutputStream::CheckForAdjustedLayout(
return false;
}
-// Note to future hackers of this function: Do not add locks here because we
-// call out to third party source that might do crazy things including adquire
-// external locks or somehow re-enter here because its legal for them to call
-// some audio functions.
+// Note to future hackers of this function: Do not add locks to this function
+// that are held through any calls made back into AudioQueue APIs, or other
+// OS audio functions. This is because the OS dispatch may grab external
+// locks, or possibly re-enter this function which can lead to a deadlock.
void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
AudioQueueRef queue,
AudioQueueBufferRef buffer) {
@@ -391,7 +392,7 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
static_cast<PCMQueueOutAudioOutputStream*>(p_this);
// Call the audio source to fill the free buffer with data. Not having a
// source means that the queue has been closed. This is not an error.
- AudioSourceCallback* source = audio_stream->source_;
+ AudioSourceCallback* source = audio_stream->GetSource();
if (!source)
return;
@@ -467,8 +468,9 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
if (err == kAudioQueueErr_EnqueueDuringReset) {
// This is the error you get if you try to enqueue a buffer and the
// queue has been closed. Not really a problem if indeed the queue
- // has been closed.
- if (!audio_stream->source_)
+ // has been closed. We recheck the value of source now to see if it has
+ // indeed been closed.
+ if (!audio_stream->GetSource())
return;
}
audio_stream->HandleError(err);
@@ -478,7 +480,7 @@ void PCMQueueOutAudioOutputStream::RenderCallback(void* p_this,
void PCMQueueOutAudioOutputStream::Start(AudioSourceCallback* callback) {
DCHECK(callback);
OSStatus err = noErr;
- source_ = callback;
+ SetSource(callback);
pending_bytes_ = 0;
// Ask the source to pre-fill all our buffers before playing.
for (uint32 ix = 0; ix != kNumBuffers; ++ix) {
@@ -500,3 +502,14 @@ void PCMQueueOutAudioOutputStream::Start(AudioSourceCallback* callback) {
return;
}
}
+
+void PCMQueueOutAudioOutputStream::SetSource(AudioSourceCallback* source) {
+ base::AutoLock lock(source_lock_);
+ source_ = source;
+}
+
+AudioOutputStream::AudioSourceCallback*
+PCMQueueOutAudioOutputStream::GetSource() {
+ base::AutoLock lock(source_lock_);
+ return source_;
+}
« no previous file with comments | « media/audio/mac/audio_output_mac.h ('k') | no next file » | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698