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

Issue 102693003: Fix flaky AudioStreamHandler initialization. (Closed)

Created:
7 years ago by DaleCurtis
Modified:
7 years ago
CC:
chromium-reviews, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Fix flaky AudioStreamHandler initialization. AudioStreamHandler was calling into the AudioManager on the wrong thread. On OSX this will lead to undefined behavior since the CoreAudio API is not thread safe. Sadly due to antique unit tests, we don't have the proper DCHECKs in place to catch bad behavior automatically. I've filed the bug below to track this effort. In this case we can simply use a fixed buffer size since ASH is using MakeAudioOutputStreamProxy() which will take care of any necessary rebuffering and resampling. BUG=325373 TEST=none R=scherkus@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238484

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -6 lines) Patch
M media/audio/sounds/audio_stream_handler.cc View 2 chunks +8 lines, -6 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
DaleCurtis
Found due to the wedge-fix CL affecting a hidden race outcome... :-/
7 years ago (2013-12-03 19:50:29 UTC) #1
scherkus (not reviewing)
lgtm
7 years ago (2013-12-03 21:23:55 UTC) #2
DaleCurtis
Committed patchset #1 manually as r238484 (presubmit successful).
7 years ago (2013-12-03 23:34:09 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/102693003/1
7 years ago (2013-12-04 00:07:37 UTC) #4
commit-bot: I haz the power
7 years ago (2013-12-04 00:07:39 UTC) #5
Message was sent while issue was closed.
Failed to apply patch for media/audio/sounds/audio_stream_handler.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/audio/sounds/audio_stream_handler.cc
  Hunk #1 succeeded at 22 with fuzz 2 (offset 3 lines).
  Hunk #2 FAILED at 127.
  1 out of 2 hunks FAILED -- saving rejects to file
media/audio/sounds/audio_stream_handler.cc.rej

Patch:       media/audio/sounds/audio_stream_handler.cc
Index: media/audio/sounds/audio_stream_handler.cc
diff --git a/media/audio/sounds/audio_stream_handler.cc
b/media/audio/sounds/audio_stream_handler.cc
index
141392c1c3c1a7debec8c74d7f65717bc6de8f4f..08608ac4187aa0863b777e5e42701f2b7a327f69
100644
--- a/media/audio/sounds/audio_stream_handler.cc
+++ b/media/audio/sounds/audio_stream_handler.cc
@@ -19,6 +19,9 @@ namespace {
 // Volume percent.
 const double kOutputVolumePercent = 0.8;
 
+// The number of frames each OnMoreData() call will request.
+const int kDefaultFrameCount = 1024;
+
 AudioStreamHandler::TestObserver* g_observer_for_testing = NULL;
 AudioOutputStream::AudioSourceCallback* g_audio_source_for_testing = NULL;
 
@@ -124,12 +127,11 @@ AudioStreamHandler::AudioStreamHandler(const
base::StringPiece& wav_data)
     LOG(ERROR) << "Can't get access to audio manager.";
     return;
   }
-  AudioParameters params(
-      AudioParameters::AUDIO_PCM_LOW_LATENCY,
-      GuessChannelLayout(wav_audio_.num_channels()),
-      wav_audio_.sample_rate(),
-      wav_audio_.bits_per_sample(),
-      manager->GetDefaultOutputStreamParameters().frames_per_buffer());
+  AudioParameters params(AudioParameters::AUDIO_PCM_LOW_LATENCY,
+                         GuessChannelLayout(wav_audio_.num_channels()),
+                         wav_audio_.sample_rate(),
+                         wav_audio_.bits_per_sample(),
+                         kDefaultFrameCount);
   if (!params.IsValid()) {
     LOG(ERROR) << "Audio params are invalid.";
     return;

Powered by Google App Engine
This is Rietveld 408576698