From b6c7a5bc832b785a445ab89b3365ed80ab2e9d47 Mon Sep 17 00:00:00 2001
From: Robert Phillips <robertphillips@google.com>
Date: Wed, 17 Jun 2026 15:01:07 -0400
Subject: [PATCH] Reland "[Ganesh] TextureOp quad illegal memory access"

This reverts commit 5e976cb2f034067e006ec8db2889791f0d4eb443.

Reason for revert: The unit test was exceeding maxTextureSize on some devices

Original change's description:
> Revert "[Ganesh] TextureOp quad illegal memory access"
>
> This reverts commit 148b2b1948019f8f89435b4df7d598224a2ab0e1.
>
> Reason for revert: Crashing on some Android devices
>
> Failure Link: <LINK TO FAILURE>
>
> Original change's description:
> > [Ganesh] TextureOp quad illegal memory access
> >
> > This CL fixes an overflow in the number of allowed quads in a TextureOp. It works on two fronts:
> >    It conservatively tracks the number of quads (incl. perspective)
> >    It prevents a fast path when there possibly might be an overflow.
> >
> > Bug: b/500172224
> > Change-Id: Icd92ed5c80d81cdbfea8d2463407cc48a0a32843
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1262376
> > Commit-Queue: Robert Phillips <robertphillips@google.com>
> > Reviewed-by: Michael Ludwig <michaelludwig@google.com>
>
> Bug: b/500172224
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Change-Id: Ic4ea8537eb642131fbbcb14a485b11ce3ed4a636
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1268256
> Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
> Auto-Submit: Robert Phillips <robertphillips@google.com>
> Commit-Queue: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>

Bug: https://issues.chromium.org/issues/500172224
Change-Id: If745b11a8e4b0dda535f114018009085ee2c0ce8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1269156
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
---
 gn/tests.gni                          |   1 +
 src/gpu/ganesh/Device_drawTexture.cpp |  22 +++--
 src/gpu/ganesh/SurfaceDrawContext.cpp |   5 +-
 src/gpu/ganesh/SurfaceDrawContext.h   |   3 +-
 src/gpu/ganesh/ops/TextureOp.cpp      |  55 ++++++++---
 src/gpu/ganesh/ops/TextureOp.h        |   3 +-
 tests/BulkRectTest.cpp                |   3 +-
 tests/crbug_500172224.cpp             | 137 ++++++++++++++++++++++++++
 8 files changed, 205 insertions(+), 24 deletions(-)
 create mode 100644 tests/crbug_500172224.cpp

diff --git a/gn/tests.gni b/gn/tests.gni
index da779fdd37..430705565a 100644
--- a/gn/tests.gni
+++ b/gn/tests.gni
@@ -523,6 +523,7 @@ ganesh_tests_sources = [
   "$_tests/VkBackendSurfaceTest.cpp",
   "$_tests/VkWrapTests.cpp",
   "$_tests/WrappedSurfaceCopyOnWriteTest.cpp",
+  "$_tests/crbug_500172224.cpp",
 ]
 
 ganesh_gl_tests_sources = [
diff --git a/src/gpu/ganesh/Device_drawTexture.cpp b/src/gpu/ganesh/Device_drawTexture.cpp
index 3864216932..103e684953 100644
--- a/src/gpu/ganesh/Device_drawTexture.cpp
+++ b/src/gpu/ganesh/Device_drawTexture.cpp
@@ -608,11 +608,11 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
     SkBlendMode mode = paint.getBlendMode_or(SkBlendMode::kSrcOver);
 
     AutoTArray<GrTextureSetEntry> textures(count);
-    // We accumulate compatible proxies until we find an an incompatible one or reach the end and
+    // We accumulate compatible proxies until we find an incompatible one or reach the end and
     // issue the accumulated 'n' draws starting at 'base'. 'p' represents the number of proxy
     // switches that occur within the 'n' entries.
     int base = 0, n = 0, p = 0;
-    auto draw = [&](int nextBase) {
+    auto draw = [&](int nextBase, bool setMayHavePersp) {
         if (n > 0) {
             auto textureXform = GrColorSpaceXform::Make(set[base].fImage->imageInfo().colorInfo(),
                                                         fSurfaceDrawContext->colorInfo());
@@ -625,12 +625,16 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
                                                 mode,
                                                 constraint,
                                                 this->localToDevice(),
-                                                std::move(textureXform));
+                                                std::move(textureXform),
+                                                setMayHavePersp);
         }
         base = nextBase;
         n = 0;
         p = 0;
     };
+    // This is a conservatively computed property of the image set that disables a fast path
+    // in TextureOp::AddTextureSetOps.
+    bool setMayHavePersp = this->localToDevice().hasPerspective();
     int dstClipIndex = 0;
     for (int i = 0; i < count; ++i) {
         SkASSERT(!set[i].fHasClip || dstClips);
@@ -644,7 +648,7 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
         // The default SkDevice implementation is based on drawImageRect which does not allow
         // non-sorted src rects. TODO: Decide this is OK or make sure we handle it.
         if (!set[i].fSrcRect.isSorted()) {
-            draw(i + 1);
+            draw(i + 1, setMayHavePersp);
             continue;
         }
 
@@ -668,7 +672,7 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
         if (!view) {
             // This image can't go through the texture op, send through general image pipeline
             // after flushing current batch.
-            draw(i + 1);
+            draw(i + 1, setMayHavePersp);
             SkTCopyOnFirstWrite<SkPaint> entryPaint(paint);
             if (set[i].fAlpha != 1.f) {
                 auto paintAlpha = paint.getAlphaf();
@@ -689,6 +693,10 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
         textures[i].fDstClipQuad = clip;
         textures[i].fPreViewMatrix =
                 set[i].fMatrixIndex < 0 ? nullptr : preViewMatrices + set[i].fMatrixIndex;
+        if (textures[i].fPreViewMatrix && textures[i].fPreViewMatrix->hasPerspective()) {
+            // Once set, this flag stays on for the rest of the image set
+            setMayHavePersp = true;
+        }
         textures[i].fColor = texture_color(paint.getColor4f(), set[i].fAlpha,
                                            SkColorTypeToGrColorType(image->colorType()),
                                            fSurfaceDrawContext->colorInfo());
@@ -701,7 +709,7 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
              textures[i].fProxyView.swizzle() != textures[base].fProxyView.swizzle() ||
              set[i].fImage->alphaType() != set[base].fImage->alphaType() ||
              !SkColorSpace::Equals(set[i].fImage->colorSpace(), set[base].fImage->colorSpace()))) {
-            draw(i);
+            draw(i, setMayHavePersp);
         }
         // Whether or not we submitted a draw in the above if(), this ith entry is in the current
         // set being accumulated so increment n, and increment p if proxies are different.
@@ -712,7 +720,7 @@ void Device::drawEdgeAAImageSet(const SkCanvas::ImageSetEntry set[], int count,
             ++p;
         }
     }
-    draw(count);
+    draw(count, setMayHavePersp);
 }
 
 bool Device::drawBlurredRRect(const SkRRect& rrect, const SkPaint& paint, float deviceSigma) {
diff --git a/src/gpu/ganesh/SurfaceDrawContext.cpp b/src/gpu/ganesh/SurfaceDrawContext.cpp
index 2ec011d254..a8339d276c 100644
--- a/src/gpu/ganesh/SurfaceDrawContext.cpp
+++ b/src/gpu/ganesh/SurfaceDrawContext.cpp
@@ -910,7 +910,8 @@ void SurfaceDrawContext::drawTextureSet(const GrClip* clip,
                                         SkBlendMode mode,
                                         SkCanvas::SrcRectConstraint constraint,
                                         const SkMatrix& viewMatrix,
-                                        sk_sp<GrColorSpaceXform> texXform) {
+                                        sk_sp<GrColorSpaceXform> texXform,
+                                        bool setMayHavePersp) {
     ASSERT_SINGLE_OWNER
     RETURN_IF_ABANDONED
     SkDEBUGCODE(this->validate();)
@@ -924,7 +925,7 @@ void SurfaceDrawContext::drawTextureSet(const GrClip* clip,
                                                       : ganesh::TextureOp::Saturate::kNo;
     ganesh::TextureOp::AddTextureSetOps(this, clip, fContext, set, cnt, proxyRunCnt, filter, mm,
                                         saturate, mode, aaType, constraint, viewMatrix,
-                                        std::move(texXform));
+                                        std::move(texXform), setMayHavePersp);
 }
 
 void SurfaceDrawContext::drawVertices(const GrClip* clip,
diff --git a/src/gpu/ganesh/SurfaceDrawContext.h b/src/gpu/ganesh/SurfaceDrawContext.h
index 16846874da..24e3838064 100644
--- a/src/gpu/ganesh/SurfaceDrawContext.h
+++ b/src/gpu/ganesh/SurfaceDrawContext.h
@@ -323,7 +323,8 @@ public:
                         SkBlendMode mode,
                         SkCanvas::SrcRectConstraint,
                         const SkMatrix& viewMatrix,
-                        sk_sp<GrColorSpaceXform> texXform);
+                        sk_sp<GrColorSpaceXform> texXform,
+                        bool setMayHavePersp);
 
     /**
      * Draw a roundrect using a paint.
diff --git a/src/gpu/ganesh/ops/TextureOp.cpp b/src/gpu/ganesh/ops/TextureOp.cpp
index 0c04a671e2..cf2287e345 100644
--- a/src/gpu/ganesh/ops/TextureOp.cpp
+++ b/src/gpu/ganesh/ops/TextureOp.cpp
@@ -1262,6 +1262,28 @@ public:
             , fTextureColorSpaceXform(textureColorSpaceXform)
             , fNumLeft(numEntries) {}
 
+    int determineClumpSize(const GrTextureSetEntry set[], int quadLimit) const {
+        bool hasPersp = fViewMatrix.hasPerspective();
+
+        int conservativeNumQuads = 0;
+        for (int i = 0; i < fNumLeft; ++i) {
+            int absIndex = this->baseIndex() + i;
+
+            bool hasPrePersp = false;
+            if (set[absIndex].fPreViewMatrix) {
+                hasPrePersp = set[absIndex].fPreViewMatrix->hasPerspective();
+            }
+
+            // A perspective quad could split into two quads
+            conservativeNumQuads += hasPersp || hasPrePersp ? 2 : 1;
+            if (conservativeNumQuads > quadLimit) {
+                return i;
+            }
+        }
+
+        return fNumLeft;
+    }
+
     void createOp(GrTextureSetEntry set[], int clumpSize, GrAAType aaType) {
 
         int clumpProxyCount = proxy_run_count(&set[fNumClumped], clumpSize);
@@ -1314,7 +1336,8 @@ void TextureOp::AddTextureSetOps(ganesh::SurfaceDrawContext* sdc,
                                  GrAAType aaType,
                                  SkCanvas::SrcRectConstraint constraint,
                                  const SkMatrix& viewMatrix,
-                                 sk_sp<GrColorSpaceXform> textureColorSpaceXform) {
+                                 sk_sp<GrColorSpaceXform> textureColorSpaceXform,
+                                 bool setMayHavePersp) {
     // Ensure that the index buffer limits are lower than the proxy and quad count limits of
     // the op's metadata so we don't need to worry about overflow.
     SkDEBUGCODE(TextureOpImpl::ValidateResourceLimits();)
@@ -1358,8 +1381,8 @@ void TextureOp::AddTextureSetOps(ganesh::SurfaceDrawContext* sdc,
 
     // Second check if we can always just make a single op and avoid the extra iteration
     // needed to clump things together.
-    if (cnt <= std::min(GrResourceProvider::MaxNumNonAAQuads(),
-                      GrResourceProvider::MaxNumAAQuads())) {
+    if (!setMayHavePersp && cnt <= std::min(GrResourceProvider::MaxNumNonAAQuads(),
+                                            GrResourceProvider::MaxNumAAQuads())) {
         auto op = TextureOpImpl::Make(context, set, cnt, proxyRunCnt, filter, mm, saturate, aaType,
                                       constraint, viewMatrix, std::move(textureColorSpaceXform));
         sdc->addDrawOp(clip, std::move(op));
@@ -1373,7 +1396,7 @@ void TextureOp::AddTextureSetOps(ganesh::SurfaceDrawContext* sdc,
     if (aaType == GrAAType::kNone || aaType == GrAAType::kMSAA) {
         // Clump these into series of MaxNumNonAAQuads-sized GrTextureOps
         while (state.numLeft() > 0) {
-            int clumpSize = std::min(state.numLeft(), GrResourceProvider::MaxNumNonAAQuads());
+            int clumpSize = state.determineClumpSize(set, GrResourceProvider::MaxNumNonAAQuads());
 
             state.createOp(set, clumpSize, aaType);
         }
@@ -1383,25 +1406,34 @@ void TextureOp::AddTextureSetOps(ganesh::SurfaceDrawContext* sdc,
         // axis-aligned.
         SkASSERT(aaType == GrAAType::kCoverage);
 
+        bool hasPersp = viewMatrix.hasPerspective();
+
         while (state.numLeft() > 0) {
             GrAAType runningAA = GrAAType::kNone;
             bool clumped = false;
 
+            int conservativeNumQuads = 0;
+
             for (int i = 0; i < state.numLeft(); ++i) {
                 int absIndex = state.baseIndex() + i;
 
+                bool hasPrePersp = false;
+                if (set[absIndex].fPreViewMatrix) {
+                    hasPrePersp = set[absIndex].fPreViewMatrix->hasPerspective();
+                }
+
+                // A perspective quad could split into two quads
+                conservativeNumQuads += hasPersp || hasPrePersp ? 2 : 1;
+
                 if (set[absIndex].fAAFlags != GrQuadAAFlags::kNone ||
                     runningAA == GrAAType::kCoverage) {
 
-                    if (i >= GrResourceProvider::MaxNumAAQuads()) {
+                    if (conservativeNumQuads > GrResourceProvider::MaxNumAAQuads()) {
                         // Here we either need to boost the AA type to kCoverage, but doing so with
                         // all the accumulated quads would overflow, or we have a set of AA quads
                         // that has just gotten too large. In either case, calve off the existing
                         // quads as their own TextureOp.
-                        state.createOp(
-                            set,
-                            runningAA == GrAAType::kNone ? i : GrResourceProvider::MaxNumAAQuads(),
-                            runningAA); // maybe downgrading AA here
+                        state.createOp(set, i, runningAA); // maybe downgrading AA here
                         clumped = true;
                         break;
                     }
@@ -1409,11 +1441,10 @@ void TextureOp::AddTextureSetOps(ganesh::SurfaceDrawContext* sdc,
                     runningAA = GrAAType::kCoverage;
                 } else if (runningAA == GrAAType::kNone) {
 
-                    if (i >= GrResourceProvider::MaxNumNonAAQuads()) {
+                    if (conservativeNumQuads > GrResourceProvider::MaxNumNonAAQuads()) {
                         // Here we've found a consistent batch of non-AA quads that has gotten too
                         // large. Calve it off as its own TextureOp.
-                        state.createOp(set, GrResourceProvider::MaxNumNonAAQuads(),
-                                       GrAAType::kNone); // definitely downgrading AA here
+                        state.createOp(set, i, GrAAType::kNone); // definitely downgrading AA here
                         clumped = true;
                         break;
                     }
diff --git a/src/gpu/ganesh/ops/TextureOp.h b/src/gpu/ganesh/ops/TextureOp.h
index d6629c2838..e8e148a4e2 100644
--- a/src/gpu/ganesh/ops/TextureOp.h
+++ b/src/gpu/ganesh/ops/TextureOp.h
@@ -87,7 +87,8 @@ public:
                                  GrAAType,
                                  SkCanvas::SrcRectConstraint,
                                  const SkMatrix& viewMatrix,
-                                 sk_sp<GrColorSpaceXform> textureXform);
+                                 sk_sp<GrColorSpaceXform> textureXform,
+                                 bool setMayHavePersp);
 
 #if defined(GPU_TEST_UTILS)
     static uint32_t ClassID();
-- 
2.53.0

