Closed
Bug 841972
Opened 12 years ago
Closed 12 years ago
Improve loading of album art in Music startup
Categories
(Firefox OS Graveyard :: Gaia::Music, defect)
Tracking
(blocking-b2g:tef+, b2g18 verified, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)
VERIFIED
FIXED
blocking-b2g | tef+ |
People
(Reporter: cjones, Assigned: cjones)
References
Details
Attachments
(4 files, 1 obsolete file)
1.22 KB,
patch
|
dkuo
:
review+
|
Details | Diff | Splinter Review |
3.73 KB,
patch
|
dkuo
:
review+
|
Details | Diff | Splinter Review |
186 bytes,
text/html
|
cjones
:
review+
vingtetun
:
approval-gaia-v1+
|
Details |
10.60 KB,
application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
|
Details |
Bug 817115 comment 23 and one more potential win.
Assignee | ||
Comment 1•12 years ago
|
||
300ms win on display of all album art and (mostly) semantics preserving.
The one semantic change was not loading the default background image until we know an album doesn't have its own album art. In my testing, those never really showed up sooner anyway, so we were wasting image decoding work.
Have one more change that alters semantics slightly, but not sure if it's a win yet ...
Assignee: nobody → jones.chris.g
Attachment #714691 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Comment 2•12 years ago
|
||
Yeah, the second change gets us 1 second :).
Comment 3•12 years ago
|
||
Comment on attachment 714691 [details] [diff] [review]
Use CSS to position and clip album art
Review of attachment 714691 [details] [diff] [review]:
-----------------------------------------------------------------
The code for this looks good, though I wouldn't mind seeing displayAlbumArt removed entirely. I still need to test it on the phone against music with album art, though. r+ (hopefully) forthcoming!
::: apps/music/js/utils.js
@@ +4,5 @@
> var span = document.createElement('span');
> span.textContent = str;
>
> if (escapeQuotes)
> + return span.innerHTML.replace(/"/g, '"').replace(/'/g, '''); //"
I'm guessing this comment works around some editor's broken syntax highlighting? A word or two about that would be helpful, since it took me a few minutes to realize why this was here.
Assignee | ||
Comment 4•12 years ago
|
||
Interdiff of changes to do this.
Sometimes I really love JavaScript.
Attachment #714701 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Comment 5•12 years ago
|
||
This is a 1 second startup win, tracking for uplift.
Comment 6•12 years ago
|
||
Comment on attachment 714691 [details] [diff] [review]
Use CSS to position and clip album art
Review of attachment 714691 [details] [diff] [review]:
-----------------------------------------------------------------
You'll need to update the other places that show album art to handle things in the new way as well. The player shows the album art tiled, and the artists and albums pages show just the top left corner. (This is just with the placeholder art, though I'm sure real art is affected too.)
Attachment #714691 -
Flags: review?(squibblyflabbetydoo) → review-
Assignee | ||
Comment 7•12 years ago
|
||
The focus of this bug is on improving startup time for v1.0.1. We need to limit risk in the changes here. I agree we should optimize the other sites in a followup.
This updated patch removes the changes to the default background sizing rules, since they don't affect the tiles anyway.
Attachment #714691 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #714706 -
Flags: review?(squibblyflabbetydoo)
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Jim Porter (:squib) from comment #3)
> Comment on attachment 714691 [details] [diff] [review]
> Use CSS to position and clip album art
>
> Review of attachment 714691 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: apps/music/js/utils.js
> @@ +4,5 @@
> > var span = document.createElement('span');
> > span.textContent = str;
> >
> > if (escapeQuotes)
> > + return span.innerHTML.replace(/"/g, '"').replace(/'/g, '''); //"
>
> I'm guessing this comment works around some editor's broken syntax
> highlighting? A word or two about that would be helpful, since it took me a
> few minutes to realize why this was here.
That's correct. Sorry, I missed this comment before I posted v2, have added it locally.
Assignee | ||
Comment 9•12 years ago
|
||
Comment on attachment 714701 [details] [diff] [review]
Load visible tiles before hidden ones
Dominic, if you get back in time to review this, please do :). Jim and I are filling in for you.
Whoever gets to it first.
Attachment #714701 -
Flags: review?(dkuo)
Assignee | ||
Comment 10•12 years ago
|
||
Comment on attachment 714706 [details] [diff] [review]
Use CSS to position and clip album art, v2
(Whoever gets to this first.)
Attachment #714706 -
Flags: review?(dkuo)
Comment 11•12 years ago
|
||
Comment on attachment 714706 [details] [diff] [review]
Use CSS to position and clip album art, v2
r+, looks good to me.
Attachment #714706 -
Flags: review?(dkuo) → review+
Comment 12•12 years ago
|
||
Comment on attachment 714701 [details] [diff] [review]
Load visible tiles before hidden ones
r+, looks good to me.
Attachment #714701 -
Flags: review?(dkuo) → review+
Comment 13•12 years ago
|
||
Chris and Jim, thanks for filling in this bug for me, I really appreciated.
I have combined those two patches that you attached and sent a new one to github, also added little fixes to keep the same UX and visual. Please check to see I am combining right or not, if there is no problem, I think this patch should be good to go, thanks.
Attachment #715080 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 715080 [details]
Link to github (combined patch)
https://github.com/mozilla-b2g/gaia/commit/3d14b28f958f4fe9735bdef5dfde7ba9c8e03f43
Relatively low-risk patch that makes a big difference in music startup time.
Attachment #715080 -
Flags: review?(jones.chris.g)
Attachment #715080 -
Flags: review+
Attachment #715080 -
Flags: approval-gaia-v1?(21)
Assignee | ||
Updated•12 years ago
|
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Attachment #714701 -
Flags: review?(squibblyflabbetydoo)
Comment 15•12 years ago
|
||
Comment on attachment 714706 [details] [diff] [review]
Use CSS to position and clip album art, v2
Clearing out review requests from me, since this is checked in now.
Attachment #714706 -
Flags: review?(squibblyflabbetydoo)
Comment 16•12 years ago
|
||
Comment on attachment 715080 [details]
Link to github (combined patch)
We also love when you do JS :)
I have been told that perf are important. Let's push this fix on v1.0.1 and v1-train.
Attachment #715080 -
Flags: approval-gaia-v1?(21) → approval-gaia-v1+
Comment 17•12 years ago
|
||
v1-train@b9f0f11b265f23adabbc1391a53da757e5200e9d
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #16)
> Comment on attachment 715080 [details]
> Link to github (combined patch)
>
> I have been told that perf are important. Let's push this fix on v1.0.1 and
> v1-train.
This seems to have only landed on v1-train.
Flags: needinfo?(jhford)
Assignee | ||
Updated•12 years ago
|
blocking-b2g: - → tef?
Flags: needinfo?(jhford)
Comment 19•12 years ago
|
||
Go ahead with uplift, like with other perf fixes, if there are regressions detected before 2/28 we'll have to backout.
blocking-b2g: tef? → tef+
status-b2g18-v1.0.0:
--- → wontfix
Comment 20•12 years ago
|
||
v1.0.1: 249ef79362a3e501f7a9f3a2952e8b84195bbf98
Comment 22•12 years ago
|
||
Can you please provide steps to verify this fix - as we will blackbox test from the UI?
Assignee | ||
Comment 23•12 years ago
|
||
This is a performance improvement, so you would need to measure startup time of the music app (with a realistic music collection installed) on builds before and after this patch landed.
Flags: needinfo?(cjones.bugs)
Comment 24•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> This is a performance improvement, so you would need to measure startup time
> of the music app (with a realistic music collection installed) on builds
> before and after this patch landed.
Chris,
Please see Excel document titled "Music App Load Times.xlsx"
From the data you will see the app load times have improved from older builds, but also please note that the new v1.0.1 build had major issues in having the music app even load.
Please let me know if you have any questions.
Comment 25•12 years ago
|
||
Comment 26•12 years ago
|
||
Document attachment title in Comment 24 is actually "Excel document with Music App Load Time Data".
Comment 27•12 years ago
|
||
As per comment #23 by comparing before the patch is landed and after, the performance of the loading is increased on start up.
Before Patch:
Unagi-V1.1 Load Time -1083ms
Unagi -V1.0.1 Load Time -1071ms
After the Patch for the latest build
Inari V1.0.1 Load time -641ms
Unagi V1.1 Load time -676ms
Environmental Variables:
Unagi Build ID: 20130603070207
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/4785b1353fd7
Gaia: 4de4354e3a99f151a834743c7b03a041ac8db12f
Inari Build ID: 20130603070208
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/42555e1e72fa
Gaia: fcae23654296c9cc645c2b7e77a2c36bf494803a
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•