Skip to content

Commit

Permalink
Disable linear blending, use SkColorSpaceXformCanvas instead (flutter…
Browse files Browse the repository at this point in the history 
…#4355)

This retains gamut correction (adjusting colors for screens with different capabilities), but does all blending and interpolation with sRGB-encoded values. That matches the behavior expected by most users, as well as the behavior of nearly all other systems. It also greatly simplifies the EGL code.

A future Skia change will make this behavior more of a first-class citizen, so some of these implementation details will change again, but the behavior will not. The bulk of this change (elimination of complication from the GL surface code) is permanent - it's just the SkColorSpaceXformCanvas that will be replaced.
  • Loading branch information
@brianosman
brianosman authored Nov 14, 2017
1 parent 242ce44 commit 0a7155d
Show file tree
Hide file tree
Showing 19 changed files with 41 additions and 161 deletions.
14 changes: 11 additions & 3 deletions flow/raster_cache.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "flutter/glue/trace_event.h"
#include "lib/fxl/logging.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"
Expand Down Expand Up @@ -95,9 +96,8 @@ RasterCacheResult RasterizePicture(SkPicture* picture,
std::fabs(logical_rect.height() * metrics_scale_y * scale.y()));

const SkImageInfo image_info = SkImageInfo::MakeN32Premul(
std::ceil(physical_rect.width()), // physical width
std::ceil(physical_rect.height()), // physical height
sk_ref_sp(dst_color_space) // colorspace
std::ceil(physical_rect.width()), // physical width
std::ceil(physical_rect.height()) // physical height
);

sk_sp<SkSurface> surface =
Expand All @@ -110,6 +110,14 @@ RasterCacheResult RasterizePicture(SkPicture* picture,
}

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->scale(std::abs(scale.x() * metrics_scale_x),
Expand Down
8 changes: 4 additions & 4 deletions lib/ui/painting/codec.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,8 @@ sk_sp<SkImage> DecodeImage(sk_sp<SkData> buffer, size_t trace_id) {

GrContext* context = ResourceContext::Get();
if (context) {
// This acts as a flag to indicate that we want a color space aware decode.
sk_sp<SkColorSpace> dstColorSpace = SkColorSpace::MakeSRGB();
// This indicates that we do not want a "linear blending" decode.
sk_sp<SkColorSpace> dstColorSpace = nullptr;
return SkImage::MakeCrossContextFromEncoded(context, std::move(buffer),
false, dstColorSpace.get());
} else {
Expand Down Expand Up @@ -241,8 +241,8 @@ sk_sp<SkImage> MultiFrameCodec::GetNextFrameImage() {
if (context) {
SkPixmap pixmap(bitmap.info(), bitmap.pixelRef()->pixels(),
bitmap.pixelRef()->rowBytes());
// This acts as a flag to indicate that we want a color space aware decode.
sk_sp<SkColorSpace> dstColorSpace = SkColorSpace::MakeSRGB();
// This indicates that we do not want a "linear blending" decode.
sk_sp<SkColorSpace> dstColorSpace = nullptr;
return SkImage::MakeCrossContextFromPixmap(context, pixmap, false,
dstColorSpace.get());
} else {
Expand Down
4 changes: 2 additions & 2 deletions lib/ui/painting/image_decoding.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ sk_sp<SkImage> DecodeImage(sk_sp<SkData> buffer, size_t trace_id) {

GrContext* context = ResourceContext::Get();
if (context) {
// This acts as a flag to indicate that we want a color space aware decode.
sk_sp<SkColorSpace> dstColorSpace = SkColorSpace::MakeSRGB();
// This indicates that we do not want a "linear blending" decode.
sk_sp<SkColorSpace> dstColorSpace = nullptr;
return SkImage::MakeCrossContextFromEncoded(context, std::move(buffer),
false, dstColorSpace.get());
} else {
Expand Down
1 change: 0 additions & 1 deletion shell/common/platform_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ void PlatformView::SetupResourceContextOnIOThreadPerform(
// other threads correctly, so the textures end up blank. For now, suppress
// that feature, which will cause texture uploads to do CPU YUV conversion.
options.fDisableGpuYUVConversion = true;
options.fRequireDecodeDisableForSRGB = false;

blink::ResourceContext::Set(GrContext::Create(
GrBackend::kOpenGL_GrBackend,
Expand Down
8 changes: 8 additions & 0 deletions shell/common/surface.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "flutter/shell/common/surface.h"
#include "lib/fxl/logging.h"
#include "third_party/skia/include/core/SkColorSpaceXformCanvas.h"
#include "third_party/skia/include/core/SkSurface.h"

namespace shell {
Expand All @@ -12,6 +13,10 @@ SurfaceFrame::SurfaceFrame(sk_sp<SkSurface> surface,
SubmitCallback submit_callback)
: submitted_(false), surface_(surface), submit_callback_(submit_callback) {
FXL_DCHECK(submit_callback_);
if (surface_) {
xform_canvas_ = SkCreateColorSpaceXformCanvas(surface_->getCanvas(),
SkColorSpace::MakeSRGB());
}
}

SurfaceFrame::~SurfaceFrame() {
Expand All @@ -32,6 +37,9 @@ bool SurfaceFrame::Submit() {
}

SkCanvas* SurfaceFrame::SkiaCanvas() {
if (xform_canvas_) {
return xform_canvas_.get();
}
return surface_ != nullptr ? surface_->getCanvas() : nullptr;
}

Expand Down
1 change: 1 addition & 0 deletions shell/common/surface.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ class SurfaceFrame {
private:
bool submitted_;
sk_sp<SkSurface> surface_;
std::unique_ptr<SkCanvas> xform_canvas_;
SubmitCallback submit_callback_;

bool PerformSubmit();
Expand Down
92 changes: 12 additions & 80 deletions shell/gpu/gpu_surface_gl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,8 @@ GPUSurfaceGL::GPUSurfaceGL(GPUSurfaceGLDelegate* delegate)
auto backend_context =
reinterpret_cast<GrBackendContext>(GrGLCreateNativeInterface());

GrContextOptions options;
options.fRequireDecodeDisableForSRGB = false;

auto context = sk_sp<GrContext>(
GrContext::Create(kOpenGL_GrBackend, backend_context, options));
auto context =
sk_sp<GrContext>(GrContext::Create(kOpenGL_GrBackend, backend_context));

if (context == nullptr) {
FXL_LOG(ERROR) << "Failed to setup Skia Gr context.";
Expand Down Expand Up @@ -65,7 +62,6 @@ GPUSurfaceGL::~GPUSurfaceGL() {
}

onscreen_surface_ = nullptr;
offscreen_surface_ = nullptr;
context_->releaseResourcesAndAbandonContext();
context_ = nullptr;

Expand All @@ -76,7 +72,7 @@ bool GPUSurfaceGL::IsValid() {
return valid_;
}

static GrPixelConfig FirstSupportedNonSRGBConfig(GrContext* context) {
static GrPixelConfig FirstSupportedConfig(GrContext* context) {
#define RETURN_IF_RENDERABLE(x) \
if (context->caps()->isConfigRenderable((x), false)) { \
return (x); \
Expand All @@ -90,15 +86,12 @@ static GrPixelConfig FirstSupportedNonSRGBConfig(GrContext* context) {

static sk_sp<SkSurface> WrapOnscreenSurface(GrContext* context,
const SkISize& size,
intptr_t fbo,
bool supports_srgb) {
intptr_t fbo) {
const GrGLFramebufferInfo framebuffer_info = {
.fFBOID = static_cast<GrGLuint>(fbo),
};

const GrPixelConfig pixel_config = supports_srgb
? kSRGBA_8888_GrPixelConfig
: FirstSupportedNonSRGBConfig(context);
const GrPixelConfig pixel_config = FirstSupportedConfig(context);

GrBackendRenderTarget render_target(size.fWidth, // width
size.fHeight, // height
Expand All @@ -108,8 +101,7 @@ static sk_sp<SkSurface> WrapOnscreenSurface(GrContext* context,
framebuffer_info // framebuffer info
);

sk_sp<SkColorSpace> colorspace =
supports_srgb ? SkColorSpace::MakeSRGB() : nullptr;
sk_sp<SkColorSpace> colorspace = nullptr;

SkSurfaceProps surface_props(
SkSurfaceProps::InitType::kLegacyFontHost_InitType);
Expand All @@ -123,52 +115,29 @@ static sk_sp<SkSurface> WrapOnscreenSurface(GrContext* context,
);
}

static sk_sp<SkSurface> CreateOffscreenSurface(GrContext* context,
const SkISize& size) {
const SkImageInfo image_info =
SkImageInfo::MakeS32(size.fWidth, size.fHeight, kOpaque_SkAlphaType);

const SkSurfaceProps surface_props(
SkSurfaceProps::InitType::kLegacyFontHost_InitType);

return SkSurface::MakeRenderTarget(
context, // context
SkBudgeted::kNo, // budgeted
image_info, // image info
0, // sample count
kBottomLeft_GrSurfaceOrigin, // surface origin
&surface_props // surface props
);
}

bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) {
if (onscreen_surface_ != nullptr &&
size == SkISize::Make(onscreen_surface_->width(),
onscreen_surface_->height())) {
// We know that if there is an offscreen surface, it will be sized to be
// equal to the size of the onscreen surface. And the onscreen surface size
// appears unchanged. So bail.
// Surface size appears unchanged. So bail.
return true;
}

// We need to do some updates.
TRACE_EVENT0("flutter", "UpdateSurfacesSize");

// Either way, we need to get rid of previous surfaces.
// Either way, we need to get rid of previous surface.
onscreen_surface_ = nullptr;
offscreen_surface_ = nullptr;

if (size.isEmpty()) {
FXL_LOG(ERROR) << "Cannot create surfaces of empty size.";
return false;
}

sk_sp<SkSurface> onscreen_surface, offscreen_surface;

const bool surface_supports_srgb = delegate_->SurfaceSupportsSRGB();
sk_sp<SkSurface> onscreen_surface;

onscreen_surface = WrapOnscreenSurface(
context_.get(), size, delegate_->GLContextFBO(), surface_supports_srgb);
onscreen_surface =
WrapOnscreenSurface(context_.get(), size, delegate_->GLContextFBO());

if (onscreen_surface == nullptr) {
// If the onscreen surface could not be wrapped. There is absolutely no
Expand All @@ -177,27 +146,7 @@ bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) {
return false;
}

if (!surface_supports_srgb) {
offscreen_surface = CreateOffscreenSurface(context_.get(), size);
if (offscreen_surface == nullptr) {
// If the offscreen surface was needed but could not be wrapped. Render to
// the onscreen surface directly but warn the user that color correctness
// is not available.
static bool warned_once = false;
if (!warned_once) {
warned_once = true;
FXL_LOG(ERROR) << "WARNING: Could not create offscreen surface. This "
"device or emulator does not support "
"color correct rendering. Fallbacks are in effect. "
"Colors on this device will differ from those "
"displayed on most other devices. This warning will "
"only be logged once.";
}
}
}

onscreen_surface_ = std::move(onscreen_surface);
offscreen_surface_ = std::move(offscreen_surface);

return true;
}
Expand Down Expand Up @@ -234,23 +183,6 @@ bool GPUSurfaceGL::PresentSurface(SkCanvas* canvas) {
return false;
}

if (offscreen_surface_ != nullptr) {
// Because the surface did not support sRGB, we rendered to an offscreen
// surface. Now we must ensure that the texture is copied onscreen.
TRACE_EVENT0("flutter", "CopyTextureOnscreen");
SkPaint paint;
const GrCaps* caps = context_->caps();
if (caps->srgbSupport() && !caps->srgbDecodeDisableSupport()) {
paint.setColorFilter(SkColorFilter::MakeLinearToSRGBGamma());
}
onscreen_surface_->getCanvas()->drawImage(
offscreen_surface_->makeImageSnapshot(), // image
0, // left
0, // top
&paint // paint
);
}

{
TRACE_EVENT0("flutter", "SkCanvas::Flush");
onscreen_surface_->getCanvas()->flush();
Expand All @@ -266,7 +198,7 @@ sk_sp<SkSurface> GPUSurfaceGL::AcquireRenderSurface(const SkISize& size) {
return nullptr;
}

return offscreen_surface_ != nullptr ? offscreen_surface_ : onscreen_surface_;
return onscreen_surface_;
}

GrContext* GPUSurfaceGL::GetContext() {
Expand Down
3 changes: 0 additions & 3 deletions shell/gpu/gpu_surface_gl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@ class GPUSurfaceGLDelegate {
virtual bool GLContextPresent() = 0;

virtual intptr_t GLContextFBO() const = 0;

virtual bool SurfaceSupportsSRGB() const = 0;
};

class GPUSurfaceGL : public Surface {
Expand All @@ -42,7 +40,6 @@ class GPUSurfaceGL : public Surface {
GPUSurfaceGLDelegate* delegate_;
sk_sp<GrContext> context_;
sk_sp<SkSurface> onscreen_surface_;
sk_sp<SkSurface> offscreen_surface_;
bool valid_ = false;
fml::WeakPtrFactory<GPUSurfaceGL> weak_factory_;

Expand Down
41 changes: 2 additions & 39 deletions shell/platform/android/android_context_gl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,6 @@
#include <EGL/eglext.h>
#include <utility>

#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 <class T>
Expand Down Expand Up @@ -128,14 +120,7 @@ bool AndroidContextGL::CreateWindowSurface(
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;
}
const EGLint attribs[] = {EGL_NONE};

surface_ = eglCreateWindowSurface(
display, config_,
Expand All @@ -150,19 +135,7 @@ bool AndroidContextGL::CreatePBufferSurface() {

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;
}
const EGLint attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE};

surface_ = eglCreatePbufferSurface(display, config_, attribs);
return surface_ != EGL_NO_SURFACE;
Expand Down Expand Up @@ -206,12 +179,6 @@ AndroidContextGL::AndroidContextGL(fxl::RefPtr<AndroidEnvironmentGL> 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()) {
FXL_LOG(ERROR) << "Could not create the EGL surface.";
LogLastEGLError();
Expand Down Expand Up @@ -302,8 +269,4 @@ bool AndroidContextGL::Resize(const SkISize& size) {
return true;
}

bool AndroidContextGL::SupportsSRGB() const {
return srgb_support_;
}

} // namespace shell
3 changes: 0 additions & 3 deletions shell/platform/android/android_context_gl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,12 @@ class AndroidContextGL : public fxl::RefCountedThreadSafe<AndroidContextGL> {

bool Resize(const SkISize& size);

bool SupportsSRGB() const;

private:
fxl::RefPtr<AndroidEnvironmentGL> environment_;
fxl::RefPtr<AndroidNativeWindow> window_;
EGLConfig config_;
EGLSurface surface_;
EGLContext context_;
bool srgb_support_;
bool valid_;

AndroidContextGL(fxl::RefPtr<AndroidEnvironmentGL> env,
Expand Down
Loading

0 comments on commit 0a7155d

Please sign in to comment.