Skip to content

Commit

Permalink
Reland "Reland "Migrate many direct and indirect uses of SkFontMgr to…
Browse files Browse the repository at this point in the history
… use TestFontMgr""

This is a reland of commit bea8af8

This adds in the custom directory SkFontMgr to FontToolUtils.cpp,
which is important for our ChromeOS tests. Also, our sanitizers
don't always build with fontconfig support, so there need to be some
fallbacks to appease the compilation step, even though we don't
run all the binaries with the sanitizers (e.g. create_test_font)

Original change's description:
> Reland "Migrate many direct and indirect uses of SkFontMgr to use TestFontMgr"
>
> This is a reland of commit 3258c98
>
> It introduces SK_FONTMGR_*_AVAILABLE defines, such that FonToolUtils.cpp
> (and our clients) can detect at compile time what GN or Bazel depends
> on (by exporting those defines when depended upon). This avoids us
> having to make opinionated decisions based on the target platform
> (which was causing issues with ChromeOS and Android Framework).
>
> Original change's description:
> > Migrate many direct and indirect uses of SkFontMgr to use TestFontMgr
> >
> > This removes most direct (e.g. SkFontMgr::RefDefault) or indirect
> > (e.g. SkTypeface::MakeFromData) uses of the default SkFontMgr in Skia.
> >
> > There are a few other cases that impact public APIs or could impact
> > current clients, which will be handled separately (e.g. Skottie)
> >
> > Suggested Reading Order:
> >  - tools/fonts/FontToolUtils.cpp to see that this absorbed
> >    tools/flags/CommonFlagsFontMgr.cpp and the two flags which
> >    controlled FontMgrs. Notice this no longer needs an initialization
> >    call, because the first call to ToolUtils::TestFontMgr() will
> >    lazily read and apply these flags.
> >  - tools/fiddle/* to see a new fontMgr global variable is made
> >    available, which is *always* the FontConfig one, due to the
> >    fact that fiddle only runs on Linux (and there are no plans
> >    to change that).
> >  - Remaining deleted files - these had bitrotting code related to
> >    fonts that was identified via grep and deemed not worth fixing
> >    since it is not being compiled anyway.
> >  - fuzz/ to see many of these now have a convenient function to
> >    call to explicitly set up the portable fontmgr. With this
> >    and the addition of ToolUtils::FontMgrIsGDI(), src/core/SkFontMgrPriv.h
> >    is no longer used (outside of the #define guarded, deprecated
> >    FontMgr Factory functions).
> >  - Other files in any order - these remaining changes were pretty
> >    mechanical.
> >
> > See also:
> >  - http://review.skia.org/772660
> >  - http://review.skia.org/772903
> >
> > Bug: b/305780908
> > Change-Id: I46a43750ebfd9f9dbe743931cb99a5fe70a0dc0c
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/772659
> > Commit-Queue: Kevin Lubick <[email protected]>
> > Reviewed-by: Ben Wagner <[email protected]>
>
> Bug: b/305780908
> Change-Id: I361ddaa56250f76a66104ddf8a8b29942d13838b
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/775996
> Reviewed-by: Ben Wagner <[email protected]>

Bug: b/305780908
Change-Id: I88bce4d9434874f3cc9674f9f019e39c04594927
Cq-Include-Trybots: luci.skia.skia.primary:Build-Debian10-Clang-x86_64-Release-MSAN
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/777536
Reviewed-by: Ben Wagner <[email protected]>
Commit-Queue: Kevin Lubick <[email protected]>
  • Loading branch information
kjlubick authored and SkCQ committed Nov 11, 2023
1 parent 290962a commit 1e97119
Show file tree
Hide file tree
Showing 111 changed files with 662 additions and 524 deletions.
8 changes: 7 additions & 1 deletion BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,12 @@ alias(
visibility = ["//visibility:public"],
)

alias(
name = "fontmgr_fontconfig",
actual = "//src/ports:fontmgr_fontconfig",
visibility = ["//visibility:public"],
)

# Load bearing comment below - gazelle looks here (and not in any other BUILD.bazel files)
# for a special comment indicating the prefix.
# gazelle:prefix go.skia.org/skia
Expand Down Expand Up @@ -166,4 +172,4 @@ alias(
name = "go",
actual = "@go_sdk//:bin/go",
visibility = ["//visibility:public"],
)
)
66 changes: 37 additions & 29 deletions BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,7 @@ optional("fontmgr_android") {
":typeface_freetype",
"//third_party/expat",
]
public_defines = [ "SK_FONTMGR_ANDROID_AVAILABLE" ]
public = [ "include/ports/SkFontMgr_android.h" ]
sources = [
"src/ports/SkFontMgr_android.cpp",
Expand All @@ -334,7 +335,7 @@ optional("fontmgr_custom") {

optional("fontmgr_custom_directory") {
enabled = skia_enable_fontmgr_custom_directory

public_defines = [ "SK_FONTMGR_FREETYPE_DIRECTORY_AVAILABLE" ]
deps = [
":fontmgr_custom",
":typeface_freetype",
Expand All @@ -350,7 +351,7 @@ optional("fontmgr_custom_directory_factory") {

optional("fontmgr_custom_embedded") {
enabled = skia_enable_fontmgr_custom_embedded

public_defines = [ "SK_FONTMGR_FREETYPE_EMBEDDED_AVAILABLE" ]
deps = [
":fontmgr_custom",
":typeface_freetype",
Expand All @@ -365,7 +366,7 @@ optional("fontmgr_custom_embedded_factory") {

optional("fontmgr_custom_empty") {
enabled = skia_enable_fontmgr_custom_empty

public_defines = [ "SK_FONTMGR_FREETYPE_EMPTY_AVAILABLE" ]
deps = [
":fontmgr_custom",
":typeface_freetype",
Expand All @@ -381,6 +382,7 @@ optional("fontmgr_custom_empty_factory") {

optional("fontmgr_fontconfig") {
enabled = skia_enable_fontmgr_fontconfig
public_defines = [ "SK_FONTMGR_FONTCONFIG_AVAILABLE" ]

# The public header includes fontconfig.h and uses FcConfig*
public_deps = [ "//third_party:fontconfig" ]
Expand Down Expand Up @@ -438,7 +440,10 @@ optional("fontmgr_fuchsia") {
optional("fontmgr_mac_ct") {
enabled = skia_use_fonthost_mac

public_defines = [ "SK_TYPEFACE_FACTORY_CORETEXT" ]
public_defines = [
"SK_TYPEFACE_FACTORY_CORETEXT",
"SK_FONTMGR_CORETEXT_AVAILABLE",
]
public = [
"include/ports/SkFontMgr_mac_ct.h",
"include/ports/SkTypeface_mac.h",
Expand Down Expand Up @@ -480,7 +485,10 @@ optional("fontmgr_mac_ct_factory") {
optional("fontmgr_win") {
enabled = skia_enable_fontmgr_win

public_defines = [ "SK_TYPEFACE_FACTORY_DIRECTWRITE" ]
public_defines = [
"SK_TYPEFACE_FACTORY_DIRECTWRITE",
"SK_FONTMGR_DIRECTWRITE_AVAILABLE",
]
public = [ "include/ports/SkTypeface_win.h" ]
sources = [
"include/ports/SkFontMgr_indirect.h",
Expand Down Expand Up @@ -517,7 +525,7 @@ optional("fontmgr_win_factory") {

optional("fontmgr_win_gdi") {
enabled = skia_enable_fontmgr_win_gdi

public_defines = [ "SK_FONTMGR_GDI_AVAILABLE" ]
public = [ "include/ports/SkTypeface_win.h" ]
sources = [ "src/ports/SkFontHost_win.cpp" ]
libs = [ "Gdi32.lib" ]
Expand Down Expand Up @@ -1794,7 +1802,7 @@ if (skia_enable_tools) {
outputs = [ skia_h ]
}

if (target_cpu == "x64") {
if (is_linux && target_cpu == "x64") {
skia_executable("fiddle") {
check_includes = false
libs = []
Expand Down Expand Up @@ -2101,10 +2109,6 @@ if (skia_enable_tools) {
sources = [ "tools/flags/CommonFlagsImages.cpp" ]
deps = [ ":flags" ]
}
test_lib("common_flags_fontmgr") {
sources = [ "tools/flags/CommonFlagsFontMgr.cpp" ]
deps = [ ":flags" ]
}
test_lib("common_flags_aa") {
sources = [ "tools/flags/CommonFlagsAA.cpp" ]
deps = [ ":flags" ]
Expand Down Expand Up @@ -2415,7 +2419,6 @@ if (skia_enable_tools) {
deps = [
":common_flags_aa",
":common_flags_config",
":common_flags_fontmgr",
":common_flags_gpu",
":common_flags_images",
":flags",
Expand Down Expand Up @@ -2521,21 +2524,25 @@ if (skia_enable_tools) {
]
}

test_app("sktexttopdf") {
sources = [ "tools/using_skia_and_harfbuzz.cpp" ]
deps = [
":skia",
"modules/skshaper",
]
if (is_linux) {
test_app("sktexttopdf") {
sources = [ "tools/using_skia_and_harfbuzz.cpp" ]
deps = [
":skia",
"modules/skshaper",
]
}
}

test_app("create_test_font") {
sources = [ "tools/fonts/create_test_font.cpp" ]
deps = [ ":skia" ]
assert_no_deps = [
# tool_utils requires the output of this app.
":tool_utils",
]
if (is_linux || is_mac) {
test_app("create_test_font") {
sources = [ "tools/fonts/create_test_font.cpp" ]
deps = [ ":skia" ]
assert_no_deps = [
# tool_utils requires the output of this app.
":tool_utils",
]
}
}

if (skia_use_expat) {
Expand Down Expand Up @@ -2982,7 +2989,6 @@ if (skia_enable_tools) {
libs = []

deps = [
":common_flags_fontmgr",
":common_flags_gpu",
":flags",
":gm",
Expand Down Expand Up @@ -3047,9 +3053,11 @@ if (skia_enable_tools) {
}
}

test_app("editor") {
is_shared_library = is_android
deps = [ "modules/skplaintexteditor:editor_app" ]
if (is_linux || is_win || is_mac) {
test_app("editor") {
is_shared_library = is_android
deps = [ "modules/skplaintexteditor:editor_app" ]
}
}

skia_executable("image_diff_metric") {
Expand Down
2 changes: 1 addition & 1 deletion bazel/buildrc
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ build:for_android_arm64 --platforms=//bazel/platform:android_arm64 --cc_output_d
build:for_android_arm64_with_rbe --config=for_android_arm64 --config=linux_rbe

# Android device-specific configurations.
build:pixel_5 --platforms=//bazel/platform:pixel_5 --cc_output_directory_tag=pixel_5
build:pixel_5 --platforms=//bazel/platform:pixel_5 --cc_output_directory_tag=pixel_5 --fontmgr_factory=android_fontmgr_factory
build:pixel_5_with_rbe --config=pixel_5 --config=linux_rbe
build:pixel_7 --platforms=//bazel/platform:pixel_7 --cc_output_directory_tag=pixel_7
build:pixel_7_with_rbe --config=pixel_7 --config=linux_rbe
Expand Down
9 changes: 5 additions & 4 deletions bench/GlyphQuadFillBench.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include "src/text/gpu/StrikeCache.h"
#include "src/text/gpu/TextBlob.h"
#include "src/utils/SkTestCanvas.h"
#include "tools/fonts/FontToolUtils.h"

// From Project Guttenberg. This is UTF-8 text.
static const char* gText =
Expand All @@ -44,15 +45,15 @@ class DirectMaskGlyphVertexFillBenchmark : public Benchmark {
}

void onPerCanvasPreDraw(SkCanvas* canvas) override {
auto typeface = SkTypeface::MakeFromName("monospace", SkFontStyle());
auto typeface = ToolUtils::CreateTestTypeface("monospace", SkFontStyle());
SkFont font(typeface);

SkMatrix view = SkMatrix::I();
size_t len = strlen(gText);
sktext::GlyphRunBuilder builder;
SkPaint paint;
auto glyphRunList = builder.textToGlyphRunList(font, paint, gText, len, {100, 100});
SkASSERT(!glyphRunList.empty());
SkASSERT_RELEASE(!glyphRunList.empty());
auto device = SkTestCanvas<FillBench>::GetDevice(canvas);
SkMatrix drawMatrix = view;
const SkPoint drawOrigin = glyphRunList.origin();
Expand All @@ -64,14 +65,14 @@ class DirectMaskGlyphVertexFillBenchmark : public Benchmark {
SkStrikeCache::GlobalStrikeCache());

const sktext::gpu::AtlasSubRun* subRun = fBlob->testingOnlyFirstSubRun();
SkASSERT(subRun);
SkASSERT_RELEASE(subRun);
subRun->testingOnly_packedGlyphIDToGlyph(&fCache);
fVertices.reset(new char[subRun->vertexStride(drawMatrix) * subRun->glyphCount() * 4]);
}

void onDraw(int loops, SkCanvas* canvas) override {
const sktext::gpu::AtlasSubRun* subRun = fBlob->testingOnlyFirstSubRun();
SkASSERT(subRun);
SkASSERT_RELEASE(subRun);

SkIRect clip = SkIRect::MakeEmpty();
SkPaint paint;
Expand Down
3 changes: 2 additions & 1 deletion bench/ParagraphBench.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "include/core/SkPaint.h"
#include "include/core/SkString.h"
#include "tools/Resources.h"
#include "tools/fonts/FontToolUtils.h"

#if defined(SK_ENABLE_PARAGRAPH)

Expand Down Expand Up @@ -42,7 +43,7 @@ class ParagraphBench final : public Benchmark {

void onDelayedSetup() override {
fFontCollection = sk_make_sp<skia::textlayout::FontCollection>();
fFontCollection->setDefaultFontManager(SkFontMgr::RefDefault());
fFontCollection->setDefaultFontManager(ToolUtils::TestFontMgr());

fTStyle.setFontFamilies({SkString("Roboto")});
fTStyle.setColor(SK_ColorBLACK);
Expand Down
3 changes: 2 additions & 1 deletion bench/SkGlyphCacheBench.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "bench/Benchmark.h"
#include "include/core/SkCanvas.h"
#include "include/core/SkColorSpace.h"
#include "include/core/SkFontMgr.h"
#include "include/core/SkGraphics.h"
#include "include/core/SkTypeface.h"
#include "include/private/chromium/SkChromeRemoteGlyphCache.h"
Expand Down Expand Up @@ -242,7 +243,7 @@ class DiffCanvasBench : public Benchmark {
auto stream = fDataProvider();
fDiscardableManager = sk_make_sp<DiscardableManager>();
fServer.init(fDiscardableManager.get());
fTrace = SkTextBlobTrace::CreateBlobTrace(stream.get());
fTrace = SkTextBlobTrace::CreateBlobTrace(stream.get(), nullptr);
}

public:
Expand Down
3 changes: 2 additions & 1 deletion bench/TypefaceBench.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "include/core/SkTypeface.h"
#include "src/base/SkUTF.h"
#include "src/base/SkUtils.h"
#include "tools/fonts/FontToolUtils.h"

// From Project Guttenberg. This is UTF-8 text.
static const char* atext[] = {
Expand Down Expand Up @@ -234,7 +235,7 @@ class UtfToGlyph : public Benchmark {
maxGlyphs = std::max(maxGlyphs, fLines.back()->glyphCount);
}
fGlyphIds.insert(fGlyphIds.begin(), maxGlyphs, 0);
fTypeface = SkTypeface::MakeFromName("monospace", SkFontStyle());
fTypeface = ToolUtils::CreateTestTypeface("monospace", SkFontStyle());
}

void onDraw(int loops, SkCanvas* canvas) override {
Expand Down
5 changes: 4 additions & 1 deletion dm/DM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
#include "tools/ToolUtils.h"
#include "tools/flags/CommonFlags.h"
#include "tools/flags/CommonFlagsConfig.h"
#include "tools/fonts/FontToolUtils.h"
#include "tools/ios_utils.h"
#include "tools/trace/ChromeTracingTracer.h"
#include "tools/trace/EventTracingPriv.h"
Expand Down Expand Up @@ -1572,7 +1573,9 @@ int main(int argc, char** argv) {
setbuf(stdout, nullptr);
setup_crash_handler();

CommonFlags::SetDefaultFontMgr();
#if !defined(SK_DISABLE_LEGACY_FONTMGR_FACTORY)
ToolUtils::SetDefaultFontMgr();
#endif
CommonFlags::SetAnalyticAA();
skiatest::SetFontTestDataDirectory();

Expand Down
2 changes: 1 addition & 1 deletion docs/examples/ColorTypeBytesPerPixel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ void draw(SkCanvas* canvas) {
"BGRA_8888", "RGBA_1010102", "RGB_101010x", "Gray_8", "RGBA_F16Norm",
"RGBA_F16" };
SkPaint paint;
SkFont font(SkTypeface::MakeFromName("monospace", SkFontStyle()), 10);
SkFont font(fontMgr->matchFamilyStyle("monospace", SkFontStyle()), 10);
int y = 15;
canvas->drawString(" colorType bytes", 10, y, font, paint);
for (SkColorType colorType : {
Expand Down
2 changes: 1 addition & 1 deletion docs/examples/ColorTypeIsAlwaysOpaque.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ void draw(SkCanvas* canvas) {
"BGRA_8888", "RGBA_1010102", "RGB_101010x", "Gray_8", "RGBA_F16Norm",
"RGBA_F16" };
SkPaint paint;
SkFont font(SkTypeface::MakeFromName("monospace", SkFontStyle()), 10);
SkFont font(fontMgr->matchFamilyStyle("monospace", SkFontStyle()), 10);
int y = 15;
canvas->drawString(" colorType bytes", 10, y, font, paint);
for (SkColorType colorType : {
Expand Down
2 changes: 1 addition & 1 deletion docs/examples/ColorTypeValidateAlphaType.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ void draw(SkCanvas* canvas) {
kUnpremul_SkAlphaType
};
SkPaint paint;
SkFont font(SkTypeface::MakeFromName("monospace", SkFontStyle()), 10);
SkFont font(fontMgr->matchFamilyStyle("monospace", SkFontStyle()), 10);
int y = 15;
canvas->drawString(" colorType alphaType canonical", 10, y, font, paint);
for (SkColorType colorType : {
Expand Down
2 changes: 1 addition & 1 deletion docs/examples/Color_Wheel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ void draw_color_wheel(SkCanvas* canvas, float scale) {
sweep.setAntiAlias(true);
canvas->drawCircle({0, 0}, (scale - stroke) * 0.5f, sweep);
}
sk_sp<SkFontMgr> fontMgr = SkFontMgr::RefDefault();

SkFont font(fontMgr->legacyMakeTypeface(nullptr, SkFontStyle::Bold()), size);
const struct { const char* str; float radius; float angle; SkColor4f color;} kLetters[] = {
{"K", 0, 0, SkColors::kBlack},
Expand Down
14 changes: 0 additions & 14 deletions docs/examples/Paint_getTypeface.cpp

This file was deleted.

19 changes: 0 additions & 19 deletions docs/examples/Paint_refTypeface.cpp

This file was deleted.

15 changes: 0 additions & 15 deletions docs/examples/Paint_setTypeface.cpp

This file was deleted.

Loading

0 comments on commit 1e97119

Please sign in to comment.