Closed Bug 958470 Opened 6 years ago Closed 6 years ago

[Ringtones][Music] Previewing the ringtones should not be mixed together with the background music

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S3 (14mar)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dkuo, Assigned: dkuo)

References

Details

(Keywords: regression, Whiteboard: [AUDIO_COMPETING] [UX_TRIAGED])

Attachments

(2 files)

When users try to change the ringtones in settings app, if music app is playing in background and users preview/select the new ringtone, ringtone will be mixed together with the background music.

Here are two issues we have to address:

1. The audio channel of ringtones app is not properly set, we should add "audio-channel-ringer" permission to the manifest, then set mozAudioChannelType = "ringer" to the audio element, see details in: https://wiki.mozilla.org/WebAPI/AudioChannels

2. Because of the issue of audio competing policy(bug 843955), after 1 is addressed the ringtones app will have the same issue that, the background music will leak when switching the preview ringtones. This is still under discussing which we probably will move the audio management from gecko to gaia to solve this problem.
See Also: → 843955
After the offline meeting with ux team and gecko devs, we decided to address this issue just like what we are going to do for bug 843955, that's, when users start to preview some ringtones, the background music should be paused/interrupted, until they leave the preview page.

Issue 1 should be easy by setting the mozAudioChannelType to ringer, but issue 2 needs some other technique. I had done some experiments and proved that web audio api is able to solve this problem, the basic idea is web audio can occupy the audio channel without the channel changes, so the background won't be able to resume when one of the ringtones finished previewing. Since my experiment was writing a prove-of-concept patch to verify it, so I am taking this and re-writing a complete patch.
Assignee: nobody → dkuo
Jim,

Since you are working on the ringtones app and also know about the audio competing policy(though David has implemented it but he is always busy), I think you should be able to review this patch, and these changes might effect your work. I have addressed the two issues in comment 0, would you please review this? thanks.
Attachment #8373144 - Flags: review?(squibblyflabbetydoo)
At first glance, this looks ok, but it really seems like we've been working around deficiencies in the audio competing policy a lot lately. I'm a little bit worried that we're going to be stuck with these workarounds forever instead of coming up with a better set of rules for audio channels.
(In reply to Jim Porter (:squib) from comment #3)
> At first glance, this looks ok, but it really seems like we've been working
> around deficiencies in the audio competing policy a lot lately. I'm a little
> bit worried that we're going to be stuck with these workarounds forever
> instead of coming up with a better set of rules for audio channels.

Jim, sorry I forgot to mention that, for the long-term solution, we have the conclusion in the offline audio competing meeeting. The main idea is we planned to move the audio channel management from gecko to gaia, that means the current management in gecko will be removed, and in gaia we will have an new web api to mute/unmute the iframes so that system app is capable of managing the audio competing, base on the audio channel types. This is traced in bug 961967.

So for the current state(1.3 and 1.4), we could only address the audio competing issues by using different approaches, they looks like workaround but they are temporary for sure, for those we cannot fix for now, we will leave them to later release, and hopefully the new gaia audio management can fix them in the future.
Blocks: 843955
[UX_TRIAGED]

Since this issue also happened in clock app(bug 843955), and clock app suppose to pick alarm tones from the ringtones app(bug 959020), the reviewing patch could potentially fix both this bug and bug 843955 in 1.4, so ux thought it would be nice if we can fix this issue first, instead of wait the new gaia audio management to be enabled, and fixed in next release.

Jim, if you think landing this patch will take more time to complete the integration of ringtones and music apps, feel free to pick up my patch/idea and directly merge them into your working branch, or you think we can patch this issue after you land the ringtones app, I will fix it later, thanks.
blocking-b2g: --- → 1.4?
Whiteboard: [AUDIO_COMPETING] → [AUDIO_COMPETING] [UX_TRIAGED]
Comment on attachment 8373144 [details] [review]
patch for master - with integration tests

Yeah, let's do this in my branch. I almost completely rewrote the ringtones app, so merging this with my code would be a mess. I'll post another comment when I've landed this on my branch: <https://github.com/jimporter/gaia/tree/ringtones-merge>.

We can keep your branch around though in case something terrible happens and we can't land mine. :)
Attachment #8373144 - Flags: review?(squibblyflabbetydoo)
Great, that's what I thought and let's do this!
Based on the recent discussion in fxos-media, the ringtones rework might be getting pushed back to 1.5. I'll figure out what we should do here; maybe it would be better to land this as-is and I'll just make sure the change sticks around in my rewrite of the ringtones app.
Blocks: 975230
Blocks: 975347
No longer blocks: 975347
No longer blocks: 975230
No longer blocks: 843955
Status: NEW → RESOLVED
blocking-b2g: 1.4? → ---
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 956811
This bug is different from bug 956811.

Bug 958470: Preview the ringtones in settings app while music app is playing in the background.
Bug 956811: Share one song from the music player to the setringtone app.

They are actually two different apps/bugs, bug 958470 is "ringtones app" and bug 956811 is "setringtone app".
Blocks: 843955
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
(In reply to Dominic Kuo [:dkuo] from comment #10)
> This bug is different from bug 956811.
> 
> Bug 958470: Preview the ringtones in settings app while music app is playing
> in the background.
> Bug 956811: Share one song from the music player to the setringtone app.
> 
> They are actually two different apps/bugs, bug 958470 is "ringtones app" and
> bug 956811 is "setringtone app".

They are exactly the same app, with pretty much the same workflow with the exception of ringtones being triggered by a share activity in bug 956811, where as this one is driven by a setringtone activity. I'd double check if this still reproduces on trunk with the current fix in place - this might already be fixed.
Jason: you should stop contradicting Dominic when the topic is audio channel stuff.  He knows more than anyone, at least on the gaia side.  In 1.3 and 1.4, ringtones and setringtone are two different apps with very different purposes.
(In reply to David Flanagan [:djf] from comment #12)
> Jason: you should stop contradicting Dominic when the topic is audio channel
> stuff.

Maybe Jason's in the wonderful future where my ringtones branch has already landed. :)

Probably what we should do here is just land Dominic's change... once we add tests. (Yes, I'm officially That Guy on the team now. I'm truly sorry.)
Also, Dominic: do you think we could reuse this code to work with the setringtone app too? David Flanagan's workaround using the mozSettings API to talk between setringtone and music is really gross, and if we can fix that by using WebAudio, we totally should.

(I might regret saying this, since I'm literally asking for my ringtones branch to be bitrotted. Oh well!)
Flags: needinfo?(dkuo)
For clarity - while this bug & bug 975230 are focusing on the same problem, I'd prefer to keep this bug focused on the long term solution, while bug 975230 focuses on a contextual safe 1.3 fix to the problem that's not risky. Introducing a Web Audio solution is definitely going to have regression risk.
I think that just makes it less obvious that the two pull requests (one for 1.4 and one for 1.3) are fixing the very same issue, but with a different evaluation of the risk for each branch. I'd much rather see these in one bug to make it really clear that we're fixing one issue and not two.

Granted, I don't really care enough to fight it any more than this one comment.

(Incidentally, the fix for 1.3 is just to set the audio channel of the audio element in the ringtones app to "notification".)
(In reply to Jim Porter (:squib) from comment #16)
> I think that just makes it less obvious that the two pull requests (one for
> 1.4 and one for 1.3) are fixing the very same issue, but with a different
> evaluation of the risk for each branch. I'd much rather see these in one bug
> to make it really clear that we're fixing one issue and not two.
> 
> Granted, I don't really care enough to fight it any more than this one
> comment.
> 
> (Incidentally, the fix for 1.3 is just to set the audio channel of the audio
> element in the ringtones app to "notification".)

We could do that, other than guideline that we're going to keep the 1.3 patch non-risky. I'll dupe it then.
Duplicate of this bug: 975230
blocking-b2g: --- → 1.3+
Keywords: regression
Blocks: 905856
(In reply to Jim Porter (:squib) from comment #14)
> Also, Dominic: do you think we could reuse this code to work with the
> setringtone app too? David Flanagan's workaround using the mozSettings API
> to talk between setringtone and music is really gross, and if we can fix
> that by using WebAudio, we totally should.

I think you should be talking about the patch of bug 956811. Unfortunately we can't reuse the same code because actually it's a window-management issue(bug 892371), even we try to set the some channel type for bug 956811, because the caller(music) app and callee(setringtone) app are both "visible"(document.hidden is false), they can be heard at the same time due to the current audio competing policy is coupling with the app is visible or not. So either setting the channel type or using WebAudio won't be able to fix bug 956811.
Flags: needinfo?(dkuo)
(In reply to Jason Smith [:jsmith] from comment #15)
> For clarity - while this bug & bug 975230 are focusing on the same problem,
> I'd prefer to keep this bug focused on the long term solution, while bug
> 975230 focuses on a contextual safe 1.3 fix to the problem that's not risky.
> Introducing a Web Audio solution is definitely going to have regression risk.

My original plan was to fix those two issues I mentioned in comment 0 together, since they are very same issue and ux wanted them. The long term solution will be a more overall audio management in gaia(bug 961967) and takes time to implement, so possibly can be enabled at least in 1.5, or after 1.5. And if the patch does not cause too many regression but fixes bugs and gives better ux, we should land it for 1.3 and 1.4 since we won't be able to fix this bug completely before 1.5.

One thing I think it's worthy to mention is, the dialer and emergency-call started to use WebAudio to play the key tones in 1.3(bug 927852), the way they used the WebAudio api is more complex than my patch in attachment 8373144 [details] [review], I am not saying the WebAudio won't cause regressions, but since the dialer was encouraged to use the WebAudio, perhaps we can also give this patch a chance.
Comment on attachment 8373144 [details] [review]
patch for master - with integration tests

Jim, I think let's bring back the patch to the review process so that we can fix this bug, I set the channel type to "ringer" because ux wanted the preview ringtones interrupt the background music, using "notification" will reduce the volume of background music to 20% but still two channels will be mixed together. would you please review this again, thanks.
Attachment #8373144 - Attachment description: use web audio api to preview the ringtones → set ringer type and use web audio api to preview the ringtones
Attachment #8373144 - Flags: review?(squibblyflabbetydoo)
This looks good overall, but I think we need some basic tests for this. You can probably do something similar to the tests I wrote for you for the FM radio bug. That is, create a "fake" app in apps/ringtones/test/marionette, then open up the ringtones picker and make sure the fake app's <audio> element receives the "mozinterruptbegin"/"mozinterruptend" events.
We'll also need a simpler version of this patch for 1.3 that only sets the audio channel to "ringer". I don't think you absolutely *need* tests for the 1.3 version, but if you can adapt them easily from the 1.4 version, that would be nice.
Attached file patch for 1.3
Here is the 1.3 patch and I am working on the integration tests.
Comment on attachment 8373144 [details] [review]
patch for master - with integration tests

Jim,

I have wrote the integration tests base on comment 22, the idea is almost the same, created a fake music then launch the ringtones activity to see if it will interrupt the fake music, but I didn't directly listen to the audio element in the fake music because I think that's something more like gecko tests, instead, I checked the playing indicator in the status bar to make sure the content channel is interrupted or resumed, this should be more close the issues we want to fix here, would you please review it again? thanks.
Attachment #8373144 - Attachment description: set ringer type and use web audio api to preview the ringtones → patch for master - with integration tests
Attachment #8386905 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 8373144 [details] [review]
patch for master - with integration tests

I didn't run this myself, but the tests look good and it's pretty straightforward. Assuming you've manually tested this too, then r=me.

I have a few comments on Github that should be addressed before this lands, but they're all very minor.
Attachment #8373144 - Flags: review?(squibblyflabbetydoo) → review+
Comment on attachment 8386905 [details] [review]
patch for 1.3

This looks good, but if you can, please try to copy the tests from the 1.4 version of the patch over to this version. I think the tests should work without having to change much, and having tests would make people less worried about landing this.
Attachment #8386905 - Flags: review?(squibblyflabbetydoo) → review+
(In reply to Jim Porter (:squib) from comment #26)
> Comment on attachment 8373144 [details] [review]
> patch for master - with integration tests
> 
> I didn't run this myself, but the tests look good and it's pretty
> straightforward. Assuming you've manually tested this too, then r=me.

Thanks Jim, I have tested manually and on travis, it works for me and passed the automation tests in https://travis-ci.org/mozilla-b2g/gaia/builds/20622760

> I have a few comments on Github that should be addressed before this lands,
> but they're all very minor.

I have addressed them all before I merged it.

Landed on master: 6f49249b429095064df345c9b24c00f4d7960972
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Jim Porter (:squib) from comment #27)
> Comment on attachment 8386905 [details] [review]
> patch for 1.3
> 
> This looks good, but if you can, please try to copy the tests from the 1.4
> version of the patch over to this version. I think the tests should work
> without having to change much, and having tests would make people less
> worried about landing this.

I have copy the tests for master to the 1.3 patch without changes, that's a good news because although the patches for master and 1.3 are slightly different, but they do have the same effect after applied them, and the marionette tests can prove this. Also because the tests needed the |marionette-file-manager| package, I added it to the configuration files to fix the dependency.

The tests passed in https://travis-ci.org/mozilla-b2g/gaia/builds/20600594, though this is one failed but not related to my patch.
Comment on attachment 8386905 [details] [review]
patch for 1.3

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): bug 905856(please see https://bugzilla.mozilla.org/show_bug.cgi?id=975230#c9 for why bug 905856 caused this issue)
[User impact] if declined: no
[Testing completed]: tests passed with new test cases.
[Risk to taking this patch] (and alternatives if risky): low
[String changes made]: no

Fabrice, comment 29 explained what's the differences between the patches of master and 1.3, hope that can help you to judge the approval of this patch, would you please check it? thanks!
Attachment #8386905 - Flags: approval-gaia-v1.3?(fabrice)
Attachment #8386905 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
v1.3: 0b9bb79f08fe8bc8ad4ee303de7eabeb39d4b6e4
Target Milestone: --- → 1.4 S3 (14mar)
Sorry - this caused an intermittent failure of around 15% on travis and is causing all sorts of greys today. We need to back this out, and I'll leave it to you to decide how to reland this (if it's urgent maybe follow-up with tests after the patch lands). Recommended debugging steps would be to throw tons of console.log statements in the code and run it a bunch on travis - it sucks that we have to resort to this usually. Also maybe worth seeing if you can reproduce locally, but the intermittent nature makes that painful.

The tests are timing out, with no real useful error log. You can see some failure examples here: https://travis-ci.org/mozilla-b2g/gaia/builds/20680200

Backed out from master: https://github.com/mozilla-b2g/gaia/commit/52477b7f6a5e9a9f5de5e0711acc939c1c6a48c7
Backed out from v1.3: https://github.com/mozilla-b2g/gaia/commit/edde65421e811e9f923034529e6c31db9413e46b
Status: RESOLVED → REOPENED
Flags: needinfo?(dkuo)
Resolution: FIXED → ---
Would be nice to get a debug dump, because currently this is the only thing in the logs:

 Ringtones app pick activity

◦ Interrupt and resume the background music:

No output has been received in the last 10 minutes, this potentially indicates a stalled build or something wrong with the build itself.
(In reply to Kevin Grandon :kgrandon from comment #33)
> Would be nice to get a debug dump, because currently this is the only thing
> in the logs:
> 
>  Ringtones app pick activity
> 
> ◦ Interrupt and resume the background music:
> 
> No output has been received in the last 10 minutes, this potentially
> indicates a stalled build or something wrong with the build itself.

Actually I did confirmed the tests passed locally and on travis so I merged them, at least on master(comment 28) and locally I cannot reproduce it, I will see what's going on and re-land this, thanks.

* Note that travis on 1.3 there is one test keep failing and maybe we should back it out first, please see https://travis-ci.org/mozilla-b2g/gaia/jobs/20600597
Flags: needinfo?(dkuo)
Blocks: 984260
I still passed my tests 100% locally and need more time to figure out why it only passed 85% on travis instead of 100%, but this is a 1.3+ blocker and I don't want anyone to be blocked on this, so I have re-landed the patches first.

re-landed on master: 30a0ea33b8b9ad601c52db28d3627226c90c2a5e
re-landed on v1.3: 56c76f486217fc97c07db049192ac28afe8c60c8

And integration tests will be in bug 984260.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: in-testsuite+ → in-testsuite?
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.