From e69791f92f11d12677b949b723cc9a5cffd0ce0d Mon Sep 17 00:00:00 2001 From: Herb Derby Date: Fri, 25 Feb 2022 05:47:07 +0000 Subject: [PATCH] Revert "store GlyphIDs in the PathOpSubmitter" This reverts commit 24d732f4ca142c60b40122d505b5313a7da63ef1. Reason for revert: Race for TSAN in DDLs and compile problem on chrome Original change's description: > store GlyphIDs in the PathOpSubmitter > > In order to support serializing, have the PathOpSubmitter store > SkPathIDs until the paths are needed for drawing, and then lookup > the paths and store them in the memory that the IDs occupied. > This is a one time operation. Wire the strike down from the > SkGlyphRunPainter down to the PathOpSubmitter. > > Change-Id: Id9382fefc252eebafeb9792f380d24ec0939316d > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/512937 > Reviewed-by: Ben Wagner > Commit-Queue: Herb Derby Change-Id: Ifab80f53f94a0bd99914bbd24201c9f933d9232b No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://skia-review.googlesource.com/c/skia/+/512941 Auto-Submit: Herb Derby Commit-Queue: Rubber Stamper Bot-Commit: Rubber Stamper --- src/core/SkGlyphRunPainter.cpp | 5 +- src/core/SkGlyphRunPainter.h | 1 - src/gpu/text/GrTextBlob.cpp | 133 +++++++++++---------------------- src/gpu/text/GrTextBlob.h | 2 - 4 files changed, 43 insertions(+), 98 deletions(-) diff --git a/src/core/SkGlyphRunPainter.cpp b/src/core/SkGlyphRunPainter.cpp index 8716cfd411..7f2aa062f7 100644 --- a/src/core/SkGlyphRunPainter.cpp +++ b/src/core/SkGlyphRunPainter.cpp @@ -420,10 +420,7 @@ void SkGlyphRunListPainter::processGlyphRun(SkGlyphRunPainterInterface* process, if (process && !fAccepted.empty()) { // processSourcePaths must be called even if there are no glyphs to make sure // runs are set correctly. - process->processSourcePaths(fAccepted.accepted(), - runFont, - strike->getUnderlyingStrike(), - strikeToSourceScale); + process->processSourcePaths(fAccepted.accepted(), runFont, strikeToSourceScale); } } } diff --git a/src/core/SkGlyphRunPainter.h b/src/core/SkGlyphRunPainter.h index 2a1d79f95c..bbc6ae4545 100644 --- a/src/core/SkGlyphRunPainter.h +++ b/src/core/SkGlyphRunPainter.h @@ -144,7 +144,6 @@ public: virtual void processSourcePaths(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) = 0; virtual void processSourceDrawables(const SkZip& accepted, diff --git a/src/gpu/text/GrTextBlob.cpp b/src/gpu/text/GrTextBlob.cpp index ef9ae52227..109f779155 100644 --- a/src/gpu/text/GrTextBlob.cpp +++ b/src/gpu/text/GrTextBlob.cpp @@ -406,23 +406,18 @@ std::tuple can_use_direct( // -- PathOpSubmitter ------------------------------------------------------------------------------ // Shared code for submitting GPU ops for drawing glyphs as paths. class PathOpSubmitter { - union Variant; - + struct PathAndPosition; public: PathOpSubmitter(bool isAntiAliased, SkScalar strikeToSourceScale, - SkSpan positions, - SkSpan variants, - sk_sp&& strike); + SkSpan paths, + std::unique_ptr pathData); PathOpSubmitter(PathOpSubmitter&& that); - ~PathOpSubmitter(); - static PathOpSubmitter Make(const SkZip& accepted, bool isAntiAliased, SkScalar strikeToSourceScale, - sk_sp&& strike, GrSubRunAllocator* alloc); void submitOps(SkCanvas*, @@ -433,83 +428,50 @@ public: skgpu::v1::SurfaceDrawContext* sdc) const; private: - union Variant { - Variant(SkGlyphID id) : glyphID{id} {} - - // A do nothing destructor. If any paths exist they are destroyed in ~PathOpSubmitter(). - ~Variant() {} - - // Initially, filled with an SkGlyphID, and is changed to a SkPath in submitOps. - SkGlyphID glyphID; - SkPath path; + struct PathAndPosition { + SkPath fPath; + SkPoint fPosition; + // Support for serialization. + SkPackedGlyphID fPackedID; }; - const bool fIsAntiAliased; const SkScalar fStrikeToSourceScale; - const SkSpan fPositions; - SkSpan fVariants; - - // If fStrike == nullptr, then the glyphIDs have been converted into Paths. - mutable sk_sp fStrike; + const SkSpan fPaths; + std::unique_ptr fPathData; }; PathOpSubmitter::PathOpSubmitter( bool isAntiAliased, SkScalar strikeToSourceScale, - SkSpan positions, - SkSpan variants, - sk_sp&& strike) - : fIsAntiAliased{isAntiAliased} - , fStrikeToSourceScale{strikeToSourceScale} - , fPositions{positions} - , fVariants{variants} - , fStrike{std::move(strike)} { - SkASSERT(!fPositions.empty()); - SkASSERT(fStrike != nullptr); + SkSpan paths, + std::unique_ptr pathData) + : fIsAntiAliased{isAntiAliased} + , fStrikeToSourceScale{strikeToSourceScale} + , fPaths{paths} + , fPathData{std::move(pathData)} { + SkASSERT(!fPaths.empty()); } PathOpSubmitter::PathOpSubmitter(PathOpSubmitter&& that) - : fIsAntiAliased{that.fIsAntiAliased} - , fStrikeToSourceScale{that.fStrikeToSourceScale} - , fPositions{that.fPositions} - , fVariants{that.fVariants} - , fStrike{std::move(that.fStrike)} { - // Clear out the variants, so "that" does not try to call the dtors of all the Paths. - that.fVariants = SkSpan(); -} - -PathOpSubmitter::~PathOpSubmitter() { - // The glyphIDs have been converted to SkPaths, so now we need to destruct them. - if (fStrike == nullptr) { - for (auto& variant : fVariants) { - variant.path.~SkPath(); - } - } -} + : fIsAntiAliased{that.fIsAntiAliased} + , fStrikeToSourceScale{that.fStrikeToSourceScale} + , fPaths{that.fPaths} + , fPathData{std::move(that.fPathData)} {} PathOpSubmitter PathOpSubmitter::Make(const SkZip& accepted, bool isAntiAliased, SkScalar strikeToSourceScale, - sk_sp&& strike, GrSubRunAllocator* alloc) { - int glyphCount = SkCount(accepted); - Variant* variantStorage = (Variant*)alloc->alignedBytes( - glyphCount * sizeof(Variant), alignof(Variant)); + auto pathData = alloc->makeUniqueArray( + accepted.size(), + [&](int i){ + auto [variant, pos] = accepted[i]; + const SkGlyph& glyph = *variant.glyph(); + return PathAndPosition{*glyph.path(), pos, glyph.getPackedID()}; + }); + SkSpan paths{pathData.get(), accepted.size()}; - SkPoint* positions = alloc->makePODArray(glyphCount); - - for (auto [pathVariant, dstPos, variant, srcPos] - : SkMakeZip(variantStorage, positions, accepted.get<0>(), accepted.get<1>())) { - const SkGlyph& glyph = *variant.glyph(); - new (&pathVariant) Variant{glyph.getGlyphID()}; - dstPos = srcPos; - } - - return PathOpSubmitter{isAntiAliased, - strikeToSourceScale, - SkMakeSpan(positions, glyphCount), - SkMakeSpan(variantStorage, glyphCount), - std::move(strike)}; + return PathOpSubmitter{isAntiAliased, strikeToSourceScale, paths, std::move(pathData)}; } void PathOpSubmitter::submitOps(SkCanvas* canvas, @@ -518,15 +480,6 @@ void PathOpSubmitter::submitOps(SkCanvas* canvas, SkPoint drawOrigin, const SkPaint& paint, skgpu::v1::SurfaceDrawContext* sdc) const { - if (fStrike != nullptr) { - // Need to convert the glyphIDs into paths. - SkBulkGlyphMetricsAndPaths pathGetter{std::move(fStrike)}; - for (Variant& variant : fVariants) { - new (&variant.path) SkPath(*pathGetter.glyph(variant.glyphID)->path()); - } - fStrike = nullptr; - } - SkPaint runPaint{paint}; runPaint.setAntiAlias(fIsAntiAliased); // If there are shaders, blurs or styles, the path must be scaled into source @@ -543,25 +496,29 @@ void PathOpSubmitter::submitOps(SkCanvas* canvas, SkMatrix strikeToSource = SkMatrix::Scale(fStrikeToSourceScale, fStrikeToSourceScale); strikeToSource.postTranslate(drawOrigin.x(), drawOrigin.y()); if (!needsExactCTM) { - for (const auto& [variant, pos] : SkMakeZip(fVariants, fPositions)) { + for (const auto& pathPos : fPaths) { + const SkPath& path = pathPos.fPath; + const SkPoint pos = pathPos.fPosition; // Transform the glyph to source space. SkMatrix pathMatrix = strikeToSource; pathMatrix.postTranslate(pos.x(), pos.y()); SkAutoCanvasRestore acr(canvas, true); canvas->concat(pathMatrix); - canvas->drawPath(variant.path, runPaint); + canvas->drawPath(path, runPaint); } } else { // Transform the path to device because the deviceMatrix must be unchanged to // draw effect, filter or shader paths. - for (const auto& [variant, pos] : SkMakeZip(fVariants, fPositions)) { + for (const auto& pathPos : fPaths) { + const SkPath& path = pathPos.fPath; + const SkPoint pos = pathPos.fPosition; // Transform the glyph to source space. SkMatrix pathMatrix = strikeToSource; pathMatrix.postTranslate(pos.x(), pos.y()); SkPath deviceOutline; - variant.path.transform(pathMatrix, &deviceOutline); + path.transform(pathMatrix, &deviceOutline); deviceOutline.setIsVolatile(true); canvas->drawPath(deviceOutline, runPaint); } @@ -576,11 +533,9 @@ public: static GrSubRunOwner Make(const SkZip& accepted, bool isAntiAliased, SkScalar strikeToSourceScale, - sk_sp&& strike, GrSubRunAllocator* alloc) { return alloc->makeUnique( - PathOpSubmitter::Make( - accepted, isAntiAliased, strikeToSourceScale, std::move(strike), alloc)); + PathOpSubmitter::Make(accepted, isAntiAliased, strikeToSourceScale, alloc)); } void draw(SkCanvas* canvas, @@ -2071,10 +2026,9 @@ void GrTextBlob::processDeviceMasks( void GrTextBlob::processSourcePaths(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) { fSubRunList.append(PathSubRun::Make( - accepted, has_some_antialiasing(runFont), strikeToSourceScale, std::move(strike), &fAlloc)); + accepted, has_some_antialiasing(runFont), strikeToSourceScale, &fAlloc)); } void GrTextBlob::processSourceDrawables(const SkZip& accepted, @@ -2779,13 +2733,11 @@ void GrSubRunNoCachePainter::processSourceMasks(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) { PathOpSubmitter pathDrawing = PathOpSubmitter::Make(accepted, has_some_antialiasing(runFont), strikeToSourceScale, - std::move(strike), fAlloc); pathDrawing.submitOps(fCanvas, fClip, fViewMatrix, fGlyphRunList.origin(), fPaint, fSDC); @@ -2863,7 +2815,7 @@ public: SkScalar strikeToSourceScale) override; void processSourcePaths( const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) override; + SkScalar strikeToSourceScale) override; void processSourceDrawables( const SkZip& drawables, const SkFont& runFont, SkScalar strikeToSourceScale) override; @@ -3456,10 +3408,9 @@ sk_sp Slug::Make(const SkMatrixProvider& viewMatrix, void Slug::processSourcePaths(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) { fSubRuns.append(PathSubRun::Make( - accepted, has_some_antialiasing(runFont), strikeToSourceScale, std::move(strike), &fAlloc)); + accepted, has_some_antialiasing(runFont), strikeToSourceScale, &fAlloc)); } void Slug::processSourceDrawables(const SkZip& accepted, diff --git a/src/gpu/text/GrTextBlob.h b/src/gpu/text/GrTextBlob.h index 18be429dd6..371c908f8f 100644 --- a/src/gpu/text/GrTextBlob.h +++ b/src/gpu/text/GrTextBlob.h @@ -262,7 +262,6 @@ private: sk_sp&& strike) override; void processSourcePaths(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) override; void processSourceDrawables(const SkZip& accepted, const SkFont& runFont, @@ -316,7 +315,6 @@ public: SkScalar strikeToSourceScale) override; void processSourcePaths(const SkZip& accepted, const SkFont& runFont, - sk_sp&& strike, SkScalar strikeToSourceScale) override; void processSourceDrawables(const SkZip& accepted, const SkFont& runFont,