Remove SkColorSpaceXformCanvas, use color-managed SkSurfaces instead (f…

…lutter#7548) Behavior (visual) changes should be very minor. Things that are to be expected: * A few things were not color managed correctly by the transform canvas (color emoji, some color filters). Those will be handled correctly with the tagged surfaces (although we're always transforming to sRGB, so nothing should change until we target a wider gamut). * Image filtering will happen in the source color space, rather than the destination. Very minor. * The transform canvas did caching of images in the destination color space. Now, the conversion happens at draw time. If there are performance issues, images can be pre-converted to the destination with makeColorSpace().
ndusart · Jan 22, 2019 · 50ddc37 · 50ddc37
1 parent 869d9f5
commit 50ddc37
Show file tree

Hide file tree

Showing 10 changed files with 23 additions and 36 deletions.
diff --git a/flow/raster_cache.cc b/flow/raster_cache.cc
@@ -11,7 +11,6 @@
 #include "flutter/fml/logging.h"
 #include "flutter/fml/trace_event.h"
 #include "third_party/skia/include/core/SkCanvas.h"
-#include "third_party/skia/include/core/SkColorSpaceXformCanvas.h"
 #include "third_party/skia/include/core/SkImage.h"
 #include "third_party/skia/include/core/SkPicture.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -97,8 +96,8 @@ static RasterCacheResult Rasterize(
     std::function<void(SkCanvas*)> draw_function) {
   SkIRect cache_rect = RasterCache::GetDeviceBounds(logical_rect, ctm);
 
-  const SkImageInfo image_info =
-      SkImageInfo::MakeN32Premul(cache_rect.width(), cache_rect.height());
+  const SkImageInfo image_info = SkImageInfo::MakeN32Premul(
+      cache_rect.width(), cache_rect.height(), sk_ref_sp(dst_color_space));
 
   sk_sp<SkSurface> surface =
       context
@@ -110,15 +109,6 @@ static RasterCacheResult Rasterize(
   }
 
   SkCanvas* canvas = surface->getCanvas();
-  std::unique_ptr<SkCanvas> xformCanvas;
-  if (dst_color_space) {
-    xformCanvas = SkCreateColorSpaceXformCanvas(surface->getCanvas(),
-                                                sk_ref_sp(dst_color_space));
-    if (xformCanvas) {
-      canvas = xformCanvas.get();
-    }
-  }
-
   canvas->clear(SK_ColorTRANSPARENT);
   canvas->translate(-cache_rect.left(), -cache_rect.top());
   canvas->concat(ctm);

diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc
@@ -98,21 +98,22 @@ sk_sp<SkImage> Rasterizer::MakeRasterSnapshot(sk_sp<SkPicture> picture,
   TRACE_EVENT0("flutter", __FUNCTION__);
 
   sk_sp<SkSurface> surface;
+  SkImageInfo image_info = SkImageInfo::MakeN32Premul(
+      picture_size.width(), picture_size.height(), SkColorSpace::MakeSRGB());
   if (surface_ == nullptr || surface_->GetContext() == nullptr) {
     // Raster surface is fine if there is no on screen surface. This might
     // happen in case of software rendering.
-    surface = SkSurface::MakeRaster(SkImageInfo::MakeN32Premul(picture_size));
+    surface = SkSurface::MakeRaster(image_info);
   } else {
     if (!surface_->MakeRenderContextCurrent()) {
       return nullptr;
     }
 
     // When there is an on screen surface, we need a render target SkSurface
     // because we want to access texture backed images.
-    surface = SkSurface::MakeRenderTarget(
-        surface_->GetContext(),                   // context
-        SkBudgeted::kNo,                          // budgeted
-        SkImageInfo::MakeN32Premul(picture_size)  // image info
+    surface = SkSurface::MakeRenderTarget(surface_->GetContext(),  // context
+                                          SkBudgeted::kNo,         // budgeted
+                                          image_info               // image info
     );
   }
 
@@ -231,7 +232,8 @@ static sk_sp<SkData> ScreenshotLayerTreeAsPicture(
 
 static sk_sp<SkSurface> CreateSnapshotSurface(GrContext* surface_context,
                                               const SkISize& size) {
-  const auto image_info = SkImageInfo::MakeN32Premul(size);
+  const auto image_info = SkImageInfo::MakeN32Premul(
+      size.width(), size.height(), SkColorSpace::MakeSRGB());
   if (surface_context) {
     // There is a rendering surface that may contain textures that are going to
     // be referenced in the layer tree about to be drawn.

diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc
@@ -39,7 +39,9 @@ class TestPlatformView : public PlatformView,
 
   // |GPUSurfaceSoftwareDelegate|
   virtual sk_sp<SkSurface> AcquireBackingStore(const SkISize& size) override {
-    return SkSurface::MakeRasterN32Premul(size.width(), size.height());
+    SkImageInfo image_info = SkImageInfo::MakeN32Premul(
+        size.width(), size.height(), SkColorSpace::MakeSRGB());
+    return SkSurface::MakeRaster(image_info);
   }
 
   // |GPUSurfaceSoftwareDelegate|

diff --git a/shell/common/surface.cc b/shell/common/surface.cc
@@ -5,7 +5,6 @@
 #include "flutter/shell/common/surface.h"
 
 #include "flutter/fml/logging.h"
-#include "third_party/skia/include/core/SkColorSpaceXformCanvas.h"
 #include "third_party/skia/include/core/SkSurface.h"
 
 namespace shell {
@@ -14,10 +13,6 @@ SurfaceFrame::SurfaceFrame(sk_sp<SkSurface> surface,
                            SubmitCallback submit_callback)
     : submitted_(false), surface_(surface), submit_callback_(submit_callback) {
   FML_DCHECK(submit_callback_);
-  if (surface_) {
-    xform_canvas_ = SkCreateColorSpaceXformCanvas(surface_->getCanvas(),
-                                                  SkColorSpace::MakeSRGB());
-  }
 }
 
 SurfaceFrame::~SurfaceFrame() {
@@ -38,9 +33,6 @@ bool SurfaceFrame::Submit() {
 }
 
 SkCanvas* SurfaceFrame::SkiaCanvas() {
-  if (xform_canvas_) {
-    return xform_canvas_.get();
-  }
   return surface_ != nullptr ? surface_->getCanvas() : nullptr;
 }
 

diff --git a/shell/common/surface.h b/shell/common/surface.h
@@ -34,7 +34,6 @@ class SurfaceFrame {
  private:
   bool submitted_;
   sk_sp<SkSurface> surface_;
-  std::unique_ptr<SkCanvas> xform_canvas_;
   SubmitCallback submit_callback_;
 
   bool PerformSubmit();

diff --git a/shell/gpu/gpu_surface_gl.cc b/shell/gpu/gpu_surface_gl.cc
@@ -182,7 +182,7 @@ static sk_sp<SkSurface> WrapOnscreenSurface(GrContext* context,
                                       framebuffer_info  // framebuffer info
   );
 
-  sk_sp<SkColorSpace> colorspace = nullptr;
+  sk_sp<SkColorSpace> colorspace = SkColorSpace::MakeSRGB();
 
   SkSurfaceProps surface_props(
       SkSurfaceProps::InitType::kLegacyFontHost_InitType);
@@ -199,8 +199,8 @@ static sk_sp<SkSurface> WrapOnscreenSurface(GrContext* context,
 
 static sk_sp<SkSurface> CreateOffscreenSurface(GrContext* context,
                                                const SkISize& size) {
-  const SkImageInfo image_info =
-      SkImageInfo::MakeN32(size.fWidth, size.fHeight, kOpaque_SkAlphaType);
+  const SkImageInfo image_info = SkImageInfo::MakeN32(
+      size.fWidth, size.fHeight, kOpaque_SkAlphaType, SkColorSpace::MakeSRGB());
 
   const SkSurfaceProps surface_props(
       SkSurfaceProps::InitType::kLegacyFontHost_InitType);

diff --git a/shell/platform/android/android_surface_software.cc b/shell/platform/android/android_surface_software.cc
@@ -83,8 +83,9 @@ sk_sp<SkSurface> AndroidSurfaceSoftware::AcquireBackingStore(
     return sk_surface_;
   }
 
-  SkImageInfo image_info = SkImageInfo::Make(
-      size.fWidth, size.fHeight, target_color_type_, target_alpha_type_);
+  SkImageInfo image_info =
+      SkImageInfo::Make(size.fWidth, size.fHeight, target_color_type_,
+                        target_alpha_type_, SkColorSpace::MakeSRGB());
 
   sk_surface_ = SkSurface::MakeRaster(image_info);
 

diff --git a/shell/platform/darwin/ios/ios_surface_software.mm b/shell/platform/darwin/ios/ios_surface_software.mm
@@ -64,7 +64,8 @@
     return sk_surface_;
   }
 
-  SkImageInfo info = SkImageInfo::MakeN32(size.fWidth, size.fHeight, kPremul_SkAlphaType);
+  SkImageInfo info = SkImageInfo::MakeN32(size.fWidth, size.fHeight, kPremul_SkAlphaType,
+                                          SkColorSpace::MakeSRGB());
   sk_surface_ = SkSurface::MakeRaster(info, nullptr);
   return sk_surface_;
 }

diff --git a/shell/platform/embedder/embedder_surface_software.cc b/shell/platform/embedder/embedder_surface_software.cc
@@ -61,8 +61,8 @@ sk_sp<SkSurface> EmbedderSurfaceSoftware::AcquireBackingStore(
     return sk_surface_;
   }
 
-  SkImageInfo info =
-      SkImageInfo::MakeN32(size.fWidth, size.fHeight, kPremul_SkAlphaType);
+  SkImageInfo info = SkImageInfo::MakeN32(
+      size.fWidth, size.fHeight, kPremul_SkAlphaType, SkColorSpace::MakeSRGB());
   sk_surface_ = SkSurface::MakeRaster(info, nullptr);
 
   if (sk_surface_ == nullptr) {

diff --git a/testing/resources/square.png b/testing/resources/square.png