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

Issue 657870 link

Starred by 1 user
Status: Fixed
Owner:
█ zakerinasab@chromium.org
Last visit > 30 days ago
Closed: Dec 2017
Cc:
█ junov@chromium.org
█ xlai@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

Unify toImageData() function in OffscreenCanvas and HTMLCanvasElement all contexts

Project Member Reported by xlai@chromium.org, Oct 20 2016 
Currently, HTMLCanvasElement has its own toImageData() function.

OffscreenCavnasRenderingContext and webglRenderingContextBase has its
own toImageData() functions which are utilized by OffscreenCanvas.convertToBlob().

This is a bit messy and we need to clean up and unify all of them
as one single virtual function in CanvasRenderingContext.

Comment 1 by xlai@chromium.org, Oct 20 2016 

TODO items (copied from junov's comment in https://codereview.chromium.org/2420203002/#ps200001):

1. Make CanvasRenderingContext::toImageData pure virtual (and implement in in all context classes)
2. Augment CanvasRenderingContext::toImageData so that it can take the same arguments as
HTMLCanvasElement::toImageData
3. Add a convertToBlob entry to the "SnapshotReason" enum, and add it to the
aprropriate UMA histograms
4. Make HTMLCanvasElement call CanvasRenderingContext::toImageData instead using its own toImageData
5. Delete HTMLCanvasElement::toImageData from the code

Comment 2 by xlai@chromium.org, Dec 5 2017

Cc: -xidac...@chromium.org xlai@chromium.org
Owner: zakerinasab@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 6 2017 

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

commit baf563db4c5ca4eb350b96a3c7dc16b697ae3c67
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Wed Dec 06 23:30:39 2017

Remove ImageData from toBlob/toDataURL code path

This change removes ImageData usage from toBlob/toDataURL code path
from HTMLCanvasElement calls down to the CanvasAsyncBlobCreator.

Bug:  792110 ,  657870 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I3e222a68767c1a476f09c1b6188699f62449b95a
Reviewed-on: https://chromium-review.googlesource.com/809097
Reviewed-by: Justin Novosad <junov@chromium.org>
Reviewed-by: Olivia Lai <xlai@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522248}
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify] https://crrev.com/baf563db4c5ca4eb350b96a3c7dc16b697ae3c67/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h

Comment 4 by zakerinasab@chromium.org, Dec 7 2017

Status: Fixed (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 11 2017 

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

commit 78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7
Author: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Date: Mon Dec 11 17:56:35 2017

Revert "Remove ImageData from toBlob/toDataURL code path"

This reverts commit baf563db4c5ca4eb350b96a3c7dc16b697ae3c67.

Reason for revert: Crash in SkPngEncoder::onEncodeRows ( crbug.com/793571 )

Original change's description:
> Remove ImageData from toBlob/toDataURL code path
> 
> This change removes ImageData usage from toBlob/toDataURL code path
> from HTMLCanvasElement calls down to the CanvasAsyncBlobCreator.
> 
> Bug:  792110 ,  657870 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
> Change-Id: I3e222a68767c1a476f09c1b6188699f62449b95a
> Reviewed-on: https://chromium-review.googlesource.com/809097
> Reviewed-by: Justin Novosad <junov@chromium.org>
> Reviewed-by: Olivia Lai <xlai@chromium.org>
> Reviewed-by: Kenneth Russell <kbr@chromium.org>
> Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#522248}

TBR=kbr@chromium.org,junov@chromium.org,fserb@chromium.org,xlai@chromium.org,zakerinasab@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  792110 ,  657870 
Change-Id: Ie3e35ab6149a1602ee023f01e8e2eea0dd1b2208
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/819491
Reviewed-by: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523132}
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify] https://crrev.com/78ab7b0cc9d04018e0b24c054c28a49dd4f68cb7/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
Project Member

Comment 6 by bugdroid1@chromium.org, Dec 13 2017 

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

commit 1893591e9688033abd956a1f4195568afabf4408
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Wed Dec 13 18:36:03 2017

Reland Remove ImageData from toBlob/toDataURL code path

The original change removed ImageData usage from toBlob/toDataURL code path
from HTMLCanvasElement calls down to the CanvasAsyncBlobCreator. It was
reverted after crashing on canvases with more than 16M pixels.

This change relands the CL. The problem with big canvases was that there
is a logic in WebGLRenderingContextBase that down scales the DrawingBuffer
if it has more than 16M pixles, The previous code was using the size asked
by user instead of the size adjusted in WebGLRenderingContextBase, leading
to a crash down the path while encoding ImageDataBuffer.

Patch set 1 is the reverted CL (chromium-review.googlesource.com/c/chromium/src/+/809097)

TBR=kbr@chromium.org

Bug:  792110 ,  657870 
Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I7ab2d6fa8359e3adb3bfa13cc24783a1072eece6
Reviewed-on: https://chromium-review.googlesource.com/822910
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523832}
[add] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/LayoutTests/fast/webgl/canvas-toDataURL-crash.html
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreatorTest.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/html/canvas/CanvasRenderingContext.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/core/offscreencanvas/OffscreenCanvas.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp
[modify] https://crrev.com/1893591e9688033abd956a1f4195568afabf4408/third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h
►

Sign in to add a comment