Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1886233006:
Add 8888 fast SrcOver mode. (Closed)
DescriptionAdd 8888 fast SrcOver mode.
The SkLinearBitmapPipeline_tile change is an edge case fix.
This will not function until gDefaultProfileSRGB in SkImageInfo is set to true.
The srcover implementation is not the fastest, and I'm working on a better version.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1886233006
Committed: https://skia.googlesource.com/skia/+/871a0484305e9c051e2d78b62e1f2a5fa4bdb3a4
Patch Set 1 #Patch Set 2 : Good. #Patch Set 3 : WIP: before squash. #Patch Set 4 : Working alpha 8888. #Patch Set 5 : sync #Patch Set 6 : more sync #Patch Set 7 : revert even more. #Patch Set 8 : revret more. #Patch Set 9 : Force whitespace change. #Patch Set 10 : WIP: for weekend. #Patch Set 11 : Working. #Patch Set 12 : Clean CL #
Total comments: 7
Patch Set 13 : Use SkOpts entry point. #Patch Set 14 : Fix includes. #Patch Set 15 : Remove srgb curve functions. #
Total comments: 1
Patch Set 16 : Addresss mtk's review. #
Messages
Total messages: 42 (13 generated)
Description was changed from ========== Add 8888 fast SrcOver mode. BUG=skia: ========== to ========== Add 8888 fast SrcOver mode. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Good.
WIP: before squash.
Working alpha 8888.
sync
more sync
revert even more.
revret more.
Force whitespace change.
WIP: for weekend.
Working.
Clean CL
Description was changed from ========== Add 8888 fast SrcOver mode. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add 8888 fast SrcOver mode. The SkLinearBitmapPipeline_tile change is an edge case fix. This will not function until gDefaultProfileSRGB in SkImageInfo is set to true. The srcover implementation is not the fastest, and I'm working on a better version. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
herb@google.com changed reviewers: + fmalita@chromium.org
The CQ bit was checked by herb@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/1886233006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886233006/220001
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-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) 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...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by herb@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/1886233006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886233006/220001
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-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) 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...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...) Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
The CQ bit was checked by herb@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/1886233006/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886233006/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:640: dstPixel *= 255.0f; Can we avoid the 255 divs/mult and lerp in 255-space instead? I think the 255 factor cancels out: invA = 1 - (As / 255); R = 255 * sqrt((Rs/255)^2 + (Rd/255)^2 * invA) => R = 255 * sqrt((Rs^2 + Rd^2 * invA)/255^2) => R = sqrt(Rs^2 + Rd^2 * invA)
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:629: void lerp_pixel(const uint32_t* src, uint32_t* dst) { Let's not call this lerp. Lerp is a function of three arguments. This function is called srcover. We almost always write functions like this with dst as the first argument. (That's how memcpy works, it's how the registers are named, there's always a dst but not necessarily a src, it's our convention, ...) Probably no need to pass these things by pointer unless you're looping like below. uint32_t srcover(uint32_t dst, uint32_t src) should work out just the same. https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:630: Sk4b srcAsBytes = Sk4b::Load(src); I think this code would be more readable with fewer named intermediate values, shorter names, and more visual parallelism to match the logical parallelism. Sk4f s = ToLinear((1/255.0f) * SkNx_cast<float>(Sk4b::Load(src))), d = ToLinear((1/255.0f) * SkNx_cast<float>(Sk4b::Load(dst))); d = s + d*(1 - s[3]); SkNx_cast<uint8_t>(255 * ToSRGB(d)).store(dst); https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:632: srcPixel = sRGBFast::sRGBToLinear(srcPixel / 255.0f); Try not to write "x / 255.0f". "x *(1/255.0f)" is faster. (Parentheses matter. "x * 1/255.0f" is even worse than "x / 255.0f". ) https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:640: dstPixel *= 255.0f; On 2016/04/27 at 15:51:17, f(malita) wrote: > Can we avoid the 255 divs/mult and lerp in 255-space instead? I think the 255 factor cancels out: > > invA = 1 - (As / 255); > > R = 255 * sqrt((Rs/255)^2 + (Rd/255)^2 * invA) > => R = 255 * sqrt((Rs^2 + Rd^2 * invA)/255^2) > => R = sqrt(Rs^2 + Rd^2 * invA) neat. https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:655: const uint8_t* bPixel = reinterpret_cast<const uint8_t*>(pixel); return *pixel >> 24; ? https://codereview.chromium.org/1886233006/diff/220001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:758: lerp_pixel(src, fDest); It might make things globally simpler to fuse lerp_pixel() into lerp_pixels() and just call lerp_pixels(src, fDest, 1) here.
Use SkOpts entry point.
Fix includes.
Remove srgb curve functions.
PTAL. I've switch over to using the SkOpt entry point.
lgtm https://codereview.chromium.org/1886233006/diff/280001/src/core/SkLinearBitma... File src/core/SkLinearBitmapPipeline.cpp (right): https://codereview.chromium.org/1886233006/diff/280001/src/core/SkLinearBitma... src/core/SkLinearBitmapPipeline.cpp:675: (SkOpts::srcover_srgb_srgb)(fDest, beginSpan, span.count() * repeatCount, span.count()); SkOpts::srcover_srgb_srgb(fDest, beginSpan, span.count() * repeatCount, span.count()) ?
lgtm
Addresss mtk's review.
The CQ bit was checked by herb@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com, fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1886233006/#ps300001 (title: "Addresss mtk's review.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886233006/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886233006/300001
Message was sent while issue was closed.
Description was changed from ========== Add 8888 fast SrcOver mode. The SkLinearBitmapPipeline_tile change is an edge case fix. This will not function until gDefaultProfileSRGB in SkImageInfo is set to true. The srcover implementation is not the fastest, and I'm working on a better version. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add 8888 fast SrcOver mode. The SkLinearBitmapPipeline_tile change is an edge case fix. This will not function until gDefaultProfileSRGB in SkImageInfo is set to true. The srcover implementation is not the fastest, and I'm working on a better version. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/871a0484305e9c051e2d78b62e1f2a5fa4bdb3a4 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://skia.googlesource.com/skia/+/871a0484305e9c051e2d78b62e1f2a5fa4bdb3a4 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
