Closed Bug 956811 Opened 6 years ago Closed 6 years ago

[B2G][Music] The song does not stop playing when previewing the song for a ringtone so overlapping audio occurs

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

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

RESOLVED FIXED
1.3 C3/1.4 S3(31jan)
blocking-b2g 1.3+
Tracking Status
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: KTucker, Assigned: djf)

References

Details

(Whiteboard: dogfood1.3, [AUDIO_COMPETING] [fxos:media])

Attachments

(3 files)

Description:
If the user plays a song, taps the "share button", taps on "Set Ringtone" and then taps the "play button" to preview it, the user will notice that the song is playing twice. 

Repro Steps:
1)  Updated Buri to Build ID: 20140106004001
2)  Tap on the "Music" app.
3)  Tap on a song to play it. 
4)  Tap on the "Share" button while the song is playing in the background.
5)  Tap on "Set Ringtone" and then tap on the "Play" button to preview the song.

Actual:
The previously played song does not stop playing so when the user plays the preview for the ringtone, they will hear the songs' audio overlapping each other.

Expected:
The previously played song stops playing when the user plays the song to preview it for a ringtone. 

Environmental Variables
Device: Buri v 1.3.0 Mozilla RIL
Build ID: 20140106004001
Gecko: http://hg.mozilla.org/releases/mozilla-aurora/rev/a43cb4b322d3
Gaia: 35a60b82f8cf2d759939a350e2dadbb9d8b2f5dc
Platform Version: 28.0a2

Notes:
Repro frequency: 100%
See attached: video clip, logcat
I could not verify this issue on the latest Buri v 1.2.0 Mozilla RIL 

Device: Buri v 1.2.0 Mozilla RIL
Build ID: 20140102004001
Gecko: http://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0c11156c7d9b
Gaia: b1bc88386c781148a25091bf2eeee3ba217281d0
Platform Version: 26.0

The share button is not functioning in Music. When the user taps the button nothing will happen.
blocking-b2g: --- → 1.3?
Attached video Ringtone.mp4
I think this is an bug of the audio competing policy.

First I tried to set mozAudioChannelType = "content" to the audio tag that the ringtones app uses, and reproduced this bug, but the background music was still playing and mixed with the foreground previewing ringtone, so I believe the visibility(document.hidden) of music app is still true, which the foreground ringtones app couldn't interrupt it then caused this issue.

This is another scenario that the original Audio Competing Policy didn't consider of, adding [AUDIO_COMPETING] to the whiteboard and changing the component to AudioChannel.
Component: Gaia::Music → AudioChannel
Whiteboard: dogfood1.3 → dogfood1.3, [AUDIO_COMPETING]
Duplicate of this bug: 957524
hi Marco, anyone from your team can look at this? Thanks
Flags: needinfo?(mchen)
After discussing with Tzu-Lin offline, the conclusion until now is

  1. In this case, the attention window didn't let music app be a background app. (window management)

  2. So both attention window for preview music and music app itself are all on visible state. (even they are two separate processes.)

  3. The AudioChannelService now depend on visibility to judge which one should be paused/muted so AudioChannelService can't pause each of them according to they are all visible.

------------

Hi Alive,

May I know that why attention screen didn't cause the app behind it as background?
Thanks.
Flags: needinfo?(mchen) → needinfo?(alive)
It's a general activity issue of https://bugzilla.mozilla.org/show_bug.cgi?id=892371
so we cannot put activity caller to background.
Flags: needinfo?(alive)
Hi Alive,

Are you saying the bug 892371 has to be fixed before the issue in this bug can be addressed? I think we need some decision points on this bug to be blocker or not. If bug 892371 is the case, I think this one should not be the blocker then.
Flags: needinfo?(alive)
Hi Marco,

I also need some comments from you about if this one should be blocker or not on 1.3 release.
Flags: needinfo?(mchen)
(In reply to Ivan Tsay (:ITsay) from comment #8)
> Hi Alive,
> 
> Are you saying the bug 892371 has to be fixed before the issue in this bug
> can be addressed? I think we need some decision points on this bug to be
> blocker or not. If bug 892371 is the case, I think this one should not be
> the blocker then.

A different bug shouldn't justify the blocking decision on this bug. This needs to be analyzed independently of that bug for it's impact to block, not the level of risk associated.

This bug impacts a new feature in the 1.3 timeframe that's a basic audio competing use case, so this is likely going to get push back in a certification process if we don't fix this. It's a bad experience in general to intermix tones, so I'd lean towards blocking on this.
I don't think bug 892371 is a 1.3 blocker, and also this one.

If we don't have bug 892371, something in my mind to fix this:
1. Stop music after share activity calling anyway.
2. Use window disposition activity other than inline activity.
3. IAC..
Flags: needinfo?(alive)
(In reply to Alive Kuo [:alive][NEEDINFO!] from comment #11)
> I don't think bug 892371 is a 1.3 blocker, and also this one.

I believe this is too basic of a use case, so I stand by blocking on this. It's very likely a user can decide to set a ringtone from the music app while the song is still playing. When they enter the preview window of the song, they are likely going to get confused, as the user's understanding is that the song is still playing, but the UI sends the impression it's not. We also know that the set ringtones feature is a top requested feature in SUMO forums, so we need to do this feature right.
According to limit from bug 892371, AudioChannelService can't know music app is on the background then pause it's audio. So maybe we need music app to pause the music when "set ringtone" is triggered by itself. 

But it need UX's approval.

Hi Dominic,

May I know your feedback and who is  the UX for music app? Thanks.
Flags: needinfo?(mchen) → needinfo?(dkuo)
We need to change both UI flow of music + ringtone app at the same time to fulfill this.
Proposed change: Use IAC to open ringtone app in background and set it as ringtone immediately.
Jacqueline

Please help with an easy path to help with this scenario.
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(jsavory)
(In reply to Marco Chen [:mchen] from comment #13)
> According to limit from bug 892371, AudioChannelService can't know music app
> is on the background then pause it's audio. So maybe we need music app to
> pause the music when "set ringtone" is triggered by itself. 

Currently music app can share the songs to email, messages(sms), bluetooth and ringtones, to pause music when sharing to ringtones seems right but for the other options, like bluetooth, users probably don't want the music to be paused while transferring, and music app cannot know which option/activity was selected, so what we can do is, pause the music no matter which option the user select, it will fix this issue but will effect the other options of sharing.

I agree with Jason in comment 12. Besides the issue of audio competing, the current usage of setting ringtones from music app also seems confused for users, the users should already knew which song they are listening which made the preview window unnecessary. Since we need to adjust the ui flow, we might be able to solve the audio competing issue as well, so let's have some inputs from ux first, see if we can fix this and get better ux.

> May I know your feedback and who is  the UX for music app? Thanks.

As Preeti mentioned Jacqueline owns the ux of music and ringtones. Jacqueline, would you please comment on this? thanks.
Flags: needinfo?(dkuo)
I think that it would be best if the audio pauses only when the user selects the "Set Ringtone" option in the share menu. We are trying to add a crop functionality that would require users to be able to hear which section of a song they are using when they set a ringtone. For the other sharing options, the music should continue to play. Once the user has finished setting the ringtone and returns to the music app, the music should automatically begin to play again.
Flags: needinfo?(jsavory)
Hi Hema,
It looks like the fix for this will be in Gaia and not audio channel, can you keep this one on your radar?  Component would be either music or ringtones.
Thanks
Flags: needinfo?(hkoka)
Does this reproduce on a 1.2 build?
Keywords: qawanted
We are coming across many bugs around audio overlap and competing policy issues that create unpleasant user experience -- surfacing on music and ringtones apps. Would be good to track the issues with audio competing policy and fix the various cases. I believe Dominic and Marco are working hashing out the various cases and the expected behavior. 

For 1.4, Jacqueline is working on coming up with UX for better Ringtone Creation experience. As part of that, we could look into adding an acceptance criteria for this case where overlapping audio when previewing ringtones while music is playing in the background. 

As for this issue, if is not a regression from 1.2, would suggest addressing it in 1.4 as part of Ringtone work. NI Sri and djf for their input as well. 

Thanks
Hema
Flags: needinfo?(skasetti)
Flags: needinfo?(hkoka)
Flags: needinfo?(dflanagan)
Keywords: qawanted
(In reply to Hema Koka [:hema] from comment #20)
> We are coming across many bugs around audio overlap and competing policy
> issues that create unpleasant user experience -- surfacing on music and
> ringtones apps. Would be good to track the issues with audio competing
> policy and fix the various cases. I believe Dominic and Marco are working
> hashing out the various cases and the expected behavior. 
> 
> For 1.4, Jacqueline is working on coming up with UX for better Ringtone
> Creation experience. As part of that, we could look into adding an
> acceptance criteria for this case where overlapping audio when previewing
> ringtones while music is playing in the background. 
> 
> As for this issue, if is not a regression from 1.2, would suggest addressing
> it in 1.4 as part of Ringtone work. NI Sri and djf for their input as well. 
> 
> Thanks
> Hema

I don't think I agree with this for the exact reasons already cited in comment 12. We have to get the basics right here on an initial partner facing release of this feature & it's top compliant on SUMO, so I think this needs to remain as a blocker. If that means we need to do something simple in 1.3 to resolve the audio contention here, then let's do that. But I don't think we can ship a feature in a partner facing release that breaks a basic flow with setting ringtones through the music app.
(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Hema Koka [:hema] from comment #20)
> > We are coming across many bugs around audio overlap and competing policy
> > issues that create unpleasant user experience -- surfacing on music and
> > ringtones apps. Would be good to track the issues with audio competing
> > policy and fix the various cases. I believe Dominic and Marco are working
> > hashing out the various cases and the expected behavior. 
> > 
> > For 1.4, Jacqueline is working on coming up with UX for better Ringtone
> > Creation experience. As part of that, we could look into adding an
> > acceptance criteria for this case where overlapping audio when previewing
> > ringtones while music is playing in the background. 
> > 
> > As for this issue, if is not a regression from 1.2, would suggest addressing
> > it in 1.4 as part of Ringtone work. NI Sri and djf for their input as well. 
> > 
> > Thanks
> > Hema
> 
> I don't think I agree with this for the exact reasons already cited in
> comment 12. We have to get the basics right here on an initial partner
> facing release of this feature & it's top compliant on SUMO, so I think this
> needs to remain as a blocker. If that means we need to do something simple
> in 1.3 to resolve the audio contention here, then let's do that. But I don't
> think we can ship a feature in a partner facing release that breaks a basic
> flow with setting ringtones through the music app.

Alright.

djf, could you please take this and put in a simple fix for 1.3?

dkuo has another music/audio channel related bug that he needs to wrap up this week before he is out for new years...

Thanks
Hema
Assignee: nobody → dflanagan
Flags: needinfo?(skasetti)
Flags: needinfo?(dflanagan)
Whiteboard: dogfood1.3, [AUDIO_COMPETING] → dogfood1.3, [AUDIO_COMPETING] [fxos:media]
Component: AudioChannel → Gaia::Music
This is a hard bug.

Jacqueline: you've said that music should only pause when the user is sharing as a ringtone, but not when sharing a song with bluetooth, MMS, etc.  But to the music app it is just a share activity, so we can't know what it is being shared with.  Unless you want to define new UX and add a new special purpose "share as ringtone" button we'll have to treat all sharing the same.

Dominic: you've asked whether we can just turn the audio preview off in the ringtone app that handles the share.  We can't do that because any app could offer to share a ringtone and that app itself may not have already provided a preview. There is a potential denial of service attack here if we don't preview each ringtone: an app could share a corrupt or empty music file and the user phone would stop ringing on incoming calls!

So we either need to get the audio competing policy working in this case or we've got to pause playback when a share activity starts.

In this case where we have two foreground processes that both want to play music I wonder if we could just break the tie in favor of the newer process.
Ah. This might be an easy fix: I'll modify the ringtone app so it uses audio type "notification" to preview the ringtone.  That should pause the "content" type playing in the music app...
That doesn't seem to work. Darn.
audio types notification, alarm, and ringer don't work to fix this bug. 

It seems to me that this is a bug in the AudioChannelService: it ought to be able to correctly pick the highest priority audio channel when there are multiple foreground processes.
We have a long term proposal to have system app be able to handle multiple foreground audio competing,
but for this bug, for v1.3, my only solution would be: stop music playing right away after music invokes a new MozActivity and recover the song when onsuccess or onerror is got.
Thanks Alive.  I was hoping that if the two foreground apps had audio at different priority levels it would be easier to fix the competition...

But assuming that can't be done, the next thing I'll try is to have the setringtone app use the music controls IAC protocol (the one that the lockscreen uses) to pause and resume the music.  Hopefully I can make that work.
Hmm.  I can use the code in system/js/media_playback.js to send a "pause" command to the music player when the set ringtone app starts.  But I don't think I can safely send a "play" command that way because I don't know whether the music was actually playing when I sent the pause command.  So resuming playback will have to be in the music app after the share command returns...  If it was playing and finds that it is paused when it returns then it can resume itself, I guess.
I don't think IAC works here.. IAC is implemented via system message, so if you are going to communicate something to music app, you would invoke it in background (by system app) if it's not running.

You won't know who is the caller in ringtone app so you might incorrectly launch music app.

The pattern for media playback is: music -> system, and since system is always alive, it would not be launched again.
Alive: thank you again for pointing this out.  I was misunderstanding how the music remote control scheme worked.  So I'll try abusing the settings API instead.  That's always fun!
This is an ugly workaround, but it does work around the bug.
Attachment #8367573 - Flags: review?(squibblyflabbetydoo)
Comment on attachment 8367573 [details] [review]
link to patch on github

The behavior is a little weird to me; I'd almost rather we just pause the music immediately, but I suppose that doesn't work for other kinds of sharing. r=me

I assume the goal for 1.4 is to fix this by making the audio competing policy handle inline activities properly?
Attachment #8367573 - Flags: review?(squibblyflabbetydoo) → review+
Laned on master: https://github.com/mozilla-b2g/gaia/commit/2c64a463f6bb053a05326ee1b6c5236eb31b898c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
Duplicate of this bug: 958470
See Also: → 1160092
You need to log in before you can comment on or make changes to this bug.