|
|
Created:
8 years ago by DaleCurtis Modified:
8 years ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBoost WaveOut thread priority for background audio.
XP seems to be lowering the priority of the thread responding to
audio callbacks when backgrounded, causing glitching.
BUG=161307
TEST=background playback on XP works without issue.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170870
Patch Set 1 : Typo. #
Total comments: 3
Messages
Total messages: 12 (0 generated)
No more glitching! AudioDeviceThread is created with kThreadPriority_RealtimeAudio, so it's possible we should just set this generally for the AudioManager thread. FWIW, WASAPI already sets kThreadPriority_RealtimeAudio for its thread. The one renderer client which is not kThreadPriority_RealtimeAudio is PPAPI, but in testing PPAPI flash worked glitch free with this change where as without it there is instant glitching in the background.
https://codereview.chromium.org/11316284/diff/2001/media/audio/win/waveout_ou... File media/audio/win/waveout_output_win.cc (right): https://codereview.chromium.org/11316284/diff/2001/media/audio/win/waveout_ou... media/audio/win/waveout_output_win.cc:114: // Boost thread priority. Required for glitch free background audio. which thread is this? the OS callback thread or a chrome-created thread? do we need to call this every invocation of BufferCallback()?
It's just the thread created by the AudioManager, not the threads in the OS thread pool for which our callback is supposedly running on. Setting it once per construction appears to be enough. I don't think we want to go around changing the priorities of the OS thread pool. My hunch is having a thread in the process w/ kThreadPriority_RealtimeAudio causes XP to treat the process differently as a whole. A downside to this change is that even after all the audio streams are closed, the high priority audio manager thread will stick around doing nothing. An alternate solution would be to plumb a render thread just for WaveOut like we do with WASAPI and use WaitForMultipleObjects() in a blocking manner. However, that's a pretty invasive change and I'd like to get this merged for M24 at least.
On 2012/11/30 22:45:23, DaleCurtis wrote: > It's just the thread created by the AudioManager, not the threads in the OS > thread pool for which our callback is supposedly running on. Setting it once > per construction appears to be enough. I don't think we want to go around > changing the priorities of the OS thread pool. "Setting it once per construction" --> does that mean you'll change your code to only call the thread boost once? > My hunch is having a thread in the process w/ kThreadPriority_RealtimeAudio > causes XP to treat the process differently as a whole. A downside to this > change is that even after all the audio streams are closed, the high priority > audio manager thread will stick around doing nothing. Since it's a base::Thread() as long as there are no messages posted to it it'll block indefinitely and shouldn't get scheduled (the windows impl of message loop does a WaitForSingleObject(..., INFINITE)) > An alternate solution would be to plumb a render thread just for WaveOut like we > do with WASAPI and use WaitForMultipleObjects() in a blocking manner. However, > that's a pretty invasive change and I'd like to get this merged for M24 at > least.
On 2012/11/30 22:59:14, scherkus wrote: > On 2012/11/30 22:45:23, DaleCurtis wrote: > > It's just the thread created by the AudioManager, not the threads in the OS > > thread pool for which our callback is supposedly running on. Setting it once > > per construction appears to be enough. I don't think we want to go around > > changing the priorities of the OS thread pool. > > "Setting it once per construction" --> does that mean you'll change your code to > only call the thread boost once? I can move this to AudioManagerBase and use SetThreadPriority(audio_thread_.thread_handle(), ...) inside a #ifdef(OS_WIN) if you'd prefer, but I figured this was better to ensure it's only done when using WaveOut. This will be called for each new WaveOut stream, but should just be a no-op if the thread priority is already set. > > > My hunch is having a thread in the process w/ kThreadPriority_RealtimeAudio > > causes XP to treat the process differently as a whole. A downside to this > > change is that even after all the audio streams are closed, the high priority > > audio manager thread will stick around doing nothing. > > Since it's a base::Thread() as long as there are no messages posted to it it'll > block indefinitely and shouldn't get scheduled (the windows impl of message loop > does a WaitForSingleObject(..., INFINITE))
On 2012/11/30 23:05:07, DaleCurtis wrote: > On 2012/11/30 22:59:14, scherkus wrote: > > On 2012/11/30 22:45:23, DaleCurtis wrote: > > > It's just the thread created by the AudioManager, not the threads in the OS > > > thread pool for which our callback is supposedly running on. Setting it > once > > > per construction appears to be enough. I don't think we want to go around > > > changing the priorities of the OS thread pool. > > > > "Setting it once per construction" --> does that mean you'll change your code > to > > only call the thread boost once? > > I can move this to AudioManagerBase and use > SetThreadPriority(audio_thread_.thread_handle(), ...) inside a #ifdef(OS_WIN) if > you'd prefer, but I figured this was better to ensure it's only done when using > WaveOut. This will be called for each new WaveOut stream, but should just be a > no-op if the thread priority is already set. Inside the ctor seems reasonable -- I was more concerned with calling it inside BufferCallback() > > > > > My hunch is having a thread in the process w/ kThreadPriority_RealtimeAudio > > > causes XP to treat the process differently as a whole. A downside to this > > > change is that even after all the audio streams are closed, the high > priority > > > audio manager thread will stick around doing nothing. > > > > Since it's a base::Thread() as long as there are no messages posted to it > it'll > > block indefinitely and shouldn't get scheduled (the windows impl of message > loop > > does a WaitForSingleObject(..., INFINITE))
ignore my comments about BufferCallback() -- I have no idea how/why I thought I saw the thread boost inside that function
lgtm
LGTM. Even if it always feels a bit scary if a thread prio really does have an effect. https://chromiumcodereview.appspot.com/11316284/diff/2001/media/audio/win/wav... File media/audio/win/waveout_output_win.cc (right): https://chromiumcodereview.appspot.com/11316284/diff/2001/media/audio/win/wav... media/audio/win/waveout_output_win.cc:114: // Boost thread priority. Required for glitch free background audio. Same question as Andrew.
https://chromiumcodereview.appspot.com/11316284/diff/2001/media/audio/win/wav... File media/audio/win/waveout_output_win.cc (right): https://chromiumcodereview.appspot.com/11316284/diff/2001/media/audio/win/wav... media/audio/win/waveout_output_win.cc:114: // Boost thread priority. Required for glitch free background audio. On 2012/12/03 08:35:43, henrika wrote: > Same question as Andrew. Henrik, please see the old messages for full discussion, but this is the thread we create in the audio manager, hence the dcheck.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11316284/2001
Retried try job too often on mac_rel for step(s) browser_tests |