Reland "Improve degenerate 2pt conical gradient cases"
This reverts commit 95af4726bf91669e51250ebd3baa2925b6975e8e.
Reason for revert: I think this may not have been the reason the Android roll was failing. We've rolled, so it's a good time to try again.
Original change's description:
> Revert "Improve degenerate 2pt conical gradient cases"
>
> This reverts commit 879dab87ab78613458f9d36f843bc3c6ffdafc74.
>
> Reason for revert: Android roll failed.
> https://sponge.corp.google.com/target?id=93bc6b8d-9b42-4805-b204-46ae62f1b005&target=x86+CtsGraphicsTestCases&searchFor=&show=FAILED&sortBy=STATUS
> A test VectorDrawableTest.testVectorDrawableGradient fails.
>
> Original change's description:
> > Improve degenerate 2pt conical gradient cases
> >
> > This was originally a reland of "Fix div-by-zero loophole in gradient factory func", c34dd6c5263490b94ef9af7a14dee1b4bc872b58, but:
> >
> > The change caused blink layout tests when encountering very small or zero radii. The original patch switched the order of checking if the radii are equal and if the start radius was 0. In the case where both radii are 0, the original code created an actual radial gradient of radius 0 and the new code rejected the shader. A radial gradient with radius of 0 properly renders the last border color as a fill.
> >
> > This made me realize that the case when the center positions and the radii are the same can be handled more correctly than just always returning an empty shader, so the fix now applies simplifications to the gradient definition depending on the tile mode and should not trigger any blink tests. I added a row to the gradient edge cases GM to make sure it degrades gracefully.
> >
> > Original change's description:
> > > Fix div-by-zero loophole in gradient factory func
> > >
> > > Bug: oss-fuzz:10373
> > > Change-Id: I4277fb63e3186ee34feaf09ecf6aeddeb532f9c1
> > > Reviewed-on: https://skia-review.googlesource.com/c/168269
> > > Reviewed-by: Kevin Lubick <kjlubick@google.com>
> > > Commit-Queue: Michael Ludwig <michaelludwig@google.com>
> >
> > Docs-Preview: https://skia.org/?cl=168487
> > Bug: oss-fuzz:10373
> > Change-Id: Ib0a6e7f807560a5dcf24d1c8e0146817af2d9606
> > Reviewed-on: https://skia-review.googlesource.com/c/168487
> > Reviewed-by: Mike Reed <reed@google.com>
> > Reviewed-by: Florin Malita <fmalita@chromium.org>
> > Commit-Queue: Michael Ludwig <michaelludwig@google.com>
>
> TBR=caryclark@google.com,fmalita@chromium.org,fmalita@google.com,reed@google.com,michaelludwig@google.com
>
> Change-Id: I91b896c4a438c02206679b327a01b47f40993965
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: oss-fuzz:10373
> Reviewed-on: https://skia-review.googlesource.com/c/170272
> Reviewed-by: Stan Iliev <stani@google.com>
> Commit-Queue: Stan Iliev <stani@google.com>
TBR=caryclark@google.com,fmalita@chromium.org,fmalita@google.com,reed@google.com,stani@google.com,michaelludwig@google.com
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: oss-fuzz:10373
Change-Id: I7577fcea9eb8a875e94723ab2cca2fcc990b82b2
Reviewed-on: https://skia-review.googlesource.com/c/170279
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
diff --git a/src/shaders/gradients/SkGradientShader.cpp b/src/shaders/gradients/SkGradientShader.cpp
index 52a9b56..4726f09 100644
--- a/src/shaders/gradients/SkGradientShader.cpp
+++ b/src/shaders/gradients/SkGradientShader.cpp
@@ -517,6 +517,74 @@
desc->fLocalMatrix = localMatrix;
}
+static SkColor4f average_gradient_color(const SkColor4f colors[], const SkScalar pos[],
+ int colorCount) {
+ // The gradient is a piecewise linear interpolation between colors. For a given interval,
+ // the integral between the two endpoints is 0.5 * (ci + cj) * (pj - pi), which provides that
+ // intervals average color. The overall average color is thus the sum of each piece. The thing
+ // to keep in mind is that the provided gradient definition may implicitly use p=0 and p=1.
+ Sk4f blend(0.0);
+ // Bake 1/(colorCount - 1) uniform stop difference into this scale factor
+ SkScalar wScale = pos ? 0.5 : 0.5 / (colorCount - 1);
+ for (int i = 0; i < colorCount - 1; ++i) {
+ // Calculate the average color for the interval between pos(i) and pos(i+1)
+ Sk4f c0 = Sk4f::Load(&colors[i]);
+ Sk4f c1 = Sk4f::Load(&colors[i + 1]);
+ // when pos == null, there are colorCount uniformly distributed stops, going from 0 to 1,
+ // so pos[i + 1] - pos[i] = 1/(colorCount-1)
+ SkScalar w = pos ? (pos[i + 1] - pos[i]) : SK_Scalar1;
+ blend += wScale * w * (c1 + c0);
+ }
+
+ // Now account for any implicit intervals at the start or end of the stop definitions
+ if (pos) {
+ if (pos[0] > 0.0) {
+ // The first color is fixed between p = 0 to pos[0], so 0.5 * (ci + cj) * (pj - pi)
+ // becomes 0.5 * (c + c) * (pj - 0) = c * pj
+ Sk4f c = Sk4f::Load(&colors[0]);
+ blend += pos[0] * c;
+ }
+ if (pos[colorCount - 1] < SK_Scalar1) {
+ // The last color is fixed between pos[n-1] to p = 1, so 0.5 * (ci + cj) * (pj - pi)
+ // becomes 0.5 * (c + c) * (1 - pi) = c * (1 - pi)
+ Sk4f c = Sk4f::Load(&colors[colorCount - 1]);
+ blend += (1 - pos[colorCount - 1]) * c;
+ }
+ }
+
+ SkColor4f avg;
+ blend.store(&avg);
+ return avg;
+}
+
+// Except for special circumstances of clamped gradients, every gradient shape--when degenerate--
+// can be mapped to the same fallbacks. The specific shape factories must account for special
+// clamped conditions separately, this will always return the last color for clamped gradients.
+static sk_sp<SkShader> make_degenerate_gradient(const SkColor4f colors[], const SkScalar pos[],
+ int colorCount, sk_sp<SkColorSpace> colorSpace,
+ SkShader::TileMode mode) {
+ switch(mode) {
+ case SkShader::kDecal_TileMode:
+ // normally this would reject the area outside of the interpolation region, so since
+ // inside region is empty when the radii are equal, the entire draw region is empty
+ return SkShader::MakeEmptyShader();
+ case SkShader::kRepeat_TileMode:
+ case SkShader::kMirror_TileMode:
+ // repeat and mirror are treated the same: the border colors are never visible,
+ // but approximate the final color as infinite repetitions of the colors, so
+ // it can be represented as the average color of the gradient.
+ return SkShader::MakeColorShader(
+ average_gradient_color(colors, pos, colorCount), std::move(colorSpace));
+ case SkShader::kClamp_TileMode:
+ // Depending on how the gradient shape degenerates, there may be a more specialized
+ // fallback representation for the factories to use, but this is a reasonable default.
+ return SkShader::MakeColorShader(colors[colorCount - 1], std::move(colorSpace));
+ default:
+ SkDEBUGFAIL("Should not be reached");
+ return nullptr;
+ }
+}
+
// assumes colors is SkColor4f* and pos is SkScalar*
#define EXPAND_1_COLOR(count) \
SkColor4f tmp[2]; \
@@ -618,6 +686,14 @@
return nullptr;
}
+ if (SkScalarNearlyZero((pts[1] - pts[0]).length())) {
+ // Degenerate gradient, the only tricky complication is when in clamp mode, the limit of
+ // the gradient approaches two half planes of solid color (first and last). However, they
+ // are divided by the line perpendicular to the start and end point, which becomes undefined
+ // once start and end are exactly the same, so just use the end color for a stable solution.
+ return make_degenerate_gradient(colors, pos, colorCount, std::move(colorSpace), mode);
+ }
+
ColorStopOptimizer opt(colors, pos, colorCount, mode);
SkGradientShaderBase::Descriptor desc;
@@ -644,7 +720,7 @@
SkShader::TileMode mode,
uint32_t flags,
const SkMatrix* localMatrix) {
- if (radius <= 0) {
+ if (radius < 0) {
return nullptr;
}
if (!valid_grad(colors, pos, colorCount, mode)) {
@@ -657,6 +733,11 @@
return nullptr;
}
+ if (SkScalarNearlyZero(radius)) {
+ // Degenerate gradient optimization, and no special logic needed for clamped radial gradient
+ return make_degenerate_gradient(colors, pos, colorCount, std::move(colorSpace), mode);
+ }
+
ColorStopOptimizer opt(colors, pos, colorCount, mode);
SkGradientShaderBase::Descriptor desc;
@@ -694,19 +775,40 @@
if (startRadius < 0 || endRadius < 0) {
return nullptr;
}
- if (SkScalarNearlyZero((start - end).length()) && SkScalarNearlyZero(startRadius)) {
- // We can treat this gradient as radial, which is faster.
- return MakeRadial(start, endRadius, colors, std::move(colorSpace), pos, colorCount,
- mode, flags, localMatrix);
- }
if (!valid_grad(colors, pos, colorCount, mode)) {
return nullptr;
}
- if (startRadius == endRadius) {
- if (start == end || startRadius == 0) {
- return SkShader::MakeEmptyShader();
+ if (SkScalarNearlyZero((start - end).length())) {
+ // If the center positions are the same, then the gradient is the radial variant of a 2 pt
+ // conical gradient, an actual radial gradient (startRadius == 0), or it is fully degenerate
+ // (startRadius == endRadius).
+ if (SkScalarNearlyEqual(startRadius, endRadius)) {
+ // Degenerate case, where the interpolation region area approaches zero. The proper
+ // behavior depends on the tile mode, which is consistent with the default degenerate
+ // gradient behavior, except when mode = clamp and the radii > 0.
+ if (mode == SkShader::TileMode::kClamp_TileMode && endRadius > SK_ScalarNearlyZero) {
+ // The interpolation region becomes an infinitely thin ring at the radius, so the
+ // final gradient will be the first color repeated from p=0 to 1, and then a hard
+ // stop switching to the last color at p=1.
+ static constexpr SkScalar circlePos[3] = {0, 1, 1};
+ SkColor4f reColors[3] = {colors[0], colors[0], colors[colorCount - 1]};
+ return MakeRadial(start, endRadius, reColors, std::move(colorSpace),
+ circlePos, 3, mode, flags, localMatrix);
+ } else {
+ // Otherwise use the default degenerate case
+ return make_degenerate_gradient(
+ colors, pos, colorCount, std::move(colorSpace), mode);
+ }
+ } else if (SkScalarNearlyZero(startRadius)) {
+ // We can treat this gradient as radial, which is faster. If we got here, we know
+ // that endRadius is not equal to 0, so this produces a meaningful gradient
+ return MakeRadial(start, endRadius, colors, std::move(colorSpace), pos, colorCount,
+ mode, flags, localMatrix);
}
+ // Else it's the 2pt conical radial variant with no degenerate radii, so fall through to the
+ // regular 2pt constructor.
}
+
if (localMatrix && !localMatrix->invert(nullptr)) {
return nullptr;
}
@@ -750,13 +852,29 @@
if (1 == colorCount) {
return SkShader::MakeColorShader(colors[0], std::move(colorSpace));
}
- if (!SkScalarIsFinite(startAngle) || !SkScalarIsFinite(endAngle) || startAngle >= endAngle) {
+ if (!SkScalarIsFinite(startAngle) || !SkScalarIsFinite(endAngle) || startAngle > endAngle) {
return nullptr;
}
if (localMatrix && !localMatrix->invert(nullptr)) {
return nullptr;
}
+ if (SkScalarNearlyEqual(startAngle, endAngle)) {
+ // Degenerate gradient, which should follow default degenerate behavior unless it is
+ // clamped and the angle is greater than 0.
+ if (mode == SkShader::kClamp_TileMode && endAngle > SK_ScalarNearlyZero) {
+ // In this case, the first color is repeated from 0 to the angle, then a hardstop
+ // switches to the last color (all other colors are compressed to the infinitely thin
+ // interpolation region).
+ static constexpr SkScalar clampPos[3] = {0, 1, 1};
+ SkColor4f reColors[3] = {colors[0], colors[0], colors[colorCount - 1]};
+ return MakeSweep(cx, cy, reColors, std::move(colorSpace), clampPos, 3, mode, 0,
+ endAngle, flags, localMatrix);
+ } else {
+ return make_degenerate_gradient(colors, pos, colorCount, std::move(colorSpace), mode);
+ }
+ }
+
if (startAngle <= 0 && endAngle >= 360) {
// If the t-range includes [0,1], then we can always use clamping (presumably faster).
mode = SkShader::kClamp_TileMode;
diff --git a/src/shaders/gradients/SkTwoPointConicalGradient.cpp b/src/shaders/gradients/SkTwoPointConicalGradient.cpp
index 49d0631..469c8ad 100644
--- a/src/shaders/gradients/SkTwoPointConicalGradient.cpp
+++ b/src/shaders/gradients/SkTwoPointConicalGradient.cpp
@@ -55,8 +55,10 @@
Type gradientType;
if (SkScalarNearlyZero((c0 - c1).length())) {
- if (SkScalarNearlyZero(SkTMax(r0, r1))) {
- return nullptr; // Degenerate case; avoid dividing by zero.
+ if (SkScalarNearlyZero(SkTMax(r0, r1)) || SkScalarNearlyEqual(r0, r1)) {
+ // Degenerate case; avoid dividing by zero. Should have been caught by caller but
+ // just in case, recheck here.
+ return nullptr;
}
// Concentric case: we can pretend we're radial (with a tiny twist).
const SkScalar scale = sk_ieee_float_divide(1, SkTMax(r0, r1));