New issue
Advanced search Search tips

Issue 902022 link

Starred by 1 user
Status: Fixed
Owner:
enne@chromium.org
Closed: Nov 14
Cc:
khushals...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Remove colorspace from DrawImage

Project Member Reported by enne@chromium.org, Nov 5 
In looking at profiles across time, one possibility of why GetDecodedImageForDraw is more expensive in m70 vs m59 is that colorspaces were added.  In looking at current profiles, getting the hash of a DrawImage is also expensive.

To address this, instead of having each DrawImage know its colorspace, associate the color space with the image decode cache itself.
Project Member

Comment 3 by 42576172...@developer.gserviceaccount.com, Nov 6 

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/1784cb4de40000

All of the runs failed. The most common error (20/20 runs) was:
BuildError: Build failed: BUILD_FAILURE
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Nov 6 

😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/10134239e40000

All of the runs failed. The most common error (20/20 runs) was:
BuildError: Build failed: BUILD_FAILURE
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 6 

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

commit e0c4b6ad08843f859e4b867a73ebccc3c6457aab
Author: Adrienne Walker <enne@chromium.org>
Date: Tue Nov 06 01:20:40 2018

Create per color space canvas image decode cache

Currently, cc image decode caches support multiple color spaces in the
same decode cache.  However, during rasterization there is only one
color space.  Additionally, each canvas that uses an image decode cache
in Blink also has only one color space (and there is only one canvas
active at once).

This patch is in support of that goal, by refactoring the canvas image
provider to use one image decode cache per color space (split in two
places because of provider dependency issues for gpu).  Once this is
done, then color space can be moved from cc::DrawImage to the image
decode cache.

Although there are now multiple active image decode caches, after
the use of each one they clean up and unlock all decodes via the
CanvasResourceProvider::CleanupLockedImages function.  This is posted
as a task, so it's possible that if there are multiple canvases
being rastered to with multiple color spaces that there may be a
higher watermark of memory usage before the cleanup tasks run.
This patch punts on this possibility for now.

Bug:  902022 
Change-Id: I2a079c4ae9383986dffd6939f7ab31a5a45ed196
Reviewed-on: https://chromium-review.googlesource.com/c/1315962
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605557}
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/content/renderer/webgraphicscontext3d_provider_impl.cc
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/content/renderer/webgraphicscontext3d_provider_impl.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/public/platform/web_graphics_context_3d_provider.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/canvas_color_params.cc
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/canvas_color_params.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/canvas_resource_provider.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/gpu/drawing_buffer_test_helpers.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/image.cc
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/image.h
[modify] https://crrev.com/e0c4b6ad08843f859e4b867a73ebccc3c6457aab/third_party/blink/renderer/platform/graphics/test/fake_web_graphics_context_3d_provider.h
Project Member

Comment 8 by 42576172...@developer.gserviceaccount.com, Nov 6 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16761ab1e40000
Project Member

Comment 9 by 42576172...@developer.gserviceaccount.com, Nov 6 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16e4e949e40000

Comment 10 by enne@chromium.org, Nov 6 

Canvas regressions from https://chromeperf.appspot.com/group_report?bug_id=839767 look unchanged.

It looks like leaves raster thread time got 5% faster, which is the case I had previously tried to bisect between m59 and m70.  

motionmark_html_composited_transforms_125 also got 7% slower (but the absolute values are 10x smaller than leaves raster time).  Even if this is true and not noise, I think the leaves improvement wins out.
Project Member

Comment 11 by bugdroid1@chromium.org, Nov 7 

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

commit 91d934793fa72bdbc09d3f4936fa597fa154f66b
Author: Adrienne Walker <enne@chromium.org>
Date: Wed Nov 07 02:11:17 2018

cc: Remove color space from DrawImage

Instead of storing and hashing the color space on every DrawImage,
instead store the color space on the image decode cache itself.

Bug:  902022 
Change-Id: Icac6e4702f21c2bd276e8941a15ba5d96a890f04
Reviewed-on: https://chromium-review.googlesource.com/c/1318734
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605928}
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/benchmarks/rasterize_and_record_benchmark_impl.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/layers/recording_source_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/paint/discardable_image_map_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/paint/draw_image.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/paint/draw_image.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/paint/oop_pixeltest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/raster/playback_image_provider.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/raster/playback_image_provider.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/raster/playback_image_provider_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/raster/raster_source_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/test/fake_tile_manager.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/checker_image_tracker.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/checker_image_tracker.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/checker_image_tracker_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/decoded_image_tracker.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/decoded_image_tracker.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/decoded_image_tracker_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/gpu_image_decode_cache.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/gpu_image_decode_cache.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/gpu_image_decode_cache_perftest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/gpu_image_decode_cache_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/image_controller_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache_perftest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache_unittest_combinations.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache_utils.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/software_image_decode_cache_utils.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/tile_manager.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/tile_manager.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/tiles/tile_manager_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/trees/layer_tree_host_impl.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/trees/layer_tree_host_impl.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/trees/layer_tree_host_unittest.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/cc/trees/layer_tree_host_unittest_context.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/content/renderer/webgraphicscontext3d_provider_impl.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/third_party/blink/renderer/platform/graphics/canvas_2d_layer_bridge_test.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/third_party/blink/renderer/platform/graphics/canvas_color_params.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/third_party/blink/renderer/platform/graphics/canvas_color_params.h
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/91d934793fa72bdbc09d3f4936fa597fa154f66b/third_party/blink/renderer/platform/graphics/image.cc
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 8 

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

commit 8ce42351caff17cc53827bfcbbab9fa50d44ef1b
Author: Daniel Bratell <bratell@opera.com>
Date: Thu Nov 08 10:51:27 2018

Unleak blink's internals from web_graphics_context_3d_provider.h

A change included an internal blink header in the public api
layer which then leaked parts of blink's internal out into the
wild dangerous world outside. This is a quick fix to close that
leak. Might need some tuning.

Bug:  902022 
Change-Id: I076f4c5d0a27cd4c3070d8fe8a5be97f2f65e1e0
Reviewed-on: https://chromium-review.googlesource.com/c/1323715
Commit-Queue: Daniel Bratell <bratell@opera.com>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606409}
[modify] https://crrev.com/8ce42351caff17cc53827bfcbbab9fa50d44ef1b/content/renderer/webgraphicscontext3d_provider_impl.cc
[modify] https://crrev.com/8ce42351caff17cc53827bfcbbab9fa50d44ef1b/third_party/blink/public/platform/web_graphics_context_3d_provider.h
[modify] https://crrev.com/8ce42351caff17cc53827bfcbbab9fa50d44ef1b/third_party/blink/renderer/platform/graphics/canvas_color_params.cc
[modify] https://crrev.com/8ce42351caff17cc53827bfcbbab9fa50d44ef1b/third_party/blink/renderer/platform/graphics/canvas_color_params.h

Comment 13 by enne@chromium.org, Nov 14

Status: Fixed (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 15 

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

commit e0cd20e8caf2b90933e61bd511819312cebacd16
Author: Adrienne Walker <enne@chromium.org>
Date: Thu Nov 15 05:32:44 2018

cc: Fix flaky ImageController crash

If non-lazy tasks are queued but never executed, it would still try to
unref them, causing a crash.  Add some tests to make sure that lazy and
non-lazy images get unref'd (or not) appropriately and reenable the
test.

Bug:  902644 , 902022 ,905239
Change-Id: I778d81aa390c1a529b7bfd88a2e146f245fb35e5
Reviewed-on: https://chromium-review.googlesource.com/c/1324874
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608271}
[modify] https://crrev.com/e0cd20e8caf2b90933e61bd511819312cebacd16/cc/tiles/image_controller.cc
[modify] https://crrev.com/e0cd20e8caf2b90933e61bd511819312cebacd16/cc/tiles/image_controller_unittest.cc
[modify] https://crrev.com/e0cd20e8caf2b90933e61bd511819312cebacd16/cc/trees/layer_tree_host_unittest.cc
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 16 

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

commit ee29a5b80ca55270322f4de7fb423f20596305a7
Author: Adrienne Walker <enne@chromium.org>
Date: Fri Nov 16 19:59:12 2018

Move CanvasColorParams out of public

This reverts this change:
https://chromium-review.googlesource.com/c/chromium/src/+/1323715

And changes the WebGraphicsContext3dProvider implementation to not
depend on Blink internals by using Skia types.

Bug:  902022 
Change-Id: I1fbe7a261095f4b30f1365e99005d332d66b7df8
Reviewed-on: https://chromium-review.googlesource.com/c/1323616
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Bratell <bratell@opera.com>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608907}
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/content/renderer/webgraphicscontext3d_provider_impl.cc
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/content/renderer/webgraphicscontext3d_provider_impl.h
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/public/platform/web_graphics_context_3d_provider.h
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/renderer/platform/graphics/canvas_color_params.cc
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/renderer/platform/graphics/canvas_color_params.h
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/renderer/platform/graphics/canvas_resource_provider.cc
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/renderer/platform/graphics/gpu/drawing_buffer_test_helpers.h
[modify] https://crrev.com/ee29a5b80ca55270322f4de7fb423f20596305a7/third_party/blink/renderer/platform/graphics/test/fake_web_graphics_context_3d_provider.h
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 19

Labels: merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/606c4393a4200bbdb1e85e969db8b66540004189

commit 606c4393a4200bbdb1e85e969db8b66540004189
Author: Adrienne Walker <enne@chromium.org>
Date: Mon Nov 19 21:51:35 2018

cc: Fix flaky ImageController crash

If non-lazy tasks are queued but never executed, it would still try to
unref them, causing a crash.  Add some tests to make sure that lazy and
non-lazy images get unref'd (or not) appropriately and reenable the
test.

TBR=enne@chromium.org

(cherry picked from commit e0cd20e8caf2b90933e61bd511819312cebacd16)

Bug:  902644 , 902022 ,905239
Change-Id: I778d81aa390c1a529b7bfd88a2e146f245fb35e5
Reviewed-on: https://chromium-review.googlesource.com/c/1324874
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#608271}
Reviewed-on: https://chromium-review.googlesource.com/c/1343339
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#758}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/606c4393a4200bbdb1e85e969db8b66540004189/cc/tiles/image_controller.cc
[modify] https://crrev.com/606c4393a4200bbdb1e85e969db8b66540004189/cc/tiles/image_controller_unittest.cc
Project Member

Comment 17 by cr-audit...@appspot.gserviceaccount.com, Nov 19

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/606c4393a4200bbdb1e85e969db8b66540004189

Commit: 606c4393a4200bbdb1e85e969db8b66540004189
Author: enne@chromium.org
Commiter: enne@chromium.org
Date: 2018-11-19 21:51:35 +0000 UTC

cc: Fix flaky ImageController crash

If non-lazy tasks are queued but never executed, it would still try to
unref them, causing a crash.  Add some tests to make sure that lazy and
non-lazy images get unref'd (or not) appropriately and reenable the
test.

TBR=enne@chromium.org

(cherry picked from commit e0cd20e8caf2b90933e61bd511819312cebacd16)

Bug:  902644 , 902022 ,905239
Change-Id: I778d81aa390c1a529b7bfd88a2e146f245fb35e5
Reviewed-on: https://chromium-review.googlesource.com/c/1324874
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#608271}
Reviewed-on: https://chromium-review.googlesource.com/c/1343339
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#758}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
►

Sign in to add a comment