Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1563183003:
Refactor resize filter to go faster (Closed)
DescriptionRefactor resize filter to go faster
Part of this CL improves the speed by using dynamic arrays
more effectively.
Part uses SIMD and more concise float expressions for speed.
Some unused code was deleted.
The latter changes are guarded by:
SK_SUPPORT_LEGACY_BITMAP_FILTER
until this lands and the corresponding layout changes in
chrome can be relanded.
With the legacy flag defined, no Skia or Chrome test results
change. Without the flag defined in Skia, only 0.01% - 0.02%
of the pixels change, and then by (1,1,1) in 8888.
codereview.chromium.org/1583533002 adds the guard to Chrome.
R=reed@google.com
BUG=skia:2261
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1563183003
Committed: https://skia.googlesource.com/skia/+/368342ccb5b88568974f66d1a36bbec667efcc4e
Patch Set 1 #Patch Set 2 : wip; 1st cut at sk4f #Patch Set 3 : wip; use simd loop #Patch Set 4 : wip; make mitchell filter use constants #Patch Set 5 : wip; speed up AddFilter #Patch Set 6 : use stack for temporary arrays #Patch Set 7 : wip; add legacy codepath back and test #Patch Set 8 : fix bugs in legacy #Patch Set 9 : remove define to test legacy #
Total comments: 2
Patch Set 10 : make mitchell filter locals conform to skia style #
Messages
Total messages: 49 (23 generated)
Description was changed from ========== wip; refactor resize filter to go faster First step at optimizing the high quality resize; improve the filter generation. R=reed@google.com BUG=skia:2261 ========== to ========== wip; refactor resize filter to go faster First step at optimizing the high quality resize; improve the filter generation. R=reed@google.com BUG=skia:2261 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/1
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 caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/40001
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 caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
caryclark@google.com changed reviewers: + mtklein@google.com
Please take a look This causes a max of 0.02 (1,1,1) diff on Gold; it is only slightly faster on nanobench. The fixes: - remove float/int int/float conversions - hoist inner loop things out - use int instead of short for locals - simplify polynomal evaluation - use SIMD Because it is not noticeably faster, and requires rebaselining, I'm unsure if this is worth checking in.
Does the bench you're running not match the screw case in the bug, or is this code really not much faster? In the first case, maybe we write a new bench before deciding? In the second, punt!
On 2016/01/08 16:30:21, mtklein wrote: > Does the bench you're running not match the screw case in the bug, or is this > code really not much faster? > > In the first case, maybe we write a new bench before deciding? In the second, > punt! Oddly enough, we landed https://codereview.chromium.org/183763047/, so there should be heavy pressure on the filter evaluation code. I'm not sure why Cary isn't observing any. Mike Reed is reported to have seen some odd mipmapping - are we going down unexpected codepaths? And although after Cary's stepping through the code I can see that the filters we're computing differ at every pixel, I still don't understand *why*, and agree with Humper's intuition that they ought to be uniform in the interior of the region.
On 2016/01/08 21:18:32, tomhudson wrote:
> And although after Cary's stepping through the code I can see that the filters
> we're computing differ at every pixel, I still don't understand *why*, and
agree
> with Humper's intuition that they ought to be uniform in the interior of the
> region.
Aha, the culprit line is probably:
float srcPixel = (static_cast<float>(destSubsetI) + 0.5f) * invScale;
A noninteger inverse scale will give us a partial-pixel offset in the filter.
I'm not convinced the math is right, though.
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/80001
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 caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/100001
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 caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/120001
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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== wip; refactor resize filter to go faster First step at optimizing the high quality resize; improve the filter generation. R=reed@google.com BUG=skia:2261 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Refactor resize filter to go faster Part of this CL improves the speed by using dynamic arrays more effectively. Part uses SIMD and more concise float expressions for speed. Some unused code was deleted. The latter changes are guarded by: SK_SUPPORT_LEGACY_BITMAP_FILTER until this lands and the corresponding layout changes in chrome can be relanded. With the legacy flag defined, no Skia or Chrome test results change. Without the flag defined in Skia, only 0.01% - 0.02% of the pixels change, and then by (1,1,1) in 8888. codereview.chromium.org/1583533002 adds the guard to Chrome. R=reed@google.com BUG=skia:2261 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by caryclark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/160001
(commit ready)
reed@google.com changed reviewers: + fmalita@chromium.org
lgtm w/ question about static-vs-method https://codereview.chromium.org/1563183003/diff/160001/src/core/SkBitmapFilter.h File src/core/SkBitmapFilter.h (right): https://codereview.chromium.org/1563183003/diff/160001/src/core/SkBitmapFilte... src/core/SkBitmapFilter.h:144: Sk4f evalcore_n(const Sk4f& val) const { can this method be static? i.e. does it use any state from the filter?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1563183003/diff/160001/src/core/SkBitmapFilter.h File src/core/SkBitmapFilter.h (right): https://codereview.chromium.org/1563183003/diff/160001/src/core/SkBitmapFilte... src/core/SkBitmapFilter.h:144: Sk4f evalcore_n(const Sk4f& val) const { On 2016/01/12 18:21:04, reed1 wrote: > can this method be static? i.e. does it use any state from the filter? Good catch. I changed the locals to use Skia style to make it easier to tell that evalcore_n does use local state.
The CQ bit was checked by caryclark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/1563183003/#ps180001 (title: "make mitchell filter locals conform to skia style")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1563183003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1563183003/180001
Message was sent while issue was closed.
Description was changed from ========== Refactor resize filter to go faster Part of this CL improves the speed by using dynamic arrays more effectively. Part uses SIMD and more concise float expressions for speed. Some unused code was deleted. The latter changes are guarded by: SK_SUPPORT_LEGACY_BITMAP_FILTER until this lands and the corresponding layout changes in chrome can be relanded. With the legacy flag defined, no Skia or Chrome test results change. Without the flag defined in Skia, only 0.01% - 0.02% of the pixels change, and then by (1,1,1) in 8888. codereview.chromium.org/1583533002 adds the guard to Chrome. R=reed@google.com BUG=skia:2261 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Refactor resize filter to go faster Part of this CL improves the speed by using dynamic arrays more effectively. Part uses SIMD and more concise float expressions for speed. Some unused code was deleted. The latter changes are guarded by: SK_SUPPORT_LEGACY_BITMAP_FILTER until this lands and the corresponding layout changes in chrome can be relanded. With the legacy flag defined, no Skia or Chrome test results change. Without the flag defined in Skia, only 0.01% - 0.02% of the pixels change, and then by (1,1,1) in 8888. codereview.chromium.org/1583533002 adds the guard to Chrome. R=reed@google.com BUG=skia:2261 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/368342ccb5b88568974f66d1a36bbec667efcc4e ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://skia.googlesource.com/skia/+/368342ccb5b88568974f66d1a36bbec667efcc4e |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
