Closed Bug 835211 Opened 7 years ago Closed 6 years ago

AudioManager cannot set correct device connection state on some platforms

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 895665

People

(Reporter: alan.yenlin.huang, Assigned: alan.yenlin.huang)

Details

Attachments

(1 file, 2 obsolete files)

For platform with headset/head phone plug/unplug events like below, AudioManager cannot set correct device connection state when unplug.

$ adb shell su -- getevent -lt /dev/input/event6
4181-378357: EV_SW        SW_HEADPHONE_INSERT  00000001            
4181-378357: EV_SW        SW_MICROPHONE_INSERT 00000001            
4181-378387: EV_SYN       SYN_REPORT           00000000            
4183-834026: EV_SW        SW_HEADPHONE_INSERT  00000000            
4183-834026: EV_SW        SW_MICROPHONE_INSERT 00000000            
4183-834026: EV_SYN       SYN_REPORT           00000000

I think it is caused by that (in InternalSetAudioRoutesICS, AudioSystem.cpp) we OR switch state(s) when AUDIO_POLICY_DEVICE_STATE_AVAILABLE, but we didn't use AND to check switch flags when AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE.
Attached patch proposed fix, v1 (obsolete) — Splinter Review
Hi Michael,
We met this bug when porting b2g to QCT msm8960 platform. Can you help to review this patch? Thanks.
Attachment #706953 - Flags: review?(mwu)
Attached patch proposed fix, v2 (obsolete) — Splinter Review
Attach full patch, including Event hub part and implementation of GeckoInputDispatcher::notifySwitch.

I port EventHub part from android Eventhub for headset using input device instead of uevent.
Attachment #706953 - Attachment is obsolete: true
Attachment #706953 - Flags: review?(mwu)
Attachment #706996 - Flags: review?(mwu)
Comment on attachment 706996 [details] [diff] [review]
proposed fix, v2

Review of attachment 706996 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/system/gonk/AudioManager.cpp
@@ +94,5 @@
>      AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_WIRED_HEADPHONE,
>                                            AUDIO_POLICY_DEVICE_STATE_AVAILABLE, "");
>      sHeadsetState |= AUDIO_DEVICE_OUT_WIRED_HEADPHONE;
>    } else if (aState == SWITCH_STATE_OFF) {
> +    if (sHeadsetState && AUDIO_DEVICE_OUT_WIRED_HEADSET) {

Did you really mean to use &&?
Attached patch proposed fix, v2Splinter Review
Attachment #706996 - Attachment is obsolete: true
Attachment #706996 - Flags: review?(mwu)
Attachment #707388 - Flags: review?(mwu)
(In reply to Michael Wu [:mwu] from comment #3)
> Comment on attachment 706996 [details] [diff] [review]
> proposed fix, v2
> 
> Review of attachment 706996 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +94,5 @@
> >      AudioSystem::setDeviceConnectionState(AUDIO_DEVICE_OUT_WIRED_HEADPHONE,
> >                                            AUDIO_POLICY_DEVICE_STATE_AVAILABLE, "");
> >      sHeadsetState |= AUDIO_DEVICE_OUT_WIRED_HEADPHONE;
> >    } else if (aState == SWITCH_STATE_OFF) {
> > +    if (sHeadsetState && AUDIO_DEVICE_OUT_WIRED_HEADSET) {
> 
> Did you really mean to use &&?

No, that's a mistake. I've updated patch with correct logical AND. Please help me to review it, thanks!
This fix does not match the bug description. Why are you adding a new source of switch events? The EventHub.cpp patch you have is a codeaurora specific patch. We try to stick to AOSP upstream copies.

Also, there's only one kind of logical AND.

&& = logical AND
& = bitwise AND
(In reply to Michael Wu [:mwu] from comment #6)
> Also, there's only one kind of logical AND.
> 
> && = logical AND
> & = bitwise AND
Thank you for correcting my terminology. I intend to use bitwise AND in this patch. 

> This fix does not match the bug description. Why are you adding a new source
> of switch events? The EventHub.cpp patch you have is a codeaurora specific
> patch. We try to stick to AOSP upstream copies.

Unlike unagi, some partner's platform uses /dev/input for headset/microphone, so we have to add a new source of switch events. Then we found this bug: when insert headphone, both SW_HEADPHONE_INSERT and SW_MICROPHONE_INSERT events are reported. When remove headset, not all connection state are disconnected. (or, maybe it would be better that I change bug title?)

I agree it would be better that we stick to AOSP copies. Do you have some good idea to make headset work in this case? Thank you.
Hey, please take look at the patch on bug 895665.  Looks like we may be trying to fix the same bug.
Comment on attachment 707388 [details] [diff] [review]
proposed fix, v2

Review of attachment 707388 [details] [diff] [review]:
-----------------------------------------------------------------

We should be able to avoid the EventHub changes in this patch by making nsAppShell.cpp a little smarter.  There are also existing devices that send buggy dev input jack events, so I really think we need this pref-ed off by default ("ro.moz.devinputjack=1").
Thanks, Michael. I believe what you do in nsAppShell.cpp on bug 895665 is better than what I attempt to do. Also, I think using read-only build property is a good idea for devices supporting this. I mark this as duplicated as 895665.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 895665
Attachment #707388 - Flags: review?(mwu)
You need to log in before you can comment on or make changes to this bug.