894744 - [Tarako][Fugu][Buri][MMS][MusicPlayer]The music play abnormal when view it in MMS

Status: RESOLVED FIXED

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

Description

[sync-1]

AU_LINUX_GECKO_ICS_STRAWBERRY.01.01.00.019.152
 Firefox os  v1.1
 Mozilla build ID:20130702230206
 
 Created an attachment (id=460111)
 PR log
 
 DEFECT DESCRIPTION:
 The music play abnormal when view it in MMS.
 
  REPRODUCING PROCEDURES:
 1.Go to MusicPlayer->Play a music "A"
 2.Go to Messages->Add attachments->Music->Select to play music "B"->There have two voices play,press stop key will have no voice->KO
 
  EXPECTED BEHAVIOUR:
 It should play normally.
 
  ASSOCIATE SPECIFICATION:
 
  TEST PLAN REFERENCE:
 
  TOOLS AND PLATFORMS USED:
 
  USER IMPACT:
 mid
  REPRODUCING RATE:
 5/5
  For FT PR, Please list reference mobile's behavior:

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Music -> Content channel
MMS audio attachment -> open activity -> Music -> Content channel
strange...

Dominic, could you confirm?

Comment 2

[Dominic Kuo [:dkuo]]

I just took a quick test and it's reproducible. In my understanding, the foreground content channel should have higher priority than the background one, so when we use the open activity of music app while another music app is playing in background, shouldn't it be the top priority to play audio? or the open activity should use a higher priority channel?

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

My goodness...I think that we ran into the issue of audio competing we never met before.
I guess gecko think "music app" and "music activity" are the same app so they two won't interrupt each other....OOPS.

So in this case I bet that background music app will also observe visibilitychange event when its another page(open.html) is opened via web activity, and they are in the same process.

:/....

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Several initial ideas:
(1) Make music app index.html communicates with open.html, and stop the music by itself.
(2) Refine audio competing identifies not only cross process but cross page.

Both are not easy now.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Confirm that background music 'index.html' still stays background without getting visibility-change event.
So the page itself knows its background but don't know its another page is opened.

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Could we change the policy based on frame instead of process?

Comment 7

[sync-1]

Any updates?

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

An ideal resolution to me may be let music activity(open.html) notifies app(index.html) to stop playing music. The problem would be how they communicate with each other because activity is opened by gecko instead of music app itself.
1) Use window.open trick.
2) Use localstorage onchange(weird)
3) Use shared worker(don't know if this is ready or not.)

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

The blocking state decides what we would do to this bug.
If this one is blocking, we would use localstorage onchange to communicate between the two opened frames in the same app. And wait for shared worker land to give us a correct fix.
If this one isn't blocking, let's wait shared worker, it's in v1.3 scope.

Comment 12

[Jason Smith [:jsmith]]

Not a blocker - we already shipped 1.1 with this bug.

Comment 13

[Tzu-Lin Huang [:dwi2][:tzhuang]]

steal it

Comment 14

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: proposed pull request for master

Hi Dominic, 
Please take a look when you are available, though this is still work in progress.
The symptom described in original STR is gone after applying this patch. However, there is still a problem in this PR and I am still working on it. 

Describe STR below:

1. Go to MusicPlayer -> Play a music "A"
2. Go to Messages -> Add attachments -> Music -> Select to play music "B" -> press HOME, let music "B" plays at background
3. Go back to MusicPlayer -> Play music "A" again -> Both "A" and "B" are playing

Comment 15

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Dominic,

I update the patch and also added some debug log.
The problem I encounter is when step 3: Play music "A" again, there is no storage event received. I am still investigate the reason.
But please provide some feedback because I might have something missed.

Thanks

Comment 16

[Evelyn Hung]

(In reply to Tzu-Lin Huang [:dwi2] from comment #14)
> Created attachment 8345648 [details] [review]
> (WIP) proposed pull request
> 
> Hi Dominic, 
> Please take a look when you are available, though this is still work in
> progress.
> The symptom described in original STR is gone after applying this patch.
> However, there is still a problem in this PR and I am still working on it. 
> 
> Describe STR below:
> 
> 1. Go to MusicPlayer -> Play a music "A"
> 2. Go to Messages -> Add attachments -> Music -> Select to play music "B" ->
> press HOME, let music "B" plays at background
Can we play B in the background as we are previewing it? As I know if you press HOME at this moment, you are cancalling activity so the music playing frame will be removed.

or do you mean selecting music B and attaching it, then playing it by click on the menu "Play"?

Comment 17

[Evelyn Hung]

(In reply to Evelyn Hung [:evelyn] from comment #16) 
> or do you mean selecting music B and attaching it, then playing it by click
> on the menu "Play"?

well, even in this case, the music won't playing in background either.

Comment 18

[Tzu-Lin Huang [:dwi2][:tzhuang]]

I just preview it and press home. Around 3/4 of the time, the music stop immediately. But there still 1/4 chance that the music selected in activity plays for at least 45 seconds.

Comment 19

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Same WIP but on v1.2f

https://github.com/dwi2/gaia/commit/d2018e4bbe2c2609580ee1628b14698d9e82a8f8

Comment 20

[Evelyn Hung]

I add more points to log, the following is my test log. I think the flow works as expected.

I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] register _handleInterpageMessage
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] call play!
I/GeckoDump( 2183): ==== set music-player-is-occupied-by = app://music.gaiamobile.org/index.html#mix

I/GeckoDump( 2183): ===== activity pick!!
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#pick] register _handleInterpageMessage
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#pick] call play!
I/GeckoDump( 2183): ==== set music-player-is-occupied-by = app://music.gaiamobile.org/index.html#pick
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] received evt storage
I/GeckoDump( 2183): ====> { app://music.gaiamobile.org/index.html#mix -> app://music.gaiamobile.org/index.html#pick }
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] call pause~
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#pick] call pause~
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#pick] remove music-player-is-occupied-by = app://music.gaiamobile.org/index.html#pick
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] received evt storage
I/GeckoDump( 2183): ====> { app://music.gaiamobile.org/index.html#pick -> null }
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#pick] call play!
I/GeckoDump( 2183): ==== set music-player-is-occupied-by = app://music.gaiamobile.org/index.html#pick
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] received evt storage
I/GeckoDump( 2183): ====> { null -> app://music.gaiamobile.org/index.html#pick }
I/GeckoDump( 2183): ==== [app://music.gaiamobile.org/index.html#mix] call pause~

Comment 21

[Dominic Kuo [:dkuo]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Tzu-Lin,

Thanks for working on this. I think because for the current audio competing policy we are unable to handle scenario like this, so the approach might be a workable solution for now(for fugu+), and the localStorage idea looks good to me.

One thing I am suggesting is, to have the same behaviour of the current audio competing policy, probably we can resume/play the song in the background(regular) music after the foreground(picker) music paused or closed, so that once the users bring the background music to foreground, the music should auto resume due to the audio competing policy, but this can be a follow up since this might need more effort to work on it, let's file a bug then fix it later on master.

Once you done the patch, feel free to request review to me and I will take a full review in detail, thanks.

Comment 22

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Attachment: pull request for v1.2f

Comment 23

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8346956 [details] [review]
pull request for v1.2f

Hi Dominic, 

This is the patch for fugu(v1.2f), please help to review it. Thanks

I am still working on patch for master.

Comment 24

[Dominic Kuo [:dkuo]]

Comment on attachment 8346956 [details] [review]
pull request for v1.2f

Tzu-Lin,

The patch for v1.2f looks good to me, there is one minor issue that I hope you to address before you land it. I found this approach also solved another scenario, that is, if the regular music and the music open activity are both launched, like when an user is listening to music and receives an audio file from bluetooth, or view an audio attachment from email. This is good and be sure the patch for master also works on this scenario, and feel free to ask if you have any problem on the new patch, thanks.

Comment 25

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Hi Dominic,

Please help to review the patch. Thanks

There are two difference from the patch in v1.2f:
1. Use a stack-like structure to keep who is playing music now (based on their url). However, we can only store string in localStorage. We need to utilize stringify and parse to transform the type of stored data in localStorage before and after using them.
2. Register unload event to remove data from localStorage before activity is about to close.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

would it be better to use asyncStorage instead of localStorage?

http://mxr.mozilla.org/gaia/source/shared/js/async_storage.js

that ensures no synchronous writes on the content process, preventing jank.

(also, check out this add-on for better integration of github pull-requests with bugzilla! https://addons.mozilla.org/en-US/firefox/addon/github-tweaks-for-bugzilla/ )

Comment 27

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Dietrich,

Thanks, it is a great idea. I didn't know localStorage was synchronous API before.
I'll try it out with asyncStorage.

Comment 28

[Tzu-Lin Huang [:dwi2][:tzhuang]]

On second thoughts, I think localStorage is much more fit in this situation since we need 'storage' event to notify background music app that there is a new music activity coming up.
Also, we won't have two music app/activity at foreground, it is very unlikely that racing condition would ever happen in this case.
I'll just stay with localStorage.

Thanks

Comment 29

[Dominic Kuo [:dkuo]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Tzu-Lin, thanks for working on this and sorry about the late review. I tested your patch and found basically it solved the issue, but there are some side effects:

1. After selected one of the preview songs, before entering the player page, the background music will be paused first then resume immediately. This is because the foreground music player paused at beginning to wait for the further action from users, it's intentional that we want to let user to start the preview on their own. So the calling sequence of the foreground music player is play > pause which caused the noticeable gap I mentioned above.

2. Once the logic of this patch is applied, if the user try to control the background music from the utility tray while he/she is previewing songs, the background pause command will trigger the foreground player to play, and users will feel the player just play automatically, this might not be a blocker but it's surely a lack of this logic.

3. Once the preview player paused, the background music will resume automatically, this is kind of annoying and probably confuses the users as well, similar to bug 843955 which we are trying to give the users better user experiences.

With these three reasons, I am cancelling the review and will try to also discuss this in the audio competing meetings in this week, after we got some conclusion, will go back to this and update.

Comment 30

[Dominic Kuo [:dkuo]]

[UX_TRIAGED]

Since the non-workaround fix takes too many effort and cannot completely solve this problem, we will leave the complete solution to later version, by using the gaia audio management. And if we really want , at least, give the user a better ux, then the patch for 1.2f is fine for 1.4, which just simple pause the background music when the foreground music is being play, without auto-resuming when it ends.

Comment 31

[Jason Smith [:jsmith]]

I think this is fixed. Can someone retest?

Comment 32

[Cody Roesch [:croesch]]

This bug still occurs on V1.3 Buri device. A user can still play a song through the Music player and while it's playing, go into MMS and open up the attachments and play the same song through the attachments. Both songs will be heard overlapping each other.

Environmental Variables:
Device: buri v1.3 moz
BuildID: 20140220004003
Gaia: a43904d9646109b48836d62f7aa51e499fbf4b2e
Gecko: 32fd5d798477
Version: 28.0
Firmware Version: v1.2-device.cfg

Comment 33

[Dominic Kuo [:dkuo]]

(In reply to Jason Smith [:jsmith] from comment #31)
> I think this is fixed. Can someone retest?

Jason, we haven't fix this bug for master yet, the patch for 1.2(attachment 8346956 [details] [review]) was manually picked up to the partner’s branch, the patch for master is not ready yet.

We have triaged this with ux team and they accepted the fix for 1.2 on 1.4, see comment 30, so if we want this fixed in 1.3, we can land 1.2 patch for 1.3 and master, and file a followup bug to trace.

Comment 34

[James Zhang (Spreadtrum)]

Hi Steven,

   Can you help merge this patch into v1.3t?

Comment 35

[Joe Cheng [:jcheng] (please needinfo)]

James, since your team is taking over the uplift ownership of 1.3T, do you think you can do the uplift? if you run into problems, we will be happy to assist. thanks

Comment 36

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ as this bug was a blocker for 1.2F as well

Comment 37

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #35)
> James, since your team is taking over the uplift ownership of 1.3T, do you
> think you can do the uplift? if you run into problems, we will be happy to
> assist. thanks

Ying will do uplift.

Comment 38

[ying.xu]

https://github.com/mozilla-b2g/gaia.git

Comment 39

[Dominic Kuo [:dkuo]]

v1.3t: 7a364840feb426d80d15c689ee209c10dc516226

Comment 40

[Joe Cheng [:jcheng] (please needinfo)]

will we fix this on trunk? Thanks

Comment 41

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Joe, 
The patches for v1.2f and v1.3t are just hotfix.
As Dominic stated in Comment 30, a proper solution for master branch will be using Gaia audio management (which is still in planning phase as far as I know).

Dominic is going to handle it.

Comment 42

[Jason Smith [:jsmith]]

(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #41)
> Hi Joe, 
> The patches for v1.2f and v1.3t are just hotfix.
> As Dominic stated in Comment 30, a proper solution for master branch will be
> using Gaia audio management (which is still in planning phase as far as I
> know).
> 
> Dominic is going to handle it.

That doesn't sound like a good strategy - we should be including the hotfix on trunk as well to avoid getting complaints in IOT for 1.4. We should open a followup to do the real fix with the long term solution elsewhere, but I don't want us not fixing trunk just because we want the long term solution up front.

Does that make sense?

Comment 43

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Jason, 

the bug number of long-term solution is bug 961967. However, I still need to ask Dominic's opinion before landing current fix on master. Because I use not-so-encouraged localStorage API in this fix.

Hi Dominic,
I think Gaia audio management is targeting on 1.5. Do you think we can land this fix on master before we have a real Gaia audio management?
Thanks

Comment 44

[Dominic Kuo [:dkuo]]

(In reply to Tzu-Lin Huang [:dwi2][:tzhuang] from comment #43)
> Hi Dominic,
> I think Gaia audio management is targeting on 1.5. Do you think we can land
> this fix on master before we have a real Gaia audio management?
> Thanks

Yes, as I commented in comment 30, ux wanted this on 1.4 and we should pass the IOT, let's also land the patch on 1.4, and file a followup bug to record this workaround, thanks.

Comment 45

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Hi Dominic,

I still need your review before landing this patch. Thanks
I filed a followup bug 985307 to remove this patch in the future once bug 961967 or long-term solution of audio competing in Gaia is landed

Comment 46

[Dominic Kuo [:dkuo]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Thanks Tzu-Lin, the patch looks good to me since it's the same as what we had landed for 1.2f. And if it looks good on master, please make uplift it to 1.3 since 1.3t is already fixed, also, uplift to 1.4 once this issue become 1.4+.

Comment 47

[Tzu-Lin Huang [:dwi2][:tzhuang]]

travis is green
https://travis-ci.org/mozilla-b2g/gaia/builds/21075349

landed on master 
https://github.com/mozilla-b2g/gaia/commit/ce8a5f8153d45000e39fc9e4b2b94521747015c3

Comment 48

[Tzu-Lin Huang [:dwi2][:tzhuang]]

uplift to v1.3
https://github.com/mozilla-b2g/gaia/commit/db1715fcaf132b22ebdf12873564b0cfe42699ac

Comment 49

[Jason Smith [:jsmith]]

Please backout on 1.3 - this is not approved to land on 1.3 & not a blocker for 1.3.

Also - we should ask for gaia approval on this for 1.4, as it's a good fix to take up to that branch.

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Reverted: bb4eb2658c7ebc62040d7c9d3a64f397bb8db369

Comment 51

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Hi Jason,

Thanks for reminding. I'll be careful with landing process next time.

Comment 52

[Tzu-Lin Huang [:dwi2][:tzhuang]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

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

[Approval Request Comment]
Besides comment 29, the risk is that we need to remove the patch once we have Gaia audio competing implementation in the future.
[Bug caused by] (feature/regressing bug #):
[User impact] if declined: as bug description stated
[Testing completed]: manually tested and worked on v1.3 and v1.4
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 53

[bhavana bajaj [:bajaj]]

Comment on attachment 8345648 [details] [review]
proposed pull request for master

Approving this on 1.4 given this will be a good fix.

A bug can only land on 1.3 if it is a 1.3+(blocker). So clearing the approval request here as this is not a blocker. I think this was nominated for 1.3? to make a blocking case but was pushed to backlog by triage.

For 1.3T this needs to be cherry-picked by partner if its desired to land on that branch. I am going to NI steven so this is taken care since its blocking 1.3T+

Comment 54

[Ryan VanderMeulen [:RyanVM]]

v1.4: b5ed43150111befe6b33cb50def3adad6f882c68

Leaving the 1.3T uplift to whoever's handling those these days.

Comment 55

[Steven Yang [:styang]]

Hi Ying, please take this to 1.3T

Comment 56

[ying.xu]

(In reply to Steven Yang [:styang] from comment #55)
> Hi Ying, please take this to 1.3T

already merged
https://github.com/mozilla-b2g/gaia/commit/7a364840feb426d80d15c689ee209c10dc516226

Comment 57

[James Zhang (Spreadtrum)]

(In reply to ying.xu from comment #56)
> (In reply to Steven Yang [:styang] from comment #55)
> > Hi Ying, please take this to 1.3T
> 
> already merged
> https://github.com/mozilla-b2g/gaia/commit/
> 7a364840feb426d80d15c689ee209c10dc516226

Thank you.

Comment 58

[Jayme Mercado [:JMercado] (Inactive 3/4/2016)]

Verified the fix for Tarako 1.3T.  The two sounds no longer overlap.

1.3T Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140501014002
Gaia: d26a776beae0070b0032248a2ce482bbe6321a6d
Gecko: e90f4b655511
Version: 28.1
Firmware Version: sp6821a-Gonk-4.0-4-29

Comment 59

[Andrew Overholt [:overholt]]

Clearing needinfo on Andrea.