Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2330053002:
Encode kIndex8 to PNG more efficiently (Closed)
DescriptionEncode kIndex8 to PNG more efficiently
Only write non-opaque alpha values to the tRNS chunk.
BUG=skia:5616
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002
Committed: https://skia.googlesource.com/skia/+/fed033491ca3e9ae80fd02bb77b08ce425f4f4b4
Patch Set 1 #
Total comments: 4
Patch Set 2 : Skip loop if all colors are non-opaque, add back in SK_RESTRICT #
Total comments: 3
Patch Set 3 : Rebase #Patch Set 4 : Fix loop #Messages
Total messages: 23 (14 generated)
Description was changed from ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia: ========== to ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002 ==========
msarett@google.com changed reviewers: + reed@google.com, scroggo@google.com
lgtm Is there an associated bug? https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (left): https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:114: png_color* SK_RESTRICT palette, Why did you remove SK_RESTRICT? https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:149: // We have already written the non-opaque colors. Now just write the opaque colors. I think you could skip this if numWithAlpha == count. I don't know how often that is the case (probably rare?), and maybe it wouldn't make a difference in speed.
Description was changed from ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002 ========== to ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002 ==========
"Is there an associated bug?" Adding the bug related to recent image encoder work. Though this change is really just something I noticed and decided to improve. https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (left): https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:114: png_color* SK_RESTRICT palette, On 2016/09/12 16:40:57, scroggo wrote: > Why did you remove SK_RESTRICT? SK_RESTRICT can sometimes allow the compiler to make optimizations by guaranteeing that only this pointer will be used to access particular memory. My personal preference is to *not* use it unless we've actually demonstrated that it improves performance - or there's a reason to think that the compiler might need it (since I think it clutters the code). But now that I think about it, it's probably equally bad for me to remove it without measuring anything. Adding the SK_RESTRICTS back in. https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2330053002/diff/1/src/images/SkPNGImageEncode... src/images/SkPNGImageEncoder.cpp:149: // We have already written the non-opaque colors. Now just write the opaque colors. On 2016/09/12 16:40:57, scroggo wrote: > I think you could skip this if numWithAlpha == count. I don't know how often > that is the case (probably rare?), and maybe it wouldn't make a difference in > speed. Yes nice catch, let's skip it.
This does seem like one of the scenarios where restrict may be helpful. It looks like we've got a color table, a PNG color array, and a PNG byte array. If those are uint32_t*, uin32_t*, and uint8_t* (or similar) the compiler may need to defensively assume any of them can alias any of the others. The rule is generally, like pointer types may alias, and so can byte pointers. If there were say, a uint16_t*, a uint32_t*, and a uint64_t* (all unlike types), restrict wouldn't add anything that the compiler couldn't already assume.
https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:152: while (i < count && currIndex != count) { I don't think you need to check currIndex on each iteration through the loop. If you do, I think you've made a mistake somewhere above here. I was thinking something like else if (numWithAlpha < count) { Maybe you want to put some asserts in here?
https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:152: while (i < count && currIndex != count) { On 2016/09/12 17:12:48, scroggo wrote: > I don't think you need to check currIndex on each iteration through the loop. If > you do, I think you've made a mistake somewhere above here. I was thinking > something like > > else if (numWithAlpha < count) { > > Maybe you want to put some asserts in here? Yeah I wrote that at first. I think the advantage of this approach is that we will exit the loop early once we hit the last opaque alpha. I'd guess which is "better" depends on the input. I don't feel strongly.
lgtm https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... File src/images/SkPNGImageEncoder.cpp (right): https://codereview.chromium.org/2330053002/diff/20001/src/images/SkPNGImageEn... src/images/SkPNGImageEncoder.cpp:152: while (i < count && currIndex != count) { On 2016/09/12 17:17:13, msarett wrote: > On 2016/09/12 17:12:48, scroggo wrote: > > I don't think you need to check currIndex on each iteration through the loop. > If > > you do, I think you've made a mistake somewhere above here. I was thinking > > something like > > > > else if (numWithAlpha < count) { > > > > Maybe you want to put some asserts in here? > > Yeah I wrote that at first. I think the advantage of this approach is that we > will exit the loop early once we hit the last opaque alpha. > > I'd guess which is "better" depends on the input. I don't feel strongly. Ah, so this would let us exit early if for example the input had all the non-opaque colors at the end. Either way is fine by me.
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/2330053002/#ps60001 (title: "Fix loop")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002 ========== to ========== Encode kIndex8 to PNG more efficiently Only write non-opaque alpha values to the tRNS chunk. BUG=skia:5616 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2330053002 Committed: https://skia.googlesource.com/skia/+/fed033491ca3e9ae80fd02bb77b08ce425f4f4b4 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/fed033491ca3e9ae80fd02bb77b08ce425f4f4b4 |
