SkRuntimeEffect: Apply uniform transforms in color filters, too
We were only respecting this feature in runtime shaders. Note that use
of any tagged matrices will cause color filter creation to fail, but
color transformation is a totally sensible thing to want in a color
filter.
Change-Id: I482226b287ab794cb341367fce453381cb581966
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/308507
Reviewed-by: Mike Klein <mtklein@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/core/SkRuntimeEffect.cpp b/src/core/SkRuntimeEffect.cpp
index 8a0193d..f30efab 100644
--- a/src/core/SkRuntimeEffect.cpp
+++ b/src/core/SkRuntimeEffect.cpp
@@ -127,7 +127,10 @@
// Color filters are not allowed to depend on position (local or device) in any way, but they
// can sample children with matrices or explicit coords. Because the children are color filters,
// we know (by induction) that they don't use those coords, so we keep the overall invariant.
- const bool allowColorFilter = !usesSampleCoords && !usesFragCoords;
+ //
+ // Further down, we also ensure that color filters can't use layout(marker), which would allow
+ // them to change behavior based on the CTM.
+ bool allowColorFilter = !usesSampleCoords && !usesFragCoords;
std::vector<const SkSL::Variable*> inVars;
std::vector<const SkSL::Variable*> uniformVars;
@@ -240,6 +243,7 @@
const SkSL::StringFragment& marker(var->fModifiers.fLayout.fMarker);
if (marker.fLength) {
v.fFlags |= Variable::kMarker_Flag;
+ allowColorFilter = false;
if (!parse_marker(marker, &v.fMarker, &v.fFlags)) {
RETURN_FAILURE("Invalid 'marker' string: '%.*s'", (int)marker.fLength,
marker.fChars);
@@ -690,6 +694,71 @@
return stack;
}
+static sk_sp<SkData> get_xformed_inputs(const SkRuntimeEffect* effect,
+ sk_sp<SkData> baseInputs,
+ const SkMatrixProvider* matrixProvider,
+ const SkColorSpace* dstCS) {
+ using Flags = SkRuntimeEffect::Variable::Flags;
+ using Type = SkRuntimeEffect::Variable::Type;
+ SkColorSpaceXformSteps steps(sk_srgb_singleton(), kUnpremul_SkAlphaType,
+ dstCS, kUnpremul_SkAlphaType);
+
+ sk_sp<SkData> inputs = nullptr;
+ auto writableData = [&]() {
+ if (!inputs) {
+ inputs = SkData::MakeWithCopy(baseInputs->data(), baseInputs->size());
+ }
+ return inputs->writable_data();
+ };
+
+ for (const auto& v : effect->inputs()) {
+ if (v.fFlags & Flags::kMarker_Flag) {
+ SkASSERT(v.fType == Type::kFloat4x4);
+ // Color filters don't provide a matrix provider, but shouldn't be allowed to get here
+ SkASSERT(matrixProvider);
+ SkM44* localToMarker = SkTAddOffset<SkM44>(writableData(), v.fOffset);
+ if (!matrixProvider->getLocalToMarker(v.fMarker, localToMarker)) {
+ // We couldn't provide a matrix that was requested by the SkSL
+ return nullptr;
+ }
+ if (v.fFlags & Flags::kMarkerNormals_Flag) {
+ // Normals need to be transformed by the inverse-transpose of the upper-left
+ // 3x3 portion (scale + rotate) of the matrix.
+ localToMarker->setRow(3, {0, 0, 0, 1});
+ localToMarker->setCol(3, {0, 0, 0, 1});
+ if (!localToMarker->invert(localToMarker)) {
+ return nullptr;
+ }
+ *localToMarker = localToMarker->transpose();
+ }
+ } else if (v.fFlags & Flags::kSRGBUnpremul_Flag) {
+ SkASSERT(v.fType == Type::kFloat3 || v.fType == Type::kFloat4);
+ if (steps.flags.mask()) {
+ float* color = SkTAddOffset<float>(writableData(), v.fOffset);
+ if (v.fType == Type::kFloat4) {
+ // RGBA, easy case
+ for (int i = 0; i < v.fCount; ++i) {
+ steps.apply(color);
+ color += 4;
+ }
+ } else {
+ // RGB, need to pad out to include alpha. Technically, this isn't necessary,
+ // because steps shouldn't include unpremul or premul, and thus shouldn't
+ // read or write the fourth element. But let's be safe.
+ float rgba[4];
+ for (int i = 0; i < v.fCount; ++i) {
+ memcpy(rgba, color, 3 * sizeof(float));
+ rgba[3] = 1.0f;
+ steps.apply(rgba);
+ memcpy(color, rgba, 3 * sizeof(float));
+ color += 3;
+ }
+ }
+ }
+ }
+ }
+ return inputs ? inputs : baseInputs;
+}
class SkRuntimeColorFilter : public SkColorFilterBase {
public:
@@ -701,7 +770,14 @@
GrFPResult asFragmentProcessor(std::unique_ptr<GrFragmentProcessor> inputFP,
GrRecordingContext* context,
const GrColorInfo& colorInfo) const override {
- auto runtimeFP = GrSkSLFP::Make(context, fEffect, "Runtime_Color_Filter", fInputs);
+ sk_sp<SkData> inputs =
+ get_xformed_inputs(fEffect.get(), fInputs, nullptr, colorInfo.colorSpace());
+ if (!inputs) {
+ return GrFPFailure(nullptr);
+ }
+
+ auto runtimeFP =
+ GrSkSLFP::Make(context, fEffect, "Runtime_Color_Filter", std::move(inputs));
if (inputFP == nullptr) {
return GrFPSuccess(std::move(runtimeFP));
}
@@ -730,7 +806,7 @@
}
skvm::Color onProgram(skvm::Builder* p, skvm::Color c,
- SkColorSpace* /*dstCS*/,
+ SkColorSpace* dstCS,
skvm::Uniforms* uniforms, SkArenaAlloc*) const override {
const SkSL::ByteCode* bc = this->byteCode();
if (!bc) {
@@ -742,10 +818,15 @@
return {};
}
+ sk_sp<SkData> inputs = get_xformed_inputs(fEffect.get(), fInputs, nullptr, dstCS);
+ if (!inputs) {
+ return {};
+ }
+
std::vector<skvm::F32> uniform;
for (int i = 0; i < (int)fEffect->uniformSize() / 4; i++) {
float f;
- memcpy(&f, (const char*)fInputs->data() + 4*i, 4);
+ memcpy(&f, (const char*)inputs->data() + 4*i, 4);
uniform.push_back(p->uniformF(uniforms->pushF(f)));
}
@@ -807,68 +888,6 @@
bool isOpaque() const override { return fIsOpaque; }
- sk_sp<SkData> getUniforms(const SkMatrixProvider& matrixProvider,
- const SkColorSpace* dstCS) const {
- using Flags = SkRuntimeEffect::Variable::Flags;
- using Type = SkRuntimeEffect::Variable::Type;
- SkColorSpaceXformSteps steps(sk_srgb_singleton(), kUnpremul_SkAlphaType,
- dstCS, kUnpremul_SkAlphaType);
-
- sk_sp<SkData> inputs = nullptr;
- auto writableData = [&]() {
- if (!inputs) {
- inputs = SkData::MakeWithCopy(fInputs->data(), fInputs->size());
- }
- return inputs->writable_data();
- };
-
- for (const auto& v : fEffect->inputs()) {
- if (v.fFlags & Flags::kMarker_Flag) {
- SkASSERT(v.fType == Type::kFloat4x4);
- SkM44* localToMarker = SkTAddOffset<SkM44>(writableData(), v.fOffset);
- if (!matrixProvider.getLocalToMarker(v.fMarker, localToMarker)) {
- // We couldn't provide a matrix that was requested by the SkSL
- return nullptr;
- }
- if (v.fFlags & Flags::kMarkerNormals_Flag) {
- // Normals need to be transformed by the inverse-transpose of the upper-left
- // 3x3 portion (scale + rotate) of the matrix.
- localToMarker->setRow(3, {0, 0, 0, 1});
- localToMarker->setCol(3, {0, 0, 0, 1});
- if (!localToMarker->invert(localToMarker)) {
- return nullptr;
- }
- *localToMarker = localToMarker->transpose();
- }
- } else if (v.fFlags & Flags::kSRGBUnpremul_Flag) {
- SkASSERT(v.fType == Type::kFloat3 || v.fType == Type::kFloat4);
- if (steps.flags.mask()) {
- float* color = SkTAddOffset<float>(writableData(), v.fOffset);
- if (v.fType == Type::kFloat4) {
- // RGBA, easy case
- for (int i = 0; i < v.fCount; ++i) {
- steps.apply(color);
- color += 4;
- }
- } else {
- // RGB, need to pad out to include alpha. Technically, this isn't necessary,
- // because steps shouldn't include unpremul or premul, and thus shouldn't
- // read or write the fourth element. But let's be safe.
- float rgba[4];
- for (int i = 0; i < v.fCount; ++i) {
- memcpy(rgba, color, 3 * sizeof(float));
- rgba[3] = 1.0f;
- steps.apply(rgba);
- memcpy(color, rgba, 3 * sizeof(float));
- color += 3;
- }
- }
- }
- }
- }
- return inputs ? inputs : fInputs;
- }
-
#if SK_SUPPORT_GPU
std::unique_ptr<GrFragmentProcessor> asFragmentProcessor(const GrFPArgs& args) const override {
SkMatrix matrix;
@@ -876,8 +895,8 @@
return nullptr;
}
- sk_sp<SkData> inputs =
- this->getUniforms(args.fMatrixProvider, args.fDstColorInfo->colorSpace());
+ sk_sp<SkData> inputs = get_xformed_inputs(fEffect.get(), fInputs, &args.fMatrixProvider,
+ args.fDstColorInfo->colorSpace());
if (!inputs) {
return nullptr;
}
@@ -929,7 +948,8 @@
return {};
}
- sk_sp<SkData> inputs = this->getUniforms(matrices, dst.colorSpace());
+ sk_sp<SkData> inputs =
+ get_xformed_inputs(fEffect.get(), fInputs, &matrices, dst.colorSpace());
if (!inputs) {
return {};
}
diff --git a/tests/SkRuntimeEffectTest.cpp b/tests/SkRuntimeEffectTest.cpp
index 0ebe63f..fad294e 100644
--- a/tests/SkRuntimeEffectTest.cpp
+++ b/tests/SkRuntimeEffectTest.cpp
@@ -8,6 +8,7 @@
#include "include/core/SkBitmap.h"
#include "include/core/SkCanvas.h"
#include "include/core/SkColorFilter.h"
+#include "include/core/SkData.h"
#include "include/core/SkPaint.h"
#include "include/core/SkSurface.h"
#include "include/effects/SkRuntimeEffect.h"
@@ -85,14 +86,21 @@
auto test = [r](const char* sksl) {
auto [effect, errorText] = SkRuntimeEffect::Make(SkString(sksl));
REPORTER_ASSERT(r, effect);
- REPORTER_ASSERT(r, effect->makeShader(nullptr, nullptr, 0, nullptr, false));
- REPORTER_ASSERT(r, !effect->makeColorFilter(nullptr));
+
+ sk_sp<SkData> inputs = SkData::MakeUninitialized(effect->inputSize());
+
+ REPORTER_ASSERT(r, effect->makeShader(inputs, nullptr, 0, nullptr, false));
+ REPORTER_ASSERT(r, !effect->makeColorFilter(inputs));
};
// Runtime effects that use sample coords or sk_FragCoord are valid shaders,
// but not valid color filters
test("void main(float2 p, inout half4 color) { color.rg = half2(p); }");
test("void main(float2 p, inout half4 color) { color.rg = half2(sk_FragCoord.xy); }");
+
+ // We also can't use layout(marker), which would give the runtime color filter CTM information
+ test("layout(marker=ctm) uniform float4x4 ctm;"
+ "void main(float2 p, inout half4 color) { color.r = half(ctm[0][0]); }");
}
class TestEffect {