Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1831353002:
Add mipmap procs for F16 format. (Closed)
|
Created:
4 years, 9 months ago by Brian Osman Modified:
4 years, 8 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd mipmap procs for F16 format.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1831353002
Committed: https://skia.googlesource.com/skia/+/1817d282cda17cb8c2db0ac6fdc937743c026016
Patch Set 1 #Patch Set 2 : Switch to divide rather than shift... Still broken #Patch Set 3 : Fix floating point math via helper utilities #
Total comments: 7
Patch Set 4 : Address Mike's feedback #Messages
Total messages: 20 (7 generated)
Description was changed from ========== Add mipmap procs for F16 format. BUG=skia: ========== to ========== Add mipmap procs for F16 format. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
brianosman@google.com changed reviewers: + fmalita@chromium.org, herb@google.com, reed@google.com
I started writing these the other day while working on mip-maps. Are people interested in having this code?
The CQ bit was checked by brianosman@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/1831353002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831353002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fmalita@chromium.org changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:88: template <typename T> T shift_right_or_divide(const T& x, int bits) { I think the shift mostly gets in the way at this point. I would rewrite the callers as F::Compact(c/2) [c/4, c*2 etc] and just assume the compiler is going to use strength reduction. Then I presume we wouldn't need these templates?
https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:88: template <typename T> T shift_right_or_divide(const T& x, int bits) { On 2016/04/05 at 20:42:00, f(malita) wrote: > I think the shift mostly gets in the way at this point. I would rewrite the callers as F::Compact(c/2) [c/4, c*2 etc] and just assume the compiler is going to use strength reduction. Then I presume we wouldn't need these templates? Hrm, sadly, no. This would work but for the Sk4b/Sk4h code. Integer SIMD doesn't generally have divide instructions, so the compiler isn't necessarily even equipped to think about what it means to divide and when it can degrade down to shift, though GCC and Clang may be, given they _do_ "support" integer division with their vector extensions. (Division serializes to idiv.) I've intentionally left operator/ out of our integer SkNx types to just avoid all this mess. https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:92: template <> float shift_right_or_divide<float>(const float& x, int bits) { This is fine, but you can also just write static float shift_right_or_divide(float x, int bits) The specific overload will take precedence over the general template. https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:93: return x / (1 << bits); Probably best to write x * (1.0f / (1<<bits)), which should compile into load-constant, multiply. The compiler can constant-propagate bits, but it can't generally strength reduce float division into float multiplication. https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:128: d[i] = F::Compact(shift_right_or_divide(c, 1)); Given that these shift_right_or_divide() and shift_left_or_multiply() are written in shift's terms, I'd just name them shift_right() and shift_left() (or shr/shl). The divide and multiply are just how we do it.
https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:75: typedef SkHalf Type; Are you sure this code works correctly? The other structs' Type typedefs correspond to a full pixel. This here corresponds to a single channel. Seems like we'd only mip a quarter of the image, wrong. I think this is fixed ~trivially by typedef uint64_t Type; // SkHalf x4 static Sk4f Expand(uint64_t x) { return SkHalfToFloat_01(x); } static uint64_t Compact(const Sk4f& x) { return SkFloatToHalf_01(x); }
https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:88: template <typename T> T shift_right_or_divide(const T& x, int bits) { On 2016/04/05 20:58:36, mtklein wrote: > On 2016/04/05 at 20:42:00, f(malita) wrote: > > I think the shift mostly gets in the way at this point. I would rewrite the > callers as F::Compact(c/2) [c/4, c*2 etc] and just assume the compiler is going > to use strength reduction. Then I presume we wouldn't need these templates? > > Hrm, sadly, no. This would work but for the Sk4b/Sk4h code. > > Integer SIMD doesn't generally have divide instructions, so the compiler isn't > necessarily even equipped to think about what it means to divide and when it can > degrade down to shift, though GCC and Clang may be, given they _do_ "support" > integer division with their vector extensions. (Division serializes to idiv.) > > I've intentionally left operator/ out of our integer SkNx types to just avoid > all this mess. Ack.
On 2016/04/05 21:18:08, f(malita) wrote: > https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp > File src/core/SkMipMap.cpp (right): > > https://codereview.chromium.org/1831353002/diff/40001/src/core/SkMipMap.cpp#n... > src/core/SkMipMap.cpp:88: template <typename T> T shift_right_or_divide(const T& > x, int bits) { > On 2016/04/05 20:58:36, mtklein wrote: > > On 2016/04/05 at 20:42:00, f(malita) wrote: > > > I think the shift mostly gets in the way at this point. I would rewrite the > > callers as F::Compact(c/2) [c/4, c*2 etc] and just assume the compiler is > going > > to use strength reduction. Then I presume we wouldn't need these templates? > > > > Hrm, sadly, no. This would work but for the Sk4b/Sk4h code. > > > > Integer SIMD doesn't generally have divide instructions, so the compiler isn't > > necessarily even equipped to think about what it means to divide and when it > can > > degrade down to shift, though GCC and Clang may be, given they _do_ "support" > > integer division with their vector extensions. (Division serializes to idiv.) > > > > > I've intentionally left operator/ out of our integer SkNx types to just avoid > > all this mess. > > Ack. Yeah - my first attempt was to do divide everywhere, and I ran into compile errors. My own investigation led me to realize much of what Mike just said, which led to this (ugly) situation.
New version to address Mike's feedback. Good catch on the incorrect size/type - I think I was looking at the Gray8 case right above it when I wrote the code. Incidentally, N32 is the only version that's tested in unit tests - I can try adding a better unit test before landing.
lgtm
lgtm
The CQ bit was checked by brianosman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1831353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1831353002/60001
Message was sent while issue was closed.
Description was changed from ========== Add mipmap procs for F16 format. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Add mipmap procs for F16 format. 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/+/1817d282cda17cb8c2db0ac6fdc937743c026016 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/1817d282cda17cb8c2db0ac6fdc937743c026016 |
