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

Issue 6760014: chromeos: Retry ALSA mixer initialization on failure. (Closed)

Created:
9 years, 9 months ago by Daniel Erat
Modified:
9 years, 7 months ago
Reviewers:
davejcool
CC:
chromium-reviews, davemoore+watch_chromium.org
Visibility:
Public.

Description

chromeos: Retry ALSA mixer initialization on failure. There's a race at boot where Chrome can attempt to use ALSA before its kernel modules have been loaded. We previously wouldn't retry the initialization in this case, resulting in the volume keys not working until Chrome was restarted. This change makes the mixer code retry on failure once per second, up to twenty times. I've typically seen the modules get loaded 5-6 seconds after we start Chrome (although we don't make it until this code until a bit later; I see one or two failures before we succeed). It would be good to also load the modules earlier, if we can do so without impacting boot time -- if the volume keys are pressed before the modules are loaded now, the "muted" bubble still comes up. BUG=chromium-os:13162 TEST=booted with my change from issue 13226 and checked that the volume keys start working after a few seconds, and also that this doesn't block the UI (it's happening on its own thread)

Patch Set 1 #

Patch Set 2 : merge and update copyright year #

Total comments: 3

Patch Set 3 : apply review feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -2 lines) Patch
M chrome/browser/chromeos/audio_mixer_alsa.cc View 1 2 4 chunks +28 lines, -2 lines 0 comments Download

Messages

Total messages: 7 (0 generated)
Daniel Erat
9 years, 9 months ago (2011-03-29 02:03:33 UTC) #1
davejcool
This looks good except for my question here on the case where we're told to ...
9 years, 9 months ago (2011-03-29 17:50:54 UTC) #2
Daniel Erat
http://codereview.chromium.org/6760014/diff/2001/chrome/browser/chromeos/audio_mixer_alsa.cc File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/6760014/diff/2001/chrome/browser/chromeos/audio_mixer_alsa.cc#newcode217 chrome/browser/chromeos/audio_mixer_alsa.cc:217: } On 2011/03/29 17:50:55, davejcool wrote: > There's a ...
9 years, 9 months ago (2011-03-29 19:45:55 UTC) #3
davejcool
http://codereview.chromium.org/6760014/diff/2001/chrome/browser/chromeos/audio_mixer_alsa.cc File chrome/browser/chromeos/audio_mixer_alsa.cc (right): http://codereview.chromium.org/6760014/diff/2001/chrome/browser/chromeos/audio_mixer_alsa.cc#newcode217 chrome/browser/chromeos/audio_mixer_alsa.cc:217: } Ah, right - for this to work sleep() ...
9 years, 8 months ago (2011-03-30 17:42:15 UTC) #4
Daniel Erat
Thanks. Mind replying with an explicit LGTM? The presubmit scripts require it now. On Wed, ...
9 years, 8 months ago (2011-03-30 22:33:27 UTC) #5
Daniel Erat
Never mind; took a brief poll among some colleagues and all said that the previous ...
9 years, 8 months ago (2011-03-30 22:41:04 UTC) #6
davejcool
9 years, 8 months ago (2011-03-31 19:08:39 UTC) #7
LGTM
I know it's late, but it really does look great now.  Sorry about not explicitly
stating it before as "LGTM if..."

Powered by Google App Engine
This is Rietveld 408576698