Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1592473002:
use triangle filter for odd dimensions in mip-levels (Closed)
Descriptionuse triangle filter for odd dimensions in mip-levels
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1592473002
Committed: https://skia.googlesource.com/skia/+/32e0b4a34a2d461927056677e0ef99241e29df0d
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : add guard for chrome #Patch Set 6 : #
Total comments: 8
Patch Set 7 : address naming comments #Messages
Total messages: 23 (10 generated)
Description was changed from ========== special mipbuilder for odd width/heights BUG=skia: ========== to ========== special mipbuilder for odd width/heights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== special mipbuilder for odd width/heights BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== use triangle filter for odd dimensions in mip-levels BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by reed@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/1592473002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592473002/100001
reed@google.com changed reviewers: + fmalita@chromium.org, mtklein@google.com
chrome already has the guard. landing this should allow us to continue tweaking perf... - push inner loop into proc and/or expose it via templates - try using SkNx to speed up expand/compact steps
reed@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:391: auto c10 = F::Expand(p0[1]); Is it just me or would these read more easily as ... auto c01 = F::Expand(p0[1]); auto c10 = F::Expand(p1[0]); ... i.e. make cXY names match pX[Y] (in all 4 functions). https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:426: *(uint32_t*)dst = F::Compact(c >> 3); typename F::Type* https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:464: SkDownSampleProc* proc2x2 = nullptr; Can you just move your typedef inside the function and call it Proc? https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:474: proc2x2 = downsample_2x2<ColorTypeFilter_8888>; This may seem anal, but I think this section of code would be easier to read if the procs were named proc22 proc23 proc32 proc33 the xs keep tripping me up when reading
ptal https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:391: auto c10 = F::Expand(p0[1]); On 2016/01/15 20:11:22, mtklein wrote: > Is it just me or would these read more easily as > > ... > auto c01 = F::Expand(p0[1]); > auto c10 = F::Expand(p1[0]); > ... > > i.e. make cXY names match pX[Y] (in all 4 functions). Done. https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:426: *(uint32_t*)dst = F::Compact(c >> 3); On 2016/01/15 20:11:22, mtklein wrote: > typename F::Type* Done. https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:464: SkDownSampleProc* proc2x2 = nullptr; On 2016/01/15 20:11:22, mtklein wrote: > Can you just move your typedef inside the function and call it Proc? Done. https://codereview.chromium.org/1592473002/diff/100001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:474: proc2x2 = downsample_2x2<ColorTypeFilter_8888>; On 2016/01/15 20:11:22, mtklein wrote: > This may seem anal, but I think this section of code would be easier to read if > the procs were named > > proc22 > proc23 > proc32 > proc33 > > the xs keep tripping me up when reading Hmmm, not sure about this one. I guess its ok either way, but we'd lose the "x" in both the variable and the function name. proc32 = downsample_32 Not sure that won't make someone think we're talking about 32bits as opposed to 3x2 as dimensions. Will try proc_3_2
The CQ bit was checked by reed@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/1592473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592473002/120001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM On 2016/01/15 19:32:08, reed1 wrote: > landing this should allow us to continue tweaking perf... > - push inner loop into proc and/or expose it via templates > - try using SkNx to speed up expand/compact steps - sliding window optimization for the 3x procs?
On 2016/01/15 21:11:26, f(malita) wrote: > LGTM > > On 2016/01/15 19:32:08, reed1 wrote: > > landing this should allow us to continue tweaking perf... > > - push inner loop into proc and/or expose it via templates > > - try using SkNx to speed up expand/compact steps > > - sliding window optimization for the 3x procs? Yes, that's my "push inner loop" comment
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/1592473002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1592473002/120001
Message was sent while issue was closed.
Description was changed from ========== use triangle filter for odd dimensions in mip-levels BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== use triangle filter for odd dimensions in mip-levels 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/+/32e0b4a34a2d461927056677e0ef99241e29df0d ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/32e0b4a34a2d461927056677e0ef99241e29df0d |
