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

Issue 771714 link

Starred by 0 users
Status: Fixed
Owner:
█ zakerinasab@chromium.org
Last visit > 30 days ago
Closed: Nov 2017
Cc:
█ junov@chromium.org
fs...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature

Blocking:
issue 634542



Sign in to add a comment

Add DCHECK for color space conversion calls in canvas code

Project Member Reported by zakerinasab@chromium.org, Oct 4 2017 
We have different locations in canvas color management code that we call one of these two API for color conversion:

SkColorSpaceXform::apply()
SkImage::makeColorSpace()

All the color conversion calls should have a DCHECK on the result to make sure that they proceed successfully.

This is necessary to detect hidden unsupported cases. No matter what is the result of the color conversion call site, we need to make sure that the renderer does not crash if the call is not successful. As a general strategy, we prefer to silently fail instead of crashing the renderer.

Comment 1 by zakerinasab@chromium.org, Oct 25 2017

Labels: -Pri-3 Pri-2

Comment 2 by zakerinasab@chromium.org, Nov 7 2017 

SkImage::readPixels() and SkPixmap::readPixels() can be used for color conversion too. Even though these functions return a bool, color conversion still may silently fail in these calls, as the code path ends up in SkConvertPixels() from SkConvertPixels.h and this is a void function.
Still it is harmless to add DCHECK to these calls.
Project Member

Comment 3 by bugdroid1@chromium.org, Nov 8 2017 

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

commit ea365a520d4ef1d4943179e060e0538f82738819
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Wed Nov 08 04:53:40 2017

Add DCHECK to color conversion API call sites

This change adds DCHECKs to SkColorSpaceXform::apply() call sites in color managed canvas code.

Bug:  771714 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: If10a5908937565f1acfb5f6730730fbcf360ef7e
Reviewed-on: https://chromium-review.googlesource.com/756862
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Reviewed-by: Justin Novosad <junov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514753}
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/core/html/ImageData.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/core/html/ImageDataTest.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/core/imagebitmap/ImageBitmapTest.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/modules/canvas/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/platform/graphics/CanvasColorParamsTest.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/platform/graphics/ColorSpaceGamut.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
[modify] https://crrev.com/ea365a520d4ef1d4943179e060e0538f82738819/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 9 2017 

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

commit 2aacfe012a147f11119887873114fec162bc5fa0
Author: Reza.Zakerinasab <zakerinasab@chromium.org>
Date: Thu Nov 09 21:04:04 2017

Add DCHECK to color conversion API call sites

This change adds DCHECKs to SkImage::readPixels() call sites in color managed
canvas code.

Bug:  771714 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I90050900dce3b1f05a52a79b7c75a7b35ddb8107
Reviewed-on: https://chromium-review.googlesource.com/757578
Reviewed-by: Justin Novosad <junov@chromium.org>
Reviewed-by: Olivia Lai <xlai@chromium.org>
Commit-Queue: Mohammad Reza Zakerinasab <zakerinasab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#515286}
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/core/imagebitmap/ImageBitmap.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/core/imagebitmap/ImageBitmapTest.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/modules/canvas/canvas2d/CanvasRenderingContext2DTest.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/modules/canvas/offscreencanvas2d/OffscreenCanvasRenderingContext2D.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/platform/graphics/ImageBuffer.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/platform/graphics/OffscreenCanvasResourceProvider.cpp
[modify] https://crrev.com/2aacfe012a147f11119887873114fec162bc5fa0/third_party/WebKit/Source/platform/graphics/StaticBitmapImage.cpp

Comment 5 by zakerinasab@chromium.org, Nov 9 2017

Status: Fixed (was: Assigned)

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

Blocking: 634542
►

Sign in to add a comment