Set maximum output size for scaled-image-cache images

Set maximum output size for scaled-image-cache images

Accessable via:
  SkScaledImageCache::{G,S}etMaximumOutputSizeForHighQualityFilter

Also, a unit test.

BUG=389439
Committed: https://skia.googlesource.com/skia/+/805ef159d197007b9529e60e5b918ee0f9d3802d

[hal.canary, 2014-07-15 21:33:47]

PTAL.

Chrome will need to set a value for the maximum size for scaling images.  Does
it need to be runtime instead?

[reveman, 2014-07-15 22:45:13]

Hm, it's a bit hard to reason about why we wouldn't want to cache really large
images from chrome's pov as we can let the cache grow very large when using
discardable memory. Avoiding the use of high quality filter for really large
images might make more sense from chrome's pov. It would make sense independent
of if we can fit it in the cache or not.

[reed1, 2014-07-16 14:16:43]

#2 -- this CL in in direct response to the bug you have commented on. The
requested image-size in that bug is 256M, which is 1/2 of your stated maximum
discardable limit. Do you have an alternate suggestion to address the problem?

[reveman, 2014-07-16 15:13:00]

On 2014/07/16 14:16:43, reed1 wrote:
> #2 -- this CL in in direct response to the bug you have commented on. The
> requested image-size in that bug is 256M, which is 1/2 of your stated maximum
> discardable limit. Do you have an alternate suggestion to address the problem?

The result of this patch looks good. I'm just not sure about the name of the
define, SK_MAXIMUM_ALLOCATION_SIZE_FOR_IMAGE_CACHE. From chrome's pov there's
nothing wrong with trying to cache a 300MB object. If you're going to reuse it,
then trying to cache it is probably a good idea instead of always re-creating
it. This case is different as we have an alternative approach for rendering this
image that doesn't require any cache usage. I think
SK_MAXIMUM_ALLOCATION_SIZE_FOR_HIGH_QUALITY_FILTER or maybe
SK_MAXIMUM_OUTPUT_SIZE_FOR_HIGH_QUALITY_FILTER would make more sense.

We need this asap so if current patch is what you prefer, don't let this hold
you back. The essence of this patch is good and it solves the problem. We can
solve possible naming issues in a follow up.

[hal.canary, 2014-07-16 15:16:54]

I'm working on a runtime switch, since reed@ suggested that (1) we could write a
unit test for it and (2) You may want to manage this budget differently on
different devices.  Does that sound good?

[reveman, 2014-07-16 15:35:37]

On 2014/07/16 15:16:54, Hal Canary wrote:
> I'm working on a runtime switch, since reed@ suggested that (1) we could write
a
> unit test for it and (2) You may want to manage this budget differently on
> different devices.  Does that sound good?

sgtm

[hal.canary, 2014-07-16 17:13:18]

On 2014/07/16 15:35:37, reveman wrote:
> On 2014/07/16 15:16:54, Hal Canary wrote:
> > I'm working on a runtime switch, since reed@ suggested that (1) we could
write
> a
> > unit test for it and (2) You may want to manage this budget differently on
> > different devices.  Does that sound good?
> 
> sgtm

please take a look.

[reed1, 2014-07-16 18:38:00]

after we're don'e bike-shedding on the name, we will need to bubble the new
setter/getter up to SkGraphics.h

https://codereview.chromium.org/394003003/diff/20001/src/core/SkScaledImageCa...
File src/core/SkScaledImageCache.h (right):

https://codereview.chromium.org/394003003/diff/20001/src/core/SkScaledImageCa...
src/core/SkScaledImageCache.h:67: static void
SetMaximumOutputSizeForHighQualityFilter(size_t);
What if we rename SetByteLimit to SetTotalByteLimit, and name the new one
SetSingleByteLimit?

I think the name should be more inline with the other names, i.e. about
allocation requests, and less about input-vs-output etc.

[hal.canary, 2014-07-16 20:24:11]

On 2014/07/16 18:38:00, reed1 wrote:
> after we're don'e bike-shedding on the name, we will need to bubble the new
> setter/getter up to SkGraphics.h

Done.

>
https://codereview.chromium.org/394003003/diff/20001/src/core/SkScaledImageCa...
> File src/core/SkScaledImageCache.h (right):
> 
>
https://codereview.chromium.org/394003003/diff/20001/src/core/SkScaledImageCa...
> src/core/SkScaledImageCache.h:67: static void
> SetMaximumOutputSizeForHighQualityFilter(size_t);
> What if we rename SetByteLimit to SetTotalByteLimit, and name the new one
> SetSingleByteLimit?
> 
> I think the name should be more inline with the other names, i.e. about
> allocation requests, and less about input-vs-output etc.

I propose the following:

    SkGraphics::GetImageCacheByteLimit()
    SkGraphics::GetImageCacheSingleAllocationByteLimit()

I don't want to change the existing names right now; let's keep this simple.

[reveman, 2014-07-16 20:38:04]

https://codereview.chromium.org/394003003/diff/60001/include/core/SkGraphics.h
File include/core/SkGraphics.h (right):

https://codereview.chromium.org/394003003/diff/60001/include/core/SkGraphics....
include/core/SkGraphics.h:97: static size_t
SetImageCacheSingleAllocationByteLimit(size_t newLimit);
I know this only affects pre-scaling of images and the comment also makes it
clear. The name of the function itself doesn't make this clear though.
SetImageCacheSingleAllocationByteLimit sounds like it might affect more than
pre-scaling. For example, decoded images and I wouldn't want to set this limit
in chrome if that was the case.

Maybe we just need some general comment making it clear that all these
"ImageCache" functions control caching of pre-scaled images.

Not a big deal, just wanted to point it out. As long as this does what we need,
I'm OK with it.

[reed1, 2014-07-16 20:46:03]

If you really don't want to rename the old field in this CL, please add a
comment above it (TODO) we can do that next. Its minor, but I think the presence
of this new field makes the old one slightly less clear, and our goal in all CLs
should be to leave the code at least as readable as how we found it.

api lgtm

[hal.canary, 2014-07-16 21:10:58]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-16 21:11:41]

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

[commit-bot: I haz the power, 2014-07-16 21:33:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[commit-bot: I haz the power, 2014-07-16 21:36:11]

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

[commit-bot: I haz the power, 2014-07-16 21:36:11]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

[hal.canary, 2014-07-16 21:47:45]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-16 21:48:38]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/394003003/120001

[commit-bot: I haz the power, 2014-07-16 22:16:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Mac10.8-Clang-x86-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86-Release-Trybot/b...)
  Build-Mac10.8-Clang-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Mac10.8-Clang-x86_64-Release-Trybo...)
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)
  Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Android-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-Arm7-Debug-Andr...)
  Build-Ubuntu13.10-GCC4.8-x86_64-Release-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-GCC4.8-x86_64-Release-...)
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)
  Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.120:10117/builders/Test-Ubuntu13.10-ShuttleA-NoGPU-x86_64-...)

[commit-bot: I haz the power, 2014-07-16 22:19:22]

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

[commit-bot: I haz the power, 2014-07-16 22:19:23]

Try jobs failed on following builders:
  Build-Ubuntu13.10-Clang-x86_64-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Ubuntu13.10-Clang-x86_64-Debug-Try...)

[hal.canary, 2014-07-16 23:28:12]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-16 23:28:25]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/394003003/140001

[commit-bot: I haz the power, 2014-07-16 23:46:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[commit-bot: I haz the power, 2014-07-16 23:57:47]

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

[commit-bot: I haz the power, 2014-07-16 23:57:48]

Try jobs failed on following builders:
  Build-Win7-VS2010-x86-Debug-Trybot on tryserver.skia
(http://108.170.220.76:10117/builders/Build-Win7-VS2010-x86-Debug-Trybot/build...)

[hal.canary, 2014-07-17 01:49:37]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-17 01:50:20]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/394003003/160001

[commit-bot: I haz the power, 2014-07-17 02:10:26]

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

[commit-bot: I haz the power, 2014-07-17 02:10:27]

Failed to commit the patch.

[hal.canary, 2014-07-17 12:31:06]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-07-17 12:32:07]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/394003003/160001

[commit-bot: I haz the power, 2014-07-17 13:58:10]

Change committed as 805ef159d197007b9529e60e5b918ee0f9d3802d