New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 739559 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 737981



Sign in to add a comment

color: load_dropbox memory regression investigation

Project Member Reported by ccameron@chromium.org, Jul 6 2017

Issue description

Separating this out from  issue 737981  to avoid creating too much noise.

The particular metric I'm looking at in this bug in memory:chrome:all_processes:reported_by_chrome:malloc:effective_size.

On my local Pixel 2106, the regression of X is from 38.9M to 50.9M.
 
Blocking: 737981
If we eliminate the ColorSpaceXformCanvas in RasterSource::PlaybackToCanvas, then the regression entirely disappears.

If we also ensure that all input images be sRGB and that the target of the ColorSpaceXformCanvas be sRGB, the regression still remains.

This is somewhat suspicious, because the ColorSpaceXformCanvas should, at this point, be a no-op, but it's using way more memory.
Cc: brianosman@chromium.org reed@chromium.org mtklein@chromium.org
This regression is coming from how we manage SkImage_Lazy::onMakeColorSpace (see  issue 729352  for some background there).

What we should probably do here is:
1. make SkImage_Lazy::onMakeColorSpace wrap the initial SkImage_Lazy, with SkColorSpace
2. pass that SkColorSpace to the call to generate_pixels in SkImage_Lazy.cpp
3. ensure that we use the useXformCanvas path in SkPictureImageGenerator::onGetPixels

This an be accomplished by setting an SkColorSpace on SkPixmap created in SkImage_Lazy::lockAsBitmap, which will then be pulled out by SkPictureImageGenerator::onGetPixels. This can be dangerous, though, because usually setting SkImageInfo::fColorSpace is non-legacy behavior, and we want to ensure legacy behavior here.

It appears that both SkPictureImageGenerator::onGetPixels and DecodingImageGenerator::onGetPixels behave the "way we ant them to" in this instance.
To be a bit more succinct about the problem, the issue is that "SkImage_Lazy::onMakeColorSpace" should return a still-lazy SkImage, rather than evaluating the lazy image (and doing color conversion) at that call.
I took a stab at fixing this in https://skia-review.googlesource.com/c/21605/ ... but I didn't get finished with all of SkImage_Lazy::lockTextureProxy -- it's probably worth having Skia folk take a quick look at the direction there to ensure it's not already off the rails.

Comment 5 by reed@google.com, Jul 6 2017

Cc: fmalita@chromium.org
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 10 2017

The following revision refers to this bug:
  https://skia.googlesource.com/skia/+/77e966647f6b495fe44772086c709528c711fc6e

commit 77e966647f6b495fe44772086c709528c711fc6e
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Jul 10 17:45:28 2017

Make SkImage_Lazy::onMakeColorSpace return a SkImage_Lazy

Make SkImage_Lazy::onMakeColorSpace return a new SkImage_Lazy
with the color space of SkImage_Lazy::fInfo changed.

Update the call to SkImageGenerator::getPixels to specify
image info with this new color space.

Update the call to SkImageGenerator::generateTexture to specify
this new color space. Update SkPictureImageGenerator to respect
the color space argument. Add a SkTransferFunctionBehavior
argument to SkImageGenerator::generateTexture to indicate if
color conversion is to be doing using an xform canvas.

Update Generator_GrYUVProvider::refAsTextureProxy to include a
color conversion step to respect this new color space.

TBR=reed@google.com
Bug:739559
Change-Id: I156a858884659e9dfae739a653bab2ef89274959
Reviewed-on: https://skia-review.googlesource.com/21605
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Christopher Cameron <ccameron@chromium.org>
Reviewed-by: Christopher Cameron <ccameron@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/core/SkPictureImageGenerator.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrBackendTextureImageGenerator.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/core/SkColorSpaceXformImageGenerator.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrYUVProvider.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/core/SkColorSpaceXformImageGenerator.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrYUVProvider.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrAHardwareBufferImageGenerator.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrAHardwareBufferImageGenerator.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/include/core/SkImageGenerator.h
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/image/SkImage_Lazy.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/gpu/GrBackendTextureImageGenerator.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/gm/image_pict.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/core/SkPictureImageGenerator.cpp
[modify] https://crrev.com/77e966647f6b495fe44772086c709528c711fc6e/src/core/SkImageGenerator.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jul 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b66fb23e3749385c06d3ef36a21acb4d9c56c41c

commit b66fb23e3749385c06d3ef36a21acb4d9c56c41c
Author: Christopher Cameron <ccameron@chromium.org>
Date: Mon Jul 10 23:36:01 2017

Suppress layout test failures from SkImage_Lazy::onMakeColorSpace change

These will be rebaselined after the roll.

TBR=fmatila

Bug:  739559 
Change-Id: I08c06b60ba9a8d2e1a9dc973c29d89b9a054ba83
Reviewed-on: https://chromium-review.googlesource.com/565905
Reviewed-by: Florin Malita <fmalita@chromium.org>
Commit-Queue: ccameron chromium <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#485444}
[modify] https://crrev.com/b66fb23e3749385c06d3ef36a21acb4d9c56c41c/third_party/WebKit/LayoutTests/TestExpectations

Status: Fixed (was: Assigned)
Project Member

Comment 9 by bugdroid1@chromium.org, Aug 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78cb52909f06fe32e6a71da39f229941e0fba697

commit 78cb52909f06fe32e6a71da39f229941e0fba697
Author: Stephen Chenney <schenney@chromium.org>
Date: Thu Aug 10 16:47:39 2017

Rebaseline some tests after a Skia image handling change

TBR=ccameron@chromium.org
BUG= 739559 

Change-Id: If97b031899e86ae9e3eb7a3acf83b0333541762a
Reviewed-on: https://chromium-review.googlesource.com/606950
Commit-Queue: Stephen Chenney <schenney@chromium.org>
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493429}
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/color-profile-background-image-space-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/color-profile-mask-image-svg-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/color-profile-svg-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/color-profile-svg-fill-text-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/cross-fade-background-size-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/linux/virtual/gpu-rasterization/images/cross-fade-overflow-position-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/mac-mac10.10/virtual/gpu-rasterization/images/color-profile-svg-fill-text-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/mac-mac10.9/virtual/gpu-rasterization/images/color-profile-svg-fill-text-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/color-profile-background-image-space-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/color-profile-mask-image-svg-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/color-profile-svg-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/color-profile-svg-fill-text-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/cross-fade-background-size-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/mac/virtual/gpu-rasterization/images/cross-fade-overflow-position-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/color-profile-background-image-space-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/color-profile-mask-image-svg-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/color-profile-svg-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/color-profile-svg-fill-text-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/cross-fade-background-size-expected.png
[delete] https://crrev.com/873308e4edb600ffd92096253f12e22f2759a05c/third_party/WebKit/LayoutTests/platform/win/virtual/gpu-rasterization/images/cross-fade-overflow-position-expected.png
[add] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/color-profile-background-image-space-expected.png
[add] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/color-profile-mask-image-svg-expected.png
[add] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/color-profile-svg-expected.png
[add] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/cross-fade-overflow-position-expected.png
[modify] https://crrev.com/78cb52909f06fe32e6a71da39f229941e0fba697/third_party/WebKit/LayoutTests/virtual/gpu-rasterization/images/cross-fade-tiled-expected.png

Cc: ccameron@chromium.org
Owner: schenney@chromium.org

Sign in to add a comment