1002897 - [Tarako] inproc music app

Status: VERIFIED FIXED

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

Description

[Marvin Khoo [:Marvin_Khoo]]

The intention here is to make music be able to playback continuously by not getting kill by LMK.

UAC:
- [P1]Once in background, Music app should be killed / self-terminated and free memory if there is no music being playback.
- [P1]Once in background, Music app UI can be killed and music be able to playback and function, remain Notification panel music control panel, and Lock screen control panel (prev, play/pause, next).
- [P2]if Music is being killed by LMK, state shall be kept. Once user re-launch music app, it should recall the state.
- [P2]Remove state if the song is deleted [1] or song doesn't exist [1] or SD card is being re-swapped. Music shall lead to Album UI [1, 2] or rescan media [2].

Comment 1

[Marvin Khoo [:Marvin_Khoo]]

preliminary evaluation per discussion, this may add 2~3MB ram to system as trade-off, need further confirmation to this.

Comment 2

[Marvin Khoo [:Marvin_Khoo]]

Please ignore P2 items. only P1 will be consider for 1.3T.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Marvin Khoo [:Marvin_Khoo] from comment #0)
> The intention here is to make music be able to playback continuously by not
> getting kill by LMK.
> 
> - Once in background, Music app should be killed / self-terminated and
> free memory if there is no music being playback.

This is the primary reason I think it's safe enough to put Music app inproc if the product spec is to keep the music playing no-matter-what.

> - Once in background, Music app UI can be killed and music be able to
> playback and function, remain Notification panel music control panel, and
> Lock screen control panel (prev, play/pause, next).

This is the tricky part we did not discuss yesterday. The lockscreen/notification widget currently shows when

A) music app is alive and playing music
B) music app is alive, in background, but not playing

and it will not show when

C) music app is closed/killed

By adopting the first rule we will get rid of state (B): music app always closes itself (to (C)) when it reaches that state. Is that desirable?

Comment 4

[Marvin Khoo [:Marvin_Khoo]]

cc Thinker
per discussed, let's continue evaluate this, meantime,
[1] I asked Mike Lien double verify music OOM/LMK case(s) if still encounter in today's build based on following workload

    500 contacts
    500 sms messages
    100 dialer history entries
    250 gallery images **
    50 songs
    10 videos

[2] adjust the music pri could be doable, however, since that will still preserved ~8MB by keeping Music app in background, i assume making music inproc could somehow lower the memory usage while playback.


[3] Widget, to be discussed. my preliminary thought is, we need to be able to identify pause and stop, if music widget is in pause stage, we don't kill music app. if there is way for user to stop / close, then we kill it. e.g: swipe left to move away music widget (could be new feature for music app).

Do not set this as blocker until we get full picture on [1]

Comment 5

[Marvin Khoo [:Marvin_Khoo]]

[4] Widget, additional way to close. user be able to launch Recent app and swipe to close music.

Comment 6

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

ni? dkuo on this approach

Comment 7

[Dominic Kuo [:dkuo]]

Putting the music app in system process could possibly be a suitable approach to fix all the OOM/LMK cases, and I agree to make sure music app will be killed when memory needed is the most important point we need to discuss here, and I think the timing of music self-terminated or system kill it could be:

1. Music app should be self-terminated/killed when it's not playing, so if the play status are |STOPPED|, |PAUSED| or |mozinterruptbegin|, we can try to close music app.
2. |PAUSED| and |mozinterruptbegin| probably not the states that uesrs expect music app will be killed, I don't know system app is able to receive the memory pressure signal, but if it could, then maybe |PAUSED| or |mozinterruptbegin| + memory pressure is a good condition to kill the music app.
3. or, start a timer after music app is |PAUSED| or |mozinterruptbegin|, maybe when it reaches a duration, like 5 mins, then we assume the user is doing some other things and won't expect music is still opened in the background.

This approach should be the last ditch if we couldn't address the OOM issue by adjusting the LMK score.

Comment 8

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

ni? Fabrice
do you think it makes sense to move music app in-process? 
during the triage, it was felt that this can be a good solution

Comment 9

[David Flanagan [:djf]]

On the face of it, moving the entire music app in process seems like the simplest solution. But it is also a heavyweight solution. And there is the issue of figuring out when the in-process app should exit on its own to save memory. From Dominic's description in comment 7, this does not seem like a straightforward thing.  It could be that this is the only possible solution given the time constraints.  But I don't think it is a good solution.

For example: what happens when during an incoming call. The music presumably pauses, and there is a spike in memory usage.  Would that cause the in-process music app to exit? If so, then music could not resume after the call.  And what if the user is using a bluetooth headset and pauses the music from the headset controls to make a call.  In that case she would likely not be able to resume her music after the call.

It seems to me that the ideal solution here is to move just a minimal playback module into the system app. The music app would no longer have its own <audio> element for playing music. When the user selects a song or album or playlist to play, the music app would send a list of files to the system app and the system app would play them and send out the appropriate signals so that the lockscreen, notfication panel and the music app (if it is still alive) know what is playing and can display the album art.

I'm not sure how hard this would be, but given that we already have lots of inter-app communcation going on between the music app and the system app maybe it would not be as hard as it seems.

I'm setting needinfo for Jim on this because I think he may know even more about the inter-app communication parts of the music app than Dominic does. Jim: do you think we can solve this? Getting a good music playback experience on Tarako is higher priority than your ringtones work for 2.0.

Comment 10

[Dave Hylands [:dhylands]]

(In reply to David Flanagan [:djf] from comment #9)
...snip...
> For example: what happens when during an incoming call. The music presumably
> pauses, and there is a spike in memory usage.  Would that cause the
> in-process music app to exit? If so, then music could not resume after the
> call.  And what if the user is using a bluetooth headset and pauses the
> music from the headset controls to make a call.  In that case she would
> likely not be able to resume her music after the call.

If it's in-process, then it will be part of the main b2g process. So if there is memory pressure, the LMK will pick one of the other apps to kill. If there are no other apps, it will likely pick the app being launched. From the kernel's perspective the Music app doesn't exist. The kernel just sees a bigger b2g process.

> It seems to me that the ideal solution here is to move just a minimal
> playback module into the system app. The music app would no longer have its
> own <audio> element for playing music. When the user selects a song or album
> or playlist to play, the music app would send a list of files to the system
> app and the system app would play them and send out the appropriate signals
> so that the lockscreen, notfication panel and the music app (if it is still
> alive) know what is playing and can display the album art.

That's what I proposed in another bug. We should probably consider doing something similar for FM radio as well.

> I'm not sure how hard this would be, but given that we already have lots of
> inter-app communcation going on between the music app and the system app
> maybe it would not be as hard as it seems.

Comment 11

[[:fabrice] Fabrice Desré]

We dispatch memory-pressure events to the system app, so it has the opportunity to close in-parent apps (we do that for the keyboard iframe for instance).

Still, I also agree with David and Dave that the best solution here is to only keep the <audio> element in the parent. Moving the whole UI makes me really nervous. We already moved the callscreen, and I don't want to end up with all the gaia apps running in the parent. So if we have someone that can work on that, please start asap. We'll send you food.

I think we keep the FM radio playing even if the app is killed now - so we're good on this one (the fm audio does not go through an <audio> element).

Comment 12

[Jim Porter (:squib)]

This all seems pretty terrible to be honest. I don't doubt that this proposal will work, but the gaia music app surely isn't the only thing that wants to play audio (as a simple example, consider the Bandcamp website). I've argued before that apps should have the ability to observe memory-pressure events so they can adjust their memory usage based on system-wide availability. Then the music app could clean up as much as possible to free memory, making it less likely to be killed.

Putting the music app's <audio> element in the system app only solves this problem in the most limited sense of the term. It doesn't solve:

1) Other apps that want to play audio uninterrupted
2) Killing the audio playback when we *really* need the memory for something more important; (I'd rather my music pause than me be unable to run an app I explicitly tried to open)
3) Moving to the next audio track (unless we want to put all the playlist logic in the system app too)

To be perfectly frank, I think the music app being killed when there's not enough system memory is a *good* thing. The bad part is that the music app can't do much to mitigate this because we don't know when we're running low on memory. (To be fair, we could try to reduce memory across the board, but I'm of the opinion that unused RAM is a waste, and we shouldn't hesitate to use more memory if it makes the user experience better.)

Comment 13

[Jim Porter (:squib)]

(In reply to David Flanagan [:djf] from comment #9)
> It seems to me that the ideal solution here is to move just a minimal
> playback module into the system app. The music app would no longer have its
> own <audio> element for playing music. When the user selects a song or album
> or playlist to play, the music app would send a list of files to the system
> app and the system app would play them and send out the appropriate signals
> so that the lockscreen, notfication panel and the music app (if it is still
> alive) know what is playing and can display the album art.

This also implies sending over all the metadata for the playlist, which could potentially be quite large if we're sending album art over as blobs. Changing this to send album art as file paths is *probably* doable, but is a significant amount of effort, since album art doesn't *have* an associated file; all the album art comes from the ID3 tags (or Vorbis comments, etc). My work-in-progress branch to fix our broken ID3 parsing changes this around some, but it's not ready to land and is probably too big a change to go onto 1.3T anyway.

This also doesn't address the "shuffle all" playlist, which is an infinite playlist. Likewise, we'd have to teach the system app to repeat playlists as needed, and maybe how to shuffle (unless we want to treat a shuffled playlist as a totally-new playlist for the system app, but that'll probably regress performance of toggling shuffle).

> I'm not sure how hard this would be, but given that we already have lots of
> inter-app communcation going on between the music app and the system app
> maybe it would not be as hard as it seems.

Fairly hard? There's a lot of logic that would need to move over.

Comment 14

[Thinker Li [:sinker]]

(In reply to Jim Porter (:squib) from comment #12)
> This all seems pretty terrible to be honest. I don't doubt that this
> proposal will work, but the gaia music app surely isn't the only thing that
> wants to play audio (as a simple example, consider the Bandcamp website).
> I've argued before that apps should have the ability to observe
> memory-pressure events so they can adjust their memory usage based on
> system-wide availability. Then the music app could clean up as much as
> possible to free memory, making it less likely to be killed.


> 
> Putting the music app's <audio> element in the system app only solves this
> problem in the most limited sense of the term. It doesn't solve:
> 
> 1) Other apps that want to play audio uninterrupted
> 2) Killing the audio playback when we *really* need the memory for something
> more important; (I'd rather my music pause than me be unable to run an app I
> explicitly tried to open)
> 3) Moving to the next audio track (unless we want to put all the playlist
> logic in the system app too)

I believe we already have the code of item 3.  For now, the music player would remember what is playing and playlist, and resume the play once it goes back from being killed.  So, all we need to do is to bring music player back once the play of current music is over.

Item 2 is most easy part of this bug, just change the priority of music player as a normal app.

This is for tarako, so item 1 is not an important case.  But, for long term, we need a new API to play music in background, at gecko/not gaia of b2g process, and bring back the player, via system message maybe.  Or, more better, use service worker api.

Comment 15

[Dominic Kuo [:dkuo]]

I had some comments in bug 997123 comment 5 which also described what we will encounter if we split the music app into two parts(puts the <audio> element into the system), besides Jim mentioned in comment 12 and comment 13.

1. It takes time to sync the state between music app and system mini player, because the operations include: Basic app launch + MediaDB initialization + Render ui.
2. The bluetooth remote controls(AVRCP) are handled in music app, these logic must move to the system mini player, or once music app is crashed, AVRCP will not function, and this means the system app needs to handle the extra AVRCP system messages, which I am not sure if it's a right design for the whole gaia architecture.
3. The current audio competing behaviours will be broken because the audio element doesn't live in the music app any more, I am sure we need to write some "extra" logic for audio competing.
4. The system mini player will be a super power for gaia music app, the 3rd party media players will still be killed due to low memory, unless we standardize it or have new api to support usages like this.(This is the same as Jim's item 1)

Summarize the above comments, if we really want to split the music app, besides the <audio> element, I feel the system mini player needs more basic capabilities, like repeat, shuffle and can use playlists as sources, and maybe it's a good chance to standardize new web api, to provide not just gaia music app but also 3rd party apps to be able to play in the background by using the mini audio player/service, which is capable to handle the remote controls and obey the audio competing policy.

Comment 17

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

triage: this is required for tarako release. 1.3T+

Comment 18

[Hema Koka [:hema]]

For 1.3T, one the ideas that being thrown out is to tear down the UI when the music app goes into background (we could leverage the GAIA_MEMORY_PROFILE low to trigger it). Dominic already has a wip patch for this. So he and Jim can work together to test out this approach for memory savings for Tarako...this seems to be the best effort approach for 1.3T

Long term, we could look into seeing if separation of audio player service from the music app makes sense and is the best approach. This requires a lot of thought, time to implement and also have to deal with various issues highlighted by dkuo, jim and others already in this bug...

Dominic, I am assigning this bug to you since you have already been working on it. 

thanks
Hema

Comment 19

[Dominic Kuo [:dkuo]]

(In reply to Hema Koka [:hema] from comment #18)
> For 1.3T, one the ideas that being thrown out is to tear down the UI when
> the music app goes into background (we could leverage the
> GAIA_MEMORY_PROFILE low to trigger it). Dominic already has a wip patch for
> this. So he and Jim can work together to test out this approach for memory
> savings for Tarako...this seems to be the best effort approach for 1.3T

Yes, I agree, and I am going to continue the wip(in bug 997123 comment 5) then attach it here, so that maybe QA or partners can test it on tarako to see if it's enough for 1.3T. Also, to have a separated patch to enable the music app in process and we can apply it individually, or combine it with my wip to see the real memory consumption on tarako, then decide which path(or both) to use.

So the action items will be:
1. Patch for tearing down/re-rendering the ui in music app when visibility changes(WIP).
2. Patch for enabling music app in process.
3. Post the results here.

> Long term, we could look into seeing if separation of audio player service
> from the music app makes sense and is the best approach. This requires a lot
> of thought, time to implement and also have to deal with various issues
> highlighted by dkuo, jim and others already in this bug...

This will need many discussions among the media team members and the other folks, also experiments before implementing, so meanwhile, I will try to have a rough proposal first, to be the baseline where we start the discussions. After I have that, I will file a bug and let's trace in it.

Comment 20

[Dominic Kuo [:dkuo]]

Attachment: 1. Patch for tearing down/re-rendering the ui in music app when visibility changes

Comment 21

[Dominic Kuo [:dkuo]]

I failed to create a memory report with |get_about_memory.py| under B2G/tools on tarako, so I used |b2g-info| to observe the memory usage before/after applied patch 1(attachment 8420049 [details] [review]), please see the below results when music app is playing with minimized/hidden window.

*(BEFORE):
---------------------------------------------------------------------------
                          |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g  578    1   89.7    0 22.4 24.8 30.8 136.4       0 root    
         (Nuwa)  615  578    1.7    0  0.1  0.9  3.6  49.4       0 root    
          Music 1245  615   47.6    7 10.4 13.3 20.2  72.0       6 app_1245
     Homescreen 1326  615    6.4    1  8.9 11.5 18.1  66.0       2 app_1326
(Preallocated a 1512  615    1.6   18  4.8  6.4 11.6  56.4       1 root    

System memory info:

            Total 98.0 MB
     Used - cache 74.7 MB
  B2G procs (PSS) 56.9 MB
    Non-B2G procs 17.7 MB
     Free + cache 23.4 MB
             Free  2.3 MB
            Cache 21.1 MB
---------------------------------------------------------------------------

*(AFTER):
---------------------------------------------------------------------------
                          |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER    
            b2g  578    1  131.7    0 25.5 27.9 33.9 136.1       0 root    
         (Nuwa)  615  578    2.0    0  0.0  0.8  3.3  49.4       0 root    
          Music 1926  615   46.6    7  9.8 12.6 19.3  72.1       6 app_1926
     Homescreen 2488  615    5.6    1  8.3 10.7 17.1  65.0       2 app_2488
(Preallocated a 2611  615    1.4   18  0.4  1.9  6.7  56.4       1 root    

System memory info:

            Total 98.0 MB
     Used - cache 69.8 MB
  B2G procs (PSS) 53.8 MB
    Non-B2G procs 16.0 MB
     Free + cache 28.2 MB
             Free  5.6 MB
            Cache 22.6 MB
---------------------------------------------------------------------------

I tried to find the differences between BEFORE and AFTER, and because the information is limited, I can only saw when every time the music became invisible, with patch 1, the AFTER used less memory in music process, and the Free memory seems more than BEFORE, but still I cannot tell people the exactly memory we can gain.

Patrick, can you help us to generate the memory report on tarako with patch 1? or someone from the performance team can help this? thanks. I am working on patch 2(music in system process) and it would be great if we can get the memory analysis while I am implementing.

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

B2G folder of tarako was recently updated to support MOZ_IGNORE_NUWA_PROCESS. You will need to update B2G git, and get memory report with |MOZ_IGNORE_NUWA_PROCESS=1 tools/get_about_memory.py|.

Comment 23

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: about-memory-after.tar.gz

Comment 24

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: about-memory-before.tar.gz

Comment 25

[Dominic Kuo [:dkuo]]

Attachment: 2. Patch for enabling music app in b2g process(with life cycle management).

Comment 26

[Dominic Kuo [:dkuo]]

Comment on attachment 8420049 [details] [review]
1. Patch for tearing down/re-rendering the ui in music app when visibility changes

Jim, would you please review this patch? thanks. The main idea is to swap the views elements' innerHTML to the temp strings when it's invisible, and swap back when it's visible, so that we don't have to write extra logic to maintain the scrolled position in every view if we use the view's clean/update methods to re-render the ui.

Comment 27

[Dominic Kuo [:dkuo]]

Comment on attachment 8420911 [details] [review]
2. Patch for enabling music app in b2g process(with life cycle management).

Alive, this patch put music in system process with life cycle management, the code block is also in the media_app_agent.js which we recently added for 1.3t branch, would you please review it? thanks.

Comment 28

[Thinker Li [:sinker]]

I am very afraid of which app would be the next?  Should don't we consider to make music app never die, or be stronger than foreground?  For now, as I know, even b2g itself would be killed for some reason.  I am not convinced by this solution.

Comment 29

[Jim Porter (:squib)]

I'd argue that the music app *should* die if we really, really need the memory for something the user is actively doing. Personally, I'd certainly rather have my music cut out than to be unable to open a particular app just because I don't have sufficient memory.

In the future, I hope that apps will be able to listen to memory pressure events so that *all* apps can take appropriate steps to clean up after themselves when the system is running low on memory. That would let us use more memory when it's available so that we can improve user experience while still being lean when we need to.

Of course, we'd still want to keep memory usage fairly low so that we don't drain the battery, but I'm not sure how much battery power that uses compared to other parts of the device.

Comment 30

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

(In reply to Thinker Li [:sinker] from comment #28)
> I am very afraid of which app would be the next?  Should don't we consider
> to make music app never die, or be stronger than foreground?  For now, as I
> know, even b2g itself would be killed for some reason.  I am not convinced
> by this solution.

From my point of view all of this - process priority manager tuning, in proc preloaded callscreen, e.t.c. - comes from project requirement. If 'we can always play music' is not a requirement, I won't support any solution; if that's a 'strong' requirement, any solution could be and any solution looks to me are all workaround.

How do you make music app never die without moving it in proc? Do you want to invent mozapptype=media and let ProcessPriorityManager to do the dirty part? Isn't this way another pollution?

IMO PPM is already too complex to add new rules. New rules means new exceptions and new regressions.

I don't think we have a correct and clean way to meet all requirements simply.

Comment 31

[Dominic Kuo [:dkuo]]

(In reply to Thinker Li [:sinker] from comment #28)
> I am very afraid of which app would be the next?  Should don't we consider
> to make music app never die, or be stronger than foreground?  For now, as I
> know, even b2g itself would be killed for some reason.  I am not convinced
> by this solution.

I think this probably depends on what music app we want to deliver to our end user. My own experience on using the music player on the mobile devices is, the background playing music never die though I didn't use it unreasonably. I believe the music app still gets killed if memory is really needed, like open 100 apps or use some browser app to view Facebook then scroll, scroll and scroll...but could we kill the foreground/background apps that consume too many memory? it doesn't make sense that the LMK kills the playing music to gain memory when the foreground app needs more, it also make no sense to kill the foreground app either, so maybe to avoid music being killed so easily, a special strategy is needed, which we are going to do the separation of the music ui and player(comment 19) for 2.0.

Comment 32

[Dominic Kuo [:dkuo]]

For tarako we have these two patches due to the time constrain, and I think we should put it on the latest build with real device to see if Patch 1 is enough for 1.3t, or we really need Patch 2 that put music in system process which makes it almost unkillable with exceptions.

1. (attachment 8420049 [details] [review]) Patch for tearing down/re-rendering the ui in music app when visibility changes.
2. (attachment 8420911 [details] [review]) Patch for enabling music app in b2g process(with life cycle management).

Peipei, can you please help to pull these two patches and test on tarako, to see if Patch 1 is enough for the test cases on 1.3t to make music less killed by LMK? or Patch 2 is needed to prevent LMK kills music? thanks!
(Maybe test Patch 1 + Patch 2 is also a good option because people always worry the inproc music will occupy extra memory than oop music is closed, but that depends on if you have time or not...)

Comment 33

[Peipei Cheng (needinfo if you need my action)]

(In reply to Dominic Kuo [:dkuo] from comment #32)
> For tarako we have these two patches due to the time constrain, and I think
> we should put it on the latest build with real device to see if Patch 1 is
> enough for 1.3t, or we really need Patch 2 that put music in system process
> which makes it almost unkillable with exceptions.
> 
> 1. (attachment 8420049 [details] [review]) Patch for tearing down/re-rendering the ui
> in music app when visibility changes.
> 2. (attachment 8420911 [details] [review]) Patch for enabling music app in b2g
> process(with life cycle management).
> 
> Peipei, can you please help to pull these two patches and test on tarako, to
> see if Patch 1 is enough for the test cases on 1.3t to make music less
> killed by LMK? or Patch 2 is needed to prevent LMK kills music? thanks!
> (Maybe test Patch 1 + Patch 2 is also a good option because people always
> worry the inproc music will occupy extra memory than oop music is closed,
> but that depends on if you have time or not...)

ok. I will test and update the results later.

Comment 34

[Peipei Cheng (needinfo if you need my action)]

I had some basic tests on these two patches.

Patch 1: 
I didn't see much difference with patch and without patch. It's very easy to reproduce LMK kill with this patch following steps bellow.
Play music at background --> Launch SMS --> attach pic from Gallery

Patch 2: 
Music play was never killed during my test. But there are many side effects, such as App cannot be launched due to OOM especially in some group apps (MMS -> Gallery/Camera).

Patch 1 + 2: did not test yet.

Comment 35

[James Zhang (Spreadtrum)]

(In reply to pcheng from comment #34)
> I had some basic tests on these two patches.
> 
> Patch 1: 
> I didn't see much difference with patch and without patch. It's very easy to
> reproduce LMK kill with this patch following steps bellow.
> Play music at background --> Launch SMS --> attach pic from Gallery
> 
> Patch 2: 
> Music play was never killed during my test. But there are many side effects,
> such as App cannot be launched due to OOM especially in some group apps (MMS
> -> Gallery/Camera).
> 
> Patch 1 + 2: did not test yet.

I suggest that don't add music to black list. It has side effect as peipei said.
You can let status bar control music playing and pausing after music app has been killed.

Comment 36

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to pcheng from comment #34)
> I had some basic tests on these two patches.
> 
> Patch 1: 
> I didn't see much difference with patch and without patch. It's very easy to
> reproduce LMK kill with this patch following steps bellow.
> Play music at background --> Launch SMS --> attach pic from Gallery
> 
> Patch 2: 
> Music play was never killed during my test. But there are many side effects,
> such as App cannot be launched due to OOM especially in some group apps (MMS
> -> Gallery/Camera).

The process is killed by gecko as it breaks the permission rule. We've got this message before it was killed

I/Gecko   ( 1111): NeckoParent::AllocPRemoteOpenFile: FATAL error: app without webapps-manage permission is requesting file '/data/local/webapps/gallery.gaia
mobi
le.org/application.zip' but is only allowed to open its own application.zip at /data/local/webapps/sms.gaiamobile.org/application.zip: KILLING CHILD PROCESS

I can reproduce locally with or without the patch in this bug, although it is not 100% reproducible. It could be another problem for launching an app by activity.

> Patch 1 + 2: did not test yet.

Comment 37

[Dominic Kuo [:dkuo]]

(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #36)
> (In reply to pcheng from comment #34)
> > Patch 2: 
> > Music play was never killed during my test. But there are many side effects,
> > such as App cannot be launched due to OOM especially in some group apps (MMS
> > -> Gallery/Camera).
> 
> The process is killed by gecko as it breaks the permission rule. We've got
> this message before it was killed
> 
> I/Gecko   ( 1111): NeckoParent::AllocPRemoteOpenFile: FATAL error: app
> without webapps-manage permission is requesting file
> '/data/local/webapps/gallery.gaia
> mobi
> le.org/application.zip' but is only allowed to open its own application.zip
> at /data/local/webapps/sms.gaiamobile.org/application.zip: KILLING CHILD
> PROCESS
> 
> I can reproduce locally with or without the patch in this bug, although it
> is not 100% reproducible. It could be another problem for launching an app
> by activity.

Thanks Patrick, I can also reproduce what Peipei had for Patch 2(with and without) in comment 34, so it should another issue and we should fix it while this bug is still under reviewing and testing.

Comment 38

[Thinker Li [:sinker]]

Fabrice, do you think it is a side-effect of process grouping?

Comment 39

[Thinker Li [:sinker]]

http://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#524

The order of PBrowserParents returned by ManagedPBrowserParent are in random order, sorted by pointers.  It causes undetermined results.  That is why it is not always happened.

Comment 40

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

The symptom I observed is that when I launch gallery successfully, the gallery is in another process, and there are 2 different ContentParents for 2 apps, so it pass the security check. For the failure case, the 2 apps are in the same process.

Comment 41

[Thinker Li [:sinker]]

I think this should be a loop to iterate over all PBrowserParents. http://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#608

Comment 42

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Patrick, Thinker, let's file a dependent bug and fix the platform issue?

Dominic, beside platform issue, have we validated inproc indeed work in Tarako?

Comment 43

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #42)
> Patrick, Thinker, let's file a dependent bug and fix the platform issue?
> 
> Dominic, beside platform issue, have we validated inproc indeed work in
> Tarako?

Please make sure music acquire audio tag on demand. If it doesn't release the audio channel and gecko thinks it's now always foreground (because it's in system app) I wonder something might break. (volume control, competing...)

Comment 44

[[:fabrice] Fabrice Desré]

(In reply to Thinker Li [:sinker] from comment #41)
> I think this should be a loop to iterate over all PBrowserParents.
> http://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#608

That, or we need to remember which one of the PBrowserParents at http://dxr.mozilla.org/mozilla-central/source/netwerk/ipc/NeckoParent.cpp#522 has set the hasManage flag.

Comment 45

[Dominic Kuo [:dkuo]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #42)
> Dominic, beside platform issue, have we validated inproc indeed work in
> Tarako?

I have tested several cases and it looks fine on tarako, like:

- Play MUSIC in background > Launch SMS > Attach picture from CAMERA > Back to SMS.
(this can be done when the platform issue(comment 36) didn't happen)
- Play MUSIC in background > Answer incoming call(DIALER) > MUSIC interrupted > End the call.
--------------------------------------------------------------------------------
Both two cases music app kept playing in the background and the ui/b2g seems still responsive.

- MUSIC paused in background > Launch BROWSER and load huge website.
--------------------------------------------------------------------------------
MUSIC will be closed by system due to the memory pressure, and proved the life cycle management does work.

I am making the memory reports to prove Patch 2 from the technical perspective, will attach later.

Comment 46

[Dominic Kuo [:dkuo]]

Attachment: INPROC_MUSIC_MEMORY_REPORTS(Part1).zip

Comment 47

[Dominic Kuo [:dkuo]]

Attachment: INPROC_MUSIC_MEMORY_REPORTS(Part2).zip

Comment 48

[Dominic Kuo [:dkuo]]

I have made the memory reports base on four different timings, and the sizes of b2g process are:

1. Before music launched: 31.96 MB
2. After music launched(STOPPED): 36.09 MB
3. After music launched(PLAYING, Foreground): 38.57 MB
4. After music launched(PAUSED, Background): 38.39 MB

From the results we can see the INPROC music used about 4~5MB when it's idle, when it plays, the memory increased about 2~3MB, I think that's because the <audio> element started to decode and play audio files, so the total INPROC music will cost about 7MB when it's playing.

And if we compare to the OOP music(idle: 14.20 MB, playing: 16.96 MB), the INPROC music saves approximatively 10MB, I believe this is why I can successfully passed the test cases in comment 45, which we usually failed when music is OOP.

These memory reports also gave us a good hint that, if the future plan is to split the music ui and player, then it should cost at least 2~3MB in b2g process because that's the difference between music is idle and playing, both INPROC and OOP music can show it.

Comment 49

[David Flanagan [:djf]]

If it helps, I have a work in progress patch that is the beginning of how we might move the player to the system app in this github branch: https://github.com/davidflanagan/gaia/tree/system-music-player

This creates a PlaylistService API in apps/system/js/music/  And then makes that API available to other apps over IAC with shared/js/media/playlist_service_shim.js

The code is completely untested and I haven't looked yet to see what it would take to modify the Music app to use the new PlaylistService.

Comment 50

[Dominic Kuo [:dkuo]]

(In reply to David Flanagan [:djf] from comment #49)
> If it helps, I have a work in progress patch that is the beginning of how we
> might move the player to the system app in this github branch:
> https://github.com/davidflanagan/gaia/tree/system-music-player
> 
> This creates a PlaylistService API in apps/system/js/music/  And then makes
> that API available to other apps over IAC with
> shared/js/media/playlist_service_shim.js
> 
> The code is completely untested and I haven't looked yet to see what it
> would take to modify the Music app to use the new PlaylistService.

David,

Thanks for the wip, I am surprised you have this wip so quickly. I took a look and found there are some issues that you probably didn't consider of, which are:

1. The audio competing issue. This is what Alive worried about in comment 43, if we put the <audio> element in system app directly, then we have to write some extra logic for the audio competing, or I am pretty sure it will cause noticeable regressions on the audio competing. And this is likely cannot be addressed because the system app is always visible, the content channel that music used won't be interrupted according to the audio channels api.

2. The remote controls. Like IAC or bluetooth AVRCP, we already have a component called "Media Remote Controls" in shared/js/media/remote_controls.js to convert all the commands into the same remote type commands, so I think we should keep it instead of having a new bluetooth service client to handle the AVRCP commands.

3. The playback logic. Once the music ui app is killed by LMK, if the system PlaylistService doesn't know the repeat or repeat-all is on, then after the playing playlist is ended, the player will stop. More, if the user choose shuffle-all songs, the PlaylistService is unable to play the songs that are not retrieved without the Music UI's help. So the PlaylistService needs to know how the player repeat and shuffle.

I have just finished the proposal for splitting the Music app into UI and Player for 2.0 in bug 1012613, let's move this wip and have the discussion there, to keep this bug clean for 1.3t.

Comment 51

[Dominic Kuo [:dkuo]]

And if it's possible, we should land bug 1010434 first or it's very inconvenient for QA and partner to verify Patch 2(attachment 8420911 [details] [review]) then land it.

Comment 52

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Setting dependency.

Comment 53

[Jim Porter (:squib)]

Comment on attachment 8420049 [details] [review]
1. Patch for tearing down/re-rendering the ui in music app when visibility changes

Clearing out review since it looks like we're going the other way. Feel free to re-add me to review for anything you need, though!

Comment 54

[jingmei.zhang]

Dear Dominic:
This also is a question in 1.4,as bug 1014422.
Could the patch be suitable in FFOS1.4?

Comment 55

[Dominic Kuo [:dkuo]]

(In reply to jingmei.zhang from comment #54)
> Dear Dominic:
> This also is a question in 1.4,as bug 1014422.
> Could the patch be suitable in FFOS1.4?

Hi Jingmei, I think bug 1014422 is another issue on bluetooth so 1.4 does not need this patch, this patch should be only for the devices with less memory, like tarako with 128MB ram.

Comment 56

[thomas tsai]

Hi Dominic: https://github.com/mozilla-b2g/gaia/pull/19107
it can't be applied to v1.3t now. Is it still useful?

Comment 57

[Dominic Kuo [:dkuo]]

(In reply to thomas tsai from comment #56)
> Hi Dominic: https://github.com/mozilla-b2g/gaia/pull/19107
> it can't be applied to v1.3t now. Is it still useful?

No, as I tested myself and Peipei confirmed in comment 34, Patch 1(https://github.com/mozilla-b2g/gaia/pull/19107) doesn't save enough memory to keep the music alive in the background, so we don't need Patch 1.

Comment 58

[yang.zhao]

(In reply to Dominic Kuo [:dkuo] from comment #57)
> (In reply to thomas tsai from comment #56)
> > Hi Dominic: https://github.com/mozilla-b2g/gaia/pull/19107
> > it can't be applied to v1.3t now. Is it still useful?
> 
> No, as I tested myself and Peipei confirmed in comment 34, Patch
> 1(https://github.com/mozilla-b2g/gaia/pull/19107) doesn't save enough memory
> to keep the music alive in the background, so we don't need Patch 1
Hi,Dominic
   We need to release a version for testing today,and I landed the following two patches on our build version v1.3t already:
1. Patch for tearing down/re-rendering the ui in music app when visibility changes 
2. Patch for enabling music app in b2g process(with life cycle management). 

I'd like to know if I could land both two of them ?If not,please tell me which patch should I land ?Thank you very much!

Comment 59

[Dominic Kuo [:dkuo]]

Please DO NOT land Patch 1, just land Patch 2(https://github.com/mozilla-b2g/gaia/pull/19160) the one with Alive's r+, thanks!

Comment 60

[yang.zhao]

(In reply to Dominic Kuo [:dkuo] from comment #59)
> Please DO NOT land Patch 1, just land Patch
> 2(https://github.com/mozilla-b2g/gaia/pull/19160) the one with Alive's r+,
> thanks!

Ok,thank you!I will just landu patch 2.And together with bug 1010434 patch.

Comment 61

[yang.zhao]

(In reply to Dominic Kuo [:dkuo] from comment #59)
> Please DO NOT land Patch 1, just land Patch
> 2(https://github.com/mozilla-b2g/gaia/pull/19160) the one with Alive's r+,
> thanks!
Hi,I'd like to know does it have any risks if I land Patch 1?Because our tester already begin test with the version which both of the patch landed.
Thank you again!

Comment 62

[Dominic Kuo [:dkuo]]

(In reply to yang.zhao from comment #61)
> Hi,I'd like to know does it have any risks if I land Patch 1?Because our
> tester already begin test with the version which both of the patch landed.
> Thank you again!

Patch 1 tears down the ui when Music is in the background, and re-renders the ui when it goes to foreground, the user will notice the ui is recovering then be able to use again, that's probably something with bad ux cause the user cannot see the ui immediately when Music is in foreground, also the reviewer has mentioned some issues in https://github.com/mozilla-b2g/gaia/pull/19107#discussion_r12651492, which I haven't address yet, so it's better not to land Patch 1 since there are known issues that might have risks, thanks.

Comment 63

[Dominic Kuo [:dkuo]]

1.3t: 41a763154fbac34bef6baf17a201e50f52f2b72a

Comment 64

[James Zhang (Spreadtrum)]

Please help to revert bug 987022 change, it makes tarako's behavior different from 1.3 and 1.4 and cause bug.

Comment 65

[Dominic Kuo [:dkuo]]

Bug 987022 has introduced the media app agent to manage the music app's life cycle, and this bug also depends on that component, so we cannot just revert it entirely, we should have another patch to disable that part, if it's fine then I will do it to make everyone happy.

Comment 66

[James Zhang (Spreadtrum)]

(In reply to Dominic Kuo [:dkuo] from comment #65)
> Bug 987022 has introduced the media app agent to manage the music app's life
> cycle, and this bug also depends on that component, so we cannot just revert
> it entirely, we should have another patch to disable that part, if it's fine
> then I will do it to make everyone happy.

OK, thank you.

Comment 67

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

There were some small side bugs that came from this; having said that, this bug has been verified by multiple people.

Gaia      33025a8b1e458ef48cb8b7fb81bdaba28c86b183
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/161d4b0448c6
BuildID   20140606014002
Version   28.1
ro.build.version.incremental=eng.cltbld.20140606.050809
ro.build.date=Fri Jun  6 05:08:16 EDT 2014
Tarako

Comment 68

[Dominic Kuo [:dkuo]]

(In reply to James Zhang from comment #66)
> (In reply to Dominic Kuo [:dkuo] from comment #65)
> > Bug 987022 has introduced the media app agent to manage the music app's life
> > cycle, and this bug also depends on that component, so we cannot just revert
> > it entirely, we should have another patch to disable that part, if it's fine
> > then I will do it to make everyone happy.
> 
> OK, thank you.

Thanks, will do in bug 1022466.

Comment 69

[jingmei.zhang]

hi Dominic Kuo :

   I doubt that the patches you provided have caused a new bug in 1.3t as :

   Enter the audio player, the phone can not load AMR-NB, AMR-WB audio files.

   the sprd bug is in http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=319601.

  I have made a test in my side and it proves that the above bug did not exit while the patch is not added.

  would you help to find the reason for the bug?

Comment 70

[Dominic Kuo [:dkuo]]

(In reply to jingmei.zhang from comment #69)
> hi Dominic Kuo :
> 
>    I doubt that the patches you provided have caused a new bug in 1.3t as :
> 
>    Enter the audio player, the phone can not load AMR-NB, AMR-WB audio files.
> 
>    the sprd bug is in
> http://bugzilla.spreadtrum.com/bugzilla/show_bug.cgi?id=319601.
> 
>   I have made a test in my side and it proves that the above bug did not
> exit while the patch is not added.
> 
>   would you help to find the reason for the bug?

Actually my patch that put music inproc did not make any change on Music app itself but System app, the issue you mentioned should be another problem, and it sounds like Bug 990957 because it's related to the AMR audio files, hope it helps, thanks.

Comment 71

[jingmei.zhang]

Attachment: 14454_16k_13kbps.amr

hi Dominic Kuo :

   I found that the codes you added in gaia/apps/system/js/window_manager.js:

>+   var musicManifestUrl =
>+         protocol + 'music.' + domain + '/manifest.webapp';
>              ……
>+         musicManifestUrl

  The above codes bring the bug I mentioned in comment 69.
  I could also not find out the relationship between the codes and the bug……

  the relevant audios are in the attachment.
 
  Please help to confirm.Thank you very much!

Comment 72

[Dominic Kuo [:dkuo]]

If you want to put some app inproc, then that's where we should add to put the app's manifest url in the black list, and once the app is inproc, the way gecko play amr files will be like system app because it's part of the b2g process, so I think it should be the same issue as bug 990957 if we are having troubles on playing amr files, could you please try the patch in bug 990957 first? though it's already landed on 1.3t but just make sure your gecko has that patch, thanks.

Comment 73

[James Zhang (Spreadtrum)]

(In reply to Dominic Kuo [:dkuo] from comment #72)
> If you want to put some app inproc, then that's where we should add to put
> the app's manifest url in the black list, and once the app is inproc, the
> way gecko play amr files will be like system app because it's part of the
> b2g process, so I think it should be the same issue as bug 990957 if we are
> having troubles on playing amr files, could you please try the patch in bug
> 990957 first? though it's already landed on 1.3t but just make sure your
> gecko has that patch, thanks.

We use origin gecko branch and code. Jingmei, please let music app out of process and find the difference.

Comment 74

[jingmei.zhang]

(In reply to James Zhang (Spreadtrum) from comment #73)
> (In reply to Dominic Kuo [:dkuo] from comment #72)
> > If you want to put some app inproc, then that's where we should add to put
> > the app's manifest url in the black list, and once the app is inproc, the
> > way gecko play amr files will be like system app because it's part of the
> > b2g process, so I think it should be the same issue as bug 990957 if we are
> > having troubles on playing amr files, could you please try the patch in bug
> > 990957 first? though it's already landed on 1.3t but just make sure your
> > gecko has that patch, thanks.
> 
> We use origin gecko branch and code. Jingmei, please let music app out of
> process and find the difference.

while the music app is out of process,it could display the amr audio file well.

Comment 75

[jingmei.zhang]

(In reply to Dominic Kuo [:dkuo] from comment #72)
> If you want to put some app inproc, then that's where we should add to put
> the app's manifest url in the black list, and once the app is inproc, the
> way gecko play amr files will be like system app because it's part of the
> b2g process, so I think it should be the same issue as bug 990957 if we are
> having troubles on playing amr files, could you please try the patch in bug
> 990957 first? though it's already landed on 1.3t but just make sure your
> gecko has that patch, thanks.

hi Dominic:

  I have checked that our gecko have the bug 990957 patch.with the music in pro,the music app could not load amr audio files.would you please help to confirm the issue?

Comment 76

[Dominic Kuo [:dkuo]]

No problem, and let's move to bug 1032678 and I am working on it, thanks.