Closed
Bug 1132081
Opened 9 years ago
Closed 9 years ago
Speed up ConvertHostARGBRow() in the PNG encoder
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: glennrp+bmo, Assigned: glennrp+bmo)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(2 files, 3 obsolete files)
396 bytes,
text/html
|
Details | |
1.63 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
In image/encoders/png/nsPNGEncoder.cpp, the ConvertHostARGBRow() function does a de-premultiply operation on all pixels that aren't 100% transparent (alpha == 0), which is a wasteful operation on pixels that are 100% opaque (alpha == 255). Also, the default zlib compression level (6) is used, while a faster, lower level (4) will still achieve reasonable compression for images that are not likely to be read more than once. My testing shows a potential reduction of 30% to 50% in cpu time spent in ConvertHostARGBRow(), typically reducing the time for encoding a 640x300 image by 15-20 milliseconds.
Comment 2•9 years ago
|
||
Comment on attachment 8562833 [details] [diff] [review] v00-1132081-speed-up-ConvertHostARGBRow Review of attachment 8562833 [details] [diff] [review]: ----------------------------------------------------------------- Please separate out style changes from the functional ones.
Assignee | ||
Comment 3•9 years ago
|
||
Style changes undone.
Attachment #8562833 -
Attachment is obsolete: true
Comment 4•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac5bcd54258c
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 5•9 years ago
|
||
Failed a bunch of apng unit tests.
Assignee | ||
Comment 6•9 years ago
|
||
It appears that the tests were requiring the generated PNGs to be bit-for-bit identical. They aren't, because the zlib compression optimization changed. The attachment compares the two base64 images from the reftest, scaled up so you can see them; they're not bit-for-bit identical but they both describe the same image.
Assignee | ||
Comment 7•9 years ago
|
||
Removed the line that changes the zlib compression level. Please "try" to make sure it passes the unit tests now.
Flags: needinfo?(ryanvm)
Assignee | ||
Updated•9 years ago
|
Attachment #8566589 -
Attachment is obsolete: true
Comment 8•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c51e0981ca1
Flags: needinfo?(ryanvm)
Assignee | ||
Updated•9 years ago
|
Attachment #8566589 -
Attachment is obsolete: false
Assignee | ||
Updated•9 years ago
|
Attachment #8563092 -
Attachment is obsolete: true
Assignee | ||
Comment 9•9 years ago
|
||
Sorry, I had marked the wrong patch as obsolete. Naturally the v01 patch failed the unit tests again. Need a "try" run of the v02 patch.
Flags: needinfo?(ryanvm)
Comment 10•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=474fd682d2b6
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8566589 [details] [diff] [review] v02-1132081-speed-up-ConvertHostARGBRow XPCShell tests passed, everything green except for two MacOS builds that evidently failed for some other reason. r?
Attachment #8566589 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 12•9 years ago
|
||
Patch v02 inserted some unnecessary leading zeros in hex constants.
Attachment #8566589 -
Attachment is obsolete: true
Attachment #8566589 -
Flags: review?(jmuizelaar)
Attachment #8567494 -
Flags: review?(jmuizelaar)
Updated•9 years ago
|
Attachment #8567494 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 13•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1c91e951034
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e1c91e951034
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•