Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1697423002:
Use geometric mean when selecting a mipmap scale (Closed)
DescriptionUse geometric mean when selecting a mipmap scale
Workaround for anisotropic mipmap quality issues.
R=reed@google.com,robertphillips@google.com
BUG=skia:4863, chromium:586894
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1697423002
Committed: https://skia.googlesource.com/skia/+/d10f5b3ac90322071b40e62fb585644ddd767223
Patch Set 1 #
Total comments: 1
Patch Set 2 : use scale average #
Total comments: 1
Patch Set 3 : geometric mean #
Total comments: 2
Patch Set 4 : don't change default behavior #Patch Set 5 : #Messages
Total messages: 33 (15 generated)
Description was changed from ========== Avoid mipmaps for extremely anisotropic scales R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 ========== to ========== Avoid mipmaps for extremely anisotropic scales R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Avoid mipmaps for extremely anisotropic scales R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Don't use mipmaps with extremely anisotropic scales R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Don't use mipmaps with extremely anisotropic scales R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Don't use mipmaps with extremely anisotropic scales Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697423002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697423002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1697423002/diff/1/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1697423002/diff/1/src/core/SkMipMap.cpp#newco... src/core/SkMipMap.cpp:332: if (SkScalarAbs(scaleSize.width() / scaleSize.height() - 1) > kMaxScaleRatio) { - The minus-1 is a little confusing I would have thought it would look like this ratio = width / height; const kTooBig = 4.0f; const kTooSmall = 1 / kTooBig; if (ratio > kTooBig || ratio < kTooSmall) { return false; }
On 2016/02/16 16:41:31, reed1 wrote: > https://codereview.chromium.org/1697423002/diff/1/src/core/SkMipMap.cpp > File src/core/SkMipMap.cpp (right): > > https://codereview.chromium.org/1697423002/diff/1/src/core/SkMipMap.cpp#newco... > src/core/SkMipMap.cpp:332: if (SkScalarAbs(scaleSize.width() / > scaleSize.height() - 1) > kMaxScaleRatio) { > - The minus-1 is a little confusing Yup, that's not symmetrical, good catch. Per your suggestion on http://code.google.com/p/skia/issues/detail?id=4863, I'm also looking for a better cpu-only scale heuristic. Will update.
Description was changed from ========== Don't use mipmaps with extremely anisotropic scales Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use scale average when selecting mip levels Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
On 2016/02/16 16:50:50, f(malita) wrote: > Per your suggestion on http://code.google.com/p/skia/issues/detail?id=4863, I'm > also looking for a better cpu-only scale heuristic. Will update. Updated to use the scale average, WDYT? https://codereview.chromium.org/1697423002/diff/20001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1697423002/diff/20001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:340: if (scale >= SK_Scalar1 || scale <= 0 || !SkScalarIsFinite(scale)) { Q: what should we do when the average > 1? We're currently degrading to kLow, which may be OK. Alternatives: a) clamp to 1 and continue using kMedium b) don't drop from kHigh to kMed in this case (means making the HQ controller aware of this heuristic)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697423002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697423002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Use scale average when selecting mip levels Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use geometric mean when selecting a mipmap scale Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697423002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697423002/40001
Per conversation, updated to use geometric mean. Works (at least) as well as prev version for all tests I've looked at. PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1697423002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1697423002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:330: #ifdef SK_SUPPORT_LEGACY_ANISOTROPIC_MIPMAP_SCALE Written this way, we are going to change skia's default to be sqrt, different from ganesh. I had thought we were adding this as an option for chrome only, which they would opt into... Did you mean to change skia across the board?
https://codereview.chromium.org/1697423002/diff/40001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1697423002/diff/40001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:330: #ifdef SK_SUPPORT_LEGACY_ANISOTROPIC_MIPMAP_SCALE On 2016/02/16 21:37:56, reed1 wrote: > Written this way, we are going to change skia's default to be sqrt, different > from ganesh. I had thought we were adding this as an option for chrome only, > which they would opt into... Did you mean to change skia across the board? Indeed - I did not consider applying this to Chrome only. As a workaround, that makes sense. Done.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697423002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1697423002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1697423002/80001
Message was sent while issue was closed.
Description was changed from ========== Use geometric mean when selecting a mipmap scale Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Use geometric mean when selecting a mipmap scale Workaround for anisotropic mipmap quality issues. R=reed@google.com,robertphillips@google.com BUG=skia:4863,chromium:586894 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/d10f5b3ac90322071b40e62fb585644ddd767223 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/d10f5b3ac90322071b40e62fb585644ddd767223 |
