Use preferred format to allocate memory for decoded images

Use preferred format to allocate memory for decoded images

We use fixed color format (kN32_SkColorType) to decode images. Since the default
preferred format is 16-bit color on low-end devices, we can reduce memory
footprint by storing decoded image in 16-bit color format when we use the
default settings for the preferred format. This CL makes
SoftwareImageDecodeController allocate memory based on the preferred format.

I ran memory.blink_memory_mobile 20 times. The results can be found at [1].
I removed SimulateMemoryPressureNotification() so that I can observe memory usage in
common patterns (still, memory pressure notifications would be sent if memory
usage is high). According to the results, we could reduce ~40% discardable
memory on average.

Note that Blink still uses fixed 32-bit color for decoding. This means that
skia/cc will convert decoded images when we store decoded image in 16-bit color.
We might want to make Blink's decoding format configurable in the future.

[1] https://drive.google.com/file/d/0B6NYyLPujP4TaWItc3lsZXR6TTg/view?usp=sharing

BUG=519146
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0c87a7ea398b77020298f09abe3fd946a0627603
Cr-Commit-Position: refs/heads/master@{#381859}

[bashi, 2016-03-16 05:41:16]

Description was changed from

==========
Use preferred format to allocate memory for decoded images

We use fixed color format (kN32_SkColorType) to decode images. Since the default
preferred format is 16-bit color on low-end devices, we can reduce memory
footprint by storing decoded image in 16-bit color format when we use the
default settings for the preferred format. This CL makes
SoftwareImageDecodeController allocate memory based on the preferred format.

I ran memory.blink_memory_mobile 20 times. The results can be found at [1].
I removed SimulateMemoryPressureNotification() so that I can observe memory
usage in
common patterns (still, memory pressure notifications would be sent if memory
usage is high). According to the results, we could reduce ~40% discardable
memory on average.

Note that Blink still uses fixed 32-bit color for decoding. This means that
skia/cc will convert decoded images when we store decoded image in 16-bit color.
We might want to make Blink's decoding format configurable in the future.

[1]
https://drive.google.com/file/d/0B6NYyLPujP4TaWItc3lsZXR6TTg/view?usp=sharing

BUG=519146
==========

to

==========
Use preferred format to allocate memory for decoded images

We use fixed color format (kN32_SkColorType) to decode images. Since the default
preferred format is 16-bit color on low-end devices, we can reduce memory
footprint by storing decoded image in 16-bit color format when we use the
default settings for the preferred format. This CL makes
SoftwareImageDecodeController allocate memory based on the preferred format.

I ran memory.blink_memory_mobile 20 times. The results can be found at [1].
I removed SimulateMemoryPressureNotification() so that I can observe memory
usage in
common patterns (still, memory pressure notifications would be sent if memory
usage is high). According to the results, we could reduce ~40% discardable
memory on average.

Note that Blink still uses fixed 32-bit color for decoding. This means that
skia/cc will convert decoded images when we store decoded image in 16-bit color.
We might want to make Blink's decoding format configurable in the future.

[1]
https://drive.google.com/file/d/0B6NYyLPujP4TaWItc3lsZXR6TTg/view?usp=sharing

BUG=519146
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[bashi, 2016-03-16 07:24:22]

bashi@chromium.org changed reviewers:
+ reveman@chromium.org

[bashi, 2016-03-16 07:24:22]

PTAL?

Though we might add one extra copy I think this would be worth doing. WDTY?

[haraken, 2016-03-16 14:07:34]

> According to the results, we could reduce ~40% discardable memory on average.

!!!

[reveman, 2016-03-16 14:53:36]

https://codereview.chromium.org/1808633002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1808633002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2149:
settings_.renderer_settings.preferred_tile_format));
What if preferred_tile_format is a compressed format? e.g. ETC1.
(--enable-tile-compression)

Maybe add an utility function to software_image_decode_controller.cc that
returns SkColorType to use for images based on a resource format instead of
using ResourceFormatToSkColorType directly.

[bashi, 2016-03-17 01:52:14]

Thanks for review!

https://codereview.chromium.org/1808633002/diff/1/cc/trees/layer_tree_host_im...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/1808633002/diff/1/cc/trees/layer_tree_host_im...
cc/trees/layer_tree_host_impl.cc:2149:
settings_.renderer_settings.preferred_tile_format));
On 2016/03/16 14:53:35, reveman wrote:
> What if preferred_tile_format is a compressed format? e.g. ETC1.
> (--enable-tile-compression)
> 
> Maybe add an utility function to software_image_decode_controller.cc that
> returns SkColorType to use for images based on a resource format instead of
> using ResourceFormatToSkColorType directly.

I didn't notice that. Done.

[reveman, 2016-03-17 04:31:14]

reveman@chromium.org changed reviewers:
+ reed@chromium.org, vmpstr@chromium.org

[reveman, 2016-03-17 04:31:14]

+reed to verify that using these SkColorTypes for images is well supported by
skia
+vmpstr fyi

lgtm % reed's review

[reed1, 2016-03-17 14:38:22]

reed@google.com changed reviewers:
+ reed@google.com

[reed1, 2016-03-17 14:38:23]

lgtm

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.cc:122: case LUMINANCE_8:
kGray_8_SkColorType

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.cc:125: case LUMINANCE_F16:
BTW -- not hard for Skia to add native support for LUM_F16, if you think that is
common/important.

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.cc:136: kPremul_SkAlphaType);
FYI -- Skia is actively working on native support for kUnpremul for src images
(not surfaces), so hopefully this year we can relax this constraint.

[bashi, 2016-03-17 23:29:38]

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.cc:122: case LUMINANCE_8:
On 2016/03/17 14:38:23, reed1 wrote:
> kGray_8_SkColorType

Done.

https://codereview.chromium.org/1808633002/diff/20001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.cc:125: case LUMINANCE_F16:
On 2016/03/17 14:38:23, reed1 wrote:
> BTW -- not hard for Skia to add native support for LUM_F16, if you think that
is
> common/important.

I'm not sure but I guess that LUM_16 is not common.

[bashi, 2016-03-17 23:31:39]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-03-17 23:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, reed@google.com
Link to the patchset: https://codereview.chromium.org/1808633002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-03-17 23:32:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1808633002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1808633002/40001

[commit-bot: I haz the power, 2016-03-18 01:05:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-18 01:07:30]

Description was changed from

==========
Use preferred format to allocate memory for decoded images

We use fixed color format (kN32_SkColorType) to decode images. Since the default
preferred format is 16-bit color on low-end devices, we can reduce memory
footprint by storing decoded image in 16-bit color format when we use the
default settings for the preferred format. This CL makes
SoftwareImageDecodeController allocate memory based on the preferred format.

I ran memory.blink_memory_mobile 20 times. The results can be found at [1].
I removed SimulateMemoryPressureNotification() so that I can observe memory
usage in
common patterns (still, memory pressure notifications would be sent if memory
usage is high). According to the results, we could reduce ~40% discardable
memory on average.

Note that Blink still uses fixed 32-bit color for decoding. This means that
skia/cc will convert decoded images when we store decoded image in 16-bit color.
We might want to make Blink's decoding format configurable in the future.

[1]
https://drive.google.com/file/d/0B6NYyLPujP4TaWItc3lsZXR6TTg/view?usp=sharing

BUG=519146
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Use preferred format to allocate memory for decoded images

We use fixed color format (kN32_SkColorType) to decode images. Since the default
preferred format is 16-bit color on low-end devices, we can reduce memory
footprint by storing decoded image in 16-bit color format when we use the
default settings for the preferred format. This CL makes
SoftwareImageDecodeController allocate memory based on the preferred format.

I ran memory.blink_memory_mobile 20 times. The results can be found at [1].
I removed SimulateMemoryPressureNotification() so that I can observe memory
usage in
common patterns (still, memory pressure notifications would be sent if memory
usage is high). According to the results, we could reduce ~40% discardable
memory on average.

Note that Blink still uses fixed 32-bit color for decoding. This means that
skia/cc will convert decoded images when we store decoded image in 16-bit color.
We might want to make Blink's decoding format configurable in the future.

[1]
https://drive.google.com/file/d/0B6NYyLPujP4TaWItc3lsZXR6TTg/view?usp=sharing

BUG=519146
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/0c87a7ea398b77020298f09abe3fd946a0627603
Cr-Commit-Position: refs/heads/master@{#381859}
==========

[commit-bot: I haz the power, 2016-03-18 01:07:31]

Patchset 3 (id:??) landed as
https://crrev.com/0c87a7ea398b77020298f09abe3fd946a0627603
Cr-Commit-Position: refs/heads/master@{#381859}

[vmpstr, 2016-03-21 20:00:45]

https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
File cc/tiles/software_image_decode_controller.h (right):

https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
cc/tiles/software_image_decode_controller.h:99: explicit
SoftwareImageDecodeController(ResourceFormat format = RGBA_8888);
Default parameters are prohibited in Chromium.

[bashi, 2016-03-21 23:42:35]

On 2016/03/21 20:00:45, vmpstr wrote:
>
https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
> File cc/tiles/software_image_decode_controller.h (right):
> 
>
https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
> cc/tiles/software_image_decode_controller.h:99: explicit
> SoftwareImageDecodeController(ResourceFormat format = RGBA_8888);
> Default parameters are prohibited in Chromium.

Thanks for fixing the style!

[vmpstr, 2016-03-21 23:59:07]

On 2016/03/21 23:42:35, bashi1 wrote:
> On 2016/03/21 20:00:45, vmpstr wrote:
> >
>
https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
> > File cc/tiles/software_image_decode_controller.h (right):
> > 
> >
>
https://codereview.chromium.org/1808633002/diff/40001/cc/tiles/software_image...
> > cc/tiles/software_image_decode_controller.h:99: explicit
> > SoftwareImageDecodeController(ResourceFormat format = RGBA_8888);
> > Default parameters are prohibited in Chromium.
> 
> Thanks for fixing the style!

:) It's not a big deal, I was just surprised to find that. Thanks for the patch!