From 73f8e1988e641f35d37bdd6b201334efb22e8fb7 Mon Sep 17 00:00:00 2001 From: mattsarett Date: Mon, 5 Jun 2017 14:59:04 -0400 Subject: [PATCH] Run Flutter on iOS and Android with color correct Skia (flutter/engine#3716) ***Turns on color correct rendering for Android and iOS ***Communicates dst color space to raster cache ***Turns on color space aware image decoding Test: ***color_testing_demo on Pixel XL ***flutter_gallery on iPad Mini and iPad Pro (haven't figured out how to run manual_tests on iOS) TODO: I needed to split up this CL somewhere. These are follow-up tasks. ***Make desktop backends color correct ***Make debugging tools (ex: encoding frames to png) preserve color space ***Investigate using UIKit API to allow iOS to fine tune color space of rendered content --- engine/src/flutter/flow/layers/layer.h | 1 + engine/src/flutter/flow/layers/layer_tree.cc | 3 +- .../src/flutter/flow/layers/picture_layer.cc | 3 +- engine/src/flutter/flow/raster_cache.cc | 16 ++- engine/src/flutter/flow/raster_cache.h | 1 + .../flutter/lib/ui/painting/image_decoding.cc | 5 +- .../src/flutter/shell/gpu/gpu_surface_gl.cc | 29 +++-- engine/src/flutter/shell/gpu/gpu_surface_gl.h | 3 + .../platform/android/android_context_gl.cc | 121 +++++++++++------- .../platform/android/android_context_gl.h | 16 +-- .../platform/android/android_surface_gl.cc | 7 +- .../platform/android/android_surface_gl.h | 9 ++ .../platform/darwin/ios/ios_gl_context.h | 5 + .../platform/darwin/ios/ios_gl_context.mm | 34 ++++- .../platform/darwin/ios/ios_surface_gl.h | 4 + .../darwin/ios/ios_surface_software.mm | 5 +- 16 files changed, 176 insertions(+), 86 deletions(-) diff --git a/engine/src/flutter/flow/layers/layer.h b/engine/src/flutter/flow/layers/layer.h index 8bf78777742..699440f37fd 100644 --- a/engine/src/flutter/flow/layers/layer.h +++ b/engine/src/flutter/flow/layers/layer.h @@ -35,6 +35,7 @@ class Layer { struct PrerollContext { RasterCache* raster_cache; GrContext* gr_context; + sk_sp dst_color_space; SkRect child_paint_bounds; }; diff --git a/engine/src/flutter/flow/layers/layer_tree.cc b/engine/src/flutter/flow/layers/layer_tree.cc index 0bbbd582235..18ecfbc7e11 100644 --- a/engine/src/flutter/flow/layers/layer_tree.cc +++ b/engine/src/flutter/flow/layers/layer_tree.cc @@ -31,7 +31,8 @@ void LayerTree::Preroll(CompositorContext::ScopedFrame& frame, checkerboard_raster_cache_images_); Layer::PrerollContext context = { ignore_raster_cache ? nullptr : &frame.context().raster_cache(), - frame.gr_context(), SkRect::MakeEmpty(), + frame.gr_context(), sk_ref_sp(frame.canvas().imageInfo().colorSpace()), + SkRect::MakeEmpty(), }; root_layer_->Preroll(&context, SkMatrix::I()); } diff --git a/engine/src/flutter/flow/layers/picture_layer.cc b/engine/src/flutter/flow/layers/picture_layer.cc index 2f53e771cf7..65d917c731a 100644 --- a/engine/src/flutter/flow/layers/picture_layer.cc +++ b/engine/src/flutter/flow/layers/picture_layer.cc @@ -24,7 +24,8 @@ PictureLayer::~PictureLayer() { void PictureLayer::Preroll(PrerollContext* context, const SkMatrix& matrix) { if (auto cache = context->raster_cache) { image_ = cache->GetPrerolledImage(context->gr_context, picture_.get(), - matrix, is_complex_, will_change_); + matrix, context->dst_color_space, + is_complex_, will_change_); } SkRect bounds = picture_->cullRect().makeOffset(offset_.x(), offset_.y()); diff --git a/engine/src/flutter/flow/raster_cache.cc b/engine/src/flutter/flow/raster_cache.cc index ef7d0ec60af..a3657529033 100644 --- a/engine/src/flutter/flow/raster_cache.cc +++ b/engine/src/flutter/flow/raster_cache.cc @@ -35,11 +35,13 @@ RasterCache::Entry::Entry() { RasterCache::Entry::~Entry() {} -sk_sp RasterCache::GetPrerolledImage(GrContext* context, - SkPicture* picture, - const SkMatrix& ctm, - bool is_complex, - bool will_change) { +sk_sp RasterCache::GetPrerolledImage( + GrContext* context, + SkPicture* picture, + const SkMatrix& ctm, + sk_sp dst_color_space, + bool is_complex, + bool will_change) { SkScalar scaleX = ctm.getScaleX(); SkScalar scaleY = ctm.getScaleY(); @@ -75,7 +77,9 @@ sk_sp RasterCache::GetPrerolledImage(GrContext* context, TRACE_EVENT2("flutter", "Rasterize picture layer", "width", std::to_string(physical_size.width()).c_str(), "height", std::to_string(physical_size.height()).c_str()); - SkImageInfo info = SkImageInfo::MakeN32Premul(physical_size); + SkImageInfo info = SkImageInfo::MakeN32Premul( + physical_size.width(), physical_size.height(), + std::move(dst_color_space)); sk_sp surface = SkSurface::MakeRenderTarget(context, SkBudgeted::kYes, info); if (surface) { diff --git a/engine/src/flutter/flow/raster_cache.h b/engine/src/flutter/flow/raster_cache.h index 13b08e852f3..22750f68736 100644 --- a/engine/src/flutter/flow/raster_cache.h +++ b/engine/src/flutter/flow/raster_cache.h @@ -24,6 +24,7 @@ class RasterCache { sk_sp GetPrerolledImage(GrContext* context, SkPicture* picture, const SkMatrix& ctm, + sk_sp dst_color_space, bool is_complex, bool will_change); void SweepAfterFrame(); diff --git a/engine/src/flutter/lib/ui/painting/image_decoding.cc b/engine/src/flutter/lib/ui/painting/image_decoding.cc index 50a4bc8f461..9c9a3518675 100644 --- a/engine/src/flutter/lib/ui/painting/image_decoding.cc +++ b/engine/src/flutter/lib/ui/painting/image_decoding.cc @@ -32,9 +32,10 @@ sk_sp DecodeImage(sk_sp buffer) { GrContext* context = ResourceContext::Get(); if (context) { - // TODO: Supply actual destination color space once available + // This acts as a flag to indicate that we want a color space aware decode. + sk_sp dstColorSpace = SkColorSpace::MakeSRGB(); return SkImage::MakeCrossContextFromEncoded(context, std::move(buffer), - false, nullptr); + false, dstColorSpace.get()); } else { return SkImage::MakeFromEncoded(std::move(buffer)); } diff --git a/engine/src/flutter/shell/gpu/gpu_surface_gl.cc b/engine/src/flutter/shell/gpu/gpu_surface_gl.cc index 8380b6bd2ce..1fe833dd433 100644 --- a/engine/src/flutter/shell/gpu/gpu_surface_gl.cc +++ b/engine/src/flutter/shell/gpu/gpu_surface_gl.cc @@ -8,6 +8,7 @@ #include "lib/ftl/arraysize.h" #include "lib/ftl/logging.h" #include "third_party/skia/include/core/SkSurface.h" +#include "third_party/skia/include/gpu/GrContextOptions.h" #include "third_party/skia/include/gpu/gl/GrGLInterface.h" namespace shell { @@ -44,9 +45,12 @@ bool GPUSurfaceGL::Setup() { auto backend_context = reinterpret_cast(GrGLCreateNativeInterface()); - context_ = - sk_sp(GrContext::Create(kOpenGL_GrBackend, backend_context)); + GrContextOptions options; + options.fRequireDecodeDisableForSRGB = false; + context_ = + sk_sp(GrContext::Create(kOpenGL_GrBackend, backend_context, + options)); if (context_ == nullptr) { FTL_LOG(INFO) << "Failed to setup Skia Gr context."; return false; @@ -104,15 +108,16 @@ bool GPUSurfaceGL::PresentSurface(SkCanvas* canvas) { } bool GPUSurfaceGL::SelectPixelConfig(GrPixelConfig* config) { - static const GrPixelConfig kConfigOptions[] = { - kSkia8888_GrPixelConfig, kRGBA_4444_GrPixelConfig, - }; + if (delegate_->ColorSpace() && delegate_->ColorSpace()->gammaCloseToSRGB()) { + FTL_DCHECK(context_->caps()->isConfigRenderable(kSRGBA_8888_GrPixelConfig, + false)); + *config = kSRGBA_8888_GrPixelConfig; + return true; + } - for (size_t i = 0; i < arraysize(kConfigOptions); i++) { - if (context_->caps()->isConfigRenderable(kConfigOptions[i], false)) { - *config = kConfigOptions[i]; - return true; - } + if (context_->caps()->isConfigRenderable(kRGBA_8888_GrPixelConfig, false)) { + *config = kRGBA_8888_GrPixelConfig; + return true; } return false; @@ -124,7 +129,6 @@ sk_sp GPUSurfaceGL::CreateSurface(const SkISize& size) { } GrBackendRenderTargetDesc desc; - if (!SelectPixelConfig(&desc.fConfig)) { return nullptr; } @@ -135,7 +139,8 @@ sk_sp GPUSurfaceGL::CreateSurface(const SkISize& size) { desc.fOrigin = kBottomLeft_GrSurfaceOrigin; desc.fRenderTargetHandle = delegate_->GLContextFBO(); - return SkSurface::MakeFromBackendRenderTarget(context_.get(), desc, nullptr); + return SkSurface::MakeFromBackendRenderTarget(context_.get(), desc, + delegate_->ColorSpace(), nullptr); } sk_sp GPUSurfaceGL::AcquireSurface(const SkISize& size) { diff --git a/engine/src/flutter/shell/gpu/gpu_surface_gl.h b/engine/src/flutter/shell/gpu/gpu_surface_gl.h index 95a6527ba55..219d8fdd176 100644 --- a/engine/src/flutter/shell/gpu/gpu_surface_gl.h +++ b/engine/src/flutter/shell/gpu/gpu_surface_gl.h @@ -22,6 +22,9 @@ class GPUSurfaceGLDelegate { virtual bool GLContextPresent() = 0; virtual intptr_t GLContextFBO() const = 0; + + // TODO: Update Mac desktop and make this pure virtual. + virtual sk_sp ColorSpace() const { return nullptr; } }; class GPUSurfaceGL : public Surface { diff --git a/engine/src/flutter/shell/platform/android/android_context_gl.cc b/engine/src/flutter/shell/platform/android/android_context_gl.cc index 9ae9dfb8e21..a8e14c87fc7 100644 --- a/engine/src/flutter/shell/platform/android/android_context_gl.cc +++ b/engine/src/flutter/shell/platform/android/android_context_gl.cc @@ -3,9 +3,17 @@ // found in the LICENSE file. #include "flutter/shell/platform/android/android_context_gl.h" - +#include #include +#ifndef EGL_GL_COLORSPACE_KHR +#define EGL_GL_COLORSPACE_KHR 0x309D +#endif + +#ifndef EGL_GL_COLORSPACE_SRGB_KHR +#define EGL_GL_COLORSPACE_SRGB_KHR 0x3089 +#endif + namespace shell { template @@ -112,43 +120,66 @@ static bool TeardownSurface(EGLDisplay display, EGLSurface surface) { } // For onscreen rendering. -static EGLResult CreateWindowSurface( - EGLDisplay display, - EGLConfig config, - AndroidNativeWindow::Handle window_handle) { +bool AndroidContextGL::CreateWindowSurface( + ftl::RefPtr window) { // The configurations are only required when dealing with extensions or VG. // We do neither. - EGLSurface surface = eglCreateWindowSurface( - display, config, reinterpret_cast(window_handle), - nullptr); - return {surface != EGL_NO_SURFACE, surface}; + + window_ = std::move(window); + EGLDisplay display = environment_->Display(); + + const EGLint srgb_attribs[] = { + EGL_GL_COLORSPACE_KHR, EGL_GL_COLORSPACE_SRGB_KHR, + EGL_NONE + }; + const EGLint default_attribs[] = { + EGL_NONE + }; + + const EGLint* attribs = default_attribs; + if (srgb_support_) { + attribs = srgb_attribs; + } + + surface_ = eglCreateWindowSurface( + display, config_, + reinterpret_cast(window_->handle()), attribs); + return surface_ != EGL_NO_SURFACE; } // For offscreen rendering. -static EGLResult CreatePBufferSurface(EGLDisplay display, - EGLConfig config) { +bool AndroidContextGL::CreatePBufferSurface() { // We only ever create pbuffer surfaces for background resource loading // contexts. We never bind the pbuffer to anything. - const EGLint attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE}; - EGLSurface surface = eglCreatePbufferSurface(display, config, attribs); - return {surface != EGL_NO_SURFACE, surface}; -} -static EGLResult CreateSurface( - EGLDisplay display, - EGLConfig config, - ftl::RefPtr window) { - return window && window->IsValid() - ? CreateWindowSurface(display, config, window->handle()) - : CreatePBufferSurface(display, config); + EGLDisplay display = environment_->Display(); + + const EGLint srgb_attribs[] = { + EGL_WIDTH, 1, + EGL_HEIGHT, 1, + EGL_GL_COLORSPACE_KHR, EGL_GL_COLORSPACE_SRGB_KHR, + EGL_NONE + }; + const EGLint default_attribs[] = { + EGL_WIDTH, 1, + EGL_HEIGHT, 1, + EGL_NONE + }; + + const EGLint* attribs = default_attribs; + if (srgb_support_) { + attribs = srgb_attribs; + } + + surface_ = eglCreatePbufferSurface(display, config_, attribs); + return surface_ != EGL_NO_SURFACE; } AndroidContextGL::AndroidContextGL(ftl::RefPtr env, - ftl::RefPtr window, PlatformView::SurfaceConfig config, const AndroidContextGL* share_context) : environment_(env), - window_(window), + window_(nullptr), config_(nullptr), surface_(EGL_NO_SURFACE), context_(EGL_NO_CONTEXT), @@ -170,17 +201,6 @@ AndroidContextGL::AndroidContextGL(ftl::RefPtr env, return; } - // Create a surface for the configuration. - - std::tie(success, surface_) = - CreateSurface(environment_->Display(), config_, window_); - - if (!success) { - FTL_LOG(ERROR) << "Could not create the EGL surface."; - LogLastEGLError(); - return; - } - // Create a context for the configuration. std::tie(success, context_) = CreateContext( @@ -193,15 +213,22 @@ AndroidContextGL::AndroidContextGL(ftl::RefPtr env, return; } + // On its own, this is not enough to guarantee that we will render in + // sRGB mode. We also need to query GL using the GrContext. + + const char* exts = eglQueryString(environment_->Display(), EGL_EXTENSIONS); + srgb_support_ = strstr(exts, "EGL_KHR_gl_colorspace"); + + if (!this->CreatePBufferSurface()) { + FTL_LOG(ERROR) << "Could not create the EGL surface."; + LogLastEGLError(); + return; + } + // All done! valid_ = true; } -AndroidContextGL::AndroidContextGL(ftl::RefPtr env, - PlatformView::SurfaceConfig config, - const AndroidContextGL* share_context) - : AndroidContextGL(env, nullptr, config, share_context) {} - AndroidContextGL::~AndroidContextGL() { if (!TeardownContext(environment_->Display(), context_)) { FTL_LOG(ERROR) << "Could not tear down the EGL context. Possible resource leak."; @@ -270,18 +297,18 @@ bool AndroidContextGL::Resize(const SkISize& size) { TeardownSurface(environment_->Display(), surface_); - bool success = false; - std::tie(success, surface_) = - CreateSurface(environment_->Display(), config_, window_); - - MakeCurrent(); - - if (!success) { + if (!this->CreateWindowSurface(window_)) { FTL_LOG(ERROR) << "Unable to create EGL window surface on resize."; return false; } + MakeCurrent(); + return true; } +bool AndroidContextGL::SupportsSRGB() const { + return srgb_support_; +} + } // namespace shell diff --git a/engine/src/flutter/shell/platform/android/android_context_gl.h b/engine/src/flutter/shell/platform/android/android_context_gl.h index aa2a081a510..0cad635a7e1 100644 --- a/engine/src/flutter/shell/platform/android/android_context_gl.h +++ b/engine/src/flutter/shell/platform/android/android_context_gl.h @@ -17,6 +17,11 @@ namespace shell { class AndroidContextGL : public ftl::RefCountedThreadSafe { public: + + bool CreateWindowSurface(ftl::RefPtr window); + + bool CreatePBufferSurface(); + ftl::RefPtr Environment() const; bool IsValid() const; @@ -31,22 +36,17 @@ class AndroidContextGL : public ftl::RefCountedThreadSafe { bool Resize(const SkISize& size); + bool SupportsSRGB() const; + private: ftl::RefPtr environment_; ftl::RefPtr window_; EGLConfig config_; EGLSurface surface_; EGLContext context_; + bool srgb_support_; bool valid_; - /// Creates a window surface context tied to the window handle for on-screen - /// rendering. - AndroidContextGL(ftl::RefPtr env, - ftl::RefPtr window, - PlatformView::SurfaceConfig config, - const AndroidContextGL* share_context = nullptr); - - /// Creates a pbuffer surface context for offscreen rendering. AndroidContextGL(ftl::RefPtr env, PlatformView::SurfaceConfig config, const AndroidContextGL* share_context = nullptr); diff --git a/engine/src/flutter/shell/platform/android/android_surface_gl.cc b/engine/src/flutter/shell/platform/android/android_surface_gl.cc index 9e86824f460..fbfd03cb0b6 100644 --- a/engine/src/flutter/shell/platform/android/android_surface_gl.cc +++ b/engine/src/flutter/shell/platform/android/android_surface_gl.cc @@ -117,7 +117,7 @@ bool AndroidSurfaceGL::SetNativeWindow(ftl::RefPtr window, // Create the onscreen context. onscreen_context_ = ftl::MakeRefCounted( - offscreen_context_->Environment(), std::move(window), config, + offscreen_context_->Environment(), config, offscreen_context_.get() /* sharegroup */); if (!onscreen_context_->IsValid()) { @@ -125,6 +125,11 @@ bool AndroidSurfaceGL::SetNativeWindow(ftl::RefPtr window, return false; } + if (!onscreen_context_->CreateWindowSurface(std::move(window))) { + onscreen_context_ = nullptr; + return false; + } + return true; } diff --git a/engine/src/flutter/shell/platform/android/android_surface_gl.h b/engine/src/flutter/shell/platform/android/android_surface_gl.h index 21a5768db74..29ef4e8e7b2 100644 --- a/engine/src/flutter/shell/platform/android/android_surface_gl.h +++ b/engine/src/flutter/shell/platform/android/android_surface_gl.h @@ -47,12 +47,21 @@ class AndroidSurfaceGL : public GPUSurfaceGLDelegate, public AndroidSurface { intptr_t GLContextFBO() const override; + sk_sp ColorSpace() const override { + // TODO: + // We can render more consistently across devices when Android makes it + // possible to query for the color space of the display. + return onscreen_context_->SupportsSRGB() ? SkColorSpace::MakeSRGB() + : nullptr; + } + void SetFlutterView( const fml::jni::JavaObjectWeakGlobalRef& flutter_view) override; private: ftl::RefPtr onscreen_context_; ftl::RefPtr offscreen_context_; + sk_sp gr_context_; FTL_DISALLOW_COPY_AND_ASSIGN(AndroidSurfaceGL); }; diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.h b/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.h index 51ad00fde79..52506758d54 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.h +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.h @@ -34,6 +34,10 @@ class IOSGLContext { bool ResourceMakeCurrent(); + sk_sp ColorSpace() const { + return color_space_; + } + private: fml::scoped_nsobject layer_; fml::scoped_nsobject context_; @@ -45,6 +49,7 @@ class IOSGLContext { GLuint depth_stencil_packed_buffer_; GLint storage_size_width_; GLint storage_size_height_; + sk_sp color_space_; bool valid_; FTL_DISALLOW_COPY_AND_ASSIGN(IOSGLContext); diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.mm b/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.mm index 4bb90aed433..082e3a11372 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_gl_context.mm @@ -3,6 +3,10 @@ // found in the LICENSE file. #include "flutter/shell/platform/darwin/ios/ios_gl_context.h" +#include "third_party/skia/include/gpu/GrContextOptions.h" +#include "third_party/skia/include/gpu/gl/GrGLInterface.h" + +#include namespace shell { @@ -96,14 +100,32 @@ IOSGLContext::IOSGLContext(PlatformView::SurfaceConfig config, CAEAGLLayer* laye } } - // The default is RGBA - NSString* drawableColorFormat = kEAGLColorFormatRGBA8; - - if (config.red_bits <= 5 && config.green_bits <= 6 && config.blue_bits <= 5 && - config.alpha_bits == 0) { - drawableColorFormat = kEAGLColorFormatRGB565; + // TODO: + // iOS displays are more variable than just P3 or sRGB. Reading the display + // gamut just tells us what color space it makes sense to render into. We + // should use iOS APIs to perform the final correction step based on the + // device properties. Ex: We can indicate that we have rendered in P3, and + // the framework will do the final adjustment for us. + NSOperatingSystemVersion version = [[NSProcessInfo processInfo] + operatingSystemVersion]; + color_space_ = SkColorSpace::MakeSRGB(); + if (version.majorVersion >= 10) { + UIDisplayGamut displayGamut = + [UIScreen mainScreen].traitCollection.displayGamut; + switch (displayGamut) { + case UIDisplayGamutP3: + // Should we consider using more than 8-bits of precision given that + // P3 specifies a wider range of colors? + color_space_ = SkColorSpace::MakeRGB( + SkColorSpace::kSRGB_RenderTargetGamma, + SkColorSpace::kDCIP3_D65_Gamut); + break; + default: + break; + } } + NSString* drawableColorFormat = kEAGLColorFormatSRGBA8; layer_.get().drawableProperties = @{ kEAGLDrawablePropertyColorFormat : drawableColorFormat, kEAGLDrawablePropertyRetainedBacking : @(NO), diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_surface_gl.h b/engine/src/flutter/shell/platform/darwin/ios/ios_surface_gl.h index 5f2abb9b3d1..c7900480b14 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_surface_gl.h +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_surface_gl.h @@ -36,6 +36,10 @@ class IOSSurfaceGL : public IOSSurface, public GPUSurfaceGLDelegate { intptr_t GLContextFBO() const override; + sk_sp ColorSpace() const override { + return context_.ColorSpace(); + } + private: IOSGLContext context_; diff --git a/engine/src/flutter/shell/platform/darwin/ios/ios_surface_software.mm b/engine/src/flutter/shell/platform/darwin/ios/ios_surface_software.mm index 7ce5f97b568..759571696c1 100644 --- a/engine/src/flutter/shell/platform/darwin/ios/ios_surface_software.mm +++ b/engine/src/flutter/shell/platform/darwin/ios/ios_surface_software.mm @@ -63,8 +63,9 @@ sk_sp IOSSurfaceSoftware::AcquireBackingStore(const SkISize& size) { return sk_surface_; } - sk_surface_ = SkSurface::MakeRasterN32Premul(size.fWidth, size.fHeight, - nullptr /* SkSurfaceProps as out */); + SkImageInfo info = SkImageInfo::MakeS32(size.fWidth, size.fHeight, + kPremul_SkAlphaType); + sk_surface_ = SkSurface::MakeRaster(info, nullptr); return sk_surface_; }