Closed
Bug 840627
Opened 12 years ago
Closed 12 years ago
Icons in the settings app takes 200ms in the startup time
Categories
(Firefox OS Graveyard :: Gaia::Settings, defect)
Firefox OS Graveyard
Gaia::Settings
Tracking
(blocking-b2g:tef+, b2g18+ fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)
VERIFIED
FIXED
blocking-b2g | tef+ |
People
(Reporter: vingtetun, Assigned: etienne)
References
Details
(Whiteboard: [FFOS_perf])
Attachments
(4 files, 2 obsolete files)
(deleted),
patch
|
vingtetun
:
review+
akeybl
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
patch
|
vingtetun
:
review+
vingtetun
:
approval-gaia-v1+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details |
No description provided.
Reporter | ||
Comment 1•12 years ago
|
||
I have been told that just changing the sprite should help there.
Assignee | ||
Comment 2•12 years ago
|
||
I should have a patch later today.
Assignee | ||
Comment 3•12 years ago
|
||
WIP - it's consistently shaving 180ms off startup time and I can't see any visual degradation (tested on unagi and otoro).
(apply with git am)
Assignee: nobody → etienne
Attachment #713371 -
Flags: feedback?(kaze)
Attachment #713371 -
Flags: feedback?(21)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 713371 [details] [diff] [review]
WIP
Have you tried to keep the same-moz-image-rect rule so you will be able to simply switch the rule on a :active?
Also you are sure that the delay is related to the alpha channel? If not I would like to see if we can keep use one format for all apps. (in the worst case maybe something can be done in gecko).
Attachment #713371 -
Flags: feedback?(21)
Comment 5•12 years ago
|
||
FYI we noticed in other bugs that re-compressing existing assets can often speed up their load time significantly. As a quick test I've tried re-compressing the settings assets using the tools/png_recompress.sh script in gaia and obtained files which were 1/3 of their original size and which yielded an overall startup time reduction of ~90ms.
So please make sure you re-compress all new PNG files before committing them; alternatively we might consider running re-compression as part of the build process (see the comments in bug 836377) or just create a ticket to manually re-compress everything just before release.
Updated•12 years ago
|
blocking-b2g: --- → tef?
Reporter | ||
Comment 6•12 years ago
|
||
(In reply to Gabriele Svelto [:gsvelto] from comment #5)
> FYI we noticed in other bugs that re-compressing existing assets can often
> speed up their load time significantly. As a quick test I've tried
> re-compressing the settings assets using the tools/png_recompress.sh script
> in gaia and obtained files which were 1/3 of their original size and which
> yielded an overall startup time reduction of ~90ms.
>
> So please make sure you re-compress all new PNG files before committing
> them; alternatively we might consider running re-compression as part of the
> build process (see the comments in bug 836377) or just create a ticket to
> manually re-compress everything just before release.
I think this is related to alpha channel and not directly to the size of the image.
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> (In reply to Gabriele Svelto [:gsvelto] from comment #5)
> > FYI we noticed in other bugs that re-compressing existing assets can often
> > speed up their load time significantly. As a quick test I've tried
> > re-compressing the settings assets using the tools/png_recompress.sh script
> > in gaia and obtained files which were 1/3 of their original size and which
> > yielded an overall startup time reduction of ~90ms.
> >
> > So please make sure you re-compress all new PNG files before committing
> > them; alternatively we might consider running re-compression as part of the
> > build process (see the comments in bug 836377) or just create a ticket to
> > manually re-compress everything just before release.
>
> I think this is related to alpha channel and not directly to the size of the
> image.
Yep, the size does matter but removing the alpha channel is what got us the biggest gain.
Assignee | ||
Comment 8•12 years ago
|
||
Here we go, cleaned up the CSS a bit and tried again with
- optimized pngs (with pngcrush)
- jpgs
- gifs
And gif wins! (I can get the same performance gain with mid-quality jpgs but the icons get "noisy").
And FTR I consistently see a >180ms startup time improvement with this patch.
Attachment #713371 -
Attachment is obsolete: true
Attachment #713371 -
Flags: feedback?(kaze)
Attachment #713934 -
Flags: review?(21)
Reporter | ||
Comment 9•12 years ago
|
||
Comment on attachment 713934 [details] [diff] [review]
Patch proposal
Review of attachment 713934 [details] [diff] [review]:
-----------------------------------------------------------------
\o/
|o/
/o/
/o|
Can you open a bug against Core::GFX to be sure it is tracked?
Attachment #713934 -
Flags: review?(21) → review+
Reporter | ||
Comment 10•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 11•12 years ago
|
||
Was it voluntary to land the patch without the added performance test?
Updated•12 years ago
|
blocking-b2g: tef? → -
tracking-b2g18:
--- → +
Updated•12 years ago
|
Comment 12•12 years ago
|
||
Comment on attachment 713934 [details] [diff] [review]
Patch proposal
Not a blocker, but IF we think this is low risk, we're approving for uplift to v1-train and v1.0.1. Also adding qawanted/verifyme to get extra eyes on these icon changes.
Attachment #713934 -
Flags: approval-gaia-v1+
Updated•12 years ago
|
status-b2g18:
--- → affected
status-b2g18-v1.0.1:
--- → affected
Comment 13•12 years ago
|
||
Next time please don't modify graphics without letting visual design know. NO UX people are CC'd. Also I don't see any screenshots of before and after.
If this looks bad, we'll need to revert it. I know we need to be as fast as Android on the same hardware, but that also means looking as good as Android.
blocking-b2g: - → tef?
status-b2g18:
affected → ---
status-b2g18-v1.0.1:
affected → ---
tracking-b2g18:
+ → ---
Comment 14•12 years ago
|
||
Next time please don't modify graphics without letting visual design know. NO UX people are CC'd. Also I don't see any screenshots of before and after.
If this looks bad, we'll need to revert it. I know we need to be as fast as Android on the same hardware, but that also means looking as good as Android.
Comment 15•12 years ago
|
||
Patryk is going to take a look at the changes on master before we uplift, to make sure we haven't taken any visual regressions.
Assignee | ||
Comment 16•12 years ago
|
||
(In reply to Patryk Adamczyk [:patryk] UX from comment #14)
> Next time please don't modify graphics without letting visual design know.
> NO UX people are CC'd.
You're right sorry about that.
> Also I don't see any screenshots of before and after.
I'll add them right now.
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Comment 19•12 years ago
|
||
Hey Etienne, it looks like in the after screenshot all the icons moved. 1px down, 1px to the left. So they are not ideally spaced anymore.
Flags: needinfo?(padamczyk)
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #715135 -
Flags: review?(21)
Assignee | ||
Updated•12 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 21•12 years ago
|
||
Attachment #714444 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Attachment #715135 -
Flags: review?(21)
Attachment #715135 -
Flags: review+
Attachment #715135 -
Flags: approval-gaia-v1+
Assignee | ||
Comment 22•12 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
We have a regression reported from this patch, please to be backing out :(
> Upon more careful inspection, I see a regression w/ this patch where tapping
> "Wi-Fi" after a cold-start of Settings doesn't work. Sometimes I can get
> the "Wi-Fi" item to respond after a bit of Settings app
> scrolling/navigating. No solid STR yet.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 24•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #23)
> We have a regression reported from this patch, please to be backing out :(
>
Are we sure about the regression window, this patch was only css... but we had a bunch of other more invasive patchs landed in the settings app...
Updated•12 years ago
|
Flags: needinfo?(mvines)
Apparently it was bug 840322.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Comment 27•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #25)
> Apparently it was bug 840322.
If the reason this caused regressions (or didn't cause them, if that's what comment 25 implies) is no longer present, let's get this perf win uplifted again to branches.
blocking-b2g: tef? → tef+
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → affected
So if I'm reading this right, I should be able to compare the difference between MC build settings and nightly build settings. To note : My camera only handles 30 frames per second.
Prepatch: v101 2/22 build
https://www.youtube.com/watch?v=jiYSvrdgKxE
2 seconds Frame 21 is when I touched the settings icon
4 seconds Frame 22 is when the settings comes up.
2 seconds and 1 frame total.
Postpatch: MC 2/22 build
https://www.youtube.com/watch?v=Qy7QpagXZdc
Frame 19 is when I touched the settings icon
2 seconds Frame 8 is when the settings comes up (brightness level changes)
1 second and 19 seconds total
12 frames *2 = .24 difference roughly which is equivalent to what this bug is trying to fix. ~200 ms.
I am unsure but I am finding that the settings app is having issues. I am not sure if it is related to this bug as of yet. bug 843846
based on https://bugzilla.mozilla.org/show_bug.cgi?id=840322#c43, I'm removing mvines. I think the intent was to get info from mwu?
Flags: needinfo?(mvines)
Bug 843846 is unrelated due to regression range check on bug 843846. Marking this as verified.
Status: RESOLVED → VERIFIED
Keywords: qawanted
Comment 31•12 years ago
|
||
v1-train (squashed): aa103c97804cc24d9ff3a6b2cccba45c0d7c4525
v1.0.1: e601b5234f7ff71514225be12118234e26f2de62
Comment 32•12 years ago
|
||
I've opened bug 840627 to track the slowness of alpha images.
You need to log in
before you can comment on or make changes to this bug.
Description
•