From 4168981bb0087f24af5683327e42041ecc53e561 Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Wed, 11 Nov 2020 14:54:00 -0500 Subject: [PATCH] don't use MASK_FORMAT_UNKNOWN to represent scaler context state MASK_FORMAT_UNKNOWN was shared state between glyphs, and SkScalerContext making hard to reason about. Change-Id: Ifb70bcc288bc46905de5f2c32891c940a0b2a11f Reviewed-on: https://skia-review.googlesource.com/c/skia/+/334157 Reviewed-by: Ben Wagner Commit-Queue: Herb Derby --- src/core/SkGlyph.cpp | 3 - src/core/SkGlyph.h | 11 +-- src/core/SkRemoteGlyphCache.cpp | 22 +++--- src/core/SkScalerCache.cpp | 3 +- src/core/SkScalerContext.cpp | 101 +++++++++++++--------------- src/core/SkScalerContext.h | 5 +- src/utils/SkCustomTypeface.cpp | 2 +- tools/fonts/RandomScalerContext.cpp | 11 +-- tools/fonts/TestTypeface.cpp | 2 +- 9 files changed, 70 insertions(+), 90 deletions(-) diff --git a/src/core/SkGlyph.cpp b/src/core/SkGlyph.cpp index 1f911ec492..deebe83e05 100644 --- a/src/core/SkGlyph.cpp +++ b/src/core/SkGlyph.cpp @@ -15,9 +15,6 @@ constexpr SkIPoint SkPackedGlyphID::kXYFieldMask; SkMask SkGlyph::mask() const { - // getMetrics had to be called. - SkASSERT(fMaskFormat != MASK_FORMAT_UNKNOWN); - SkMask mask; mask.fImage = (uint8_t*)fImage; mask.fBounds.setXYWH(fLeft, fTop, fWidth, fHeight); diff --git a/src/core/SkGlyph.h b/src/core/SkGlyph.h index a18d070025..68718dd882 100644 --- a/src/core/SkGlyph.h +++ b/src/core/SkGlyph.h @@ -19,9 +19,6 @@ class SkArenaAlloc; class SkScalerContext; -// needs to be != to any valid SkMask::Format -#define MASK_FORMAT_UNKNOWN (0xFF) - // A combination of SkGlyphID and sub-pixel position information. struct SkPackedGlyphID { static constexpr uint32_t kImpossibleID = ~0u; @@ -64,7 +61,6 @@ struct SkPackedGlyphID { : fID{PackIDSkPoint(glyphID, pt, mask)} { } constexpr explicit SkPackedGlyphID(uint32_t v) : fID{v & kMaskAll} { } - constexpr SkPackedGlyphID() : fID{kImpossibleID} {} bool operator==(const SkPackedGlyphID& that) const { @@ -400,15 +396,12 @@ private: float fAdvanceX = 0, fAdvanceY = 0; - // This is a combination of SkMask::Format and SkGlyph state. The SkGlyph can be in one of two - // states, just the advances have been calculated, and all the metrics are available. The - // illegal mask format is used to signal that only the advances are available. - uint8_t fMaskFormat = MASK_FORMAT_UNKNOWN; + uint8_t fMaskFormat = 0; // Used by the DirectWrite scaler to track state. int8_t fForceBW = 0; - const SkPackedGlyphID fID; + SkPackedGlyphID fID; }; #endif diff --git a/src/core/SkRemoteGlyphCache.cpp b/src/core/SkRemoteGlyphCache.cpp index 6f69bea2c7..a42e75d350 100644 --- a/src/core/SkRemoteGlyphCache.cpp +++ b/src/core/SkRemoteGlyphCache.cpp @@ -474,12 +474,10 @@ void RemoteStrike::commonMaskLoop( MaskSummary* summary = fSentGlyphs.find(packedID); if (summary == nullptr) { // Put the new SkGlyph in the glyphs to send. - fMasksToSend.emplace_back(packedID); + this->ensureScalerContext(); + fMasksToSend.emplace_back(fContext->makeGlyph(packedID)); SkGlyph* glyph = &fMasksToSend.back(); - // Build the glyph - this->ensureScalerContext(); - fContext->getMetrics(glyph); MaskSummary newSummary = {packedID.value(), CanDrawAsMask(*glyph), CanDrawAsSDFT(*glyph)}; summary = fSentGlyphs.set(newSummary); @@ -507,13 +505,11 @@ void RemoteStrike::prepareForMaskDrawing( MaskSummary* summary = fSentGlyphs.find(packedID); if (summary == nullptr) { - // Put the new SkGlyph in the glyphs to send. - fMasksToSend.emplace_back(packedID); - SkGlyph* glyph = &fMasksToSend.back(); - // Build the glyph + // Put the new SkGlyph in the glyphs to send. this->ensureScalerContext(); - fContext->getMetrics(glyph); + fMasksToSend.emplace_back(fContext->makeGlyph(packedID)); + SkGlyph* glyph = &fMasksToSend.back(); MaskSummary newSummary = {packedID.value(), CanDrawAsMask(*glyph), CanDrawAsSDFT(*glyph)}; @@ -545,13 +541,11 @@ void RemoteStrike::prepareForPathDrawing( SkGlyphID glyphID = packedID.glyphID(); PathSummary* summary = fSentPaths.find(glyphID); if (summary == nullptr) { - // Put the new SkGlyph in the glyphs to send. - fPathsToSend.emplace_back(SkPackedGlyphID{glyphID}); - SkGlyph* glyph = &fPathsToSend.back(); - // Build the glyph + // Put the new SkGlyph in the glyphs to send. this->ensureScalerContext(); - fContext->getMetrics(glyph); + fPathsToSend.emplace_back(fContext->makeGlyph(SkPackedGlyphID{glyphID})); + SkGlyph* glyph = &fPathsToSend.back(); uint16_t maxDimensionOrPath = glyph->maxDimension(); // Only try to get the path if the glyphs is not color. diff --git a/src/core/SkScalerCache.cpp b/src/core/SkScalerCache.cpp index 9f47e66811..f949446018 100644 --- a/src/core/SkScalerCache.cpp +++ b/src/core/SkScalerCache.cpp @@ -48,8 +48,7 @@ std::tuple SkScalerCache::digest(SkPackedGlyphID packedGl return {*digest, 0}; } - SkGlyph* glyph = fAlloc.make(packedGlyphID); - fScalerContext->getMetrics(glyph); + SkGlyph* glyph = fAlloc.make(fScalerContext->makeGlyph(packedGlyphID)); return {this->addGlyph(glyph), sizeof(SkGlyph)}; } diff --git a/src/core/SkScalerContext.cpp b/src/core/SkScalerContext.cpp index b9b9694621..df1235f9f1 100644 --- a/src/core/SkScalerContext.cpp +++ b/src/core/SkScalerContext.cpp @@ -177,70 +177,67 @@ bool SkScalerContext::GetGammaLUTData(SkScalar contrast, SkScalar paintGamma, Sk return true; } -void SkScalerContext::getMetrics(SkGlyph* glyph) { +SkGlyph SkScalerContext::makeGlyph(SkPackedGlyphID packedID) { + return internalMakeGlyph(packedID, (SkMask::Format) fRec.fMaskFormat); +} + +SkGlyph SkScalerContext::internalMakeGlyph(SkPackedGlyphID packedID, SkMask::Format format) { + SkGlyph glyph{packedID}; + glyph.fMaskFormat = format; bool generatingImageFromPath = fGenerateImageFromPath; if (!generatingImageFromPath) { - generateMetrics(glyph); - SkASSERT(glyph->fMaskFormat != MASK_FORMAT_UNKNOWN); + generateMetrics(&glyph); } else { SkPath devPath; - generatingImageFromPath = this->internalGetPath(glyph->getPackedID(), &devPath); + generatingImageFromPath = this->internalGetPath(glyph.getPackedID(), &devPath); if (!generatingImageFromPath) { - generateMetrics(glyph); - SkASSERT(glyph->fMaskFormat != MASK_FORMAT_UNKNOWN); + generateMetrics(&glyph); } else { - uint8_t originMaskFormat = glyph->fMaskFormat; - if (!generateAdvance(glyph)) { - generateMetrics(glyph); - } - - if (originMaskFormat != MASK_FORMAT_UNKNOWN) { - glyph->fMaskFormat = originMaskFormat; - } else { - glyph->fMaskFormat = fRec.fMaskFormat; + if (!generateAdvance(&glyph)) { + generateMetrics(&glyph); } // If we are going to create the mask, then we cannot keep the color - if (SkMask::kARGB32_Format == glyph->fMaskFormat) { - glyph->fMaskFormat = SkMask::kA8_Format; + if (SkMask::kARGB32_Format == glyph.fMaskFormat) { + glyph.fMaskFormat = SkMask::kA8_Format; } const SkIRect ir = devPath.getBounds().roundOut(); if (ir.isEmpty() || !SkRectPriv::Is16Bit(ir)) { goto SK_ERROR; } - glyph->fLeft = ir.fLeft; - glyph->fTop = ir.fTop; - glyph->fWidth = SkToU16(ir.width()); - glyph->fHeight = SkToU16(ir.height()); + glyph.fLeft = ir.fLeft; + glyph.fTop = ir.fTop; + glyph.fWidth = SkToU16(ir.width()); + glyph.fHeight = SkToU16(ir.height()); const bool a8FromLCD = fRec.fFlags & SkScalerContext::kGenA8FromLCD_Flag; - const bool fromLCD = (glyph->fMaskFormat == SkMask::kLCD16_Format) || - (glyph->fMaskFormat == SkMask::kA8_Format && a8FromLCD); - if (0 < glyph->fWidth && fromLCD) { + const bool fromLCD = (glyph.fMaskFormat == SkMask::kLCD16_Format) || + (glyph.fMaskFormat == SkMask::kA8_Format && a8FromLCD); + if (0 < glyph.fWidth && fromLCD) { if (fRec.fFlags & SkScalerContext::kLCD_Vertical_Flag) { - glyph->fHeight += 2; - glyph->fTop -= 1; + glyph.fHeight += 2; + glyph.fTop -= 1; } else { - glyph->fWidth += 2; - glyph->fLeft -= 1; + glyph.fWidth += 2; + glyph.fLeft -= 1; } } } } // if either dimension is empty, zap the image bounds of the glyph - if (0 == glyph->fWidth || 0 == glyph->fHeight) { - glyph->fWidth = 0; - glyph->fHeight = 0; - glyph->fTop = 0; - glyph->fLeft = 0; - glyph->fMaskFormat = 0; - return; + if (0 == glyph.fWidth || 0 == glyph.fHeight) { + glyph.fWidth = 0; + glyph.fHeight = 0; + glyph.fTop = 0; + glyph.fLeft = 0; + glyph.fMaskFormat = 0; + return glyph; } if (fMaskFilter) { - SkMask src = glyph->mask(), + SkMask src = glyph.mask(), dst; SkMatrix matrix; @@ -252,24 +249,23 @@ void SkScalerContext::getMetrics(SkGlyph* glyph) { goto SK_ERROR; } SkASSERT(dst.fImage == nullptr); - glyph->fLeft = dst.fBounds.fLeft; - glyph->fTop = dst.fBounds.fTop; - glyph->fWidth = SkToU16(dst.fBounds.width()); - glyph->fHeight = SkToU16(dst.fBounds.height()); - glyph->fMaskFormat = dst.fFormat; + glyph.fLeft = dst.fBounds.fLeft; + glyph.fTop = dst.fBounds.fTop; + glyph.fWidth = SkToU16(dst.fBounds.width()); + glyph.fHeight = SkToU16(dst.fBounds.height()); + glyph.fMaskFormat = dst.fFormat; } } - return; + return glyph; SK_ERROR: // draw nothing 'cause we failed - glyph->fLeft = 0; - glyph->fTop = 0; - glyph->fWidth = 0; - glyph->fHeight = 0; - // put a valid value here, in case it was earlier set to - // MASK_FORMAT_JUST_ADVANCE - glyph->fMaskFormat = fRec.fMaskFormat; + glyph.fLeft = 0; + glyph.fTop = 0; + glyph.fWidth = 0; + glyph.fHeight = 0; + glyph.fMaskFormat = fRec.fMaskFormat; + return glyph; } #define SK_SHOW_TEXT_BLIT_COVERAGE 0 @@ -533,17 +529,16 @@ static void generateMask(const SkMask& mask, const SkPath& path, void SkScalerContext::getImage(const SkGlyph& origGlyph) { const SkGlyph* glyph = &origGlyph; - SkGlyph tmpGlyph{origGlyph.getPackedID()}; - // in case we need to call generateImage on a mask-format that is different // (i.e. larger) than what our caller allocated by looking at origGlyph. SkAutoMalloc tmpGlyphImageStorage; - + SkGlyph tmpGlyph; if (fMaskFilter) { // restore the prefilter bounds // need the original bounds, sans our maskfilter sk_sp mf = std::move(fMaskFilter); - this->getMetrics(&tmpGlyph); + tmpGlyph = this->internalMakeGlyph(origGlyph.getPackedID(), + (SkMask::Format) fRec.fMaskFormat); fMaskFilter = std::move(mf); // we need the prefilter bounds to be <= filter bounds diff --git a/src/core/SkScalerContext.h b/src/core/SkScalerContext.h index 3ed4e6bf58..06a2958837 100644 --- a/src/core/SkScalerContext.h +++ b/src/core/SkScalerContext.h @@ -284,7 +284,7 @@ public: bool isVertical() const { return false; } unsigned getGlyphCount() { return this->generateGlyphCount(); } - void getMetrics(SkGlyph*); + SkGlyph makeGlyph(SkPackedGlyphID); void getImage(const SkGlyph&); bool SK_WARN_UNUSED_RESULT getPath(SkPackedGlyphID, SkPath*); void getFontMetrics(SkFontMetrics*); @@ -371,7 +371,7 @@ protected: /** Generates the contents of glyph.fWidth, fHeight, fTop, fLeft, * as well as fAdvanceX and fAdvanceY if not already set. * - * TODO: fMaskFormat is set by getMetrics later; cannot be set here. + * TODO: fMaskFormat is set by internalMakeGlyph later; cannot be set here. */ virtual void generateMetrics(SkGlyph* glyph) = 0; @@ -421,6 +421,7 @@ private: /** Returns false if the glyph has no path at all. */ bool internalGetPath(SkPackedGlyphID id, SkPath* devPath); + SkGlyph internalMakeGlyph(SkPackedGlyphID packedID, SkMask::Format format); // SkMaskGamma::PreBlend converts linear masks to gamma correcting masks. protected: diff --git a/src/utils/SkCustomTypeface.cpp b/src/utils/SkCustomTypeface.cpp index debffa3637..2afe60d304 100644 --- a/src/utils/SkCustomTypeface.cpp +++ b/src/utils/SkCustomTypeface.cpp @@ -217,7 +217,7 @@ protected: void generateMetrics(SkGlyph* glyph) override { glyph->zeroMetrics(); this->generateAdvance(glyph); - // Always generates from paths, so SkScalerContext::getMetrics will figure the bounds. + // Always generates from paths, so SkScalerContext::makeGlyph will figure the bounds. } void generateImage(const SkGlyph&) override { SK_ABORT("Should have generated from path."); } diff --git a/tools/fonts/RandomScalerContext.cpp b/tools/fonts/RandomScalerContext.cpp index 5dd1ae5c53..6c1f75fc4e 100644 --- a/tools/fonts/RandomScalerContext.cpp +++ b/tools/fonts/RandomScalerContext.cpp @@ -55,14 +55,15 @@ bool RandomScalerContext::generateAdvance(SkGlyph* glyph) { return fProxy->gener void RandomScalerContext::generateMetrics(SkGlyph* glyph) { // Here we will change the mask format of the glyph // NOTE: this may be overridden by the base class (e.g. if a mask filter is applied). + SkMask::Format format = SkMask::kA8_Format; switch (glyph->getGlyphID() % 4) { - case 0: glyph->fMaskFormat = SkMask::kLCD16_Format; break; - case 1: glyph->fMaskFormat = SkMask::kA8_Format; break; - case 2: glyph->fMaskFormat = SkMask::kARGB32_Format; break; - case 3: glyph->fMaskFormat = SkMask::kBW_Format; break; + case 0: format = SkMask::kLCD16_Format; break; + case 1: format = SkMask::kA8_Format; break; + case 2: format = SkMask::kARGB32_Format; break; + case 3: format = SkMask::kBW_Format; break; } - fProxy->getMetrics(glyph); + *glyph = fProxy->internalMakeGlyph(glyph->getPackedID(), format); if (fFakeIt || (glyph->getGlyphID() % 4) != 2) { return; diff --git a/tools/fonts/TestTypeface.cpp b/tools/fonts/TestTypeface.cpp index c6927197c5..2e5398b3c9 100644 --- a/tools/fonts/TestTypeface.cpp +++ b/tools/fonts/TestTypeface.cpp @@ -175,7 +175,7 @@ protected: void generateMetrics(SkGlyph* glyph) override { glyph->zeroMetrics(); this->generateAdvance(glyph); - // Always generates from paths, so SkScalerContext::getMetrics will figure the bounds. + // Always generates from paths, so SkScalerContext::makeGlyph will figure the bounds. } void generateImage(const SkGlyph&) override { SK_ABORT("Should have generated from path."); }