1147862 - FindMyDevice should use the ringer audio context type

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Frederik Braun [:freddy]]

STR:
- Play some Music with the Music app in the foreground
- Use FMD to make the phone ring
- The current music will just be louder.

I think we can prevent that if we use a ringer audio context. That will also have a higher playback priority than any other app (including music).

NB: The Music will still be played on the headphones if attached. I am not sure whether this can be fixed so easily

Comment 1

[GitHub Autolander Bot]

Attachment: [gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Comment 2

[Frederik Braun [:freddy]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Can you take a look, please?

Comment 4

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

(wish I could turn the r? into f+ like in the good old days)

Thanks for this! r=me when the tests are updated and passing.

Comment 5

[Frederik Braun [:freddy]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

(Autolander wants an r+ from a module peer, for automatical check-ins to work :))

All tests are green now, except for some Music app failure, that I hope is completely unrelated to us changing audio things.

Comment 6

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Thanks!

Comment 7

[GitHub Autolander Bot]

Autolander could not locate a review from a user within the suggested reviewer list. Either the patch author or the reviewer should be in the suggested reviewer list.

Comment 8

[Kevin Grandon :kgrandon]

Adding checkin-needed again as the suggested review list has been updated.

Comment 9

[GitHub Autolander Bot]

Pull request has landed in master: https://github.com/mozilla-b2g/gaia/commit/e90863f014c0a6a6888fb30be4acd8f92a00d25f

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Applying this patch seems to resolve a 2.2+ blocker bug 1116732 at least on the master branch. We don't have any other ideas about how to fix that bug, so I want to begin the conversation about uplifting this.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: user won't be able to play music after setting a ringtone.


[Testing completed]: not yet; we need lots of testing if we uplift

[Risk to taking this patch] (and alternatives if risky): possibly risky. FindMyDevice was somehow preventing the music app from playing. It changed to use a different audio context priority level and no longer blocks the music app. But with this patch applied, I suppose there is a chance that find my device will now block other apps, like the ringtones app.  So we will need a lot of qa help to check whether this is a safe patch to uplift.

[String changes made]: none

Comment 11

[bhavana bajaj [:bajaj]]

:ktucker/njpark, reading the last few comments in 1116732, I think its best to apply this on 2.2 and do some exploratory testing and verification here...Can either of you help ? Jim probably is going to help on the engg side per https://bugzilla.mozilla.org/show_bug.cgi?id=1116732#c94, but i think this requires QA qualification as well. Thanks!

Comment 12

[No-Jun Park [:njpark]]

So I applied this patch to 2.2 gaia, and this bug is not reproducible in 2.2.  More exploratory testing is needed, so once I have the full 2.2 build image with the patch, I'll send it off to :ktucker so we can check for any fallouts.

Comment 13

[No-Jun Park [:njpark]]

The patch was applied on following build:
gaia: commit 265ca0bc9408c21fc4b25a259fcee7fb642cd06b
gecko: commit 7bb2fd8324164102476c8212391a48641d6f8d8c

Comment 14

[KTucker [:KTucker][Inactive 3/4/2016]]

After further testing, it appears that this patch is safe to uplift to 2.2. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=1116732#c97

Comment 15

[David Flanagan [:djf]]

KTucker and No-Jun: when you tested this, did you have find my device enabled?  (I've never used it, so I don't know what is involved in using it.)  The risk here, based on bug 1116734 is that with find my device on, sleeping the phone will someone disrupt other audio. I wish I understood audio channels better so I could say what you need to be on the lookout for.

But if find my device is now using audio-channel-ringer, that means that it has higher priority than SMS and email notifications, higher priority than alarm clock and calendar alarms, and equal priority to incoming phone calls (at least according to https://developer.mozilla.org/en-US/docs/Web/API/AudioChannels_API/Using_the_AudioChannels_API) 

In bug 1116734, Find My Device is disrupting music playback when it uses low priority audio-channel-content. That is the same level that the music app uses. So now that it is using the same priority as incoming calls, lets be really, really sure it doesn't prevent incoming calls.  Like maybe apply this patch, then follow the ringtones-based STR in bug 1116734, then place a call to the phone and see if it rings correctly.

But in order to test, we've got to know how to get Find My Device active. Maybe Frederick can explain how to do that.

Comment 16

[No-Jun Park [:njpark]]

(In reply to David Flanagan [:djf] from comment #15)
> KTucker and No-Jun: when you tested this, did you have find my device
> enabled?  (I've never used it, so I don't know what is involved in using

Yes, actually, it's pretty straightforward. All you need to do is to sign up a firefox account, and log in from the device.  In settings menu, then you enable find my device.

Once you done that, you go to find.firefox.com and login using the same credential.  If your phone can receive the gps signal, it will show your phone location on the map.  Even if it doesn't, you can still ring your phone using the 'Ring' button on the top left.

Comment 17

[KTucker [:KTucker][Inactive 3/4/2016]]

I did several calls to and from the device while playing music and no issues were observed. The phone received several calls while in the music app, other apps, the lockscreen, notifications tray and while the device was asleep and when awake. I also manually locked the device while on calls and such and no serious issues were observed. I also changed the ringtone several times, paused, shuffled, rewind and fast forward songs from the music app, lockscreen and notifications tray in between making and receiving calls. 

Unfortunately, I cannot accessed bug 1116734 but I did enabled FMD on the phone, ring the phone and lock the phone from the FMD webiste. I did this while the phone was asleep, awake, in the music app and while in other apps. 

The phone would ring normally when the device was asleep and then continue playing the song from the music app without issue. However, when the phone was awake playing music, the ring was heard in the background. It did not interrupt the song playing on the patched build. Both the ringtone and the song were heard at the same time. I tried to reproduce this on the regular non patched nightly build but the phone wouldn't even ring at all when activated from the FMD website.

We are currently investigating this one issue and writing it up but presently it seems like this patch is a winner.

Comment 18

[KTucker [:KTucker][Inactive 3/4/2016]]

Bug mentioned in Comment 17 where the phone does not ring when prompted to from the FMD site when a song is currently playing in background has been logged here bug 1159041. If the patch gets uplifted to 2.2 then this bug will change.

Comment 20

[David Flanagan [:djf]]

KTucker: Bug 1159041 is just a dupe of this bug :-)

Comment 21

[David Flanagan [:djf]]

In comment #15, I kept writing bug 1116734 when I meant bug 1116732. Sorry for the confusion there.

This bug (can't use find my device ringer when music is playing) seems important enough that we should uplift the fix regardless.

But we're depending on that fix to fix the 2.2+ bug 1116732, and the problem is that no one really understands why it fixes that bug. So that's why I'm being so compulsive about testing this carefully. If this patch fixes 1116732 but breaks something else important then we might be worse off if we uplift it.

KTucker: it sounds like you've tested very thoroughly. You didn't list clock or calendar alarms, however. Could you try something like this:

- set an alarm for a minute or two in the future
- sleep the phone and wake it up
- launch music and set the ringtone from a song
- wait to see if the alarm goes off when it should.

If that works, then I'm convinced and I think we should uplift.

I do wish we understood the underlying bug, though!

Comment 22

[KTucker [:KTucker][Inactive 3/4/2016]]

I feel silly. I thought I was doing exploratory testing around a patch that resolves bug 1116732 which this patch does fix that major issue on 2.2. However, this patch doesn't really fix the bug that is written here. Sure the phone rings now when a song is playing but it is in the background and doesn't pause the song that is currently playing on the DUT. The user will not be able to hear that their device is ringing.

Sorry about this. I hope this clears everything up.

Comment 23

[KTucker [:KTucker][Inactive 3/4/2016]]

Adding qawanted to further test the patch using alarms that is mentioned in Comment 21.

Comment 24

[Frederik Braun [:freddy]]

Looks like the needinfo was already resolved by someone else in comment 16.

Comment 25

[KTucker [:KTucker][Inactive 3/4/2016]]

Several alarms from the Clock app as well as the Calendar app were created on the dut. I set multiple songs as ringtones and tested the alarms going off while the phone was asleep as well as when it was awake in a variety of apps. No issues were observed. The alarms went off without a hitch and the songs resumed playing as expected after dismissing the alarms.

Comment 26

[bhavana bajaj [:bajaj]]

Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Given the qa testing here and since its blocking another blocker, i am approving this for 2.2

Comment 27

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/d819cd1ae581f73038a66f333b3f1441d0a4226a