Introduce delegate for Atlas regeneration I tried a few different things for this (see earlier PS if interested) and settled on adding another delegate at the recommendation of herb@, which has ended in a relatively clean way to break things up. The Ganesh- and Graphite-specific part for GlyphVector (how to add glyphs to the respective atlas) already live in src/gpu/ganesh and src/gpu/graphite, so they are only compiled and linked in when that particular backend is. However, to make either or both of them available without ifdefs, I needed a way to call them from SubRuns without forcing both of them to be there during link-time. Thus, an additional delegate which the Ganesh TextAtlasOp or Graphite Device supplies to make the call to the right backend code. Out of caution, I moved the GPU specific calls to be private and then added friends to allow the blessed part in Ganesh/Graphite to make the call. This way we don't have unintentional calls to that, which could cause link-time errors or other issues since regenerating the atlas should only be done in a single-threaded environment due to how it mutates the underlying data. I also added a way to query the required padding for a given AtlasSubRun. This was necessary in an earlier version, and continues to exist to document some otherwise magic numbers. Next step in the ongoing effort to remove #ifdefs from src/text would be looking at how the graphite instancing parts work and how we can decouple that. Change-Id: I272773ec5bc492994b04171c981f0c079e05961d Bug: skia:14317 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/711682 Reviewed-by: Herb Derby <herb@google.com> Commit-Queue: Kevin Lubick <kjlubick@google.com>
diff --git a/src/gpu/ganesh/ops/AtlasTextOp.cpp b/src/gpu/ganesh/ops/AtlasTextOp.cpp index 2dc3325..6cf037b 100644 --- a/src/gpu/ganesh/ops/AtlasTextOp.cpp +++ b/src/gpu/ganesh/ops/AtlasTextOp.cpp
@@ -26,6 +26,7 @@ #include "src/gpu/ganesh/text/GrAtlasManager.h" #include "src/text/GlyphRun.h" #include "src/text/gpu/DistanceFieldAdjustTable.h" +#include "src/text/gpu/GlyphVector.h" #include <new> #include <utility> @@ -302,11 +303,19 @@ (int)subRun.vertexStride(geo->fDrawMatrix), vertexStride); const int subRunEnd = subRun.glyphCount(); + auto regenerateDelegate = [&](sktext::gpu::GlyphVector* glyphs, + int begin, + int end, + skgpu::MaskFormat maskFormat, + int padding) { + return glyphs->regenerateAtlasForGanesh(begin, end, maskFormat, padding, target); + }; for (int subRunCursor = 0; subRunCursor < subRunEnd;) { // Regenerate the atlas for the remainder of the glyphs in the run, or the remainder // of the glyphs to fill the vertex buffer. int regenEnd = subRunCursor + std::min(subRunEnd - subRunCursor, quadEnd - quadCursor); - auto[ok, glyphsRegenerated] = subRun.regenerateAtlas(subRunCursor, regenEnd, target); + auto[ok, glyphsRegenerated] = subRun.regenerateAtlas(subRunCursor, regenEnd, + regenerateDelegate); // There was a problem allocating the glyph in the atlas. Bail. if (!ok) { return;
diff --git a/src/gpu/ganesh/text/GrAtlasManager.cpp b/src/gpu/ganesh/text/GrAtlasManager.cpp index a25edbe..477ab54 100644 --- a/src/gpu/ganesh/text/GrAtlasManager.cpp +++ b/src/gpu/ganesh/text/GrAtlasManager.cpp
@@ -364,10 +364,8 @@ namespace sktext::gpu { -std::tuple<bool, int> GlyphVector::regenerateAtlas(int begin, int end, - MaskFormat maskFormat, - int srcPadding, - GrMeshDrawTarget* target) { +std::tuple<bool, int> GlyphVector::regenerateAtlasForGanesh( + int begin, int end, MaskFormat maskFormat, int srcPadding, GrMeshDrawTarget* target) { GrAtlasManager* atlasManager = target->atlasManager(); GrDeferredUploadTarget* uploadTarget = target->deferredUploadTarget();
diff --git a/src/gpu/graphite/Device.cpp b/src/gpu/graphite/Device.cpp index dabc30d..209d19a 100644 --- a/src/gpu/graphite/Device.cpp +++ b/src/gpu/graphite/Device.cpp
@@ -55,6 +55,7 @@ #include "src/core/SkVerticesPriv.h" #include "src/shaders/SkImageShader.h" #include "src/text/GlyphRun.h" +#include "src/text/gpu/GlyphVector.h" #include "src/text/gpu/SlugImpl.h" #include "src/text/gpu/SubRunContainer.h" #include "src/text/gpu/TextBlobRedrawCoordinator.h" @@ -820,9 +821,9 @@ sktext::gpu::AtlasDrawDelegate Device::atlasDelegate() { return [&](const sktext::gpu::AtlasSubRun* subRun, - SkPoint drawOrigin, - const SkPaint& paint, - sk_sp<SkRefCnt> subRunStorage) { + SkPoint drawOrigin, + const SkPaint& paint, + sk_sp<SkRefCnt> subRunStorage) { this->drawAtlasSubRun(subRun, drawOrigin, paint, subRunStorage); }; } @@ -844,9 +845,17 @@ const SkPaint& paint, sk_sp<SkRefCnt> subRunStorage) { const int subRunEnd = subRun->glyphCount(); + auto regenerateDelegate = [&](sktext::gpu::GlyphVector* glyphs, + int begin, + int end, + skgpu::MaskFormat maskFormat, + int padding) { + return glyphs->regenerateAtlasForGraphite(begin, end, maskFormat, padding, fRecorder); + }; for (int subRunCursor = 0; subRunCursor < subRunEnd;) { // For the remainder of the run, add any atlas uploads to the Recorder's AtlasManager - auto[ok, glyphsRegenerated] = subRun->regenerateAtlas(subRunCursor, subRunEnd, fRecorder); + auto[ok, glyphsRegenerated] = subRun->regenerateAtlas(subRunCursor, subRunEnd, + regenerateDelegate); // There was a problem allocating the glyph in the atlas. Bail. if (!ok) { return;
diff --git a/src/gpu/graphite/text/AtlasManager.cpp b/src/gpu/graphite/text/AtlasManager.cpp index 1d7d12c..27f7825 100644 --- a/src/gpu/graphite/text/AtlasManager.cpp +++ b/src/gpu/graphite/text/AtlasManager.cpp
@@ -299,10 +299,11 @@ using DrawAtlas = skgpu::graphite::DrawAtlas; -std::tuple<bool, int> GlyphVector::regenerateAtlas(int begin, int end, - skgpu::MaskFormat maskFormat, - int srcPadding, - skgpu::graphite::Recorder* recorder) { +std::tuple<bool, int> GlyphVector::regenerateAtlasForGraphite(int begin, + int end, + skgpu::MaskFormat maskFormat, + int srcPadding, + skgpu::graphite::Recorder* recorder) { auto atlasManager = recorder->priv().atlasManager(); auto tokenTracker = recorder->priv().tokenTracker();
diff --git a/src/text/gpu/GlyphVector.h b/src/text/gpu/GlyphVector.h index 98d4740..6bb2964 100644 --- a/src/text/gpu/GlyphVector.h +++ b/src/text/gpu/GlyphVector.h
@@ -24,12 +24,12 @@ class SkStrikeClient; class SkWriteBuffer; -#if defined(SK_GANESH) class GrMeshDrawTarget; -#endif -#if defined(SK_GRAPHITE) -namespace skgpu::graphite { class Recorder; } -#endif +namespace skgpu::ganesh { class AtlasTextOp; } +namespace skgpu::graphite { +class Device; +class Recorder; +} namespace sktext::gpu { class Glyph; @@ -69,28 +69,30 @@ void packedGlyphIDToGlyph(StrikeCache* cache); -#if defined(SK_GANESH) - std::tuple<bool, int> regenerateAtlas( - int begin, int end, - skgpu::MaskFormat maskFormat, - int srcPadding, - GrMeshDrawTarget*); -#endif - -#if defined(SK_GRAPHITE) - std::tuple<bool, int> regenerateAtlas( - int begin, int end, - skgpu::MaskFormat maskFormat, - int srcPadding, - skgpu::graphite::Recorder*); -#endif - static size_t GlyphVectorSize(size_t count) { return sizeof(Variant) * count; } private: friend class GlyphVectorTestingPeer; + friend class ::skgpu::graphite::Device; + friend class ::skgpu::ganesh::AtlasTextOp; + + // This function is implemented in ganesh/text/GrAtlasManager.cpp, and should only be called + // from AtlasTextOp or linking issues may occur. + std::tuple<bool, int> regenerateAtlasForGanesh( + int begin, int end, + skgpu::MaskFormat maskFormat, + int srcPadding, + GrMeshDrawTarget*); + + // This function is implemented in graphite/text/AtlasManager.cpp, and should only be called + // from graphite::Device or linking issues may occur. + std::tuple<bool, int> regenerateAtlasForGraphite( + int begin, int end, + skgpu::MaskFormat maskFormat, + int srcPadding, + skgpu::graphite::Recorder*); SkStrikePromise fStrikePromise; SkSpan<Variant> fGlyphs;
diff --git a/src/text/gpu/SubRunContainer.cpp b/src/text/gpu/SubRunContainer.cpp index 2fc5c6a..24a57ac 100644 --- a/src/text/gpu/SubRunContainer.cpp +++ b/src/text/gpu/SubRunContainer.cpp
@@ -761,6 +761,8 @@ MaskFormat maskFormat() const override { return fVertexFiller.grMaskType(); } + int glyphSrcPadding() const override { return 0; } + void testingOnly_packedGlyphIDToGlyph(StrikeCache* cache) const override { fGlyphs.packedGlyphIDToGlyph(cache); } @@ -839,11 +841,6 @@ return {clip, std::move(op)}; } - std::tuple<bool, int> - regenerateAtlas(int begin, int end, GrMeshDrawTarget* target) const override { - return fGlyphs.regenerateAtlas(begin, end, fVertexFiller.grMaskType(), 0, target); - } - void fillVertexData(void* vertexDst, int offset, int count, GrColor color, const SkMatrix& drawMatrix, SkPoint drawOrigin, @@ -858,12 +855,14 @@ } #endif // defined(SK_GANESH) -#if defined(SK_GRAPHITE) - std::tuple<bool, int> - regenerateAtlas(int begin, int end, Recorder* target) const override { - return fGlyphs.regenerateAtlas(begin, end, fVertexFiller.grMaskType(), 0, target); + std::tuple<bool, int> regenerateAtlas(int begin, int end, + RegenerateAtlasDelegate regenerateAtlas) const override { + return regenerateAtlas( + &fGlyphs, begin, end, fVertexFiller.grMaskType(), this->glyphSrcPadding()); } +#if defined(SK_GRAPHITE) + std::tuple<gr::Rect, Transform> boundsAndDeviceMatrix( const Transform& localToDevice, SkPoint drawOrigin) const override { return fVertexFiller.boundsAndDeviceMatrix(localToDevice, drawOrigin); @@ -983,6 +982,8 @@ MaskFormat maskFormat() const override { return fVertexFiller.grMaskType(); } + int glyphSrcPadding() const override { return 1; } + void draw(SkCanvas*, SkPoint drawOrigin, const SkPaint& paint, @@ -1036,11 +1037,6 @@ return {clip, std::move(op)}; } - std::tuple<bool, int> regenerateAtlas(int begin, int end, - GrMeshDrawTarget* target) const override { - return fGlyphs.regenerateAtlas(begin, end, fVertexFiller.grMaskType(), 1, target); - } - void fillVertexData( void* vertexDst, int offset, int count, GrColor color, @@ -1056,12 +1052,14 @@ } #endif // defined(SK_GANESH) -#if defined(SK_GRAPHITE) - - std::tuple<bool, int> regenerateAtlas(int begin, int end, Recorder* recorder) const override { - return fGlyphs.regenerateAtlas(begin, end, fVertexFiller.grMaskType(), 1, recorder); + std::tuple<bool, int> regenerateAtlas(int begin, int end, + RegenerateAtlasDelegate regenerateAtlas) const override { + return regenerateAtlas( + &fGlyphs, begin, end, fVertexFiller.grMaskType(), this->glyphSrcPadding()); } +#if defined(SK_GRAPHITE) + std::tuple<gr::Rect, Transform> boundsAndDeviceMatrix(const Transform& localToDevice, SkPoint drawOrigin) const override { return fVertexFiller.boundsAndDeviceMatrix(localToDevice, drawOrigin); @@ -1228,6 +1226,7 @@ SkASSERT(fVertexFiller.grMaskType() == MaskFormat::kA8); return MaskFormat::kA8; } + int glyphSrcPadding() const override { return SK_DistanceFieldInset; } void draw(SkCanvas*, SkPoint drawOrigin, @@ -1288,11 +1287,6 @@ return {clip, std::move(op)}; } - std::tuple<bool, int> regenerateAtlas( - int begin, int end, GrMeshDrawTarget* target) const override { - return fGlyphs.regenerateAtlas(begin, end, MaskFormat::kA8, SK_DistanceFieldInset, target); - } - void fillVertexData( void *vertexDst, int offset, int count, GrColor color, @@ -1310,13 +1304,13 @@ #endif // defined(SK_GANESH) -#if defined(SK_GRAPHITE) - - std::tuple<bool, int> regenerateAtlas(int begin, int end, Recorder *recorder) const override { - return fGlyphs.regenerateAtlas( - begin, end, MaskFormat::kA8, SK_DistanceFieldInset, recorder); + std::tuple<bool, int> regenerateAtlas(int begin, int end, + RegenerateAtlasDelegate regenerateAtlas) const override { + return regenerateAtlas(&fGlyphs, begin, end, MaskFormat::kA8, this->glyphSrcPadding()); } +#if defined(SK_GRAPHITE) + std::tuple<gr::Rect, Transform> boundsAndDeviceMatrix(const Transform& localToDevice, SkPoint drawOrigin) const override { return fVertexFiller.boundsAndDeviceMatrix(localToDevice, drawOrigin);
diff --git a/src/text/gpu/SubRunContainer.h b/src/text/gpu/SubRunContainer.h index d6c20dd..af687c3 100644 --- a/src/text/gpu/SubRunContainer.h +++ b/src/text/gpu/SubRunContainer.h
@@ -45,7 +45,6 @@ #include "src/gpu/ganesh/ops/GrOp.h" class GrClip; -class GrMeshDrawTarget; namespace skgpu::ganesh { class SurfaceDrawContext; } @@ -57,7 +56,6 @@ #include "src/gpu/graphite/geom/Transform_graphite.h" namespace skgpu::graphite { -class Device; class DrawWriter; class Recorder; class Renderer; @@ -66,8 +64,15 @@ #endif namespace sktext::gpu { +class GlyphVector; class StrikeCache; +using RegenerateAtlasDelegate = std::function<std::tuple<bool, int>(GlyphVector*, + int begin, + int end, + skgpu::MaskFormat, + int padding)>; + // -- AtlasSubRun -------------------------------------------------------------------------------- // AtlasSubRun is the API that AtlasTextOp uses to generate vertex data for drawing. // There are three different ways AtlasSubRun is specialized. @@ -88,6 +93,7 @@ virtual int glyphCount() const = 0; virtual skgpu::MaskFormat maskFormat() const = 0; + virtual int glyphSrcPadding() const = 0; #if defined(SK_GANESH) virtual size_t vertexStride(const SkMatrix& drawMatrix) const = 0; @@ -106,17 +112,12 @@ const SkMatrix& drawMatrix, SkPoint drawOrigin, SkIRect clip) const = 0; - - // This call is not thread safe. It should only be called from GrDrawOp::onPrepare which - // is single threaded. - virtual std::tuple<bool, int> regenerateAtlas( - int begin, int end, GrMeshDrawTarget* target) const = 0; #endif + // This call is not thread safe. It should only be called from a known single-threaded env. + virtual std::tuple<bool, int> regenerateAtlas( + int begin, int end, RegenerateAtlasDelegate) const = 0; #if defined(SK_GRAPHITE) - virtual std::tuple<bool, int> regenerateAtlas( - int begin, int end, skgpu::graphite::Recorder*) const = 0; - // returns bounds of the stored data and matrix to transform it to device space virtual std::tuple<skgpu::graphite::Rect, skgpu::graphite::Transform> boundsAndDeviceMatrix( const skgpu::graphite::Transform& localToDevice, SkPoint drawOrigin) const = 0;