Remove all clamping to "valid" premultiplied colors
Previously, we were abundantly cautious about producing premultiplied
colors with RGB > A, when the color-type was normalized fixed-point.
I'm not seeing any evidence that code cares about that. If it does, I
suggest we fix the issue at the API border, or via dedicated pass. In
the common case, this makes Skia less opinionated, which (in general)
makes it more versatile.
Change-Id: Iae524ff61f6c81073c34d0f2dced973c229cdfb7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/452558
Reviewed-by: Brian Salomon <bsalomon@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 4b12857..d6fddb2 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -1002,19 +1002,7 @@
if (fIsOpaque) {
fp = GrFragmentProcessor::SwizzleOutput(std::move(fp), GrSwizzle::RGB1());
}
- fp = GrMatrixEffect::Make(matrix, std::move(fp));
- // Three cases of GrClampType to think about:
- // kAuto - Normalized fixed-point. If fIsOpaque, then A is 1 (above), and the format's
- // range ensures RGB must be no larger. If !fIsOpaque, we clamp here.
- // kManual - Normalized floating point. Whether or not we set A above, the format's range
- // means we need to clamp RGB.
- // kNone - Unclamped floating point. No clamping is done, ever.
- GrClampType clampType = GrColorTypeClampType(args.fDstColorInfo->colorType());
- if (clampType == GrClampType::kManual || (clampType == GrClampType::kAuto && !fIsOpaque)) {
- return GrFragmentProcessor::ClampPremulOutput(std::move(fp));
- } else {
- return std::move(fp);
- }
+ return GrMatrixEffect::Make(matrix, std::move(fp));
}
#endif
diff --git a/src/core/SkVMBlitter.cpp b/src/core/SkVMBlitter.cpp
index a1e9996..5218e8a 100644
--- a/src/core/SkVMBlitter.cpp
+++ b/src/core/SkVMBlitter.cpp
@@ -324,25 +324,12 @@
src.b = min(src.b * M + A, src.a);
}
- // If we can determine this we can skip a fair bit of clamping!
- bool src_in_gamut = false;
-
- // Normalized premul formats can surprisingly represent some out-of-gamut
- // values (e.g. r=0xff, a=0xee fits in unorm8 but r = 1.07), but most code
- // working with normalized premul colors is not prepared to handle r,g,b > a.
- // So we clamp the shader to gamut here before blending and coverage.
- //
- // In addition, GL clamps all its color channels to limits of the format just
- // before the blend step (~here). To match that auto-clamp, we clamp alpha to
- // [0,1] too, just in case someone gave us an out of range alpha.
- if (!src_in_gamut
- && params.dst.alphaType() == kPremul_SkAlphaType
- && SkColorTypeIsNormalized(params.dst.colorType())) {
- src.a = clamp(src.a, 0.0f, 1.0f);
- src.r = clamp(src.r, 0.0f, src.a);
- src.g = clamp(src.g, 0.0f, src.a);
- src.b = clamp(src.b, 0.0f, src.a);
- src_in_gamut = true;
+ // GL clamps all its color channels to limits of the format just before the blend step (~here).
+ // TODO: Below, we also clamp after the blend step. If we can prove that none of the work here
+ // (especially blending, for built-in blend modes) will produce colors outside [0, 1] we may be
+ // able to skip the second clamp. For now, we clamp twice.
+ if (SkColorTypeIsNormalized(params.dst.colorType())) {
+ src = clamp01(src);
}
// Load the destination color.
@@ -425,17 +412,7 @@
}
// Clamp to fit destination color format if needed.
- if (as_blendmode && src_in_gamut) {
- // An in-gamut src blended with an in-gamut dst should stay in gamut.
- // Being in-gamut implies all channels are in [0,1], so no need to clamp.
- // We allow one ulp error above 1.0f, and about that much (~1.2e-7) below 0.
- skvm::F32 lo = pun_to_F32(p->splat(0xb400'0000)),
- hi = pun_to_F32(p->splat(0x3f80'0001));
- assert_true(src.r == clamp(src.r, lo, hi), src.r);
- assert_true(src.g == clamp(src.g, lo, hi), src.g);
- assert_true(src.b == clamp(src.b, lo, hi), src.b);
- assert_true(src.a == clamp(src.a, lo, hi), src.a);
- } else if (SkColorTypeIsNormalized(params.dst.colorType())) {
+ if (SkColorTypeIsNormalized(params.dst.colorType())) {
src = clamp01(src);
}
diff --git a/tests/SkDSLRuntimeEffectTest.cpp b/tests/SkDSLRuntimeEffectTest.cpp
index f9a197e..8b7f0e9 100644
--- a/tests/SkDSLRuntimeEffectTest.cpp
+++ b/tests/SkDSLRuntimeEffectTest.cpp
@@ -142,7 +142,7 @@
effect.uniform(SkString(gColor.name()).c_str()) = float4{ 0.0f, 0.25f, 0.75f, 1.0f };
effect.test(0xFFBF4000);
effect.uniform(SkString(gColor.name()).c_str()) = float4{ 1.0f, 0.0f, 0.0f, 0.498f };
- effect.test(0x7F00007F); // Tests that we clamp to valid premul
+ effect.test(0x7F0000FF); // Tests that we don't clamp to valid premul
}
// Same, with integer uniforms
@@ -158,7 +158,7 @@
effect.uniform(SkString(gColor.name()).c_str()) = int4{ 0x00, 0x40, 0xBF, 0xFF };
effect.test(0xFFBF4000);
effect.uniform(SkString(gColor.name()).c_str()) = int4{ 0xFF, 0x00, 0x00, 0x7F };
- effect.test(0x7F00007F); // Tests that we clamp to valid premul
+ effect.test(0x7F0000FF); // Tests that we don't clamp to valid premul
}
// Test sk_FragCoord (device coords). Rotate the canvas to be sure we're seeing device coords.
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 428af17..7fac5ee 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -439,14 +439,14 @@
effect.uniform("gColor") = float4{ 0.0f, 0.25f, 0.75f, 1.0f };
effect.test(0xFFBF4000);
effect.uniform("gColor") = float4{ 1.0f, 0.0f, 0.0f, 0.498f };
- effect.test(0x7F00007F); // Tests that we clamp to valid premul
+ effect.test(0x7F0000FF); // Tests that we don't clamp to valid premul
// Same, with integer uniforms
effect.build("uniform int4 gColor; half4 main(float2 p) { return half4(gColor) / 255.0; }");
effect.uniform("gColor") = int4{ 0x00, 0x40, 0xBF, 0xFF };
effect.test(0xFFBF4000);
effect.uniform("gColor") = int4{ 0xFF, 0x00, 0x00, 0x7F };
- effect.test(0x7F00007F); // Tests that we clamp to valid premul
+ effect.test(0x7F0000FF); // Tests that we don't clamp to valid premul
// Test sk_FragCoord (device coords). Rotate the canvas to be sure we're seeing device coords.
// Since the surface is 2x2, we should see (0,0), (1,0), (0,1), (1,1). Multiply by 0.498 to
@@ -540,7 +540,7 @@
effect.uniform("gColor") = float4{ 0.0f, 0.25f, 0.75f, 1.0f };
effect.test(0xFFBF4000);
effect.uniform("gColor") = float4{ 1.0f, 0.0f, 0.0f, 0.498f };
- effect.test(0x7F0000FF); // Unlike SkShaders, we don't clamp here
+ effect.test(0x7F0000FF); // We don't clamp here either
// Same, with integer uniforms
effect.build("uniform int4 gColor;"
@@ -548,7 +548,7 @@
effect.uniform("gColor") = int4{ 0x00, 0x40, 0xBF, 0xFF };
effect.test(0xFFBF4000);
effect.uniform("gColor") = int4{ 0xFF, 0x00, 0x00, 0x7F };
- effect.test(0x7F0000FF); // Unlike SkShaders, we don't clamp here
+ effect.test(0x7F0000FF); // We don't clamp here either
// Verify that mutating the source and destination colors is allowed
effect.build("half4 main(half4 s, half4 d) { s += d; d += s; return half4(1); }");