Don't trust glyph runs in XPS.
In XPS if a glyph is out of range, ignore it. Also resolve the default
font in the new way, removing the last user of SkTypefacePriv.
In PDF handle fonts with zero glyphs correctly.
Rewrite SkBitSet to keep track of its size, move properly, and make it
more obvious when certain checks are actually made instead of relying on
undefined behavior.
Add a test in a GM to ensure we don't draw anything when a glyph is
out of range on all backends.
Fix the DirectWrite SkScalerContext to pass this new test for
consistency.
Bug: chromium:1071311
Change-Id: I2583970bf1425f59d0d64e3dd7d28109991f9ea9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/286776
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>
diff --git a/gm/typeface.cpp b/gm/typeface.cpp
index 58139b5..437768b 100644
--- a/gm/typeface.cpp
+++ b/gm/typeface.cpp
@@ -154,8 +154,7 @@
////////////////////////////////////////////////////////////////////////////////
-static void draw_typeface_rendering_gm(SkCanvas* canvas, sk_sp<SkTypeface> face,
- char character = 'A') {
+static void draw_typeface_rendering_gm(SkCanvas* canvas, sk_sp<SkTypeface> face, SkGlyphID glyph) {
struct AliasType {
SkFont::Edging edging;
bool inLayer;
@@ -245,12 +244,12 @@
canvas->rotate(2, x + subpixel.offset.x(),
y + subpixel.offset.y());
}
- canvas->drawSimpleText(&character, 1, SkTextEncoding::kUTF8,
+ canvas->drawSimpleText(&glyph, sizeof(glyph), SkTextEncoding::kGlyphID,
x + subpixel.offset.x(),
y + subpixel.offset.y(), font, paint);
- SkScalar dx = SkScalarCeilToScalar(
- font.measureText(&character, 1, SkTextEncoding::kUTF8)) + 5;
+ SkScalar dx = SkScalarCeilToScalar(font.measureText(
+ &glyph, sizeof(glyph), SkTextEncoding::kGlyphID)) + 5;
x += dx;
xMax = std::max(x, xMax);
}
@@ -295,10 +294,11 @@
for (const StyleTests& style : styleTypes) {
paint.setStyle(style.style);
paint.setStrokeWidth(style.strokeWidth);
- canvas->drawSimpleText(&character, 1, SkTextEncoding::kUTF8, x, y, font, paint);
+ canvas->drawSimpleText(&glyph, sizeof(glyph), SkTextEncoding::kGlyphID,
+ x, y, font, paint);
- SkScalar dx = SkScalarCeilToScalar(font.measureText(&character, 1,
- SkTextEncoding::kUTF8)) + 5;
+ SkScalar dx = SkScalarCeilToScalar(font.measureText(
+ &glyph, sizeof(glyph), SkTextEncoding::kGlyphID)) + 5;
x += dx;
}
}
@@ -345,10 +345,11 @@
}
for (const MaskTests& mask : maskTypes) {
paint.setMaskFilter(SkMaskFilter::MakeBlur(mask.style, mask.sigma));
- canvas->drawSimpleText(&character, 1, SkTextEncoding::kUTF8, x, y, font, paint);
+ canvas->drawSimpleText(&glyph, sizeof(glyph), SkTextEncoding::kGlyphID,
+ x, y, font, paint);
- SkScalar dx = SkScalarCeilToScalar(font.measureText(&character, 1,
- SkTextEncoding::kUTF8)) + 5;
+ SkScalar dx = SkScalarCeilToScalar(font.measureText(
+ &glyph, sizeof(glyph), SkTextEncoding::kGlyphID)) + 5;
x += dx;
}
paint.setMaskFilter(nullptr);
@@ -360,7 +361,10 @@
DEF_SIMPLE_GM(typefacerendering, canvas, 640, 840) {
if (sk_sp<SkTypeface> face = MakeResourceAsTypeface("fonts/hintgasp.ttf")) {
- draw_typeface_rendering_gm(canvas, std::move(face));
+ draw_typeface_rendering_gm(canvas, face, face->unicharToGlyph('A'));
+
+ // Should draw nothing and not do anything undefined.
+ draw_typeface_rendering_gm(canvas, face, 0xFFFF);
}
}
@@ -369,14 +373,13 @@
DEF_SIMPLE_GM(typefacerendering_pfa, canvas, 640, 840) {
if (sk_sp<SkTypeface> face = MakeResourceAsTypeface("fonts/Roboto2-Regular.pfa")) {
- // This subsetted typeface doesn't have the character 'A'.
- draw_typeface_rendering_gm(canvas, std::move(face), 'O');
+ draw_typeface_rendering_gm(canvas, face, face->unicharToGlyph('O'));
}
}
DEF_SIMPLE_GM(typefacerendering_pfb, canvas, 640, 840) {
if (sk_sp<SkTypeface> face = MakeResourceAsTypeface("fonts/Roboto2-Regular.pfb")) {
- draw_typeface_rendering_gm(canvas, std::move(face), 'O');
+ draw_typeface_rendering_gm(canvas, face, face->unicharToGlyph('O'));
}
}
diff --git a/gn/core.gni b/gn/core.gni
index e86c8bc..2a01d41 100644
--- a/gn/core.gni
+++ b/gn/core.gni
@@ -386,7 +386,6 @@
"$_src/core/SkTypeface.cpp",
"$_src/core/SkTypefaceCache.cpp",
"$_src/core/SkTypefaceCache.h",
- "$_src/core/SkTypefacePriv.h",
"$_src/core/SkTypeface_remote.cpp",
"$_src/core/SkTypeface_remote.h",
"$_src/core/SkUnPreMultiply.cpp",
diff --git a/src/core/SkTypefacePriv.h b/src/core/SkTypefacePriv.h
deleted file mode 100644
index cc25b84..0000000
--- a/src/core/SkTypefacePriv.h
+++ /dev/null
@@ -1,38 +0,0 @@
-/*
- * Copyright 2013 Google Inc.
- *
- * Use of this source code is governed by a BSD-style license that can be
- * found in the LICENSE file.
- */
-
-#ifndef SkTypefacePriv_DEFINED
-#define SkTypefacePriv_DEFINED
-
-#include "include/core/SkTypeface.h"
-
-/**
- * Return a ref'd typeface, which must later be unref'd
- *
- * If the parameter is non-null, it will be ref'd and returned, otherwise
- * it will be the default typeface.
- */
-static inline sk_sp<SkTypeface> ref_or_default(SkTypeface* face) {
- return face ? sk_ref_sp(face) : SkTypeface::MakeDefault();
-}
-
-/**
- * Always resolves to a non-null typeface, either the value passed to its
- * constructor, or the default typeface if null was passed.
- */
-class SkAutoResolveDefaultTypeface : public sk_sp<SkTypeface> {
-public:
- SkAutoResolveDefaultTypeface() : INHERITED(SkTypeface::MakeDefault()) {}
-
- SkAutoResolveDefaultTypeface(SkTypeface* face)
- : INHERITED(ref_or_default(face)) {}
-
-private:
- typedef sk_sp<SkTypeface> INHERITED;
-};
-
-#endif
diff --git a/src/pdf/SkPDFDevice.cpp b/src/pdf/SkPDFDevice.cpp
index ea1c8f6..dc55238 100644
--- a/src/pdf/SkPDFDevice.cpp
+++ b/src/pdf/SkPDFDevice.cpp
@@ -868,7 +868,7 @@
ScopedOutputMarkedContentTags mark(fNodeId, fDocument, out);
- const SkGlyphID maxGlyphID = SkToU16(typeface->countGlyphs() - 1);
+ const int numGlyphs = typeface->countGlyphs();
if (clusterator.reversedChars()) {
out->writeText("/ReversedChars BMC\n");
@@ -920,7 +920,7 @@
}
for (; index < glyphLimit; ++index) {
SkGlyphID gid = glyphIDs[index];
- if (gid > maxGlyphID) {
+ if (numGlyphs <= gid) {
continue;
}
SkPoint xy = glyphRun.positions()[index];
diff --git a/src/ports/SkScalerContext_win_dw.cpp b/src/ports/SkScalerContext_win_dw.cpp
index debc51a..2200a74 100644
--- a/src/ports/SkScalerContext_win_dw.cpp
+++ b/src/ports/SkScalerContext_win_dw.cpp
@@ -217,9 +217,9 @@
const SkScalerContextEffects& effects,
const SkDescriptor* desc)
: SkScalerContext(std::move(typefaceRef), effects, desc)
- , fGlyphCount(-1) {
-
+{
DWriteFontTypeface* typeface = this->getDWriteTypeface();
+ fGlyphCount = typeface->fDWriteFontFace->GetGlyphCount();
fIsColorFont = typeface->fFactory2 &&
typeface->fDWriteFontFace2 &&
typeface->fDWriteFontFace2->IsColorFont();
@@ -373,9 +373,6 @@
}
unsigned SkScalerContext_DW::generateGlyphCount() {
- if (fGlyphCount < 0) {
- fGlyphCount = this->getDWriteTypeface()->fDWriteFontFace->GetGlyphCount();
- }
return fGlyphCount;
}
@@ -384,6 +381,13 @@
glyph->fAdvanceY = 0;
uint16_t glyphId = glyph->getGlyphID();
+
+ // DirectWrite treats all out of bounds glyph ids as having the same data as glyph 0.
+ // For consistency with all other backends, treat out of range glyph ids as an error.
+ if (fGlyphCount <= glyphId) {
+ return false;
+ }
+
DWRITE_GLYPH_METRICS gm;
if (DWRITE_MEASURING_MODE_GDI_CLASSIC == fMeasuringMode ||
@@ -654,7 +658,6 @@
void SkScalerContext_DW::generateMetrics(SkGlyph* glyph) {
-
// GetAlphaTextureBounds succeeds but sometimes returns empty bounds like
// { 0x80000000, 0x80000000, 0x80000000, 0x80000000 }
// for small, but not quite zero, sized glyphs.
@@ -1164,9 +1167,14 @@
bool SkScalerContext_DW::generatePath(SkGlyphID glyph, SkPath* path) {
SkASSERT(path);
-
path->reset();
+ // DirectWrite treats all out of bounds glyph ids as having the same data as glyph 0.
+ // For consistency with all other backends, treat out of range glyph ids as an error.
+ if (fGlyphCount <= glyph) {
+ return false;
+ }
+
SkTScopedComPtr<IDWriteGeometrySink> geometryToPath;
HRBM(SkDWriteGeometrySink::Create(path, &geometryToPath),
"Could not create geometry to path converter.");
diff --git a/src/utils/SkBitSet.h b/src/utils/SkBitSet.h
index b5c7cb2..b2bb6ef 100644
--- a/src/utils/SkBitSet.h
+++ b/src/utils/SkBitSet.h
@@ -8,51 +8,65 @@
#ifndef SkBitSet_DEFINED
#define SkBitSet_DEFINED
+#include "include/private/SkMalloc.h"
#include "include/private/SkTemplates.h"
#include "src/core/SkMathPriv.h"
+#include <climits>
+#include <limits>
+#include <memory>
class SkBitSet {
public:
- explicit SkBitSet(int numberOfBits) {
- SkASSERT(numberOfBits >= 0);
- fDwordCount = (numberOfBits + 31) / 32; // Round up size to 32-bit boundary.
- if (fDwordCount > 0) {
- fBitData.reset((uint32_t*)sk_calloc_throw(fDwordCount * sizeof(uint32_t)));
- }
- }
+ explicit SkBitSet(size_t size)
+ : fSize(size)
+ // May http://wg21.link/p0593 be accepted.
+ , fChunks((Chunk*)sk_calloc_throw(numChunksFor(fSize) * sizeof(Chunk)))
+ {}
- /** Set the value of the index-th bit to true. */
- void set(int index) {
- uint32_t mask = 1 << (index & 31);
- uint32_t* chunk = this->internalGet(index);
- SkASSERT(chunk);
- *chunk |= mask;
+ SkBitSet(const SkBitSet&) = delete;
+ SkBitSet& operator=(const SkBitSet&) = delete;
+ SkBitSet(SkBitSet&& that) : fSize(that.fSize), fChunks(std::move(that.fChunks)) {
+ that.fSize = 0;
+ }
+ SkBitSet& operator=(SkBitSet&& that) {
+ this->fSize = that.fSize;
+ this->fChunks = std::move(that.fChunks);
+ that.fSize = 0;
+ return *this;
+ }
+ ~SkBitSet() = default;
+
+ /** Set the value of the index-th bit to true. */
+ void set(size_t index) {
+ SkASSERT(index < fSize);
+ *this->chunkFor(index) |= chunkMaskFor(index);
}
/** Set the value of the index-th bit to false. */
- void clear(int index) {
- uint32_t mask = ~(1 << (index & 31));
- uint32_t* chunk = this->internalGet(index);
- SkASSERT(chunk);
- *chunk &= mask;
+ void clear(size_t index) {
+ SkASSERT(index < fSize);
+ *this->chunkFor(index) &= ~chunkMaskFor(index);
}
- bool has(int index) const {
- const uint32_t* chunk = this->internalGet(index);
- uint32_t mask = 1 << (index & 31);
- return chunk && SkToBool(*chunk & mask);
+ bool has(size_t index) const {
+ return (index < fSize) && SkToBool(*this->chunkFor(index) & chunkMaskFor(index));
}
- // Calls f(unsigned) for each set value.
+ size_t size() const {
+ return fSize;
+ }
+
+ // Calls f(size_t) for each set value.
template<typename FN>
void getSetValues(FN f) const {
- const uint32_t* data = fBitData.get();
- for (unsigned i = 0; i < fDwordCount; ++i) {
- if (uint32_t value = data[i]) { // There are set bits
- unsigned index = i * 32;
- for (unsigned j = 0; j < 32; ++j) {
- if (0x1 & (value >> j)) {
- f(index | j);
+ const Chunk* chunks = fChunks.get();
+ const size_t numChunks = numChunksFor(fSize);
+ for (size_t i = 0; i < numChunks; ++i) {
+ if (Chunk chunk = chunks[i]) { // There are set bits
+ const size_t index = i * ChunkBits;
+ for (size_t j = 0; j < ChunkBits; ++j) {
+ if (0x1 & (chunk >> j)) {
+ f(index + j);
}
}
}
@@ -62,26 +76,36 @@
// Returns the index of the first set bit
// Returns -1 if no bits are set
int leadingBitIndex() {
- const uint32_t* data = fBitData.get();
- for (unsigned i = 0; i < fDwordCount; ++i) {
- if (uint32_t value = data[i]) { // There are set bits
- int index = SkPrevLog2(value);
- return index + i * 32;
+ const Chunk* chunks = fChunks.get();
+ const size_t numChunks = numChunksFor(fSize);
+ for (size_t i = 0; i < numChunks; ++i) {
+ if (Chunk chunk = chunks[i]) { // There are set bits
+ static_assert(ChunkBits <= std::numeric_limits<uint32_t>::digits, "SkPrevLog2");
+ return i * ChunkBits + SkPrevLog2(chunk);
}
}
return -1;
}
private:
- std::unique_ptr<uint32_t, SkFunctionWrapper<void(void*), sk_free>> fBitData;
- size_t fDwordCount; // Dword (32-bit) count of the bitset.
+ size_t fSize;
- uint32_t* internalGet(int index) const {
- size_t internalIndex = index / 32;
- if (internalIndex >= fDwordCount) {
- return nullptr;
- }
- return fBitData.get() + internalIndex;
+ using Chunk = uint32_t;
+ static_assert(std::numeric_limits<Chunk>::radix == 2);
+ static constexpr size_t ChunkBits = std::numeric_limits<Chunk>::digits;
+ static_assert(ChunkBits == sizeof(Chunk)*CHAR_BIT, "It would work, but don't waste bits.");
+ std::unique_ptr<Chunk, SkFunctionWrapper<void(void*), sk_free>> fChunks;
+
+ Chunk* chunkFor(size_t index) const {
+ return fChunks.get() + (index / ChunkBits);
+ }
+
+ static constexpr Chunk chunkMaskFor(size_t index) {
+ return (Chunk)1 << (index & (ChunkBits-1));
+ }
+
+ static constexpr size_t numChunksFor(size_t size) {
+ return (size + (ChunkBits-1)) / ChunkBits;
}
};
diff --git a/src/xps/SkXPSDevice.cpp b/src/xps/SkXPSDevice.cpp
index 08eb2a4..53cd5e6 100644
--- a/src/xps/SkXPSDevice.cpp
+++ b/src/xps/SkXPSDevice.cpp
@@ -44,7 +44,6 @@
#include "src/core/SkRasterClip.h"
#include "src/core/SkStrikeCache.h"
#include "src/core/SkTLazy.h"
-#include "src/core/SkTypefacePriv.h"
#include "src/core/SkUtils.h"
#include "src/image/SkImage_Base.h"
#include "src/sfnt/SkSFNTHeader.h"
@@ -1715,7 +1714,7 @@
HRESULT SkXPSDevice::CreateTypefaceUse(const SkFont& font,
TypefaceUse** typefaceUse) {
- SkAutoResolveDefaultTypeface typeface(font.getTypeface());
+ SkTypeface* typeface = font.getTypefaceOrDefault();
//Check cache.
const SkFontID typefaceID = typeface->uniqueID();
@@ -1909,10 +1908,15 @@
// (XPS Spec 5.1.3).
FLOAT centemPerUnit = 100.0f / SkScalarToFLOAT(font.getSize());
SkAutoSTMalloc<32, XPS_GLYPH_INDEX> xpsGlyphs(glyphCount);
+ size_t numGlyphs = typeface->glyphsUsed.size();
+ size_t actualGlyphCount = 0;
for (size_t i = 0; i < glyphCount; ++i) {
+ if (numGlyphs <= glyphIDs[i]) {
+ continue;
+ }
const SkPoint& position = run.positions()[i];
- XPS_GLYPH_INDEX& xpsGlyph = xpsGlyphs[i];
+ XPS_GLYPH_INDEX& xpsGlyph = xpsGlyphs[actualGlyphCount++];
xpsGlyph.index = glyphIDs[i];
xpsGlyph.advanceWidth = 0.0f;
xpsGlyph.horizontalOffset = (SkScalarToFloat(position.fX) * centemPerUnit);
@@ -1920,6 +1924,10 @@
typeface->glyphsUsed.set(xpsGlyph.index);
}
+ if (actualGlyphCount == 0) {
+ return;
+ }
+
XPS_POINT origin = {
glyphRunList.origin().x(),
glyphRunList.origin().y(),
@@ -1929,7 +1937,7 @@
this->fCurrentXpsCanvas.get(),
typeface,
nullptr,
- xpsGlyphs.get(), glyphCount,
+ xpsGlyphs.get(), actualGlyphCount,
&origin,
SkScalarToFLOAT(font.getSize()),
XPS_STYLE_SIMULATION_NONE,