Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1707883002:
lcd blits for sRGB (Closed)
Descriptionlcd blits for sRGB
Unimplemented for F16 for the moment, but not needed yet.
Surprisingly not much slower than current impl (not srgb correct)
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1707883002
Committed: https://skia.googlesource.com/skia/+/4528c867c4ba65fdf5cfd6de35ea4c36a16c73f6
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 17
Patch Set 5 : #Patch Set 6 : #Patch Set 7 : more clarity on names/docs for new flags #
Total comments: 1
Messages
Total messages: 24 (11 generated)
Description was changed from ========== SkOSFile_stdio: less :: It's not clear why we wrote this file in this manner. On OpenBSD, fileno is a macro, so ::fileno makes no sense. Also remove some end-of-line whitespace. BUG=skia:4953 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Review URL: https://codereview.chromium.org/1695333003 ========== to ========== lcd blits for sRGB ==========
Description was changed from ========== lcd blits for sRGB ========== to ========== lcd blits for sRGB GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== lcd blits for sRGB GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== lcd blits for sRGB Unimplemented for F16 for the moment, but not needed yet. Surprisingly not much slower than current impl (not srgb correct) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
reed@google.com changed reviewers: + fmalita@chromium.org, herb@google.com, mtklein@google.com
ptal
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707883002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707883002/40001
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-02-18 09:22 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: 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-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 reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707883002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707883002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode... include/core/SkXfermode.h:254: kDstIsLinearInt_LCDFlag = 1 << 2, // srgb or float LinearInt // srgb or float? What's Int mean here? Maybe just kDstIsLinear? https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.cpp File src/core/SkXfermode4f.cpp (right): https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:430: Sk4i rgbi = Sk4i(SkGetPackedR16(rgb), SkGetPackedG16(rgb)>>1, SkGetPackedB16(rgb), 0); Why throw away a green bit? Sk4i rgbi = Sk4i(SkGetPackedR16(rgb), SkGetPackedG16(rgb), SkGetPackedB16(rgb), 0); return SkNx_cast<float>(rgbi) * Sk4f(1.0f/31, 1.0f/63, 1.0f/31, 0); https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:440: const Sk4f& s4bias = s4 * Sk4f(255); Seems odd (wrong?) to declare this as a const&. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:442: const unsigned rgb = lcd[i]; Somewhat weird to use unsigned here but pass it as uint16_t to lcd16_to_uint_4f. I'd go with uint16_t everywhere... https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:451: const unsigned rgb = lcd[i]; (here too) https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:464: const unsigned rgb = lcd[i]; No 0 check? https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:477: for (int i = 0; i < count; ++i) { Hmm... Compilers _hate_ him! One weird trick to speed up your loop by a sqrt! https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:495: for (int i = 0; i < count; ++i) { (ditto)
https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode... include/core/SkXfermode.h:254: kDstIsLinearInt_LCDFlag = 1 << 2, // srgb or float On 2016/02/18 13:35:55, mtklein wrote: > LinearInt // srgb or float? > > What's Int mean here? Maybe just kDstIsLinear? Yea, incomplete comment. fixed. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.cpp File src/core/SkXfermode4f.cpp (right): https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:430: Sk4i rgbi = Sk4i(SkGetPackedR16(rgb), SkGetPackedG16(rgb)>>1, SkGetPackedB16(rgb), 0); On 2016/02/18 13:35:55, mtklein wrote: > Why throw away a green bit? > > Sk4i rgbi = Sk4i(SkGetPackedR16(rgb), SkGetPackedG16(rgb), > SkGetPackedB16(rgb), 0); > return SkNx_cast<float>(rgbi) * Sk4f(1.0f/31, 1.0f/63, 1.0f/31, 0); Was wondering if a single value for the scale was cheaper, and worth the lost bit in green. We can try it both ways. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:440: const Sk4f& s4bias = s4 * Sk4f(255); On 2016/02/18 13:35:55, mtklein wrote: > Seems odd (wrong?) to declare this as a const&. Yes, copy/paste error. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:442: const unsigned rgb = lcd[i]; On 2016/02/18 13:35:55, mtklein wrote: > Somewhat weird to use unsigned here but pass it as uint16_t to lcd16_to_uint_4f. > I'd go with uint16_t everywhere... Done. old habit of not wanting the register to be smaller than natural https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:451: const unsigned rgb = lcd[i]; On 2016/02/18 13:35:55, mtklein wrote: > (here too) Done. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:464: const unsigned rgb = lcd[i]; On 2016/02/18 13:35:56, mtklein wrote: > No 0 check? Done. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:477: for (int i = 0; i < count; ++i) { On 2016/02/18 13:35:55, mtklein wrote: > Hmm... > > Compilers _hate_ him! > One weird trick to speed up your loop by a sqrt! CLEARLY I haven't tested the N case yet :) fixed. https://codereview.chromium.org/1707883002/diff/60001/src/core/SkXfermode4f.c... src/core/SkXfermode4f.cpp:495: for (int i = 0; i < count; ++i) { On 2016/02/18 13:35:55, mtklein wrote: > (ditto) Done.
lgtm but deferring to Mike/Herb. https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1707883002/diff/60001/include/core/SkXfermode... include/core/SkXfermode.h:254: kDstIsLinearInt_LCDFlag = 1 << 2, // srgb or float On 2016/02/18 15:11:35, reed1 wrote: > On 2016/02/18 13:35:55, mtklein wrote: > > LinearInt // srgb or float? > > > > What's Int mean here? Maybe just kDstIsLinear? > > Yea, incomplete comment. fixed. I take it "Int" here stands for interpolation? kDstIsLinear_LCDFlag seems less confusing to me too.
The else for kLinearInt is either sRGB or half-float. Since half-float is "linear", I wanted to clarify the enum value more.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org Link to the patchset: https://codereview.chromium.org/1707883002/#ps120001 (title: "more clarity on names/docs for new flags")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707883002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707883002/120001
lgtm https://codereview.chromium.org/1707883002/diff/120001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1707883002/diff/120001/include/core/SkXfermod... include/core/SkXfermode.h:254: kDstIsLinearInt_LCDFlag = 1 << 2, // else srgb/half-float Ah, IsLinearInt -> u32 == 8888, u64 = 16161616 !IsLinearInt -> u32 == sRGB, u64 = hhhh ? Guess this is so confusing because 3 of the destination options are int, 3 of the options are linear.
Message was sent while issue was closed.
Description was changed from ========== lcd blits for sRGB Unimplemented for F16 for the moment, but not needed yet. Surprisingly not much slower than current impl (not srgb correct) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== lcd blits for sRGB Unimplemented for F16 for the moment, but not needed yet. Surprisingly not much slower than current impl (not srgb correct) GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/4528c867c4ba65fdf5cfd6de35ea4c36a16c73f6 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/4528c867c4ba65fdf5cfd6de35ea4c36a16c73f6 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
