Skip to content

Commit

Permalink
Reland "Move Skia SkFontLCDConfig globals to Chromium"
Browse files Browse the repository at this point in the history
This is a reland of 78b97c8

Also updated the tsan_suppressions.cc file with the updated names of the
globals that were moved.

Original change's description:
> Move Skia SkFontLCDConfig globals to Chromium
>
> This change eliminates the usage of the deprecated SkFontLCDConfig,
> which stores font-related monitor properties in some global variables.
> Chromium is the only client of this deprecated API, and the Skia team
> has wanted to remove it for some time.
>
> Unfortunately, the use is fairly widespread within Chromium:
> 1. The globals are set directly in a handful of places via calls to
>    SkFontLCDConfig functions.
> 2. The globals are used (indirectly) whenever an SkSurfaceProps is
>    initialized with the kLegacyFontHost_InitType flag, and also when
>    an SkCanvas or SkSurface are created without specifying the props.
>
> The correct long-term plan would be to retrieve the values from the
> Display (ideally per-monitor values) and pass them along as needed.
> But given the extensive usage, the only reasonable short-term approach
> is to lift the globals out of Skia and bring them "in-house" into
> Chromium.
>
> To that end:
> -- Undefines SK_LEGACY_SURFACE_PROPS.
> -- New globals and APIs in /skia/ext/legacy_display_globals.*
> -- Replaced "SkSurfaceProps(kLegacyFontHost_InitType)" with calls to
>    new API:  skia::LegacyDisplayGlobals::GetSkSurfaceProps()
> -- Found places where SkCanvas and SkSurface were created and props
>    weren't already specified; passing the LegacyDisplayGlobals in most
>    cases, but `nullptr` (or empty SkSurfaceProps) if it was clear
>    without doing much analysis that no text was being drawn.
>
> This gets us closer to the end-goal by allowing Skia to fully remove
> the deprecated APIs, and also surfacing a lot of places where the use
> of the globals was hidden by indirect usage.
>
> Change-Id: I8fffaee4933c03ee828d2ffab858f53d92c59f4d
> Bug: 1126992, skia:3934
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2378763
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Ben Wagner <bungeman@chromium.org>
> Reviewed-by: Michael Spang <spang@chromium.org>
> Commit-Queue: Ian Prest <iapres@microsoft.com>
> Cr-Commit-Position: refs/heads/master@{#818197}

TBR=sky@chromium.org

Bug: 1126992, 328826, skia:3934
Change-Id: I8911a67cb8a9d4079b993feaff29ff8abd02dd94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2486073
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Ian Prest <iapres@microsoft.com>
Commit-Queue: Ian Prest <iapres@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#818688}
  • Loading branch information
ijprest authored and Commit Bot committed Oct 19, 2020
1 parent 91fb74f commit e598eec
Show file tree
Hide file tree
Showing 97 changed files with 322 additions and 247 deletions.
3 changes: 1 addition & 2 deletions build/sanitizers/tsan_suppressions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ char kTSanDefaultSuppressions[] =
"deadlock:cc::VideoLayerImpl::WillDraw\n"

// http://crbug.com/328826
"race:gLCDOrder\n"
"race:gLCDOrientation\n"
"race:skia::(anonymous namespace)::g_pixel_geometry\n"

// http://crbug.com/328868
"race:PR_Lock\n"
Expand Down
3 changes: 2 additions & 1 deletion cc/benchmarks/rasterize_and_record_benchmark_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "cc/raster/raster_buffer_provider.h"
#include "cc/trees/layer_tree_host_impl.h"
#include "cc/trees/layer_tree_impl.h"
#include "skia/ext/legacy_display_globals.h"
#include "ui/gfx/geometry/axis_transform2d.h"
#include "ui/gfx/geometry/rect.h"

Expand Down Expand Up @@ -56,7 +57,7 @@ void RunBenchmark(RasterSource* raster_source,
SkBitmap bitmap;
bitmap.allocPixels(SkImageInfo::MakeN32Premul(content_rect.width(),
content_rect.height()));
SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, skia::LegacyDisplayGlobals::GetSkSurfaceProps());

// Pass an empty settings to make sure that the decode cache is used to
// replace all images.
Expand Down
11 changes: 8 additions & 3 deletions cc/layers/heads_up_display_layer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
#include "gpu/command_buffer/common/shared_image_trace_utils.h"
#include "gpu/command_buffer/common/shared_image_usage.h"
#include "gpu/config/gpu_feature_info.h"
#include "skia/ext/legacy_display_globals.h"
#include "third_party/khronos/GLES2/gl2.h"
#include "third_party/khronos/GLES2/gl2ext.h"
#include "third_party/skia/include/core/SkFont.h"
Expand Down Expand Up @@ -368,7 +369,9 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
context_provider->GrContext(),
pool_resource.color_space().ToSkColorSpace(), mailbox_texture_id,
backing->texture_target, pool_resource.size(),
pool_resource.format(), false /* can_use_lcd_text */,
pool_resource.format(),
skia::LegacyDisplayGlobals::ComputeSurfaceProps(
false /* can_use_lcd_text */),
0 /* msaa_sample_count */);
SkSurface* surface = scoped_surface.surface();
if (!surface) {
Expand All @@ -395,8 +398,9 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
if (!staging_surface_ ||
gfx::SkISizeToSize(staging_surface_->getCanvas()->getBaseLayerSize()) !=
pool_resource.size()) {
SkSurfaceProps props = skia::LegacyDisplayGlobals::GetSkSurfaceProps();
staging_surface_ = SkSurface::MakeRasterN32Premul(
pool_resource.size().width(), pool_resource.size().height());
pool_resource.size().width(), pool_resource.size().height(), &props);
}

SkiaPaintCanvas canvas(staging_surface_->getCanvas());
Expand Down Expand Up @@ -435,8 +439,9 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
pool_resource.size().width(), pool_resource.size().height());
auto* backing =
static_cast<HudSoftwareBacking*>(pool_resource.software_backing());
SkSurfaceProps props = skia::LegacyDisplayGlobals::GetSkSurfaceProps();
sk_sp<SkSurface> surface = SkSurface::MakeRasterDirect(
info, backing->shared_mapping.memory(), info.minRowBytes());
info, backing->shared_mapping.memory(), info.minRowBytes(), &props);

SkiaPaintCanvas canvas(surface->getCanvas());
DrawHudContents(&canvas);
Expand Down
62 changes: 22 additions & 40 deletions cc/paint/oop_pixeltest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
#include "gpu/skia_bindings/grcontext_for_gles2_interface.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "third_party/khronos/GLES2/gl2ext.h"
#include "third_party/skia/include/core/SkFontLCDConfig.h"
#include "third_party/skia/include/core/SkGraphics.h"
#include "third_party/skia/include/core/SkSurface.h"
#include "third_party/skia/include/gpu/GrBackendSurface.h"
Expand All @@ -48,18 +47,6 @@

namespace cc {
namespace {
class ScopedEnableLCDText {
public:
ScopedEnableLCDText() {
order_ = SkFontLCDConfig::GetSubpixelOrder();
SkFontLCDConfig::SetSubpixelOrder(SkFontLCDConfig::kRGB_LCDOrder);
}
~ScopedEnableLCDText() { SkFontLCDConfig::SetSubpixelOrder(order_); }

private:
SkFontLCDConfig::LCDOrder order_;
};

scoped_refptr<DisplayItemList> MakeNoopDisplayItemList() {
auto display_item_list = base::MakeRefCounted<DisplayItemList>();
display_item_list->StartPaint();
Expand Down Expand Up @@ -335,8 +322,7 @@ class OopPixelTest : public testing::Test,
uint32_t flags = 0;
SkSurfaceProps surface_props(flags, kUnknown_SkPixelGeometry);
if (options.use_lcd_text) {
surface_props =
SkSurfaceProps(flags, SkSurfaceProps::kLegacyFontHost_InitType);
surface_props = SkSurfaceProps(flags, kRGB_H_SkPixelGeometry);
}
SkImageInfo image_info = SkImageInfo::MakeN32Premul(
options.resource_size.width(), options.resource_size.height(),
Expand Down Expand Up @@ -519,7 +505,7 @@ TEST_P(OopImagePixelTest, DrawImage) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -558,7 +544,7 @@ TEST_P(OopImagePixelTest, DrawImageScaled) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -595,7 +581,7 @@ TEST_P(OopImagePixelTest, DrawImageShaderScaled) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -635,7 +621,7 @@ TEST_P(OopImagePixelTest, DrawRecordShaderWithImageScaled) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -727,7 +713,7 @@ TEST_P(OopImagePixelTest, DrawImageWithTargetColorSpace) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -772,7 +758,7 @@ TEST_P(OopImagePixelTest, DrawImageWithSourceColorSpace) {
color_space),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -817,7 +803,7 @@ TEST_P(OopImagePixelTest, DrawImageWithSourceAndTargetColorSpace) {
color_space),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -858,7 +844,7 @@ TEST_P(OopImagePixelTest, DrawImageWithSetMatrix) {
SkImageInfo::MakeN32Premul(image_size.width(), image_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -920,7 +906,7 @@ TEST_F(OopPixelTest, DrawMailboxBackedImage) {
SkBitmap expected_bitmap;
expected_bitmap.allocPixels(backing_info);

SkCanvas canvas(expected_bitmap);
SkCanvas canvas(expected_bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down Expand Up @@ -1014,7 +1000,7 @@ TEST_P(OopClearPixelTest, ClearingOpaqueCorner) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1065,7 +1051,7 @@ TEST_F(OopPixelTest, ClearingOpaqueCornerExactEdge) {
SkBitmap::kZeroPixels_AllocFlag);

// Expect a one pixel border on the bottom/right edge.
SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1110,7 +1096,7 @@ TEST_F(OopPixelTest, ClearingOpaqueCornerPartialRaster) {
SkBitmap::kZeroPixels_AllocFlag);

// Expect no clearing here because the playback rect is internal.
SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);

ExpectEquals(oop_result, bitmap, "oop");
Expand Down Expand Up @@ -1156,7 +1142,7 @@ TEST_P(OopClearPixelTest, ClearingOpaqueLeftEdge) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1213,7 +1199,7 @@ TEST_P(OopClearPixelTest, ClearingOpaqueRightEdge) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1269,7 +1255,7 @@ TEST_P(OopClearPixelTest, ClearingOpaqueTopEdge) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1327,7 +1313,7 @@ TEST_P(OopClearPixelTest, ClearingOpaqueBottomEdge) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
SkPaint green;
green.setColor(options.background_color);
Expand Down Expand Up @@ -1377,7 +1363,7 @@ TEST_F(OopPixelTest, ClearingOpaqueInternal) {

// Expect no clears here, as this tile does not intersect the edge of the
// tile.
SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);

ExpectEquals(oop_result, bitmap, "oop");
Expand Down Expand Up @@ -1413,7 +1399,7 @@ TEST_F(OopPixelTest, ClearingTransparentCorner) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorTRANSPARENT);

ExpectEquals(oop_result, bitmap, "oop");
Expand Down Expand Up @@ -1453,7 +1439,7 @@ TEST_F(OopPixelTest, ClearingTransparentInternalTile) {
options.resource_size.height()),
SkBitmap::kZeroPixels_AllocFlag);

SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorTRANSPARENT);

ExpectEquals(oop_result, bitmap, "oop");
Expand Down Expand Up @@ -1490,7 +1476,7 @@ TEST_F(OopPixelTest, ClearingTransparentCornerPartialRaster) {

// Result should be a red background with a cleared hole where the
// playback_rect is.
SkCanvas canvas(bitmap);
SkCanvas canvas(bitmap, SkSurfaceProps{});
canvas.drawColor(options.preclear_color);
canvas.translate(-arbitrary_offset.x(), -arbitrary_offset.y());
canvas.clipRect(gfx::RectToSkRect(options.playback_rect));
Expand Down Expand Up @@ -1709,8 +1695,6 @@ class OopRecordShaderPixelTest : public OopPixelTest,
public:
bool UseLcdText() const { return GetParam(); }
void RunTest() {
ScopedEnableLCDText enable_lcd;

RasterOptions options;
options.resource_size = gfx::Size(100, 100);
options.content_size = options.resource_size;
Expand Down Expand Up @@ -1753,8 +1737,6 @@ class OopRecordFilterPixelTest : public OopPixelTest,
public:
bool UseLcdText() const { return GetParam(); }
void RunTest(const SkMatrix& mat) {
ScopedEnableLCDText enable_lcd;

RasterOptions options;
options.resource_size = gfx::Size(100, 100);
options.content_size = options.resource_size;
Expand Down Expand Up @@ -2006,7 +1988,7 @@ TEST_F(OopPixelTest, ReadbackImagePixels) {
SkBitmap expected_bitmap;
expected_bitmap.allocPixels(dest_info);

SkCanvas canvas(expected_bitmap);
SkCanvas canvas(expected_bitmap, SkSurfaceProps{});
canvas.drawColor(SK_ColorMAGENTA);
SkPaint green;
green.setColor(SK_ColorGREEN);
Expand Down
2 changes: 1 addition & 1 deletion cc/paint/paint_op_buffer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2760,7 +2760,7 @@ void PaintOpBuffer::Playback(SkCanvas* canvas,
!has_effects_preventing_lcd_text_for_save_layer_alpha_;
if (save_layer_alpha_should_preserve_lcd_text) {
// Check if the canvas supports LCD text.
SkSurfaceProps props(SkSurfaceProps::kLegacyFontHost_InitType);
SkSurfaceProps props;
canvas->getProps(&props);
if (props.pixelGeometry() == kUnknown_SkPixelGeometry)
save_layer_alpha_should_preserve_lcd_text = false;
Expand Down
17 changes: 3 additions & 14 deletions cc/paint/paint_op_buffer_serializer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/trace_event/trace_event.h"
#include "cc/paint/clear_for_opaque_raster.h"
#include "cc/paint/scoped_raster_flags.h"
#include "skia/ext/legacy_display_globals.h"
#include "ui/gfx/skia_util.h"

namespace cc {
Expand All @@ -31,19 +32,6 @@ class ScopedFlagsOverride {
PaintOp::SerializeOptions* options_;
};

// Copied from viz::ClientResourceProvider.
SkSurfaceProps ComputeSurfaceProps(bool can_use_lcd_text) {
uint32_t flags = 0;
// Use unknown pixel geometry to disable LCD text.
SkSurfaceProps surface_props(flags, kUnknown_SkPixelGeometry);
if (can_use_lcd_text) {
// LegacyFontHost will get LCD text and skia figures out what type to use.
surface_props =
SkSurfaceProps(flags, SkSurfaceProps::kLegacyFontHost_InitType);
}
return surface_props;
}

PlaybackParams MakeParams(const SkCanvas* canvas) {
// We don't use an ImageProvider here since the ops are played onto a no-draw
// canvas for state tracking and don't need decoded images.
Expand Down Expand Up @@ -81,7 +69,8 @@ PaintOpBufferSerializer::PaintOpBufferSerializer(
? std::make_unique<SkTextBlobCacheDiffCanvas>(
kMaxExtent,
kMaxExtent,
ComputeSurfaceProps(can_use_lcd_text),
skia::LegacyDisplayGlobals::ComputeSurfaceProps(
can_use_lcd_text),
strike_server,
std::move(color_space),
context_supports_distance_field_text)
Expand Down
4 changes: 3 additions & 1 deletion cc/paint/skia_paint_canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "cc/paint/display_item_list.h"
#include "cc/paint/paint_recorder.h"
#include "cc/paint/scoped_raster_flags.h"
#include "skia/ext/legacy_display_globals.h"
#include "third_party/skia/include/core/SkAnnotation.h"
#include "third_party/skia/include/docs/SkPDFDocument.h"

Expand All @@ -24,7 +25,8 @@ SkiaPaintCanvas::SkiaPaintCanvas(SkCanvas* canvas,

SkiaPaintCanvas::SkiaPaintCanvas(const SkBitmap& bitmap,
ImageProvider* image_provider)
: canvas_(new SkCanvas(bitmap)),
: canvas_(new SkCanvas(bitmap,
skia::LegacyDisplayGlobals::GetSkSurfaceProps())),
bitmap_(bitmap),
owned_(canvas_),
image_provider_(image_provider) {}
Expand Down
Loading

0 comments on commit e598eec

Please sign in to comment.