835211 - AudioManager cannot set correct device connection state on some platforms

Status: RESOLVED DUPLICATE of bug 895665

(Firefox OS Graveyard :: General, defect)

Description

[Alan Huang]

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.

Comment 1

[Alan Huang]

Attachment: proposed fix, v1

Hi Michael,
We met this bug when porting b2g to QCT msm8960 platform. Can you help to review this patch? Thanks.

Comment 2

[Alan Huang]

Attachment: proposed fix, v2

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.

Comment 3

[Michael Wu [: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 &&?

Comment 4

[Alan Huang]

Attachment: proposed fix, v2

Comment 5

[Alan Huang]

(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!

Comment 6

[Michael Wu [:mwu]]

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

Comment 7

[Alan Huang]

(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.

Comment 8

[Michael Vines [:m1] [:evilmachines]]

Hey, please take look at the patch on bug 895665.  Looks like we may be trying to fix the same bug.

Comment 9

[Michael Vines [:m1] [:evilmachines]]

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").

Comment 10

[Alan Huang]

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.