FindMyDevice should use the ringer audio context type

RESOLVED FIXED in Firefox OS v2.2

Status

Firefox OS
FindMyDevice
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: freddyb, Assigned: freddyb)

Tracking

unspecified
2.2 S9 (3apr)
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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
Created attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master
(Assignee)

Comment 2

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

Can you take a look, please?
Attachment #8583809 - Flags: review?(guilherme.p.gonc+bmo)
Duplicate of this bug: 1026728
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.
Attachment #8583809 - Flags: review?(guilherme.p.gonc+bmo)
(Assignee)

Comment 5

3 years ago
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.
Attachment #8583809 - Flags: review?(guilherme.p.gonc+bmo)
Comment on attachment 8583809 [details] [review]
[gaia] mozfreddyb:bug-1147862-fmd-ringer-context > mozilla-b2g:master

Thanks!
Attachment #8583809 - Flags: review?(guilherme.p.gonc+bmo) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed
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.
(Assignee)

Updated

3 years ago
Depends on: 1148396
Adding checkin-needed again as the suggested review list has been updated.
Assignee: nobody → fbraun
Keywords: checkin-needed

Updated

3 years ago
Keywords: checkin-needed

Updated

3 years ago
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
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
Attachment #8583809 - Flags: approval-gaia-v2.2?(bbajaj)
: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!
Flags: needinfo?(npark)
Flags: needinfo?(ktucker)
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.
Flags: needinfo?(npark)
The patch was applied on following build:
gaia: commit 265ca0bc9408c21fc4b25a259fcee7fb642cd06b
gecko: commit 7bb2fd8324164102476c8212391a48641d6f8d8c
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
Flags: needinfo?(ktucker)
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.
Flags: needinfo?(npark)
Flags: needinfo?(ktucker)
Flags: needinfo?(fbraun)
(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.
Flags: needinfo?(npark)
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.
Flags: needinfo?(ktucker)
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.

Updated

3 years ago
Duplicate of this bug: 1159041
KTucker: Bug 1159041 is just a dupe of this bug :-)
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!
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.
Adding qawanted to further test the patch using alarms that is mentioned in Comment 21.
Keywords: qawanted
(Assignee)

Comment 24

3 years ago
Looks like the needinfo was already resolved by someone else in comment 16.
Flags: needinfo?(fbraun)
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.
QA Whiteboard: [QAnalyst-Triage+]
Keywords: qawanted
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
Attachment #8583809 - Flags: approval-gaia-v2.2?(bbajaj) → approval-gaia-v2.2+
v2.2: https://github.com/mozilla-b2g/gaia/commit/d819cd1ae581f73038a66f333b3f1441d0a4226a
status-b2g-v2.2: --- → fixed
status-b2g-master: --- → fixed
Target Milestone: --- → 2.2 S9 (3apr)
You need to log in before you can comment on or make changes to this bug.