When ringtone application is in background, music volume cant be changed

VERIFIED FIXED in Firefox OS v2.0

Status

Firefox OS
Gaia::Ringtones
--
major
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: Sharaf, Assigned: squib)

Tracking

unspecified
2.0 S5 (4july)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 verified)

Details

(Whiteboard: [LibGLA, TD-53728, WW, B])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
STR

* Open Music
* Play some audio file
* During play press Home key
* Open Settings->Sound->Manage ringtone
* Click on any tone, preview will be played
* Press Home key
* Go to Music, once music starts playing change the volume.

Issues:

1. It is observed that volume change dont have any effect on the music being played.
2. When we press home key again from Music app, music is stopped.
(Reporter)

Updated

4 years ago
Summary: When ringtone application is in foreground, music volume cant be changed → When ringtone application is in background, music volume cant be changed
(Reporter)

Comment 1

4 years ago
This issue happens because settings "launches" ringtone app when user selects manage ringtone option.
By default ringtone application occupies the highest "ringer" channel. But when the user presses homekey, as it is a launched application it is just pushed to the background, still occupying the ringer channel.

This issue is not happening when we do ringtone "pick", where the ringtone application is launched as inline activity and when user presses homekey the application is exited.

possible solutions:
* It seems we are launching the ringtone app, instead of launching as activity, is because of Bug 1015513 (Need confirmation). If thats the case we can try to emulate inline activity behavior by closing the window when the visibility is hidden

* release the channel when the window is hidden. I tried the following code, with it the issue is not happening.

  release: function() {
    this._player.removeAttribute('src');
    this._player.load();
    this._context.mozAudioChannelType = 'normal';
    this._currentTone = null;
  }

One problem with this solution is that even when the screen is locked, this code will be executed and music play will resume.

If any of the options are ok, please let me know, I can upload a patch.

ni? to dominic and jim porter for their comments.
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(dkuo)

Comment 2

4 years ago
I would suggest NOT to use this workaround because once you set the mozAudioChannelType of the audio context to |normal| and it only release the object when app/document is closed, the active channel will always be normal/content so that's why you can adjust the media volume, but I guess if you pause music then press the volumedown/volumeup buttons, probably you can only adjust the media volume which should cause a regression.

We should find out why the audio context is not released in this case, the pick activity case seems works fine.
Flags: needinfo?(dkuo)
(Reporter)

Comment 3

4 years ago
pick activity case its working because, when we press home key, the activity is killed.

But in this case its launched as an application, so pressing homekey puts it in background.
(this may be the reason why audio context is not released in this case, app is still in background)

Comment 4

4 years ago
(In reply to Sharaf from comment #3)
> pick activity case its working because, when we press home key, the activity
> is killed.
> 
> But in this case its launched as an application, so pressing homekey puts it
> in background.
> (this may be the reason why audio context is not released in this case, app
> is still in background)

This explains why the audio context is not released, but it's strange that I cannot find the background ringtone app in the card view, also after I am backed to settings app where we triggered it to launch, it disappeared as well, so maybe this is also a window management issue that the ringtone window is not correctly managed by system app.
(Assignee)

Comment 5

4 years ago
The reason we open the ringtone manager as an app and not an activity is because the manager can open activities of its own, and we wanted to avoid having an activity launch another activity (which would result in a total of 3 foreground windows).

This also makes closing the manager when hidden hard because we hide it when creating new ringtones (to show the "Create Ringtone" dialog). Maybe there's a better way to open the "Create Ringtone" popup that doesn't cause this. Right now, I'm just using window.open('share.html');
Flags: needinfo?(squibblyflabbetydoo)
(Reporter)

Comment 6

4 years ago
(In reply to Dominic Kuo [:dkuo] from comment #4)
> 
> This explains why the audio context is not released, but it's strange that I
> cannot find the background ringtone app in the card view, also after I am
> backed to settings app where we triggered it to launch, it disappeared as
> well, so maybe this is also a window management issue that the ringtone
> window is not correctly managed by system app.

ni? to alive for his comments from system app perspective.
Flags: needinfo?(alive)
(In reply to Sharaf from comment #3)
> pick activity case its working because, when we press home key, the activity
> is killed.
> 

Please note this will not be permanent. Activity will be kept in the future.
Flags: needinfo?(alive)
(Reporter)

Comment 8

4 years ago
hi Dominic,

Can you think of any Gaia solution for this issue?
Also is it expected for application to hold on to audio context when they are in background?

we feel that this is a major issue as the user will have to restart the device to normal situation.

ni? Dominic for his opinion.
Flags: needinfo?(dkuo)
(Assignee)

Comment 9

4 years ago
Some ideas for what we could do:

* Close the ringtone manager whenever it is hidden; this adds some extra complexity, but it would fix this, and also an issue where the manager's list of ringtones can get out of date (e.g. if you open the ringtone manager, go to music, create a new ringtone, and go back to the manager, then you won't see the newly-created ringtone[1]).

* Change the audio channel to "normal" when the ringtone manager is hidden.

* Close down the AudioContext and stop the ringtone preview entirely when the ringtone manager is hidden.

[1] I think the "correct" fix for this would be for indexedDB to let us listen for changes from other tabs/instances and automatically update.
(Reporter)

Comment 10

4 years ago
(In reply to Jim Porter (:squib) from comment #9)
> Some ideas for what we could do:
> 
> * Close the ringtone manager whenever it is hidden; this adds some extra
> complexity, but it would fix this, and also an issue where the manager's
> list of ringtones can get out of date (e.g. if you open the ringtone
> manager, go to music, create a new ringtone, and go back to the manager,
> then you won't see the newly-created ringtone[1]).
> 

As mentioned in comment #1 (first option) , I have already tried this, and its working well.
Also the problem you mentioned in Comment #5 (second part) is not happening. When ringtone manager opens the activity and after that the share window, we are not losing visibility.

> * Change the audio channel to "normal" when the ringtone manager is hidden.
>

This is the second option I mentioned in Comment #1. But it will cause the issue Dominic mentioned in comment #2.
 
> * Close down the AudioContext and stop the ringtone preview entirely when
> the ringtone manager is hidden.
> 

As far as I know, we cant do this from Gaia, correct me if I am wrong.

> [1] I think the "correct" fix for this would be for indexedDB to let us
> listen for changes from other tabs/instances and automatically update.

So I still feel that the option 1 is better, but needed yours and Dominic opinion on it.
I don't know how ringtone manager works but I prefer option (1).
(Assignee)

Comment 12

4 years ago
(In reply to Sharaf from comment #10)
> (In reply to Jim Porter (:squib) from comment #9)
> > Some ideas for what we could do:
> > 
> > * Close the ringtone manager whenever it is hidden; this adds some extra
> > complexity, but it would fix this, and also an issue where the manager's
> > list of ringtones can get out of date (e.g. if you open the ringtone
> > manager, go to music, create a new ringtone, and go back to the manager,
> > then you won't see the newly-created ringtone[1]).
> > 
> 
> As mentioned in comment #1 (first option) , I have already tried this, and
> its working well.
> Also the problem you mentioned in Comment #5 (second part) is not happening.
> When ringtone manager opens the activity and after that the share window, we
> are not losing visibility.

Unless the system app has changed recently, when the share window opens, the mange ringtone window loses visibility. Nothing happens right now when visibility is lost, but I briefly tried closing the manager whenever it lost visibility, and the share window was causing issues with that. The end result was some needlessly-complex code, so I removed it entirely. If you could post a diff of what you tried, that would help.

> > * Close down the AudioContext and stop the ringtone preview entirely when
> > the ringtone manager is hidden.
> > 
> 
> As far as I know, we cant do this from Gaia, correct me if I am wrong.

That's easy: just destroy the AudioContext object we hold in the ringtones app.

Comment 13

4 years ago
If I recalled correctly, I have done some experiments on the AudioContext api, and once it's created by the new function with the mozAudioChannelType set, it will trigger the channel change and the channel that the audio context used will be the active channel before connecting any audio source(in our case the source is the audio element), this means the audio context should like playing a silent sound after it's created, this is still fine for ringtone app because we can new the AudioContext when the first tone needs to be previewed.

At the time we need to stop the silent audio context(visibility changes), we should destroy it. We didn't do it before because the ringtone is opened as an activity, the audio context will be destroyed after the activity is closed. Now we want to fix this issue by setting the audio context's mozAudioChannelType to |normal|, I think it works because it could successfully trigger the channel change, and plus the characteristic that normal channel is not allow to play in the background. So here is the problem, why not directly destroy the audio context but change the mozAudioChannelType? it seems pretty hack to me, but I guess it's because we don't know the correct way to do it, or by setting the audio context to |null| does not equal to release/destroy it.

Have you try anything else to release/destroy the audio context? I am also a tyro on web audio so don't know how to do it, maybe we should ask someone from the media platform to answer this. BTW, I saw the tone player in dialer has the same way as yours to release the audio context, in TonePlayer.trashAudio(), but it's strange that mozAudioChannelType is set to |normal| and audio context is set to |null| as well.
Flags: needinfo?(dkuo)

Updated

4 years ago
See Also: → bug 1019294
(Assignee)

Comment 14

4 years ago
(In reply to Dominic Kuo [:dkuo] from comment #13)
> BTW, I saw the tone player in dialer
> has the same way as yours to release the audio context, in
> TonePlayer.trashAudio(), but it's strange that mozAudioChannelType is set to
> |normal| and audio context is set to |null| as well.

That's something I added. Without setting mozAudioChannelType to "normal", we have to wait for the garbage collector to clean up the AudioContext after it's set to null, which usually takes about 5 seconds and was causing a bad experience for music. See bug 961986.
flagging for 2.0 triage, as according to https://bugzilla.mozilla.org/show_bug.cgi?id=1019294#c5, could be blocking a blocker.
Status: UNCONFIRMED → NEW
blocking-b2g: --- → 2.0?
Depends on: 1019294
Ever confirmed: true
status-b2g-v2.0: --- → affected

Comment 16

4 years ago
Blocking because volume control on music is broken when ringtone previews goes into background (basic scenario). 

Jim, please take this one. 

Thanks
Hema
Assignee: nobody → squibblyflabbetydoo
blocking-b2g: 2.0? → 2.0+
(Assignee)

Comment 17

4 years ago
Created attachment 8445374 [details] [review]
Fix it

Ok, this resets the mozAudioChannel back to "normal" when we lose visibility, which fixes things. It also fixes bug 1028481 because why not?

I feel like this should have tests but I have no clue how to test audio channel stuff.
Attachment #8445374 - Flags: review?(dflanagan)
(Assignee)

Comment 18

4 years ago
Also, I would just like to say: I hate the audio channel stuff.

Comment 19

4 years ago
(In reply to Jim Porter (:squib) from comment #17)
> Created attachment 8445374 [details] [review]
> Fix it
> 
> Ok, this resets the mozAudioChannel back to "normal" when we lose
> visibility, which fixes things. It also fixes bug 1028481 because why not?
> 
> I feel like this should have tests but I have no clue how to test audio
> channel stuff.

Thank you!
Comment on attachment 8445374 [details] [review]
Fix it

In reviewing it this patch I found a bug in the code that sends validated events. You use the undefined identifier 'self' twice in tone_player.js. As far as I can tell, the validated events are busted. It seems easiest to just fix that as part of this patch. But if not, then at least file a followup bug.

I was surprised to see that the visiblity change handler does not automatically call setExclusiveMode right away when the app comes back to the foreground. If I understand correctly background music will keep playing until and unless the user taps on a ringtone, both on the initial launch and subsequently any time the app comes back to the foreground.  If that is the desired behavior, then it is fine with me.

Please double check this situation: start playing a ringtone, hit the home button and then go back to the ringtone manager and tap on the same ringtone again. Will that hit the if clause at line 75 (correct) or the else clause at line 79 (incorrect)?  I don't know if calling stop() on the player sets player.paused or if you need to do !player.playing or player.stopped || player.paused or something.

With that fix and that check, this looks good to me.
Attachment #8445374 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 21

4 years ago
(In reply to David Flanagan [:djf] from comment #20)
> I was surprised to see that the visiblity change handler does not
> automatically call setExclusiveMode right away when the app comes back to
> the foreground. If I understand correctly background music will keep playing
> until and unless the user taps on a ringtone, both on the initial launch and
> subsequently any time the app comes back to the foreground.  If that is the
> desired behavior, then it is fine with me.

What I wanted to do was to let the background audio keep playing until you start previewing a tone, whether the ringtones app was already opened in the past or not. This way, if you preview a tone and hide the app, when you come back, it'll be similar to how it was when you first opened it (i.e. not in exclusive mode yet). I tried both ways and found this one less jarring.
 
> Please double check this situation: start playing a ringtone, hit the home
> button and then go back to the ringtone manager and tap on the same ringtone
> again. Will that hit the if clause at line 75 (correct) or the else clause
> at line 79 (incorrect)?  I don't know if calling stop() on the player sets
> player.paused or if you need to do !player.playing or player.stopped ||
> player.paused or something.

I tested that and it works fine; stop() actually just calls player.pause(), but it's the public-facing method for doing so on the TonePlayer. I use it in manage.js too.
(Assignee)

Comment 22

4 years ago
Landed with the fix from comment 20:
  https://github.com/mozilla-b2g/gaia/commit/610d7cb253471c7517aaa5e15453d24fb4c8c822
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
v2.0: https://github.com/mozilla-b2g/gaia/commit/7b766f602e0219b3e04d65027a4924abd7c3a27f
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: --- → fixed
Target Milestone: --- → 2.0 S5 (4july)
(Reporter)

Updated

3 years ago
Whiteboard: [LibGLA, TD-53728, WW, B]
Created attachment 8532978 [details]
Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/10

Flame v2.0 version:
Gaia-Rev        ead3b72a84512750bc5faff4e9e8faa1715c0d05
Gecko-Rev       c715df57d1d1593ede48140ebc88e101f8c3f7da
Build-ID        20141208050313
Version         32.0
Device-Name     jrdhz72_w_ff
FW-Release      4.4.2

Flame v2.1 version:
Gaia-Rev        38e17b0219cbc50a4ad6f51101898f89e513a552
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a
Build-ID        20141205001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141205.035305
FW-Date         Fri Dec  5 03:53:16 EST 2014
Bootloader      L1TC00011880
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.