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

Issue 740197 link

Starred by 2 users
Status: Fixed
Owner:
█ zakerinasab@chromium.org
Last visit > 30 days ago
Closed: May 2018
Cc:
█ junov@chromium.org
brianosman@chromium.org
fs...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 634542



Sign in to add a comment

Avoid GPU readbacks in createImageBitmap

Project Member Reported by junov@chromium.org, Jul 7 2017 
There are many use cases where createImageBitmap receives a GPU-accelerated source image (a canvas, for example) and produces an ImageBitmap object that resides in RAM.  This should be avoided whenever possible.  And we should have regression tests for this

Idea for regression test: Put DCHECKs that verify that we get Gpu-accelerated output for gpu-accelerated input, and count on layout tests for covering all the permutations.
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 18 2017 

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

commit d16fd252591ff20f731c434badf16f2f8589f6b6
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Mon Sep 18 21:41:28 2017

Avoid GPU readbacks in ImageBitmap when tagging SkImages as sRGB

In chromium-review.googlesource.com/646508 SkImage objects without color space info are tagged
as sRGB through a readPixels - writePixels round-trip, converting accelerated SkImage objects
to de-accelerated ones. This change list resolves this by drawing onto a surface (instead of
using readPixels).

Bug:  754713 ,  740197 
Change-Id: I9609fe6bb2a8d32ba66a4eceacacb8de3a79f588
Reviewed-on: https://chromium-review.googlesource.com/670519
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Reviewed-by: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502682}
[modify] https://crrev.com/d16fd252591ff20f731c434badf16f2f8589f6b6/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 26 2017 

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

commit 982d26079cf7f293d3dd05cf6783e591ca937259
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Tue Sep 26 17:37:34 2017

Remove GPU readback in ImageBitmap when tagging SkImages as sRGB

In chromium-review.googlesource.com/670519 SkImage objects without color space
info are tagged as sRGB by drawin the image onto a sRGB canvas. This is not
needed anymore as makeColorSpace() assumes the no-color-space images as sRGB.

Bug:  754713 ,  740197 ,  767275 
Change-Id: Ic6a9461919bf49d3bf316d93ca6a5e10bc6f3d7c
Reviewed-on: https://chromium-review.googlesource.com/672023
Reviewed-by: Justin Novosad <junov@chromium.org>
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504414}
[modify] https://crrev.com/982d26079cf7f293d3dd05cf6783e591ca937259/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 28 2017 

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

commit 03414af91b3af6aa6176b256ad77ec03aff592cd
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Sat Oct 28 00:00:32 2017

Merge kLegacy and kSRGB canvas color spaces

This change merges kLegacy and kSRGB canvas color spaces and sets
the default canvas color space to sRGB.

Bug:  770330 , 770760 , 773247 , 740197 , 744636 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I06c5e87339a0a1c932e81630049cbd9718f170a5
Reviewed-on: https://chromium-review.googlesource.com/731466
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512356}
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/VirtualTestSuites
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/virtual/color_space/fast/canvas/color-space/canvas-colorspace-arguments.html
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/virtual/color_space/fast/canvas/color-space/canvas-createImageBitmap-linear-rgb.html
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/virtual/color_space/fast/canvas/color-space/canvas-createImageBitmap-p3.html
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/LayoutTests/virtual/color_space/fast/canvas/color-space/canvas-createImageBitmap-rec2020.html
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/html/ImageData.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/html/ImageData.h
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/html/ImageDataTest.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.h
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/imagebitmap/ImageBitmapFactories.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/core/imagebitmap/ImageBitmapTest.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/modules/canvas/canvas2d/CanvasRenderingContext2DSettings.idl
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/modules/canvas/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/CanvasColorParams.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/CanvasColorParams.h
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/CanvasColorParamsTest.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/ColorCorrectionTestUtils.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/ColorCorrectionTestUtils.h
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/GraphicsTypes.h
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
[modify] https://crrev.com/03414af91b3af6aa6176b256ad77ec03aff592cd/third_party/WebKit/Source/platform/graphics/gpu/AcceleratedImageBufferSurface.cpp

Comment 4 by zakerinasab@chromium.org, Nov 27 2017

Blocking: 634542

Comment 5 by zakerinasab@chromium.org, Mar 7 2018 

We still have CopyImageData() which uses readPixels() and causes a GPU readback.

Comment 6 by zakerinasab@chromium.org, Mar 16 2018 

Update: There are two points in the code that we still do readback:

- premul / unpremul: Skia does not provide any accelerated path. For premul we have two solutions: draw on a raster surface or use SkColorSpaceXform. Both require readback and the result is a software image.

- scaling: the only scaling API provided by Skia is SkImage::scalePixels() which causes a readback. It puts the scaled pixels in a SkPixmap, therefore the result will be a software image.

Comment 7 by zakerinasab@chromium.org, Mar 16 2018

Cc: fs...@chromium.org junov@chromium.org

Comment 8 by zakerinasab@chromium.org, Mar 16 2018

Cc: brianosman@chromium.org
Adding brianosman@ to validate https://bugs.chromium.org/p/chromium/issues/detail?id=740197#c6.

Comment 9 by brianosman@chromium.org, Mar 19 2018 

Re: #6, That sounds roughly right. Scaling can be done as a draw to a GPU surface, so that's possible to implement without any readback (though we don't have a dedicated API). Assuming no other effects are happening alongside the scale, it would be a fairly short chunk of code: Create a GPU backed SkSurface of the new size, draw (a subset of, optionally) the old image using drawImageRect, then snapshot the surface.
Project Member

Comment 10 by bugdroid1@chromium.org, May 2 2018 

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

commit 41fc494c53b5c5f56d05648b1a9f5361f1e91060
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Wed May 02 21:27:12 2018

Avoid GPU readback in CreateImageBitmap

This change list removes GPU readback from scaling and premul
code path in ImageBitmap and adds a unit test to verify that.

Bug:  740197 , 822724 
Change-Id: Iff74a8cd2d2641b1caab275c22bf3384053b27d2
Reviewed-on: https://chromium-review.googlesource.com/1035790
Reviewed-by: Justin Novosad <junov@chromium.org>
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#555558}
[modify] https://crrev.com/41fc494c53b5c5f56d05648b1a9f5361f1e91060/third_party/blink/renderer/core/imagebitmap/image_bitmap.cc
[modify] https://crrev.com/41fc494c53b5c5f56d05648b1a9f5361f1e91060/third_party/blink/renderer/core/imagebitmap/image_bitmap_test.cc

Comment 11 by zakerinasab@chromium.org, May 2 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment