Simplified Skyline

Preparing for glyph zero-padding optimization.
This is the main reason for this change: the current data
structures do not support the optimization.

After I put this change under supportBilerpFromGlyphAtlas flag I could not even compare the results with and without this change (the flag increases glyph sizes). But from my previous measurement it gives saves about 1% of the memory (or 1 plot eviction) on the test.

Change-Id: Ibb66821d220a98487acba88f0ce27ece23e5a669
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/550617
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Julia Lavrova <jlavrova@google.com>

gn/gpu.gni

@@ -789,6 +789,8 @@ skia_shared_gpu_sources = [
   "$_src/gpu/BufferWriter.h",
   "$_src/gpu/KeyBuilder.h",
   "$_src/gpu/Rectanizer.h",
+  "$_src/gpu/RectanizerOptimized.cpp",
+  "$_src/gpu/RectanizerOptimized.h",
   "$_src/gpu/RectanizerPow2.cpp",
   "$_src/gpu/RectanizerPow2.h",
   "$_src/gpu/RectanizerSkyline.cpp",

public.bzl

@@ -791,6 +791,8 @@ BASE_SRCS_ALL = [
     "src/gpu/GrRectanizer.h",
     "src/gpu/KeyBuilder.h",
     "src/gpu/Rectanizer.h",
+    "src/gpu/RectanizerOptimized.cpp",
+    "src/gpu/RectanizerOptimized.h",
     "src/gpu/RectanizerPow2.cpp",
     "src/gpu/RectanizerPow2.h",
     "src/gpu/RectanizerSkyline.cpp",

src/gpu/AtlasTypes.cpp

@@ -5,15 +5,16 @@
  * found in the LICENSE file.
  */
 
-#include "src/gpu/AtlasTypes.h"
-
 #include "include/private/SkMalloc.h"
 #include "src/core/SkOpts.h"
+#include "src/gpu/AtlasTypes.h"
+#include "src/gpu/RectanizerOptimized.h"
 
 namespace skgpu {
 
 Plot::Plot(int pageIndex, int plotIndex, AtlasGenerationCounter* generationCounter,
-           int offX, int offY, int width, int height, SkColorType colorType, size_t bpp)
+           int offX, int offY, int width, int height, SkColorType colorType, size_t bpp,
+           PadAllGlyphs padAllGLyphs)
         : fLastUpload(DrawToken::AlreadyFlushedToken())
         , fLastUse(DrawToken::AlreadyFlushedToken())
         , fFlushesSinceLastUse(0)
@@ -27,7 +28,6 @@ Plot::Plot(int pageIndex, int plotIndex, AtlasGenerationCounter* generationCount
         , fHeight(height)
         , fX(offX)
         , fY(offY)
-        , fRectanizer(width, height)
         , fOffset(SkIPoint16::Make(fX * fWidth, fY * fHeight))
         , fColorType(colorType)
         , fBytesPerPixel(bpp)
@@ -35,6 +35,11 @@ Plot::Plot(int pageIndex, int plotIndex, AtlasGenerationCounter* generationCount
         , fDirty(false)
 #endif
 {
+    if (padAllGLyphs == PadAllGlyphs::kYes) {
+        fRectanizer = std::make_unique<RectanizerOptimized>(width, height);
+    } else {
+        fRectanizer = std::make_unique<RectanizerSkyline>(width, height);
+    }
     // We expect the allocated dimensions to be a multiple of 4 bytes
     SkASSERT(((width*fBytesPerPixel) & 0x3) == 0);
     // The padding for faster uploads only works for 1, 2 and 4 byte texels
@@ -46,11 +51,15 @@ Plot::~Plot() {
     sk_free(fData);
 }
 
+PadAllGlyphs Plot::padAllGlyphs() const {
+    return this->fRectanizer->padAllGlyphs();
+}
+
 bool Plot::addSubImage(int width, int height, const void* image, AtlasLocator* atlasLocator) {
     SkASSERT(width <= fWidth && height <= fHeight);
 
     SkIPoint16 loc;
-    if (!fRectanizer.addRect(width, height, &loc)) {
+    if (!fRectanizer->addRect(width, height, &loc)) {
         return false;
     }
 
@@ -114,7 +123,7 @@ std::pair<const void*, SkIRect> Plot::prepareForUpload() {
 }
 
 void Plot::resetRects() {
-    fRectanizer.reset();
+    fRectanizer->reset();
 
     fGenID = fGenerationCounter->next();
     fPlotLocator = PlotLocator(fPageIndex, fPlotIndex, fGenID);

src/gpu/AtlasTypes.h

@@ -18,11 +18,12 @@
 #include "include/private/SkTo.h"
 #include "src/core/SkIPoint16.h"
 #include "src/core/SkTInternalLList.h"
-#include "src/gpu/RectanizerSkyline.h"
 
 class GrOpFlushState;
 class TestingUploadTarget;
 namespace skgpu::graphite { class AtlasManager; }
+namespace skgpu { class Rectanizer; }
+namespace skgpu { enum class PadAllGlyphs : bool; }
 
 /**
  * This file includes internal types that are used by all of our gpu backends for atlases.
@@ -300,6 +301,10 @@ public:
         return {fUVs[0] & 0x1FFF, fUVs[1]};
     }
 
+    SkIPoint bottomRight() const {
+        return {fUVs[2] & 0x1FFF, fUVs[3]};
+    }
+
     uint16_t width() const {
         return fUVs[2] - fUVs[0];
     }
@@ -421,7 +426,8 @@ class Plot : public SkRefCnt {
 
 public:
     Plot(int pageIndex, int plotIndex, AtlasGenerationCounter* generationCounter,
-         int offX, int offY, int width, int height, SkColorType colorType, size_t bpp);
+         int offX, int offY, int width, int height, SkColorType colorType, size_t bpp,
+         PadAllGlyphs padAllGlyphs);
 
     uint32_t pageIndex() const { return fPageIndex; }
 
@@ -438,7 +444,10 @@ public:
     }
     SkDEBUGCODE(size_t bpp() const { return fBytesPerPixel; })
 
-    bool addSubImage(int width, int height, const void* image, AtlasLocator* atlasLocator);
+    bool addSubImage(int width,
+                     int height,
+                     const void* image,
+                     AtlasLocator* atlasLocator);
 
     /**
      * To manage the lifetime of a plot, we use two tokens. We use the last upload token to
@@ -467,9 +476,11 @@ public:
     sk_sp<Plot> clone() const {
         return sk_sp<Plot>(new Plot(
             fPageIndex, fPlotIndex, fGenerationCounter, fX, fY, fWidth, fHeight, fColorType,
-            fBytesPerPixel));
+            fBytesPerPixel, this->padAllGlyphs()));
     }
 
+    PadAllGlyphs padAllGlyphs() const;
+
 #ifdef SK_DEBUG
     void resetListPtrs() {
         fPrev = fNext = nullptr;
@@ -478,6 +489,7 @@ public:
 #endif
 
 private:
+    friend class PlotTestingPeer;
     ~Plot() override;
 
     skgpu::DrawToken fLastUpload;
@@ -496,7 +508,7 @@ private:
     const int fHeight;
     const int fX;
     const int fY;
-    skgpu::RectanizerSkyline fRectanizer;
+    std::unique_ptr<skgpu::Rectanizer> fRectanizer;
     const SkIPoint16 fOffset;  // the offset of the plot in the backing texture
     const SkColorType fColorType;
     const size_t fBytesPerPixel;

src/gpu/BUILD.bazel

@@ -16,6 +16,8 @@ CORE_FILES = [
     "Rectanizer.h",
     "RectanizerPow2.cpp",
     "RectanizerPow2.h",
+    "RectanizerOptimized.cpp",
+    "RectanizerOptimized.h",
     "RectanizerSkyline.cpp",
     "RectanizerSkyline.h",
     "RefCntedCallback.h",

src/gpu/Rectanizer.h

@@ -14,6 +14,11 @@ struct SkIPoint16;
 
 namespace skgpu {
 
+enum class PadAllGlyphs : bool {
+    kNo = false,
+    kYes = true
+};
+
 class Rectanizer {
 public:
     Rectanizer(int width, int height) : fWidth(width), fHeight(height) {
@@ -23,24 +28,30 @@ public:
 
     virtual ~Rectanizer() {}
 
-    virtual void reset() = 0;
+    virtual void reset() {
+        fAreaSoFar = 0;
+    }
 
     int width() const { return fWidth; }
     int height() const { return fHeight; }
+    float percentFull() const {
+        return fAreaSoFar / ((float)this->width() * this->height());
+    }
 
     // Attempt to add a rect. Return true on success; false on failure. If
     // successful the position in the atlas is returned in 'loc'.
     virtual bool addRect(int width, int height, SkIPoint16* loc) = 0;
-    virtual float percentFull() const = 0;
+    virtual PadAllGlyphs padAllGlyphs() const = 0;
-
     /**
      *  Our factory, which returns the subclass du jour
      */
     static Rectanizer* Factory(int width, int height);
 
-private:
+protected:
+    friend class RectanizerSkylineTestingPeer;
     const int fWidth;
     const int fHeight;
+    int32_t fAreaSoFar;
 };
 
 } // End of namespace skgpu

src/gpu/RectanizerOptimized.cpp

@@ -0,0 +1,86 @@
+/*
+ * Copyright 2022 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#include "src/core/SkIPoint16.h"
+#include "src/gpu/RectanizerOptimized.h"
+
+#include <algorithm>
+
+namespace skgpu {
+
+// This method walks across the entire profile trying to find the area big enough to place a
+// rectangle width * hight.
+// It returns the area with mininum possible y coordinage,
+// and if there are few like this, minumum x.
+bool RectanizerOptimized::addRect(int width, int height, SkIPoint16* loc) {
+
+    SkASSERT(width > 0 && height > 0);
+    if (width > this->width() || height > this->height()) {
+        return false;
+    }
+
+    SkIPoint16 bestLoc = SkIPoint16::Make(this->width(), this->height());
+    SkIPoint16 currLoc = SkIPoint16::Make(0, 0);
+    bool found = false;
+    int16_t x = 0;
+    // We are looking for a rectangle [currLoc.fX:x) * [0:currLoc.fY)
+    while (x < this->width() && currLoc.fX <= this->width() - width) {
+
+        if (currLoc.fX + width == x) {
+            // We found the area: [currLoc.fX:x) * [0:currLoc.fY)
+            found = true;
+            // Let's see if it's better (we do not take in account optimized flag)
+            if (currLoc.fY < bestLoc.fY) {
+                bestLoc = currLoc;
+            }
+            // Let's see if we can find better y starting over from the currLoc.fX+1.
+            x = (++currLoc.fX);
+            currLoc.fY = fProfile[x];
+            continue;
+        }
+
+        // Still looking
+        currLoc.fY = std::max(currLoc.fY, fProfile[x]);
+        ++x;
+        // If this column does not fit vertically start all over from the next one
+        if (this->height() < currLoc.fY + height) {
+            // We start over from the next x (nothing before will be good enough)
+            currLoc.fY = fProfile[x];
+            currLoc.fX = x;
+        }
+    }
+
+    if (found) {
+        // Mark the area as allocated
+        fAreaSoFar += width*height;
+        this->markAreaOccupied(bestLoc, width, height);
+        *loc = bestLoc;
+        return true;
+    }
+
+    loc->fX = 0;
+    loc->fY = 0;
+    return false;
+
+}
+
+void RectanizerOptimized::markAreaOccupied(SkIPoint16 loc,
+                                           int width,
+                                           int height) {
+    auto y = loc.fY + SkToS16(height);
+    for (int16_t x = loc.fX; x < loc.fX + SkToS16(width); ++x) {
+        fProfile[x] = y;
+    }
+}
+
+///////////////////////////////////////////////////////////////////////////////
+
+Rectanizer* Rectanizer::Factory(int width, int height) {
+    return new RectanizerOptimized(width, height);
+}
+
+} // End of namespace skgpu

src/gpu/RectanizerOptimized.h

@@ -0,0 +1,75 @@
+/*
+ * Copyright 2022 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+#ifndef skgpu_RectanizerOptimized_DEFINED
+#define skgpu_RectanizerOptimized_DEFINED
+
+#include "include/private/SkBitmaskEnum.h"
+#include "include/private/SkTDArray.h"
+#include "src/gpu/RectanizerSkyline.h"
+#include "src/utils/SkBitSet.h"
+
+#include <vector>
+
+namespace skgpu {
+enum class OptFlags {
+    None = 0x00,
+    T = 0x01,
+    L = 0x02,
+    R = 0x04,
+    TL = 0x03,
+    TR = 0x05,
+    LR = 0x06,
+    TLR = 0x07,
+};
+}
+
+namespace sknonstd {
+template <> struct is_bitmask_enum<skgpu::OptFlags> : std::true_type {};
+}
+
+namespace skgpu {
+// This class tracks the entire profile line without breaking it into ranges.
+// Each column of a plot has an integer indicating the hight of an allocated area in this column.
+// It works slightly better that RectanizerSkyline because it always selects a free area with
+// the minimum y, minimum x. RectanizerSkyline selects minimin y, minimum width of a range. It
+// gives RectanizerOptimized slight advantage (~1% of saved Atlas memory and 1 less plot eviction
+// for a very big test).
+// That advantage alone would never justify changing already working algorithm, but it makes much
+// easier the upcoming zero padding glyphs optimization.
+// (It also makes easier changing the skyline somewhat, but again that is not the point)
+// Mark this class final in an effort to avoid the vtable when this subclass is used explicitly.
+class RectanizerOptimized final : public Rectanizer {
+public:
+    RectanizerOptimized(int w, int h) : Rectanizer(w, h) {
+        this->reset();
+    }
+
+    ~RectanizerOptimized() final { }
+
+    void reset() final {
+        Rectanizer::reset();
+        fProfile.resize(this->width() + 1); // One extra element to avoid extra checking
+        std::fill(fProfile.begin(), fProfile.end(), 0);
+    }
+
+    bool addRect(int w, int h, SkIPoint16* loc) override;
+
+    PadAllGlyphs padAllGlyphs() const override { return PadAllGlyphs::kYes; }
+
+private:
+    // Update the skyline structure to include a width x height rect located
+    // at x,y and the information about the pixels
+    void markAreaOccupied(SkIPoint16 loc, int width, int height);
+
+    // The distance from the top edge which is already occupied
+    std::vector<int16_t> fProfile;
+};
+
+} // End of namespace skgpu
+
+#endif

src/gpu/RectanizerPow2.h

@@ -31,16 +31,13 @@ public:
     ~RectanizerPow2() final {}
 
     void reset() final {
+        Rectanizer::reset();
         fNextStripY = 0;
-        fAreaSoFar = 0;
         sk_bzero(fRows, sizeof(fRows));
     }
 
     bool addRect(int w, int h, SkIPoint16* loc) final;
-
+    PadAllGlyphs padAllGlyphs() const override { return PadAllGlyphs::kNo; }
-    float percentFull() const final {
-        return fAreaSoFar / ((float)this->width() * this->height());
-    }
 
 private:
     static const int kMIN_HEIGHT_POW2 = 2;
@@ -60,7 +57,6 @@ private:
     Row fRows[kMaxExponent];    // 0-th entry will be unused
 
     int fNextStripY;
-    int32_t fAreaSoFar;
 
     static int HeightToRowIndex(int height) {
         SkASSERT(height >= kMIN_HEIGHT_POW2);

src/gpu/RectanizerSkyline.cpp

@@ -118,10 +118,4 @@ void RectanizerSkyline::addSkylineLevel(int skylineIndex, int x, int y, int widt
     }
 }
 
-///////////////////////////////////////////////////////////////////////////////
-
-Rectanizer* Rectanizer::Factory(int width, int height) {
-    return new RectanizerSkyline(width, height);
-}
-
 } // End of namespace skgpu

src/gpu/RectanizerSkyline.h

@@ -23,10 +23,10 @@ public:
         this->reset();
     }
 
-    ~RectanizerSkyline() final { }
+    ~RectanizerSkyline() override { }
 
-    void reset() final {
+    void reset() override {
-        fAreaSoFar = 0;
+        Rectanizer::reset();
         fSkyline.reset();
         SkylineSegment* seg = fSkyline.append(1);
         seg->fX = 0;
@@ -34,31 +34,26 @@ public:
         seg->fWidth = this->width();
     }
 
-    bool addRect(int w, int h, SkIPoint16* loc) final;
+    bool addRect(int w, int h, SkIPoint16* loc) override;
-
+    PadAllGlyphs padAllGlyphs() const override { return PadAllGlyphs::kNo; }
-    float percentFull() const final {
-        return fAreaSoFar / ((float)this->width() * this->height());
-    }
 
 private:
-    struct SkylineSegment {
+    // Update the skyline structure to include a width x height rect located
-        int  fX;
+    // at x,y.
-        int  fY;
+    void addSkylineLevel(int skylineIndex, int x, int y, int width, int height);
-        int  fWidth;
-    };
-
-    SkTDArray<SkylineSegment> fSkyline;
-
-    int32_t fAreaSoFar;
 
     // Can a width x height rectangle fit in the free space represented by
     // the skyline segments >= 'skylineIndex'? If so, return true and fill in
     // 'y' with the y-location at which it fits (the x location is pulled from
     // 'skylineIndex's segment.
     bool rectangleFits(int skylineIndex, int width, int height, int* y) const;
-    // Update the skyline structure to include a width x height rect located
+
-    // at x,y.
+    struct SkylineSegment {
-    void addSkylineLevel(int skylineIndex, int x, int y, int width, int height);
+        int  fX;
+        int  fY;
+        int  fWidth;
+    };
+    SkTDArray<SkylineSegment> fSkyline;
 };
 
 } // End of namespace skgpu

src/gpu/ganesh/GrDirectContext.cpp

@@ -262,13 +262,19 @@ bool GrDirectContext::init() {
     } else {
         allowMultitexturing = GrDrawOpAtlas::AllowMultitexturing::kYes;
     }
+    skgpu::PadAllGlyphs padAllGlyphs;
+    if (this->options().fSupportBilerpFromGlyphAtlas) {
+        padAllGlyphs = skgpu::PadAllGlyphs::kYes;
+    } else {
+        padAllGlyphs = skgpu::PadAllGlyphs::kNo;
+    }
 
     GrProxyProvider* proxyProvider = this->priv().proxyProvider();
 
     fAtlasManager = std::make_unique<GrAtlasManager>(proxyProvider,
                                                      this->options().fGlyphCacheTextureMaximumBytes,
                                                      allowMultitexturing,
-                                                     this->options().fSupportBilerpFromGlyphAtlas);
+                                                     padAllGlyphs);
     this->priv().addOnFlushCallbackObject(fAtlasManager.get());
 
     return true;

src/gpu/ganesh/GrDrawOpAtlas.cpp

@@ -72,7 +72,8 @@ std::unique_ptr<GrDrawOpAtlas> GrDrawOpAtlas::Make(GrProxyProvider* proxyProvide
                                                    GenerationCounter* generationCounter,
                                                    AllowMultitexturing allowMultitexturing,
                                                    EvictionCallback* evictor,
-                                                   std::string_view label) {
+                                                   std::string_view label,
+                                                   skgpu::PadAllGlyphs padAllGlyphs) {
     if (!format.isValid()) {
         return nullptr;
     }
@@ -80,7 +81,8 @@ std::unique_ptr<GrDrawOpAtlas> GrDrawOpAtlas::Make(GrProxyProvider* proxyProvide
     std::unique_ptr<GrDrawOpAtlas> atlas(new GrDrawOpAtlas(proxyProvider, format, colorType, bpp,
                                                            width, height, plotWidth, plotHeight,
                                                            generationCounter,
-                                                           allowMultitexturing, label));
+                                                           allowMultitexturing, label,
+                                                           padAllGlyphs));
     if (!atlas->getViews()[0].proxy()) {
         return nullptr;
     }
@@ -96,7 +98,8 @@ std::unique_ptr<GrDrawOpAtlas> GrDrawOpAtlas::Make(GrProxyProvider* proxyProvide
 GrDrawOpAtlas::GrDrawOpAtlas(GrProxyProvider* proxyProvider, const GrBackendFormat& format,
                              SkColorType colorType, size_t bpp, int width, int height,
                              int plotWidth, int plotHeight, GenerationCounter* generationCounter,
-                             AllowMultitexturing allowMultitexturing, std::string_view label)
+                             AllowMultitexturing allowMultitexturing, std::string_view label,
+                             skgpu::PadAllGlyphs padAllGlyphs)
         : fFormat(format)
         , fColorType(colorType)
         , fBytesPerPixel(bpp)
@@ -111,7 +114,8 @@ GrDrawOpAtlas::GrDrawOpAtlas(GrProxyProvider* proxyProvider, const GrBackendForm
         , fFlushesSinceLastUse(0)
         , fMaxPages(AllowMultitexturing::kYes == allowMultitexturing ?
                             PlotLocator::kMaxMultitexturePages : 1)
-        , fNumActivePages(0) {
+        , fNumActivePages(0)
+        , fPadAllGlyphs(padAllGlyphs) {
     int numPlotsX = width/plotWidth;
     int numPlotsY = height/plotHeight;
     SkASSERT(numPlotsX * numPlotsY <= PlotLocator::kMaxPlots);
@@ -495,9 +499,16 @@ bool GrDrawOpAtlas::createPages(
         for (int y = numPlotsY - 1, r = 0; y >= 0; --y, ++r) {
             for (int x = numPlotsX - 1, c = 0; x >= 0; --x, ++c) {
                 uint32_t plotIndex = r * numPlotsX + c;
-                currPlot->reset(new Plot(
+                currPlot->reset(new Plot(i,
-                    i, plotIndex, generationCounter, x, y, fPlotWidth, fPlotHeight, fColorType,
+                                         plotIndex,
-                    fBytesPerPixel));
+                                         generationCounter,
+                                         x,
+                                         y,
+                                         fPlotWidth,
+                                         fPlotHeight,
+                                         fColorType,
+                                         fBytesPerPixel,
+                                         fPadAllGlyphs));
 
                 // build LRU list
                 fPages[i].fPlotList.addToHead(currPlot->get());

src/gpu/ganesh/GrDrawOpAtlas.h

@@ -82,7 +82,8 @@ public:
                                                skgpu::AtlasGenerationCounter* generationCounter,
                                                AllowMultitexturing allowMultitexturing,
                                                skgpu::PlotEvictionCallback* evictor,
-                                               std::string_view label);
+                                               std::string_view label,
+                                               skgpu::PadAllGlyphs padAllGlyphs);
 
     /**
      * Adds a width x height subimage to the atlas. Upon success it returns 'kSucceeded' and returns
@@ -164,12 +165,19 @@ public:
 
     int numAllocated_TestingOnly() const;
     void setMaxPages_TestingOnly(uint32_t maxPages);
+    skgpu::Plot* getPlot_testingOnly(int pageIdx, int plotIdx) {
+        return fPages[pageIdx].fPlotArray[plotIdx].get();
+    }
+    void checkEvictedPlot_testingOnly(skgpu::PlotEvictionCallback* checker) {
+        this->fEvictionCallbacks.emplace_back(checker);
+    }
 
 private:
     GrDrawOpAtlas(GrProxyProvider*, const GrBackendFormat& format, SkColorType, size_t bpp,
                   int width, int height, int plotWidth, int plotHeight,
                   skgpu::AtlasGenerationCounter* generationCounter,
-                  AllowMultitexturing allowMultitexturing, std::string_view label);
+                  AllowMultitexturing allowMultitexturing, std::string_view label,
+                  skgpu::PadAllGlyphs padAllGlyphs);
 
     inline bool updatePlot(GrDeferredUploadTarget*, skgpu::AtlasLocator*, skgpu::Plot*);
 
@@ -241,6 +249,8 @@ private:
 
     uint32_t fNumActivePages;
 
+    skgpu::PadAllGlyphs fPadAllGlyphs;
+
     SkDEBUGCODE(void validate(const skgpu::AtlasLocator& atlasLocator) const;)
 };
 

src/gpu/ganesh/ops/SmallPathAtlasMgr.cpp

@@ -66,7 +66,11 @@ bool SmallPathAtlasMgr::initAtlas(GrProxyProvider* proxyProvider, const GrCaps*
                                  kPlotWidth, kPlotHeight, this,
                                  GrDrawOpAtlas::AllowMultitexturing::kYes,
                                  this,
-                                 /*label=*/"SmallPathAtlas");
+                                 /*label=*/"SmallPathAtlas",
+    // TODO: Investigate if this is reall the right choice. The truly small paths are uploaded
+    //  as bitmaps and rendered with Nearest sampling. It's a question how many are falling
+    //  back to SDF at this point, as opposed to using one of Chris's path renderers.
+                                 skgpu::PadAllGlyphs::kYes);
 
     return SkToBool(fAtlas);
 }

src/gpu/ganesh/text/GrAtlasManager.cpp

@@ -23,9 +23,9 @@ using MaskFormat = skgpu::MaskFormat;
 GrAtlasManager::GrAtlasManager(GrProxyProvider* proxyProvider,
                                size_t maxTextureBytes,
                                GrDrawOpAtlas::AllowMultitexturing allowMultitexturing,
-                               bool supportBilerpAtlas)
+                               skgpu::PadAllGlyphs padAllGlyphs)
             : fAllowMultitexturing{allowMultitexturing}
-            , fSupportBilerpAtlas{supportBilerpAtlas}
+            , fPadAllGlyphs{padAllGlyphs}
             , fProxyProvider{proxyProvider}
             , fCaps{fProxyProvider->refCaps()}
             , fAtlasConfig{fCaps->maxTextureSize(), maxTextureBytes} { }
@@ -158,7 +158,7 @@ GrDrawOpAtlas::ErrorCode GrAtlasManager::addGlyphToAtlas(const SkGlyph& skGlyph,
         case 0:
             // The direct mask/image case.
             padding = 0;
-            if (fSupportBilerpAtlas) {
+            if (fPadAllGlyphs == skgpu::PadAllGlyphs::kYes) {
                 // Force direct masks (glyph with no padding) to have padding.
                 padding = 1;
                 srcPadding = 1;
@@ -343,7 +343,8 @@ bool GrAtlasManager::initAtlas(MaskFormat format) {
                                               this,
                                               fAllowMultitexturing,
                                               nullptr,
-                                              /*label=*/"TextAtlas");
+                                              /*label=*/"TextAtlas",
+                                              fPadAllGlyphs);
         if (!fAtlases[index]) {
             return false;
         }

src/gpu/ganesh/text/GrAtlasManager.h

@@ -32,7 +32,7 @@ public:
     GrAtlasManager(GrProxyProvider*,
                    size_t maxTextureBytes,
                    GrDrawOpAtlas::AllowMultitexturing,
-                   bool supportBilerpAtlas);
+                   skgpu::PadAllGlyphs padAllGlyphs);
     ~GrAtlasManager() override;
 
     // if getViews returns nullptr, the client must not try to use other functions on the
@@ -122,6 +122,12 @@ public:
 
     void setAtlasDimensionsToMinimum_ForTesting();
     void setMaxPages_TestingOnly(uint32_t maxPages);
+    GrDrawOpAtlas* getAtlas_TestingOnly(skgpu::MaskFormat format) {
+        format = this->resolveMaskFormat(format);
+        int atlasIndex = MaskFormatToAtlasIndex(format);
+        SkASSERT(fAtlases[atlasIndex]);
+        return fAtlases[atlasIndex].get();
+    }
 
 private:
     bool initAtlas(skgpu::MaskFormat);
@@ -155,7 +161,7 @@ private:
     GrDrawOpAtlas::AllowMultitexturing fAllowMultitexturing;
     std::unique_ptr<GrDrawOpAtlas> fAtlases[skgpu::kMaskFormatCount];
     static_assert(skgpu::kMaskFormatCount == 3);
-    bool fSupportBilerpAtlas;
+    skgpu::PadAllGlyphs fPadAllGlyphs;
     GrProxyProvider* fProxyProvider;
     sk_sp<const GrCaps> fCaps;
     GrDrawOpAtlasConfig fAtlasConfig;

src/gpu/graphite/DrawAtlas.cpp

@@ -383,7 +383,7 @@ bool DrawAtlas::createPages(AtlasGenerationCounter* generationCounter) {
                 uint32_t plotIndex = r * numPlotsX + c;
                 currPlot->reset(new Plot(
                     i, plotIndex, generationCounter, x, y, fPlotWidth, fPlotHeight, fColorType,
-                    fBytesPerPixel));
+                    fBytesPerPixel, skgpu::PadAllGlyphs::kNo));
 
                 // build LRU list
                 fPages[i].fPlotList.addToHead(currPlot->get());

src/gpu/graphite/text/AtlasManager.cpp

@@ -26,7 +26,6 @@ namespace skgpu::graphite {
 
 AtlasManager::AtlasManager(Recorder* recorder)
         : fRecorder(recorder)
-        , fSupportBilerpAtlas{recorder->priv().caps()->supportBilerpFromGlyphAtlas()}
         , fAtlasConfig{recorder->priv().caps()->maxTextureSize(),
                        recorder->priv().caps()->glyphCacheTextureMaximumBytes()} {
     if (!recorder->priv().caps()->allowMultipleGlyphCacheTextures() ||
@@ -37,6 +36,11 @@ AtlasManager::AtlasManager(Recorder* recorder)
     } else {
        fAllowMultitexturing = DrawAtlas::AllowMultitexturing::kYes;
     }
+    if (recorder->priv().caps()->supportBilerpFromGlyphAtlas()) {
+        fPadAllGlyphs = skgpu::PadAllGlyphs::kYes;
+    } else {
+        fPadAllGlyphs = skgpu::PadAllGlyphs::kNo;
+    }
 }
 
 AtlasManager::~AtlasManager() = default;
@@ -177,7 +181,7 @@ DrawAtlas::ErrorCode AtlasManager::addGlyphToAtlas(const SkGlyph& skGlyph,
         case 0:
             // The direct mask/image case.
             padding = 0;
-            if (fSupportBilerpAtlas) {
+            if (fPadAllGlyphs == PadAllGlyphs::kYes) {
                 // Force direct masks (glyph with no padding) to have padding.
                 padding = 1;
                 srcPadding = 1;

src/gpu/graphite/text/AtlasManager.h

@@ -106,7 +106,7 @@ private:
     DrawAtlas::AllowMultitexturing fAllowMultitexturing;
     std::unique_ptr<DrawAtlas> fAtlases[kMaskFormatCount];
     static_assert(kMaskFormatCount == 3);
-    bool fSupportBilerpAtlas;
+    PadAllGlyphs fPadAllGlyphs;
     DrawAtlasConfig fAtlasConfig;
 };
 

tests/DrawOpAtlasTest.cpp

@@ -35,15 +35,25 @@
 #include "src/gpu/ganesh/ops/GrOp.h"
 #include "src/gpu/ganesh/text/GrAtlasManager.h"
 #include "tests/Test.h"
+#include "tools/flags/CommandLineFlags.h"
 #include "tools/gpu/GrContextFactory.h"
 
+#include <functional>
 #include <memory>
 #include <utility>
+#include "include/core/SkTextBlob.h"
+
+#include "src/core/SkStrikeCache.h"
+#include "src/text/gpu/Glyph.h"
+
+using Glyph = sktext::gpu::Glyph;
 
 using MaskFormat = skgpu::MaskFormat;
 
 class GrResourceProvider;
 
+DEFINE_bool(verboseSkyline, false, "Skyline will be very verbose.");
+
 static const int kNumPlots = 2;
 static const int kPlotSize = 32;
 static const int kAtlasSize = kNumPlots * kPlotSize;
@@ -116,15 +126,20 @@ static bool fill_plot(GrDrawOpAtlas* atlas,
                       GrResourceProvider* resourceProvider,
                       GrDeferredUploadTarget* target,
                       skgpu::AtlasLocator* atlasLocator,
-                      int alpha) {
+                      int alpha,
-    SkImageInfo ii = SkImageInfo::MakeA8(kPlotSize, kPlotSize);
+                      int plotSize = kPlotSize,
+                      void** plot = nullptr) {
+    SkImageInfo ii = SkImageInfo::MakeA8(plotSize, plotSize);
 
     SkBitmap data;
     data.allocPixels(ii);
     data.eraseARGB(alpha, 0, 0, 0);
+    if (plot != nullptr) {
+        *plot = data.getAddr(0, 0);
+    }
 
     GrDrawOpAtlas::ErrorCode code;
-    code = atlas->addToAtlas(resourceProvider, target, kPlotSize, kPlotSize,
+    code = atlas->addToAtlas(resourceProvider, target, plotSize, plotSize,
                              data.getAddr(0, 0), atlasLocator);
     return GrDrawOpAtlas::ErrorCode::kSucceeded == code;
 }
@@ -159,7 +174,8 @@ DEF_GPUTEST_FOR_RENDERING_CONTEXTS(BasicDrawOpAtlas, reporter, ctxInfo) {
                                                 &counter,
                                                 GrDrawOpAtlas::AllowMultitexturing::kYes,
                                                 &evictor,
-                                                /*label=*/"BasicDrawOpAtlasTest");
+                                                /*label=*/"BasicDrawOpAtlasTest",
+                                                skgpu::PadAllGlyphs::kNo);
     check(reporter, atlas.get(), 0, 4, 0);
 
     // Fill up the first level
@@ -316,3 +332,280 @@ DEF_GPUTEST(GrDrawOpAtlasConfig_Basic, reporter, options) {
     test_atlas_config(reporter, 65536, 0, MaskFormat::kA8,
                       { 512, 512 }, { 256, 256 });
 }
+
+static void supportBilerpFromGlyphAtlas(GrContextOptions* options) {
+    options->fSupportBilerpFromGlyphAtlas = true;
+}
+static void doNotSupportBilerpFromGlyphAtlas(GrContextOptions* options) {
+    options->fSupportBilerpFromGlyphAtlas = false;
+}
+
+namespace skgpu {
+class RectanizerSkylineTestingPeer {
+public:
+    static int allocatedBytes(skgpu::Rectanizer* rectanizer) { return rectanizer->fAreaSoFar; }
+
+    static int allBytes(skgpu::Rectanizer* rectanizer) {
+        return rectanizer->fWidth * rectanizer->fHeight;
+    }
+};
+
+class PlotTestingPeer {
+public:
+    static int allocatedBytes(skgpu::Plot* plot) {
+        return RectanizerSkylineTestingPeer::allocatedBytes(plot->fRectanizer.get());
+    }
+
+    static int allBytes(skgpu::Plot* plot) {
+        return RectanizerSkylineTestingPeer::allBytes(plot->fRectanizer.get());
+    }
+
+    static SkIRect getRect(skgpu::Plot* plot) {
+        return SkIRect::MakeXYWH(plot->fOffset.fX, plot->fOffset.fY, plot->fWidth, plot->fHeight);
+    }
+
+    static unsigned char* data(skgpu::Plot* plot) { return plot->fData; }
+};
+}
+
+class Data : public skgpu::PlotEvictionCallback
+           , public skgpu::RectanizerSkylineTestingPeer
+           , public skgpu::PlotTestingPeer {
+public:
+    SkArenaAlloc alloc;
+    TestingUploadTarget uploadTarget;
+    GrResourceProvider* resourceProvider;
+    std::vector<SkGlyph> skGlyphs;
+    std::vector<Glyph> grGlyphs;
+    std::vector<int> srcPaddings;
+    std::vector<bool> checked;      // To avoid double checked glyphs from evicted plots
+    GrAtlasManager* atlasManager;
+    skiatest::Reporter* reporter;
+    sk_sp<SkTypeface> typeface;
+    SkString text;
+
+    int allocatedBytes = 0;
+    int optimizedBytes = 0;
+    int allBytes = 0;
+    int evicted = 0;
+    int evictedGlyphs = 0;
+    int evictedArea = 0;
+
+    Data(const sk_gpu_test::ContextInfo& ctxInfo,
+         skiatest::Reporter* reporter,
+         const SkString& text)
+            : alloc(1 << 12)
+            , reporter(reporter)
+            , text(text) {
+        auto dContext = ctxInfo.directContext();
+        resourceProvider = dContext->priv().resourceProvider();
+        atlasManager = dContext->priv().getAtlasManager();
+        atlasManager->setAtlasDimensionsToMinimum_ForTesting();
+        atlasManager->freeAll();
+        unsigned int numProxies;
+        atlasManager->getViews(MaskFormat::kA8, &numProxies);
+        atlasManager->setMaxPages_TestingOnly(1);
+        GrDrawOpAtlas* atlas = atlasManager->getAtlas_TestingOnly(MaskFormat::kA8);
+        atlas->checkEvictedPlot_testingOnly(this);
+
+        typeface = SkTypeface::MakeFromName("Segoe UI", SkFontStyle());
+        this->reset();
+
+    }
+    void evict(skgpu::PlotLocator plotLocator) override {
+        ++evicted;
+        auto genID = plotLocator.genID();
+        bool once = true;
+        auto glyphs = 0;
+        for (auto i = 0ul; i < skGlyphs.size(); ++i) {
+            auto& grGlyph = grGlyphs[i];
+            auto pageIndex = grGlyph.fAtlasLocator.pageIndex();
+            auto plotIndex = grGlyph.fAtlasLocator.plotIndex();
+            skgpu::Plot* plot = atlasManager->getAtlas_TestingOnly(MaskFormat::kA8)->
+                                            getPlot_testingOnly(pageIndex, plotIndex);
+            if (genID == plot->genID()) {
+                if (checkPadding(i)) {
+                    ++glyphs;
+                }
+                if (once) {
+                    once = false;
+                    int allocatedBytes1 = PlotTestingPeer::allocatedBytes(plot);
+                    int allBytes1 = PlotTestingPeer::allBytes(plot);
+                    allBytes += allBytes1;
+                    allocatedBytes += allocatedBytes1;
+                    evictedArea += allocatedBytes1;
+                    if (FLAGS_verboseSkyline) {
+                        SkDebugf(
+                                "Plot #%u: allocated=%d (%.2f)",
+                                plotIndex,
+                                allocatedBytes1,
+                                allocatedBytes1 * 100.0f / allBytes1);
+                    }
+                }
+            }
+        }
+        if (FLAGS_verboseSkyline) {
+            SkDebugf(", %d glyphs\n", glyphs);
+        }
+        evictedGlyphs += glyphs;
+    }
+
+    void reset() {
+        skGlyphs.clear();
+        grGlyphs.clear();
+        srcPaddings.clear();
+        checked.clear();
+    }
+
+    void add(SkGlyph skGlyph, Glyph grGlyph, int srcPadding) {
+        skGlyphs.emplace_back(skGlyph);
+        grGlyphs.emplace_back(grGlyph);
+        srcPaddings.emplace_back(srcPadding);
+        checked.emplace_back(false);
+    }
+
+    bool checkPadding(int i) {
+        if (checked[i]) {
+            return false;
+        }
+        auto& skGlyph = skGlyphs[i];
+        auto& grGlyph = grGlyphs[i];
+        // Check sizes
+        // Check if grGlyph is surrounded with zero padding
+        auto pageIndex = grGlyph.fAtlasLocator.pageIndex();
+        auto plotIndex = grGlyph.fAtlasLocator.plotIndex();
+        skgpu::Plot* plot = atlasManager->getAtlas_TestingOnly(MaskFormat::kA8)->
+                                        getPlot_testingOnly(pageIndex, plotIndex);
+        auto plotRect = PlotTestingPeer::getRect(plot);
+        auto loc = grGlyph.fAtlasLocator.getUVs();
+        SkRect glyphRect = SkRect::MakeLTRB(loc[0], loc[1], loc[2], loc[3]);
+        if (srcPaddings[i] == 1) {
+            REPORTER_ASSERT(reporter, skGlyph.width() == glyphRect.width());
+            REPORTER_ASSERT(reporter, skGlyph.height() == glyphRect.height());
+            glyphRect.offset(-plotRect.fLeft, -plotRect.fTop);
+            auto data = PlotTestingPeer::data(plot);
+            // Check if there is a zero padding around each glyph
+            REPORTER_ASSERT(reporter, glyphRect.fTop > 0);
+            REPORTER_ASSERT(reporter, glyphRect.fBottom + 1 <= plotRect.height());
+            int y0 = glyphRect.fTop - 1;
+            int y1 = glyphRect.fBottom;
+            for (int x = glyphRect.fLeft; x < glyphRect.fRight; ++x) {
+                auto byte0 = data + y0 * plotRect.width() + x;
+                REPORTER_ASSERT(reporter, *byte0 == 0);
+                auto byte1 = data + y1 * plotRect.width() + x;
+                REPORTER_ASSERT(reporter, *byte1 == 0);
+            }
+            for (int x = glyphRect.fLeft; x < glyphRect.fRight; ++x) {
+            }
+        } else if (srcPaddings[i] == 0) {
+            REPORTER_ASSERT(reporter, skGlyph.width() == glyphRect.width());
+            REPORTER_ASSERT(reporter, skGlyph.height() == glyphRect.height());
+        } else {
+            REPORTER_ASSERT(reporter, skGlyph.width() - 4 == glyphRect.width());
+            REPORTER_ASSERT(reporter, skGlyph.height() - 4 == glyphRect.height());
+            // TODO: Check that there are no zeros at all?
+        }
+        checked[i] = true;
+        return true;
+    }
+
+    void drawText(int srcPadding, SkScalar fontSize) {
+
+        SkFont defaultFont(typeface, fontSize);
+        SkStrikeSpec strikeSpec = SkStrikeSpec::MakeWithNoDevice(defaultFont);
+        sk_sp<SkStrike> strike = strikeSpec.findOrCreateStrike();
+
+        for (auto i = 0ul; i < text.size(); ++i) {
+            char c = text[i];
+            SkPackedGlyphID id(defaultFont.unicharToGlyph(c));
+            SkGlyph skGlyph = strike->getScalerContext()->makeGlyph(id, &alloc);
+            SkTArray<unsigned char> ones;
+            ones.push_back_n(skGlyph.imageSize(), 0xff);
+            skGlyph.setImage(&alloc, ones.data());
+            SkPackedGlyphID glyphID;
+            Glyph grGlyph(glyphID);
+            auto errorCode = atlasManager->addGlyphToAtlas(
+                    skGlyph, &grGlyph, srcPadding, resourceProvider, &uploadTarget);
+            REPORTER_ASSERT(reporter, errorCode == GrDrawOpAtlas::ErrorCode::kSucceeded);
+            add(std::move(skGlyph), std::move(grGlyph), std::move(srcPadding));
+        }
+    }
+};
+
+// Testing that every glyph has a zero-pixel padding
+// (0, 1, or 2 without slug or 1, 2 with slug)
+// Testing happen on plot eviction event to make sure all plots are checked
+// (the active plots will be manually evicted, too)
+void testPlots(skiatest::Reporter* reporter,
+              const sk_gpu_test::ContextInfo& ctxInfo,
+              SkString text) {
+    Data data(ctxInfo, reporter, text);
+    auto repeat = 1;
+    // Draw glyphs and tests them on plot eviction
+    for (; repeat > 0; --repeat) {
+        for (auto i = 0ul; i < text.size(); ++i) {
+            auto srcPadding = i % 2;
+            auto fontSize = i + 10.0f;
+            data.drawText(srcPadding, fontSize);
+            data.drawText(srcPadding, text.size() - i + 10.0f);
+
+            auto oldText = data.text;
+            data.text = SkString(&text[i], 1);
+            data.drawText(2, 180);
+            data.text = oldText;
+        }
+    }
+
+    if (FLAGS_verboseSkyline) {
+        // Print all the plots that are not evicted
+        SkDebugf("Summary: optimized=%.2f%% / %.2f%%, allocated=%.2f%% sum=%d "
+                 "evicted plots=%d evicted glyphs/plot=%.2f evicted area/plot=%.2f\n",
+                 data.optimizedBytes*100.0f/data.allocatedBytes,
+                 data.optimizedBytes*100.0f/data.allBytes,
+                 data.allocatedBytes*100.0f/data.allBytes,
+                 data.optimizedBytes + data.allocatedBytes,
+                 data.evicted,
+                 (float)data.evictedGlyphs/data.evicted,
+                 (float)data.evictedArea/data.evicted);
+        SkDebugf("Current plots:\n");
+    }
+
+    // Manually call evict to test all the glyphs that were not evicted
+    GrDrawOpAtlas* atlas = data.atlasManager->getAtlas_TestingOnly(MaskFormat::kA8);
+    for (auto page = 0ul; page < atlas->numActivePages(); ++page) {
+        for (auto plot = 0; plot < 4; ++plot) {
+            auto p = atlas->getPlot_testingOnly(page, plot);
+            data.evict(p->plotLocator());
+        }
+    }
+
+    if (FLAGS_verboseSkyline) {
+        SkDebugf("Summary: optimized=%.2f%% / %.2f%%, allocated=%.2f%% sum=%d "
+                 "used plots=%d average glyphs/plot=%.2f average area/plot=%.2f\n",
+                 data.optimizedBytes*100.0f/data.allocatedBytes,
+                 data.optimizedBytes*100.0f/data.allBytes,
+                 data.allocatedBytes*100.0f/data.allBytes,
+                 data.optimizedBytes + data.allocatedBytes,
+                 data.evicted,
+                 (float)data.evictedGlyphs/data.evicted,
+                 (float)data.evictedArea/data.evicted);
+    }
+}
+
+static const char* TEXT = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ";
+
+DEF_GPUTEST_FOR_CONTEXTS(GrAtlasManager_withOpt,
+                         sk_gpu_test::GrContextFactory::IsRenderingContext,
+                         reporter,
+                         ctxInfo,
+                         supportBilerpFromGlyphAtlas) {
+    testPlots(reporter, ctxInfo, SkString(TEXT));
+}
+
+DEF_GPUTEST_FOR_CONTEXTS(GrAtlasManager_withoutOpt,
+                         sk_gpu_test::GrContextFactory::IsRenderingContext,
+                         reporter,
+                         ctxInfo,
+                         doNotSupportBilerpFromGlyphAtlas) {
+    testPlots(reporter, ctxInfo, SkString(TEXT));
+}