Add ColorBehavior to blink::Image draw methods

third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp

Index: third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
diff --git a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
index d3fb0e25fd12d224c0cfb32603fb0776ed59ae46..ddafd62e7a398a28af62161b8a158ac9bf9e65d9 100644
--- a/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
+++ b/third_party/WebKit/Source/modules/canvas2d/BaseRenderingContext2D.cpp
@@ -1078,8 +1078,11 @@ void BaseRenderingContext2D::drawImageInternal(SkCanvas* c,
 
   if (!imageSource->isVideoElement()) {
     imagePaint.setAntiAlias(shouldDrawImageAntialiased(dstRect));
+    // TODO(ccameron): Canvas should draw in sRGB by default.

[Justin Novosad, 2016/12/09 02:17:35]

Actually, we want to use whatever the canvas's colorspace is. You can assign
this to me.

+    // https://crbug.com/672299
     image->draw(c, imagePaint, dstRect, srcRect, DoNotRespectImageOrientation,
-                Image::DoNotClampImageToSourceRect);
+                Image::DoNotClampImageToSourceRect,
+                ColorBehavior::transformToGlobalTarget());

[Justin Novosad, 2016/12/09 02:17:35]

I am concerned about this approach, I understand that BitmapImage will end-up
caching the transformed copy of the source image to avoid re-tranforming it over
and over. Still, it means holding on to long-lived extra copy of the decoded
image in many cases.  Also, it means that the cached copy will thrash if the
image resource is requested in more that one colorspace at each animation frame.
Also, If the source image is another kind of Blink::Image, then what, do we want
to add caches in all subclasses? If we do that we may endup further multiplying
the redundant copies of images.

Transforming the images is not necessary.  The rasterization code only needs the
SkImages to be correctly tagged with colorspaces, and then skia will infer the
required transform and apply it on the fly at blit time. MY understanding is
that the required overhead for the on-the-fly transforms is not that high,
especially on the GPU.

Bottom line: There is an important compute vs. memory tradeoff here that needs
to be considered with caution.

[ccameron, 2016/12/09 03:30:56]

It is true, as you mention, that, if two different monitors are repeatedly
requesting the same image, we will thrash the decoder. This is a conscious
choice.
For other sub-classes, it may be feasible to do on-the-fly conversion, instead
of caching. Adding this interface doesn't force either behavior here.
This is not true. Skia will only transform images if you opt in to linear
blending, which is not permitted by the web. This is the reason that we're going
with this approach of pre-transforming all inputs, instead of using Skia's color
space support.

On-the-fly transform is efficient and precise with GPU rasterization (and
especially desirable for high-bit-depth rendering), but it's not clear if it is
efficient for CPU rasterization.
Either way that we make this trade-off, we will still need to use the interface
that I've added here. How we decide to do conversion (on the fly v not) will
only affect how each blink::Image sub-class behaves.

The default implementation that I've given (which thrashes the decoder in the
worst case) is the least-invasive initial behavior to add.

Also of note is that this is still all behind a flag (well, behind TODOs, which
will eventually become behind a flag), so we can perform measurements and
determine the best course action from that.

   } else {
     c->save();
     c->clipRect(dstRect);