911117 - [flatfish] Headset icon is lost on status bar if booting up with headset plugged.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System, defect)

Description

[viral [:viralwang]]

Plug in/out headset will not generate any event from input system.
It's randomly set plug in/out when boot up.
Audio path looks good in both ways (I can hear the sound from either speaker or headset when playing video)
It looks like the only problem is headset detection not working now.

Comment 1

[Steven Yang [:styang]]

Jeff, please check this issue. Thanks!

Comment 2

[Jeff Chuang]

John, please help to check driver level.
Terry, please help to study Audio patch control

Comment 3

[Terry Li]

Attachment: Fix bind() error in UeventPoller initialization

I found UeventPoller failed to initialize itself because bind() returns -EADDRINUSE.  UeventPoller uses netlink socket to read uevents(same as Android) without the need of filling out address or port.  A possible EADDRINUSE error in netlink socket bind() is bind 2 or more netlink sockets with the same pid.  In the manpage of netlink(7), it says the following:

There are two ways to assign  nl_pid to a netlink socket.  If the application sets nl_pid before calling bind(2), then it is up to the application to make sure that nl_pid is unique.  If the application sets it to 0, the kernel takes care of assigning it.  The kernel assigns the process ID to the first netlink socket the process opens and assigns a unique nl_pid to every netlink socket that the process  subsequently creates.

This patch fix the bind() error in UeventPoller.cpp by setting nl_pid to 0 and add error handling after bind() fails.

Comment 4

[Terry Li]

Another bug I'm working on:
When I boot up the device with headset inserted, the audio output is no problem(from headset), but status bar does not show headset icon.  I found a similar issue of bug 796723.

After debugging, I found that AudioManager's 1st mozChromeEvent broadcast happens before statusbar.js regisgers event listener and it seems no way for statusbar.js to check initial state of headphone plug status.

Related codes are as follows,
1. statusbar.js register mozChomeEvent
   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L179
2. statusbar.js handles headphone change event
   https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.js#L291
3. AudioManager broadcast 1st event in constructor
   http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.cpp#376

Comment 5

[Evelyn Hung]

> Related codes are as follows,
> 1. statusbar.js register mozChomeEvent
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.
> js#L179
> 2. statusbar.js handles headphone change event
>   
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/statusbar.
> js#L291
> 3. AudioManager broadcast 1st event in constructor
>   
> http://mxr.mozilla.org/mozilla-central/source/dom/system/gonk/AudioManager.
> cpp#376

Seems a gaia bug.
Marco told me because of some Gecko change, the first headset change Chrome event fired from Gecko is earlier than shell.js been loaded, so System app can't register it in time. He also suggests to use `mozAudioChannelManager` API to detect and listen headset status.
Music app is using the API: https://github.com/mozilla-b2g/gaia/blob/master/apps/music/js/Player.js#L26

Comment 6

[Gregor Wagner [:gwagner]]

I don't think it should block but please ask for approval if you think it should be fixed in 1.2

Comment 7

[Vincent Liu[:vliu]]

(In reply to Terry Li from comment #3)
> Created attachment 811949 [details] [diff] [review]
> Fix bind() error in UeventPoller initialization
> 
> I found UeventPoller failed to initialize itself because bind() returns
> -EADDRINUSE.  UeventPoller uses netlink socket to read uevents(same as
> Android) without the need of filling out address or port.  A possible
> EADDRINUSE error in netlink socket bind() is bind 2 or more netlink sockets
> with the same pid.  In the manpage of netlink(7), it says the following:
> 

As you said, I also think it might be the problem that b2g process contains more than 1 netlink sockets in different thread. Because I didn't have Flatfish on my hand, can you please help me to modify the code like the below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid got the unique value. Thanks


   bzero(&saddr, sizeof(saddr));
   saddr.nl_family = AF_NETLINK;
   saddr.nl_groups = 1;
-  saddr.nl_pid = getpid();
+  //saddr.nl_pid = getpid();
+  saddr.nl_pid = pthread_self() << 16 | getpid();

Comment 8

[Marco Chen [:mchen]]

(In reply to Gregor Wagner [:gwagner] from comment #6)
> I don't think it should block but please ask for approval if you think it
> should be fixed in 1.2

Hi,

I renom to koi? because if we don't fix bug here the user impact will be

  1. The audio will not route to headset device (big impact) on flatfish project.
  2. The status bar will not show the headset icon if headset was plugged during boot up. (small impact).

Thanks.

Comment 9

[Francis Lee [:frlee]]

It's a must for flatfish. Marked as koi+ according to triage result.

Comment 11

[Terry Li]

(In reply to Vincent Liu[:vliu] from comment #7)
> 
> As you said, I also think it might be the problem that b2g process contains
> more than 1 netlink sockets in different thread. Because I didn't have
> Flatfish on my hand, can you please help me to modify the code like the
> below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid
> got the unique value. Thanks
> 
> 
>    bzero(&saddr, sizeof(saddr));
>    saddr.nl_family = AF_NETLINK;
>    saddr.nl_groups = 1;
> -  saddr.nl_pid = getpid();
> +  //saddr.nl_pid = getpid();
> +  saddr.nl_pid = pthread_self() << 16 | getpid();
This works as you expected.
By the way, when I print out the pid, it turns out to be the process of b2g.

Comment 12

[Vincent Liu[:vliu]]

(In reply to Terry Li from comment #11)
> (In reply to Vincent Liu[:vliu] from comment #7)
> > 
> > As you said, I also think it might be the problem that b2g process contains
> > more than 1 netlink sockets in different thread. Because I didn't have
> > Flatfish on my hand, can you please help me to modify the code like the
> > below patch in UeventPoller.cpp? This statement can make sure saddr.nl_pid
> > got the unique value. Thanks
> > 
> > 
> >    bzero(&saddr, sizeof(saddr));
> >    saddr.nl_family = AF_NETLINK;
> >    saddr.nl_groups = 1;
> > -  saddr.nl_pid = getpid();
> > +  //saddr.nl_pid = getpid();
> > +  saddr.nl_pid = pthread_self() << 16 | getpid();
> This works as you expected.
> By the way, when I print out the pid, it turns out to be the process of b2g.

NetlinkPoller::OpenSocket() runs in b2g process so it should be the expected value. From the result of Comment 11, it is better using |saddr.nl_pid = pthread_self() << 16 | getpid()| instead of |saddr.nl_pid = getpid()| to make sure the uniqueness of nl_pid. I will file another bug for checking.

Comment 13

[Terry Li]

(In reply to Vincent Liu[:vliu] from comment #12)
> NetlinkPoller::OpenSocket() runs in b2g process so it should be the expected
> value. From the result of Comment 11, it is better using |saddr.nl_pid =
> pthread_self() << 16 | getpid()| instead of |saddr.nl_pid = getpid()| to
> make sure the uniqueness of nl_pid. I will file another bug for checking.

nl_pid identifies a netlink socket, not a process(or thread). Using this way may have the risk to fail if 2 or more netlink sockets exist in the same thread. Using 0 to have kernel handle it, which is probably safer for both cases. What do you think?

Following is the related statement about nl_pid in netlink(7):

"If the application sets it to 0, the kernel takes care of assigning it.  The kernel assigns the process ID to the first netlink socket the process opens and assigns a unique nl_pid to every netlink socket that the process subsequently creates."

Comment 14

[(Inactive after June) George Duan [:gduan] [:喬智]]

Attachment: PR to master

As stated in comment 4 and comment 5,
//AudioManager's 1st mozChromeEvent broadcast happens before statusbar.js regisgers event listener//
this patch will use navigator.mozAudioChannelManager.headphones to show/hide the headphone icon when firstly initialized.

Comment 15

[(Inactive after June) George Duan [:gduan] [:喬智]]

Thanks Alive.
Merge into master,
https://github.com/mozilla-b2g/gaia/commit/3d4f1107e6e91e5f5649edc0f2565ac837111d7d

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted 3d4f1107e6e91e5f5649edc0f2565ac837111d7d to:
v1.2: d7d8b406b33ae0dbccf8fe03990a7053575b9ed5