|
|
Created:
3 years, 8 months ago by ccameron Modified:
3 years, 8 months ago CC:
chromium-reviews, krit, drott+blinkwatch_chromium.org, ajuma+watch-canvas_chromium.org, dshwang, pdr+graphicswatchlist_chromium.org, blink-reviews-platform-graphics_chromium.org, Rik, fmalita+watch_chromium.org, blink-reviews, kinuko+watch, ajuma+watch_chromium.org, Stephen Chennney, rwlbuis Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake Canvas2DLayerBridge use SkColorSpaceXformCanvas
This makes legacy mode canvases render into sRGB (transforming images
as needed).
BUG=709695
Review-Url: https://codereview.chromium.org/2820823004
Cr-Commit-Position: refs/heads/master@{#465069}
Committed: https://chromium.googlesource.com/chromium/src/+/2639037b06c07f5613640e1a5f5a98c5932cb8a0
Patch Set 1 #
Total comments: 6
Patch Set 2 : Put behind flag #
Total comments: 2
Patch Set 3 : Add comment #Messages
Total messages: 19 (6 generated)
https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:786: if (!sk_surfaces_use_color_space_) { Is there a reason why we can't plumb the CanvasColorSpace and CanvasPixelFormat parameters from CanvasRenderingContext directly to here? As it is, we derive the gfx::ColorSpace, sk_surfaces_use_color_space_, and SkColorType arguments from them -- I'd rather just add helper functions to convert from CanvasColorSpace+CanvasPixelFormat the various parameters we need.
ccameron@chromium.org changed reviewers: + msarett@chromium.org, pdr@chromium.org
Adding msarett@ for content, pdr@ for OWNERs. I'd like to plumb the CanvasColorSpace and CanvasPixelFormat arguments through, instead of divining them... but that patch is huge, so I'm taking it on separately.
junov@chromium.org changed reviewers: + junov@chromium.org
This changes current shipping behavior in a significant way. Requires an Intent To Ship IMHO. CL TItle should preferably reflect the change in behavior https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:786: if (!sk_surfaces_use_color_space_) { On 2017/04/15 01:15:49, ccameron wrote: > Is there a reason why we can't plumb the CanvasColorSpace and CanvasPixelFormat > parameters from CanvasRenderingContext directly to here? > > As it is, we derive the gfx::ColorSpace, sk_surfaces_use_color_space_, and > SkColorType arguments from them -- I'd rather just add helper functions to > convert from CanvasColorSpace+CanvasPixelFormat the various parameters we need. We could. I think that we originally thought gfx::ColorSpace and SkColorType would be the most convenient forms for passing this info, but we weren't really sure where the code was heading when we wrote that. https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:788: SkCreateColorSpaceXformCanvas(canvas, color_space_.ToSkColorSpace()); Why is this necessary? In CreateSkSurface (above), we create a surface that is tagged with the right color space. Instead of patching through an intermediate canvas here, could we just set up the surface correctly in CreateSkSurface for the legacy case?
On 2017/04/17 14:41:21, junov - OoO - back April 24 wrote: > This changes current shipping behavior in a significant way. Requires an Intent > To Ship IMHO. > Actually, shouldn't this behaviour just be hidden behind the color correct rendering flag?
Yes, this was supposed to be behind a RuntimeEnabledFeatures::colorCorrectRenderingEnabled. Updated. https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:786: if (!sk_surfaces_use_color_space_) { On 2017/04/17 14:41:21, junov - OoO - back April 24 wrote: > On 2017/04/15 01:15:49, ccameron wrote: > > Is there a reason why we can't plumb the CanvasColorSpace and > CanvasPixelFormat > > parameters from CanvasRenderingContext directly to here? > > > > As it is, we derive the gfx::ColorSpace, sk_surfaces_use_color_space_, and > > SkColorType arguments from them -- I'd rather just add helper functions to > > convert from CanvasColorSpace+CanvasPixelFormat the various parameters we > need. > > We could. I think that we originally thought gfx::ColorSpace and SkColorType > would be the most convenient forms for passing this info, but we weren't really > sure where the code was heading when we wrote that. Okay -- I may add a CanvasColorSpace.h with some helpers & structures to plumb these throughout. https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:788: SkCreateColorSpaceXformCanvas(canvas, color_space_.ToSkColorSpace()); On 2017/04/17 14:41:21, junov - OoO - back April 24 wrote: > Why is this necessary? In CreateSkSurface (above), we create a surface that is > tagged with the right color space. Instead of patching through an intermediate > canvas here, could we just set up the surface correctly in CreateSkSurface for > the legacy case? Tagging a surface with a color space opts us in to linear blending. Getting color conversion without linear blending requires running the draw command through a ColorSpaceXformCanvas.
> Yes, this was supposed to be behind a RuntimeEnabledFeatures::colorCorrectRenderingEnabled. Updated. Did you mean to upload a new Patch Set? https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:788: SkCreateColorSpaceXformCanvas(canvas, color_space_.ToSkColorSpace()); On 2017/04/17 18:27:17, ccameron wrote: > On 2017/04/17 14:41:21, junov - OoO - back April 24 wrote: > > Why is this necessary? In CreateSkSurface (above), we create a surface that is > > tagged with the right color space. Instead of patching through an > intermediate > > canvas here, could we just set up the surface correctly in CreateSkSurface for > > the legacy case? > > Tagging a surface with a color space opts us in to linear blending. Getting > color conversion without linear blending requires running the draw command > through a ColorSpaceXformCanvas. +1, this is the recommended approach.
On 2017/04/17 18:42:46, msarett1 wrote: > > Yes, this was supposed to be behind a > RuntimeEnabledFeatures::colorCorrectRenderingEnabled. Updated. > > Did you mean to upload a new Patch Set? Oop, PS2 is uploaded now.
lgtm https://codereview.chromium.org/2820823004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:789: SkCreateColorSpaceXformCanvas(canvas, color_space_.ToSkColorSpace()); Just curious, is color_space_ always sRGB?
Thanks! https://codereview.chromium.org/2820823004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp (right): https://codereview.chromium.org/2820823004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:789: SkCreateColorSpaceXformCanvas(canvas, color_space_.ToSkColorSpace()); On 2017/04/17 18:57:50, msarett1 wrote: > Just curious, is color_space_ always sRGB? For legacy canvas, yes. For the rasterization of web content (in cc/raster/raster_source.cc), it can be any parametric (approximating an ICC profile).
lgtm. But please add a comment that explains why the ColorSpaceXformCanvas is required since it is not obvious from reading the code.
Thanks!
The CQ bit was checked by ccameron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarett@chromium.org, junov@chromium.org Link to the patchset: https://codereview.chromium.org/2820823004/#ps40001 (title: "Add comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492466275968450, "parent_rev": "96a94ba94eb9a691ead21e134e28bbe6cd47580a", "commit_rev": "2639037b06c07f5613640e1a5f5a98c5932cb8a0"}
Message was sent while issue was closed.
Description was changed from ========== Make Canvas2DLayerBridge use SkColorSpaceXformCanvas This makes legacy mode canvases render into sRGB (transforming images as needed). BUG=709695 ========== to ========== Make Canvas2DLayerBridge use SkColorSpaceXformCanvas This makes legacy mode canvases render into sRGB (transforming images as needed). BUG=709695 Review-Url: https://codereview.chromium.org/2820823004 Cr-Commit-Position: refs/heads/master@{#465069} Committed: https://chromium.googlesource.com/chromium/src/+/2639037b06c07f5613640e1a5f5a... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/2639037b06c07f5613640e1a5f5a... |