[VER.] BuildID revision="7cb46499e0b91ca20f6aed58d6067d7c451875b9" gaia revision="5cbb19e4bb78a7ad879fbe4b9a841e1c35714f5c" gecko revision="950b402b6188bb2f3ce3176e620ed5249719d720" [Precondition] Currently, We can`t reproduce this issues. we'll let you know when it become available. Generally, We set the media`s volume during playing Video/Camera/Music/FM Radio [Symptoms] Actual volume is not updated when volume is adjusted while UI is updated as it's changed. All channel volume is not updated. After rebooting, we checked setting up-down volume nomally. What we can get for now is android logs below only. And for setting volume, the flow dosen`t call gonk function(AudioManager`s SetStreamVolumeIndex()) Please let us know if any of additional info can be delivered to us. ----- GeckoConsole: Content JS LOG at app://system.gaiamobile.org/shared/js/event_helper.js:4 in handle_mozChromeEvent: event_helper :handle_mozChromeEvent: volume-down-button-release GeckoConsole: [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAudioManager.setStreamVolumeIndex]" {file: "chrome://browser/content/settings.js" line: 96}] ----- Email: oedipus31@gmail.com

Hi, Marco Chen Recently, This issue is reported three times by QA, but It is not reproduced. Is there any issues/risk in the recent audio patches?

(In reply to leo.bugzilla.gecko from comment #1) > Hi, Marco Chen > Is there any issues/risk in the recent audio patches? Hi Leo, Randy (:rlin) will give some information here for what we found about this issue.

For this problem which may cause by audioflinger's volume index isn't set correctly after media server crash. BTW, it also need to recover the phone call mode and audio routing path information and stream volume index.

That can for fix the volume can't be adjust problem temporarily.

correct function naming

Comment on attachment 746732 [details] [diff] [review] WIP patch v1 HI Michanel, Should we change the AudioManager object to be a parent/child design and keep the audio information and try to recover?

Try to register binder die callback on audiomanager and recover the volume /headset status when media server crash.

(In reply to leo.bugzilla.gecko from comment #0) > Currently, We can`t reproduce this issues. we'll let you know when it become > available. Please re-nominate when this does reproduce. For now, we'd take a patch approval but wouldn't block.

> Please re-nominate when this does reproduce. For now, we'd take a patch > approval but wouldn't block. It`s good to recover volume. After I am force to kill mediaserver, I tested and confirmed to reproduce without your patch. After patch, I checked to recover volume in audiomanager nomally.

1. use hardware rocker, and set the volume to 0 2. launch music app. 3. play a song 4. hit the volume up button once. Expected: one volume up Actual: full blast music

I should mention that you have to hit the volume button while the song is playing.

Hi Naoki, I think you switch to silence mode and just adjust the ringer volume, You can try to silence the volume during music playback and do your reproduce step again, I think it's the behavior that you wish.

update patch v2. sync android recover mechanism, also recover setMode and setForceUse.

Comment on attachment 749183 [details] [diff] [review] patch v2 Review of attachment 749183 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +51,5 @@ > 15, // TTS > 15, // FM > }; > + > +static int sCurrentStreamVolumeTbl[AUDIO_STREAM_CNT] = { Let's make this part of AudioManager. Global variables are usually worth avoiding. @@ +77,5 @@ > + NS_IMETHODIMP Run() { > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + Leading whitespace @@ +79,5 @@ > + > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > + > + NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE); > + if (NS_FAILED(obs->NotifyObservers(nullptr, Why not call InitAudioSubSystem directly? @@ +210,5 @@ > } > > return NS_OK; > } > + Leading whitespace @@ +274,5 @@ > + } > + if (NS_FAILED(obs->RemoveObserver(this, AUDIO_SUBSYSTEM_CRASHED))) { > + NS_WARNING("Failed to remove audio-subsystem-crashed oberver!"); > + } > + Leading whitespace @@ +291,5 @@ > + // Force publicnotification to output at maximal volume > + AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_ENFORCED_AUDIBLE), > + sMaxStreamVolumeTbl[AUDIO_STREAM_ENFORCED_AUDIBLE]); > + > + if (sHeadsetState | AUDIO_DEVICE_OUT_WIRED_HEADSET) wrong @@ +302,5 @@ > + AudioSystem::setPhoneState(mPhoneState); > + SetForceForUse(nsIAudioManager::USE_COMMUNICATION, mForcedUseForComm); > + > + // only recover the chrome process's audiomanager instance > + if (!(XRE_GetProcessType() == GeckoProcessType_Content)) { Content processes can init an AudioManager? @@ +449,5 @@ > status = AudioSystem::setForceUse((audio_policy_force_use_t)aUsage, > (audio_policy_forced_cfg_t)aForce); > } > + if (aUsage == nsIAudioManager::USE_COMMUNICATION && status == NS_OK) > + mForcedUseForComm = aForce; mForcedUseForComm never seems to get reset. Is that intentional?

Comment on attachment 749183 [details] [diff] [review] patch v2 Review of attachment 749183 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/gonk/AudioManager.cpp @@ +449,5 @@ > status = AudioSystem::setForceUse((audio_policy_force_use_t)aUsage, > (audio_policy_forced_cfg_t)aForce); > } > + if (aUsage == nsIAudioManager::USE_COMMUNICATION && status == NS_OK) > + mForcedUseForComm = aForce; Nevermind - I misread this code.

(In reply to Michael Wu [:mwu] from comment #15) > Comment on attachment 749183 [details] [diff] [review] > patch v2 > > Review of attachment 749183 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/system/gonk/AudioManager.cpp > @@ +51,5 @@ > > 15, // TTS > > 15, // FM > > }; > > + > > +static int sCurrentStreamVolumeTbl[AUDIO_STREAM_CNT] = { > > Let's make this part of AudioManager. Global variables are usually worth > avoiding. oh, It's because AUDIO_STREAM_CNT...define in "AudioSystem.h" > > @@ +77,5 @@ > > + NS_IMETHODIMP Run() { > > + MOZ_ASSERT(NS_IsMainThread()); > > + > > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > + > > Leading whitespace > > @@ +79,5 @@ > > + > > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > + > > + NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE); > > + if (NS_FAILED(obs->NotifyObservers(nullptr, > > Why not call InitAudioSubSystem directly? This scope can't access the AudioManager instance. the binderDie thread isn't in the MainThread, so I use this way to notify. > > @@ +210,5 @@ > > } > > > > return NS_OK; > > } > > + > > Leading whitespace > > @@ +274,5 @@ > > + } > > + if (NS_FAILED(obs->RemoveObserver(this, AUDIO_SUBSYSTEM_CRASHED))) { > > + NS_WARNING("Failed to remove audio-subsystem-crashed oberver!"); > > + } > > + > > Leading whitespace > > @@ +291,5 @@ > > + // Force publicnotification to output at maximal volume > > + AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_ENFORCED_AUDIBLE), > > + sMaxStreamVolumeTbl[AUDIO_STREAM_ENFORCED_AUDIBLE]); > > + > > + if (sHeadsetState | AUDIO_DEVICE_OUT_WIRED_HEADSET) > > wrong change to = > > @@ +302,5 @@ > > + AudioSystem::setPhoneState(mPhoneState); > > + SetForceForUse(nsIAudioManager::USE_COMMUNICATION, mForcedUseForComm); > > + > > + // only recover the chrome process's audiomanager instance > > + if (!(XRE_GetProcessType() == GeckoProcessType_Content)) { > > Content processes can init an AudioManager? > Now only chrome process init the AudioManager, I will remove it. > @@ +449,5 @@ > > status = AudioSystem::setForceUse((audio_policy_force_use_t)aUsage, > > (audio_policy_forced_cfg_t)aForce); > > } > > + if (aUsage == nsIAudioManager::USE_COMMUNICATION && status == NS_OK) > > + mForcedUseForComm = aForce; > > mForcedUseForComm never seems to get reset. Is that intentional? This also want recover the phone call in speaker mode..

(In reply to Randy Lin [:rlin] from comment #17) > (In reply to Michael Wu [:mwu] from comment #15) > > @@ +79,5 @@ > > > + > > > + nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService(); > > > + > > > + NS_ENSURE_TRUE(obs, NS_ERROR_FAILURE); > > > + if (NS_FAILED(obs->NotifyObservers(nullptr, > > > > Why not call InitAudioSubSystem directly? > This scope can't access the AudioManager instance. the binderDie thread > isn't in the MainThread, so I use this way to notify. I mean call InitAudioSubSystem directly instead of using NotifyObservers. There's no need to use the observer service here as far as I can tell.

the setforceuse case I would create another follow bug for fixing. Bug 872419 - [B2G][Audio] Recover the device connect state and setForceUse when media server was dead.

Comment on attachment 749714 [details] [diff] [review] patch v2 Review of attachment 749714 [details] [diff] [review]: ----------------------------------------------------------------- This looks pretty close. It's a lot simpler now. ::: dom/system/gonk/AudioManager.cpp @@ +34,5 @@ > #define HEADPHONES_STATUS_OFF NS_LITERAL_STRING("off").get() > #define HEADPHONES_STATUS_UNKNOWN NS_LITERAL_STRING("unknown").get() > #define BLUETOOTH_SCO_STATUS_CHANGED "bluetooth-sco-status-changed" > > +static void BinderDeadcb(status_t aErr); The function name looks a bit odd. "BinderDeadCallback" might be better. @@ +58,5 @@ > +class RecoverTask : public nsRunnable > +{ > +public: > + RecoverTask() {} > + NS_IMETHODIMP Run() { This function's indentation is off. @@ +61,5 @@ > + RecoverTask() {} > + NS_IMETHODIMP Run() { > + nsCOMPtr<nsIAudioManager> am = do_GetService(NS_AUDIOMANAGER_CONTRACTID); > + NS_ENSURE_TRUE(am, NS_OK); > + for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) { 'i' is more typically used as a counter in loops when there aren't any more meaningful names. @@ +66,5 @@ > + AudioSystem::initStreamVolume(static_cast<audio_stream_type_t>(loop), 0, > + sMaxStreamVolumeTbl[loop]); > + int32_t volidx = 0; > + am->GetStreamVolumeIndex(loop, &volidx); > + // restore the volum I think that if you want to make a comment, a comment for the whole function will be more useful than for this line of the function. It seems pretty obvious what this line is doing if you know the general purpose of the function. @@ +70,5 @@ > + // restore the volum > + am->SetStreamVolumeIndex(static_cast<audio_stream_type_t>(loop), > + volidx); > + } > + if (sHeadsetState == AUDIO_DEVICE_OUT_WIRED_HEADSET) Isn't sHeadState a bitfield? Should it not be treated as one, or should we be using & here? @@ +76,5 @@ > + else if (sHeadsetState == AUDIO_DEVICE_OUT_WIRED_HEADPHONE) > + InternalSetAudioRoutes(SWITCH_STATE_HEADPHONE); > + else > + InternalSetAudioRoutes(SWITCH_STATE_OFF); > + int32_t phoneState = nsIAudioManager::PHONE_STATE_INVALID; Leading whitespace @@ +80,5 @@ > + int32_t phoneState = nsIAudioManager::PHONE_STATE_INVALID; > + am->GetPhoneState(&phoneState); > + AudioSystem::setPhoneState(phoneState); > + AudioSystem::get_audio_flinger(); > + AudioSystem::setErrorCallback(BinderDeadcb); AudioSystem.cpp never seems to reset the callback, so this doesn't seem necessary. @@ +259,5 @@ > AudioSystem::setStreamVolumeIndex(static_cast<audio_stream_type_t>(AUDIO_STREAM_ENFORCED_AUDIBLE), > sMaxStreamVolumeTbl[AUDIO_STREAM_ENFORCED_AUDIBLE]); > + > + for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) > + mCurrentStreamVolumeTbl[loop] = sMaxStreamVolumeTbl[loop]; Can you merge this into the previous loop? @@ +262,5 @@ > + for (int loop = 0; loop < AUDIO_STREAM_CNT; loop++) > + mCurrentStreamVolumeTbl[loop] = sMaxStreamVolumeTbl[loop]; > + > + // create an audio flinger instance in thos context, let the cb can register correctly. > + AudioSystem::get_audio_flinger(); AFAICT, the error callback is stored as part of AudioSystem.cpp and no connection to AudioFlinger is required to set the error callback. http://androidxref.com/4.0.4/xref/frameworks/base/media/libmedia/AudioSystem.cpp @@ +271,5 @@ > UnregisterSwitchObserver(SWITCH_HEADPHONES, mObserver); > > nsCOMPtr<nsIObserverService> obs = services::GetObserverService(); > if (NS_FAILED(obs->RemoveObserver(this, BLUETOOTH_SCO_STATUS_CHANGED))) { > + NS_WARNING("Failed to remove bluetooth-sco-status-changed oberver!"); Might as well fix the misspelling of observer here

For the AudioSystem::get_audio_flinger(); It's a little tricky, In the Audio System, binderDied function will clear the gAudioFlinger object, if there is no function call trigger the get_audio_flinger yet, then the media server dead again, The error callback won't be triggered because those code section list below haven't executed yet.. 52 if (gAudioFlinger.get() == 0) { 53 sp<IServiceManager> sm = defaultServiceManager(); 54 sp<IBinder> binder; 55 do { 56 binder = sm->getService(String16("media.audio_flinger")); 57 if (binder != 0) 58 break; 59 LOGW("AudioFlinger not published, waiting..."); 60 usleep(500000); // 0.5 s 61 } while(true); 62 if (gAudioFlingerClient == NULL) { 63 gAudioFlingerClient = new AudioFlingerClient(); 64 } else { 65 if (gAudioErrorCallback) { 66 gAudioErrorCallback(NO_ERROR); 67 } 68 } 69 binder->linkToDeath(gAudioFlingerClient); <----

I'm not sure how that is relevant. The callback is stored on a static variable in AudioSystem. http://androidxref.com/4.0.4/xref/frameworks/base/include/media/AudioSystem.h#231 - I don't see any way that would get cleared.

The gAudioErrorCallback is set, but after media server crashed, the gAudioFlingerClient/gAudioFlinger maybe be null and won't linkToDeath with the gAudioFlingerClient. So if media server crash again, there is no binder dead callback from binder because this :binder->linkToDeath(gAudioFlingerClient); haven't registered....

Fix the mwu's suggestion.

During the first boot up, the get_audio_flinger(); was triggered and set gAudioFlinger by the SetMasterVolume(1) function call.

(In reply to Randy Lin [:rlin] from comment #23) > The gAudioErrorCallback is set, but after media server crashed, the > gAudioFlingerClient/gAudioFlinger maybe be null and won't linkToDeath with > the gAudioFlingerClient. So if media server crash again, there is no binder > dead callback from binder because this > :binder->linkToDeath(gAudioFlingerClient); haven't registered.... This doesn't explain why we need to set gAudioErrorCallback again. gAudioErrorCallback is set to null when the library is loaded, and I see no code that resets it back to null. The only thing AudioSystem::setErrorCallback does is set gAudioErrorCallback.

hmm, the AudioSystem::setErrorCallback(BinderDeadCallback); can be eliminated, but still need to keep AudioSystem::get_audio_flinger(); or SetMasterVolume(1) ?

(In reply to Randy Lin [:rlin] from comment #27) > hmm, the AudioSystem::setErrorCallback(BinderDeadCallback); can be > eliminated, but still need to keep AudioSystem::get_audio_flinger(); or > SetMasterVolume(1) ? No idea. I think AudioSystem is suppose to call get_audio_flinger() when it needs it, but I didn't check very deeply to verify that.

Comment on attachment 750245 [details] [diff] [review] patch v3 Review of attachment 750245 [details] [diff] [review]: ----------------------------------------------------------------- Looks good - thanks. I'm a bit undecided about whether we need get_audio_flinger at the end. Everything that needs it seems to call get_audio_flinger first, but we probably need to test to be really sure. Up to you. ::: dom/system/gonk/AudioManager.cpp @@ +76,5 @@ > + InternalSetAudioRoutes(SWITCH_STATE_HEADPHONE); > + else > + InternalSetAudioRoutes(SWITCH_STATE_OFF); > + > + int32_t phoneState = nsIAudioManager::PHONE_STATE_INVALID; Leading whitespace @@ +79,5 @@ > + > + int32_t phoneState = nsIAudioManager::PHONE_STATE_INVALID; > + am->GetPhoneState(&phoneState); > + AudioSystem::setPhoneState(phoneState); > + //ensure the AudioSystem register the BinderDeadCallback successfully. Don't think this comment applies anymore.

check in patch

https://hg.mozilla.org/releases/mozilla-b2g18/rev/4331cc0a4527 https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/63dfdb7a83c6 Note that this had a non-trivial conflict in AudioManager.cpp. You might want to take a look to make sure I handled it correctly.

So much for that idea. Backed out for bustage. https://hg.mozilla.org/releases/mozilla-b2g18/rev/07159fef136a https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/1b9503395769 https://tbpl.mozilla.org/php/getParsedLog.php?id=23043977&tree=Mozilla-B2g18

oh, I will provide 1.0.1 b2g18 version later.

For 1.0.1, it also needs this bug's two patch to ensure this function works. Bug 803471 - Use hw codec and camera hw in mediaserver process https://bugzilla.mozilla.org/attachment.cgi?id=722443 https://bugzilla.mozilla.org/attachment.cgi?id=722457 b2g-18 branch won't has this problem.

Found I can't directly include the gonk/AudioSystem.h in the AudioManager.h. (Can't found this header during build b2g) So I define the audio_stream_count at the AudioManager.h for avoiding this problem.

Comment on attachment 750867 [details] [diff] [review] b2g-18, v1.0.1 patch Review of attachment 750867 [details] [diff] [review]: ----------------------------------------------------------------- Not great, but I guess it'll be ok for a release branch.

https://hg.mozilla.org/releases/mozilla-b2g18/rev/7f177cd557ab https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/70d7422c9a16

Hi, I receviced the related issue from call engineer. --------------------------------------------------------- // A bitwise variable for recording what kind of headset is attached. -static int sHeadsetState; +static int sHeadsetState = SWITCH_STATE_OFF; --------------------------------------------------------- Your patch(sHeadsetState) is initialized by the wrong value(SWITCH_STATE_OFF). Can you change the value to "0"? It is different from switch state value(aState).

already land on rev: https://hg.mozilla.org/releases/mozilla-b2g18/rev/39c232e1572a https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/0cc62659b472

The issue still reproduces on V1.1 and V1.0.1 Tested on Inari, Unagi Steps to reproduce: 1. On the device set volume to 0 2. launch music app. 3. play a song 4. hit the volume up button once. Expected: one volume up Actual: full blast music Notes: This issue reproduces only for the first time when the music application is launched or if restart the device The issue doesn't reproduce if the application runs in next time Environmental Variables: Inari Build ID: 20130531070208 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/be5c2ee11d02 Gaia: e7114bdf4078274fc127a3b2a58dad91d6884219 Environmental Variables: Unagi Build ID: 20130531070205 Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4f318822e72c Gaia: e1c59baed29e4665d1da9392f86239272073f07a

I think this should be another bug, Can you open another one?

opened the new bug https://bugzilla.mozilla.org/show_bug.cgi?id=878181