Naoki Hirata :nhirata (please use needinfo instead of cc) Reporter
	Description • 12 years ago

## Environment :
Unagi phone, build 2012/11/21
  
## Repro :
1. place 30 songs on the SDCard
2. plug in the SDcard and reboot the phone
3. launch the music app once you get to the home screen

## Expected :
1. music will load a playlist once the app finishes launching

## Actual :
1. music takes a while to load because it tries to scan everything on the card, sometimes can take up to 30 seconds or more.

## Note :

	Naoki Hirata :nhirata (please use needinfo instead of cc) Reporter
	Comment 1 • 12 years ago

I can't seem to find a bug on this yet?

	Marcia Knous [:marcia]
	Comment 2 • 12 years ago

There is a similar problem with video.

If we don't have dedicated folders for media, I am assuming this will continue to be an issue if it is scanning the entire card each time.

	David Flanagan [:djf] Assignee
	Comment 3 • 12 years ago

Dominic: Feel free to take this one if you want it.

The scanning performance of the music app is probably as good as it is going to get.  Maybe there are bugs in that code that slow it down unnecessarily.

Marcia: even if there was a dedicated Music folder, scanning new files for metadata would still take a while. The process of finding new files on the sdcard doesn't take all that long, and wouldn't be all that much faster with a Music folder. But that's a Gecko issue and not something we can change now, anyway.

In my opinion, there are two issues here  (Dominic is the owner of the music app, so he would know better than I about these):

1) The music app seems to display its "busy" animation only when enumerating files from its database (to list songs on an album or albums by an artist). It should (but does not) display that crawling ants animation when MediaDB is scanning.  (Dominic: you can listen for scanstart and scanend events to know when mediadb is scanning). Letting the user know when the app is busy should help a lot.

2) I think the app does not build its main "tiles" view until the initial scan is complete.  I think that instead, it should build it right away based on the songs it already knows about, and then, if the scan discovers any changes, it should rebuild the tiles to include (or even feature) the new songs.

The MediaDB architecture should allow the music app to be responsive and available quickly, even if scanning the sdcard takes a while.

	Dominic Kuo [:dkuo]
	Comment 4 • 12 years ago

David,

> In my opinion, there are two issues here  (Dominic is the owner of the music
> app, so he would know better than I about these):
> 
> 1) The music app seems to display its "busy" animation only when enumerating
> files from its database (to list songs on an album or albums by an artist).
> It should (but does not) display that crawling ants animation when MediaDB
> is scanning.  (Dominic: you can listen for scanstart and scanend events to
> know when mediadb is scanning). Letting the user know when the app is busy
> should help a lot.

Actually, Music already listened for scanstart and scanend events, I believe 
it's I didn't handle the events properly so that users feel nothing when 
mediadb is scanning.

> 2) I think the app does not build its main "tiles" view until the initial
> scan is complete.  I think that instead, it should build it right away based
> on the songs it already knows about, and then, if the scan discovers any
> changes, it should rebuild the tiles to include (or even feature) the new
> songs.

Yes, that make sense and I should try to make the rebuild logic like that.

> The MediaDB architecture should allow the music app to be responsive and
> available quickly, even if scanning the sdcard takes a while.

I would like to take this issue since this might be a problem of Music app only,
thanks David.

	Alex Keybl [:akeybl]
	Comment 5 • 12 years ago

Mass Modify: All un-milestoned, unresolved blocking-basecamp+ bugs are being moved into the C3 milestone. Note that the target milestone does not mean that these bugs can't be resolved prior to 12/10, rather C2 bugs should be prioritized ahead of C3 bugs.

	Marcia Knous [:marcia]
	Comment 6 • 12 years ago

During testing, my music library has grown and I am noticing this continues to be a problem. Anything we can do to optimize the performance would be great.

	Dominic Kuo [:dkuo]
	Comment 7 • 12 years ago

Hi Naoki and Marica,

Does this happen when you flashed your device and launch Music first time?
I think when the first time Music scans all the files on device, it takes time to index and record then in the MediaDB, but it should be much faster when you relaunch again.
So I think I am going to adjust the first time scanning for Music.

	Dietrich Ayala (:dietrich)
	Comment 8 • 12 years ago

Driver retriage: The performance on collections of a couple of gigs is not terrible, and the scanning is communicated to the user clearly. We will not hold the release for this.

We would take a patch on approval if a feasible and low-risk solution is found.

	Marcia Knous [:marcia]
	Comment 9 • 12 years ago

I have 167 music files on my device, and the first time the music app runs after a fresh flash takes a very long time (I can time it if necessary). Parul who is testing Music app with me has 513 music tracks and her experience was even worse. Users might think there is something wrong the first time they launch. Anything we can do to improve the first run scanning experience would be a huge win in my opinion.

	David Flanagan [:djf] Assignee
	Comment 10 • 12 years ago

I think there are some relatively simple fixes we can make here to improve the first run long scan...

During the scanning process, the app should rebuild its home "tiles" page every time it gets another batch of songs from mediadb.  This way the user can see that something is happening at least.  Gallery does something like this and Music could, too.  

Anyone think we should try to get tef+ on this?

	Marcia Knous [:marcia]
	Comment 11 • 12 years ago

Yes for tef+, especially since the target market will likely be managing the music on their device vs. streaming.

(In reply to David Flanagan [:djf] from comment #10)
> I think there are some relatively simple fixes we can make here to improve
> the first run long scan...
> 
> During the scanning process, the app should rebuild its home "tiles" page
> every time it gets another batch of songs from mediadb.  This way the user
> can see that something is happening at least.  Gallery does something like
> this and Music could, too.  
> 
> Anyone think we should try to get tef+ on this?

	David Flanagan [:djf] Assignee
	Comment 12 • 12 years ago

Taking this.

	David Flanagan [:djf] Assignee
	Comment 13 • 12 years ago

The motivation for fixing this bug is to improve the first-use experience for someone who has just copied a large music collection to the sdcard. But the reason that we just got tef+ is that we're getting pressure about media apps startup time from a partner (not tef)  Fixing this bug ought to reduce the pressure.

I don't think we can scan and parse metadata much faster than we already do. But we can make the music app respond more quickly than it does now. My plan for this bug is to display music as soon it is found, updating the UI periodically during the scanning process.  

Since this app also has a title bar, I'll experiment with using it to display the scan results, so the user can see just how quickly their music is being scanned. 

Also, I wonder if we can optimize the metadata parsing if we assume that all tracks on the same album will have the same cover art.  We shouldn't parse and save the same cover image over and over again. Instead of saving album art in the mediadb, maybe we could map album name to album art using asyncStorage as a really easy interface to indexedDB.

	David Flanagan [:djf] Assignee
	Comment 14 • 12 years ago

My first attempt was to just get updates in batches, and rebuild the UI each time I got a batch.  But it flashed a lot and updated too often.

The next approach I'll try will be to modify the TilesView.update() and ListView.update() methods so that they don't always insert at the end of the view. If the new object is out of order, they'll insert it in the right place.  Then those methods ought to work while enumerating and for scan results.

	Dominic Kuo [:dkuo]
	Comment 15 • 12 years ago

David,

I think this issue is also related to the logic of mix page(bug 800843), so actually I was planning to make some adjustments in both of these two bugs, I am not sure how much you have done on this work, an since you are already working on it, just let you know maybe you can also consider the updating logic of mix page, thanks.

	David Flanagan [:djf] Assignee
	Comment 16 • 12 years ago

I'm not quite done with this bug, but am going to try to attach a work in progress to get Dominic's feedback overnight with the goal of checking something in by the Thursday afternoon (pacific time) deadline.

What I've done so far:

1) Modify the scan logic so that it rebuilds the display (with showCurrentView()) every time it finds 25 new songs, and when the scan ends.  To make this work, though, I had to modify showCurrentView() so that it enumerates the songs into an array and then builds the UI all at once.  Adding one tile or list item at a time asynchronously caused way too much repainting.

2) Display a spinner between the time that the app starts and the time that it gets its initial tiles view displayed. This avoids a few seconds of black screen time.

3) Get rid of the crawling ants scanning animation and instead replace it with a scan progress that displays more details to the user. Scans are displayed in (over, really) the titlebar. A spinner is on the left. Inside the spinner we display the number of songs that have been scanned. To the right, we display the artist and song title of each song as it is scanned.  This allows the user to see how busy the app is scanning. (This has no UX review at all, but I think it is better than what we have now, and I'm willing to land it even if I can't get Casey to comment tomorrow).

4) Don't start displaying scan progress when the scan starts. We save this until the scan actually returns results. So in the normal case where the app starts up and doesn't find any new music, no scan UI will ever be displayed.

5) Tweak the mediadb scan algorithm to optimize the first-run case. Previously on the first run, mediadb would enumerate the entire file system before it began parsing the metadata for new songs. Now it doesn't do that.

My own music collection that I've been testing with hasn't had album art.  I'm adding that now, and hope to make a big optimization tomorrow.  I'm going to assume that all songs with the same artist and album have the same album art, so I won't have to create 10 copies of the same thumbnail image.  I bet that will make a big difference, but isn't ready yet.

Dominic: in order to get the logic right for the tiles view, I'd suggest modifying the app so that it stores its own in-memory copy of all the song data. On start up it does a getAll() to get all database records, and then builds a big data structures with its own indexes for album, artist, and so on.  Then queries just becomes javascript algorithms intead of calls to musicdb.enumerate(), and it is possible to do interesting things for the tile view.  Unfortunately, that doesn't seem like something we can do for v1.0.  Maybe we can just enumerate all albums and pick 12 or 18 at random so at least the display is different each time.

Dominic: do you know if is there a way to distinguish singles (songs released alone and not part of an album) from songs that have incomplete metadata and are therefore "unknown album"?

	David Flanagan [:djf] Assignee
	Comment 17 • 12 years ago

I've just tested with 270 music files (more than 50% now have album art) and the app scanned them in 110 seconds. So about 400ms each.  With the feedback, I think users' will be willing to wait.  I'll see if that speed improves when I'm not generating so many thumbnails.

	David Flanagan [:djf] Assignee
	Comment 18 • 12 years ago

Dominic,

As I noted above, I'm not done with this bug... I still want to optimize thumbnail generation. Because of the timezone difference, I won't be able to ask for your review before the Thursday afternoon deadline, but I hope you'll have time to give feedback on this work in progress.

	David Flanagan [:djf] Assignee
	Comment 19 • 12 years ago

As a followup to comment 17, when I run the same test without this patch applied, the 270 files take about 105 seconds to scan, but the app gives no feedback at all (just the crawling ants) for those 105 seconds, so the user doesn't know what it is doing and can't see or play any music for that entire time.

	David Flanagan [:djf] Assignee
	Comment 20 • 12 years ago

Comment on attachment 705727 [details]
link to patch on github

On second thought, Dominic, why don't you just review this PR. If it seems good to you (and safe enough for this last minute landing) I can just land this patch with your review, and then do the thumbnail parsing optimization as a separate patch that I can get someone else to review tomorrow.

Thanks!

	David Flanagan [:djf] Assignee
	Comment 21 • 12 years ago

If I remove the thumbnail generation step, my 270 files parse in 75s instead of 110s, shaving off about 30% of the scan time. So it is definitely worth fixing the overprocessing of thumbnails, both from a time and a storage space perspective.

	Dominic Kuo [:dkuo]
	Comment 22 • 12 years ago

Comment on attachment 705727 [details]
link to patch on github

r+, this patch looks great.

I think David improved not just the startup user experience, but also the scanning and rebuilding logics which makes users feel better while Music app is still in preparing state.

So I also think it will be nice to land this if everyone thinks Music app takes a while at first time startup.

	Dominic Kuo [:dkuo]
	Comment 23 • 12 years ago

(In reply to David Flanagan [:djf] from comment #16)

> Dominic: do you know if is there a way to distinguish singles (songs
> released alone and not part of an album) from songs that have incomplete
> metadata and are therefore "unknown album"?

Maybe use the size of the cover image if there is one?

	David Flanagan [:djf] Assignee
	Comment 24 • 12 years ago

In his review, Dominic raised (and then resolved) a concern about file deletions and showing a list of songs on an album when some (or all) of those songs had actually been deleted from the sdcard.  Dominic's particular concern didn't apply because the app always goes back to the tiles view before rescanning.

But, because the app now allows the user to interact with music while scanning is going on, we are exposed to the possiblity that the app will display music that isn't actually there.  So I need to handle this case somehow to be sure we don't crash.  At a minimum, just returning to the tiles view so the user can see the ongoing scan.  Or I could use an alert() first and then return to the tiles view.  Though I worry that scanning would be blocked by the alert. An alert would also require l10n. So maybe just doing something that does not break is all we can hope for here. I'll address that in a followup patch

	David Flanagan [:djf] Assignee
	Comment 25 • 12 years ago

Casey,

This screenshot is what the new scanning progress indicator looks like.  There's a spinner, with the number of new songs scanned inside, and then the artist and song title for each song as it is found. The number and song titles are updated about 4 times a second, so the user gets the impression of rapid scanning.

This is much better than what we have now (no real feedback at all) so I'm inclined to land it without your feedback, but I thought I should at least show it to you!

	David Flanagan [:djf] Assignee
	Comment 26 • 12 years ago

Merged into Gaia master: 

https://github.com/mozilla-b2g/gaia/commit/ebac3046280299ec320ecae04ca5c34a163bc3eb

	David Flanagan [:djf] Assignee
	Comment 27 • 12 years ago

Merged into the v1.0.0 branch: 

https://github.com/mozilla-b2g/gaia/commit/7cacbcb8042b49d816a34de8e9767dcedefa6d84

	David Flanagan [:djf] Assignee
	Comment 28 • 12 years ago

Casey's feedback (on IRC) was that we should do this.  In the future, he'd like to consider putting the scanning progress on the bottom, above the bottom row of buttons.  And he'd like the top line to say something like "Importing music"  But he agreed that it was worth landing this, even in its imperfect state.

	David Flanagan [:djf] Assignee
	Comment 29 • 12 years ago

Part 2 of this patch is to speed up metadata parsing by not creating thumbnails for every single song. The main speedup here is just the assumption that all tracks on an album will have the same thumbnail, so we don't have to recreate the thumbnail for each track.

Also, we don't have to create any thumbnails as part of the scanning process. We can create them on-demand when they are needed. Right now I'm wondering whether the thumbnail cache should be created each time the app runs, or whether I should persist it to asyncStorage or something similar.  Also do I just cache the thumbnail blobs or should I attempt to cache the blob urls?

Also for this part 2 patch: I want to address the missing file issue Dominic raised (see comment 24) and it would be nice to incorporate the fix from bug 833595 here too.

	David Flanagan [:djf] Assignee
	Comment 30 • 12 years ago

James and/or Dominic:

This is the thumbnail optimization I told you about. Previously my metadata code was creating a thumbnail for every song, even when a bunch of songs were part of the same album and shared the same thumbnail.

With this patch scanning speed goes from 2.5 songs per second to 4 songs per second.

I've got tef+ on this bug and would love to get this optimization in to 1.0.0, if either of you have time to review it quickly.

I've tested that it works when bluetooth file transfer opens a new song via the open activity.

	James Lal [:lightsofapollo]
	Comment 31 • 12 years ago

Comment on attachment 706190 [details]
part 2: patch on github

I ran some analysis before/after the patch and can't really see much performance difference it takes somewhere between 55 and 66 seconds (vs similar numbers prior to this patch) to scan the entire 700 music files I have. My case is likely specialized and so I am not seeing much difference (I only have two albums over all files). Functionality it seems to work well and the code makes sense to me...

	David Flanagan [:djf] Assignee
	Comment 32 • 12 years ago

So part two didn't make it before the v1.0.0 deadline, so I'm going to resolve this bug because the first part did land.  Then I'll open a new bug for the thumbnail optimization.

	Casey Yee
	Comment 33 • 12 years ago

Comment on attachment 705897 [details]
screenshot of new scanning feedback

As per bug.  Looks good for now as a in-term solution and is better than what was already in place.   We will be working on the IxD to fit better with other already established patterns.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 34 • 12 years ago

Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.

	Rob MacDonald [:robmac]
	Comment 36 • 12 years ago

Comment on attachment 709650 [details]
proposed music SD card scanning solution from Josh

Any concerns with this as a proposed solution?

	David Flanagan [:djf] Assignee
	Comment 37 • 12 years ago

(In reply to Rob MacDonald from comment #36)
> Comment on attachment 709650 [details]
> proposed music SD card scanning solution from Josh
> 
> Any concerns with this as a proposed solution?

I like this solution.  Here are my concerns:

1) I don't think that the text (the names of the songs and artists) will be legible over the album covers below if the background is as translucent as shown.

2) I don't think that we need the crawling ants at the top if we're showing these scan details at the bottom.

3) What screens do you want this scanning overlay to appear over?  Just the initial tiles screen?  Or should it also be over the albums and artists lists as it is now?  I assume you don't want it to show on the now playing screen, but I'd like to be clearer about this.

4) I don't know what the priority for this change is. Is this still tef+ even though this bug is marked resolved?  Or is this a change we'd like to make for shira or leo?  If tef, does it have to go through triage again?  If not tef, then let's open a new bug for it.

In any case, the fix should be easy, mostly (but not exclusively) CSS.

	Sam Shipley
	Comment 38 • 12 years ago

Issue no longer reproduces on the Unagi device
Build ID: 20130214070203
December 5th Kernel
Gaia: 6544fdb8dddc56f1aefe94482402488c89eeec49
Gecko http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/d1288313218e

Media loads quickly when starting the media app.