Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1668033002:
don't get dismayed by negative scales for HQ (Closed)
Descriptiondon't get dismayed by negative scales for HQ
needs https://codereview.chromium.org/1663793003/ to land first
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1668033002
Committed: https://skia.googlesource.com/skia/+/a3d99a515bb379b60c38189de435b47bdc8dd528
Patch Set 1 #
Total comments: 1
Patch Set 2 : add guard #
Total comments: 1
Messages
Total messages: 24 (11 generated)
Description was changed from ========== don't get dismayed by negative scales for HQ BUG=skia: ========== to ========== don't get dismayed by negative scales for HQ 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/1668033002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668033002/1
reed@google.com changed reviewers: + fmalita@chromium.org, vmpstr@google.com
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
lgtm https://codereview.chromium.org/1668033002/diff/1/src/core/SkBitmapController... File src/core/SkBitmapController.cpp (right): https://codereview.chromium.org/1668033002/diff/1/src/core/SkBitmapController... src/core/SkBitmapController.cpp:101: invScaleX = SkScalarAbs(invScaleX); This is the only bit that requires some thought for me. I think this is OK, since we won't undo the flip as a result (ie the matrix would retain the fact that the image is flipped).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== don't get dismayed by negative scales for HQ BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== don't get dismayed by negative scales for HQ needs https://codereview.chromium.org/1663793003/ to land first 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/1668033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668033002/20001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by reed@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1668033002/#ps20001 (title: "add guard")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1668033002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1668033002/20001
Message was sent while issue was closed.
Description was changed from ========== don't get dismayed by negative scales for HQ needs https://codereview.chromium.org/1663793003/ to land first BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== don't get dismayed by negative scales for HQ needs https://codereview.chromium.org/1663793003/ to land first 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/+/a3d99a515bb379b60c38189de435b47bdc8dd528 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/a3d99a515bb379b60c38189de435b47bdc8dd528
Message was sent while issue was closed.
https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... File src/core/SkBitmapController.cpp (right): https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... src/core/SkBitmapController.cpp:68: SkScalar invScaleSqr = invMat.getScaleX() * invMat.getScaleY(); I forgot to mention this, but there's another small issue here with rotations. If we have a rotated image (but otherwise unscaled), then these scales are not correct. It again doesn't really affect much since it only changes the perceived amount that the decode requires, but maybe we can rework this and reuse the scales we get in processHQRequest instead.
Message was sent while issue was closed.
On 2016/02/04 17:56:07, vmpstr wrote: > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > File src/core/SkBitmapController.cpp (right): > > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > src/core/SkBitmapController.cpp:68: SkScalar invScaleSqr = invMat.getScaleX() * > invMat.getScaleY(); > I forgot to mention this, but there's another small issue here with rotations. > If we have a rotated image (but otherwise unscaled), then these scales are not > correct. It again doesn't really affect much since it only changes the perceived > amount that the decode requires, but maybe we can rework this and reuse the > scales we get in processHQRequest instead. The scales from decomposeScale are wrong? Can you show an example? I tried this test, and always got 1,1 for the scales... for (int i = 0; i < 360; i += 5) { SkSize scale; SkMatrix m; m.setRotate(i, 0, 0); m.decomposeScale(&scale); SkDebugf("rotate %d scale [%g %g]\n", i, scale.width(), scale.height()); }
Message was sent while issue was closed.
On 2016/02/04 19:38:58, reed1 wrote: > On 2016/02/04 17:56:07, vmpstr wrote: > > > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > > File src/core/SkBitmapController.cpp (right): > > > > > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > > src/core/SkBitmapController.cpp:68: SkScalar invScaleSqr = invMat.getScaleX() > * > > invMat.getScaleY(); > > I forgot to mention this, but there's another small issue here with rotations. > > If we have a rotated image (but otherwise unscaled), then these scales are not > > correct. It again doesn't really affect much since it only changes the > perceived > > amount that the decode requires, but maybe we can rework this and reuse the > > scales we get in processHQRequest instead. > > The scales from decomposeScale are wrong? Can you show an example? I tried this > test, and always got 1,1 for the scales... > > for (int i = 0; i < 360; i += 5) { > SkSize scale; > SkMatrix m; > m.setRotate(i, 0, 0); > m.decomposeScale(&scale); > SkDebugf("rotate %d scale [%g %g]\n", i, scale.width(), scale.height()); > } Nono, decompose scale gives us the correct answer. However, we're not using decompose scale in cache_size_ok, we're just getting the getScaleX() and getScaleY() components of the matrix. My proposal is to maybe reuse the decompose scale that we get instead.
Message was sent while issue was closed.
On 2016/02/04 20:01:31, vmpstr wrote: > On 2016/02/04 19:38:58, reed1 wrote: > > On 2016/02/04 17:56:07, vmpstr wrote: > > > > > > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > > > File src/core/SkBitmapController.cpp (right): > > > > > > > > > https://codereview.chromium.org/1668033002/diff/20001/src/core/SkBitmapContro... > > > src/core/SkBitmapController.cpp:68: SkScalar invScaleSqr = > invMat.getScaleX() > > * > > > invMat.getScaleY(); > > > I forgot to mention this, but there's another small issue here with > rotations. > > > If we have a rotated image (but otherwise unscaled), then these scales are > not > > > correct. It again doesn't really affect much since it only changes the > > perceived > > > amount that the decode requires, but maybe we can rework this and reuse the > > > scales we get in processHQRequest instead. > > > > The scales from decomposeScale are wrong? Can you show an example? I tried > this > > test, and always got 1,1 for the scales... > > > > for (int i = 0; i < 360; i += 5) { > > SkSize scale; > > SkMatrix m; > > m.setRotate(i, 0, 0); > > m.decomposeScale(&scale); > > SkDebugf("rotate %d scale [%g %g]\n", i, scale.width(), > scale.height()); > > } > > Nono, decompose scale gives us the correct answer. However, we're not using > decompose scale in cache_size_ok, we're just getting the getScaleX() and > getScaleY() components of the matrix. My proposal is to maybe reuse the > decompose scale that we get instead. Ah, gotcha. I agree we should use the same logic to compute the width/height in both places. Will fix. |
