always scale to an integer; compensate for fractional image sizes by leaving the fractional scale i…

always scale to an integer; compensate for fractional image sizes by leaving the fractional scale in the matrix

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/ce7b1d5fadda5e0fd5a40257d4c8e1b6a0279597

[humper, 2014-08-14 16:16:27]

PTAL; this approach is more robust than the previous one and appears artifact
free in my tests.

[reed1, 2014-08-14 18:59:10]

do we need the fAdjusted bool? Seems like we use it *after* we have re-sniffed
the matrix.

[reed1, 2014-08-14 18:59:41]

do we already have a gm that exercises this path, so we can see before/after?

[reed1, 2014-08-14 19:00:02]

On 2014/08/14 18:59:41, reed1 wrote:
> do we already have a gm that exercises this path, so we can see before/after?

Ah, I guess its all those things you have to ignore :)

[humper, 2014-08-15 11:37:12]

I think we need something like that flag; if we leave fractional scaling
components in the matrix, then we don't detect later that the matrix is
"trivial", which causes us to do a postIDiv with the width/height of the image;
this results in wildly wrong characteristics.

Basically we were using the triviality of the matrix as a proxy for "we messed
with the matrix, leave it alone please", which is really something that should
be in a semantic flag.

[reed1, 2014-08-15 14:40:39]

Hmmm, you're saying that sometimes we need to respect just-translate in the
matrix, and sometimes we should ignore them?

If that's true, is it because (for back compatibility) we have this "feature"
that we ignore that in Low quality?

If so, perhaps a clearer type of flag could be (set at the top of chooseProc)

fIgnoreFractionTranslate = (quality <= kLow);   // until we decide to slow down
a little and always respect this

Then initial requests for HQ would naturally still respect any leftover
fractional translate...

[humper, 2014-08-22 16:51:09]

Sorry this got derailed; I was pretty sick this week.

I'm saying that what we *used* to do was call possiblyScaleImage, and then if
the resulting matrix was translateOnly we would leave it alone.

Now possiblyScaleImage can potentially leave a little bit of scaling behind, so
we needed a more explicit flag that says "I messed with the matrix, I know what
I'm doing, please  just use the one I computed, thanks".

On 2014/08/15 14:40:39, reed1 wrote:
> Hmmm, you're saying that sometimes we need to respect just-translate in the
> matrix, and sometimes we should ignore them?
> 
> If that's true, is it because (for back compatibility) we have this "feature"
> that we ignore that in Low quality?
> 
> If so, perhaps a clearer type of flag could be (set at the top of chooseProc)
> 
> fIgnoreFractionTranslate = (quality <= kLow);   // until we decide to slow
down
> a little and always respect this
> 
> Then initial requests for HQ would naturally still respect any leftover
> fractional translate...

[humper, 2014-08-25 18:42:24]

Latest patch uses the rounded integral image size for the cache key, which
should result in more cache hits if we have an image that's scaled multiple
times to very similar sizes.

Improves the (recently submitted) fractional image benchmark by quite a lot for
high quality scaling (25ms to 14ms on my mac).

[reed1, 2014-08-25 18:48:13]

who defines the IGNORE_PROPER flag? Should that be defined in
skia_for_chromium.gypi?

lgtm otherwise

[humper, 2014-08-25 20:24:13]

Yeah, that'll need to get defined for the next roll; then jbroman can undefine
it as part of his
blink-layout-test-rebaseline step (tomorrow, presumably).

I'll notify the sherriff.

On 2014/08/25 18:48:13, reed1 wrote:
> who defines the IGNORE_PROPER flag? Should that be defined in
> skia_for_chromium.gypi?
> 
> lgtm otherwise

[humper, 2014-08-25 20:27:04]

On 2014/08/25 20:24:13, humper wrote:
> Yeah, that'll need to get defined for the next roll; then jbroman can undefine
> it as part of his
> blink-layout-test-rebaseline step (tomorrow, presumably).
> 
> I'll notify the sherriff.
> 
> On 2014/08/25 18:48:13, reed1 wrote:
> > who defines the IGNORE_PROPER flag? Should that be defined in
> > skia_for_chromium.gypi?
> > 
> > lgtm otherwise

Added the flag myself; talked to sherriff about it.

[humper, 2014-08-25 20:27:10]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-08-25 20:27:26]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/humper@google.com/470233002/60001

[commit-bot: I haz the power, 2014-08-25 20:35:06]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug...)
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[commit-bot: I haz the power, 2014-08-25 20:39:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-25 20:39:47]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Release-Shared-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-GCE-NoGPU-x86_64-Relea...)

[humper, 2014-08-27 14:45:25]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-08-27 14:45:52]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/humper@google.com/470233002/80001

[commit-bot: I haz the power, 2014-08-27 14:45:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-27 14:45:57]

Failed to apply patch for expectations/gm/ignored-tests.txt:
While running git apply --index -p1;
  error: patch failed: expectations/gm/ignored-tests.txt:33
  error: expectations/gm/ignored-tests.txt: patch does not apply

Patch:       expectations/gm/ignored-tests.txt
Index: expectations/gm/ignored-tests.txt
diff --git a/expectations/gm/ignored-tests.txt
b/expectations/gm/ignored-tests.txt
index
5c8817cd6c2c894b3c0cebfa24e6fbfe7a5d6bab..58bc8955c84dfde64e1f0c5e7f2867d6a11a8f88
100644
--- a/expectations/gm/ignored-tests.txt
+++ b/expectations/gm/ignored-tests.txt
@@ -33,3 +33,25 @@
 ## epoger will rebaseline by 25 Dec 2013
 #gradtext
 
+# To be rebaselined (fmalita)
+textblob
+
+# reed
+matrixconvolution
+
+#humper: https://codereview.chromium.org/470233002/
+downsamplebitmap_checkerboard_high_512_256
+downsamplebitmap_image_high_mandrill_512.png
+downsamplebitmap_text_high_72.00pt
+filterbitmap_checkerboard_192_192
+filterbitmap_checkerboard_32_2
+filterbitmap_checkerboard_32_32
+filterbitmap_checkerboard_32_8
+filterbitmap_checkerboard_4_4
+filterbitmap_image_mandrill_128.png
+filterbitmap_image_mandrill_16.png
+filterbitmap_image_mandrill_256.png
+filterbitmap_image_mandrill_32.png
+filterbitmap_image_mandrill_64.png
+filterbitmap_text_3.00pt
+filterbitmap_text_7.00pt

[humper, 2014-08-27 14:48:37]

The CQ bit was checked by humper@google.com

[commit-bot: I haz the power, 2014-08-27 14:48:53]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/humper@google.com/470233002/100001

[commit-bot: I haz the power, 2014-08-27 14:56:56]

Committed patchset #6 (100001) as ce7b1d5fadda5e0fd5a40257d4c8e1b6a0279597

[humper, 2014-08-27 23:57:59]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/509183002/ by humper@google.com.

The reason for reverting is: Patch seems to have broken image scaling replay
tests on one android platform and chromeos.

http://108.170.220.21:10117/builders/Test-Android-Xoom-Tegra2-Arm7-Release/bu....