Skip to content

Commit

Permalink
Revert "[graphite] Require colorspaces for precomp color xform objects"
Browse files Browse the repository at this point in the history 
This reverts commit 06b3b04.

Reason for revert: Breaking the Chromium roll
https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/2126776/overview

Original change's description:
> [graphite] Require colorspaces for precomp color xform objects
>
> This cuts down on precompilation combinations, but requires more
> information up-front from clients.
>
> Bug: b/391403921
> Change-Id: I6ab9f7e221467ae725e1224a1c69efe06f4f8ef7
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/946696
> Reviewed-by: Robert Phillips <[email protected]>
> Commit-Queue: James Godfrey-Kittle <[email protected]>

Bug: b/391403921
Change-Id: I21414d20b1d3a96e4611b6ebf8b4a773a2532c5f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/953950
Bot-Commit: Rubber Stamper <[email protected]>
Owners-Override: Kaylee Lubick <[email protected]>
  • Loading branch information
@kjlubick
kjlubick committed Feb 21, 2025
1 parent 94bcf7a commit cca9328
Show file tree
Hide file tree
Showing 8 changed files with 78 additions and 111 deletions.
7 changes: 3 additions & 4 deletions include/gpu/graphite/precompile/Precompile.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,9 @@ class PrecompileContext;
* a pipeline.
*/
struct SK_API RenderPassProperties {
DepthStencilFlags fDSFlags = DepthStencilFlags::kNone;
SkColorType fDstCT = kRGBA_8888_SkColorType;
sk_sp<SkColorSpace> fDstCS = nullptr;
bool fRequiresMSAA = false;
DepthStencilFlags fDSFlags = DepthStencilFlags::kNone;
SkColorType fDstCT = kRGBA_8888_SkColorType;
bool fRequiresMSAA = false;
};

/**
Expand Down
2 changes: 1 addition & 1 deletion src/gpu/graphite/PublicPrecompile.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ void Precompile(PrecompileContext* precompileContext,
writeSwizzle,
caps->getDstReadStrategy(info));

SkColorInfo ci(rpp.fDstCT, kPremul_SkAlphaType, rpp.fDstCS);
SkColorInfo ci(rpp.fDstCT, kPremul_SkAlphaType, nullptr);
KeyContext keyContext(caps, dict, rtEffectDict.get(), ci);

for (Coverage coverage : { Coverage::kNone, Coverage::kSingleChannel }) {
Expand Down
121 changes: 54 additions & 67 deletions src/gpu/graphite/precompile/PrecompileColorFilter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@
#include "include/private/SkColorData.h"
#include "src/core/SkColorSpacePriv.h"
#include "src/core/SkKnownRuntimeEffects.h"
#include "src/effects/colorfilters/SkWorkingFormatColorFilter.h"
#include "src/gpu/graphite/BuiltInCodeSnippetID.h"
#include "src/gpu/graphite/KeyContext.h"
#include "src/gpu/graphite/KeyHelpers.h"
#include "src/gpu/graphite/PaintParams.h"
#include "src/gpu/graphite/PaintParamsKey.h"
Expand Down Expand Up @@ -224,54 +222,47 @@ sk_sp<PrecompileColorFilter> PrecompileColorFilters::HSLAMatrix() {

//--------------------------------------------------------------------------------------------------
class PrecompileColorSpaceXformColorFilter : public PrecompileColorFilter {
public:
PrecompileColorSpaceXformColorFilter(SkSpan<const sk_sp<SkColorSpace>> src,
SkSpan<const sk_sp<SkColorSpace>> dst)
: fSrc(src.begin(), src.end())
, fDst(dst.begin(), dst.end())
, fNumCombinations(src.size() * dst.size()) {}
inline static constexpr int kNumCombinations = 3;
inline static constexpr int kPremul = 2;
inline static constexpr int kSRGB = 1;
inline static constexpr int kGeneral = 0;

private:
int numIntrinsicCombinations() const override { return fNumCombinations; }
int numIntrinsicCombinations() const override { return kNumCombinations; }

void addToKey(const KeyContext& keyContext,
PaintParamsKeyBuilder* builder,
PipelineDataGatherer* gatherer,
int desiredCombination) const override {
const int srcCombination = desiredCombination % fSrc.size();
const int dstCombination = desiredCombination / fSrc.size();
SkASSERT(dstCombination < static_cast<int>(fDst.size()));

// The alpha type is unused for determining which color space transform block to use.
constexpr SkAlphaType kAlphaType = kPremul_SkAlphaType;
SkASSERT(desiredCombination < this->numCombinations());

static sk_sp<SkColorSpace> srgbSpinColorSpace = sk_srgb_singleton()->makeColorSpin();
ColorSpaceTransformBlock::ColorSpaceTransformData csData =
ColorSpaceTransformBlock::ColorSpaceTransformData(
fSrc[srcCombination].get(), kAlphaType,
fDst[dstCombination].get(), kAlphaType);
desiredCombination == kPremul
? ColorSpaceTransformBlock::ColorSpaceTransformData(
nullptr, kPremul_SkAlphaType,
nullptr, kUnpremul_SkAlphaType) :
desiredCombination == kSRGB
? ColorSpaceTransformBlock::ColorSpaceTransformData(
sk_srgb_singleton(), kPremul_SkAlphaType,
srgbSpinColorSpace.get(), kPremul_SkAlphaType)
: ColorSpaceTransformBlock::ColorSpaceTransformData(
sk_srgb_singleton(), kPremul_SkAlphaType,
sk_srgb_linear_singleton(), kPremul_SkAlphaType);

ColorSpaceTransformBlock::AddBlock(keyContext, builder, gatherer, csData);
}

std::vector<sk_sp<SkColorSpace>> fSrc;
std::vector<sk_sp<SkColorSpace>> fDst;

const int fNumCombinations;
};

sk_sp<PrecompileColorFilter> PrecompileColorFilters::LinearToSRGBGamma() {
return PrecompileColorFiltersPriv::ColorSpaceXform({ SkColorSpace::MakeSRGBLinear() },
{ SkColorSpace::MakeSRGB() });
return sk_make_sp<PrecompileColorSpaceXformColorFilter>();
}

sk_sp<PrecompileColorFilter> PrecompileColorFilters::SRGBToLinearGamma() {
return PrecompileColorFiltersPriv::ColorSpaceXform({ SkColorSpace::MakeSRGB() },
{ SkColorSpace::MakeSRGBLinear() });
return sk_make_sp<PrecompileColorSpaceXformColorFilter>();
}

sk_sp<PrecompileColorFilter> PrecompileColorFiltersPriv::ColorSpaceXform(
SkSpan<const sk_sp<SkColorSpace>> src, SkSpan<const sk_sp<SkColorSpace>> dst) {
return sk_make_sp<PrecompileColorSpaceXformColorFilter>(src, dst);
sk_sp<PrecompileColorFilter> PrecompileColorFiltersPriv::ColorSpaceXform() {
return sk_make_sp<PrecompileColorSpaceXformColorFilter>();
}

//--------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -334,13 +325,7 @@ sk_sp<PrecompileColorFilter> PrecompileColorFilters::HighContrast() {
if (!cf) {
return nullptr;
}

// These color space working format arguments should match those from
// src/effects/SkHighContrastFilter.cpp.
const skcms_TransferFunction kTF = SkNamedTransferFn::kLinear;
const SkAlphaType kUnpremul = kUnpremul_SkAlphaType;
return PrecompileColorFiltersPriv::WithWorkingFormat(
{std::move(cf)}, &kTF, /* gamut= */ nullptr, &kUnpremul);
return PrecompileColorFiltersPriv::WithWorkingFormat({ std::move(cf) });
}

//--------------------------------------------------------------------------------------------------
Expand Down Expand Up @@ -378,36 +363,47 @@ sk_sp<PrecompileColorFilter> PrecompileColorFiltersPriv::Gaussian() {
//--------------------------------------------------------------------------------------------------
class PrecompileWithWorkingFormatColorFilter : public PrecompileColorFilter {
public:
PrecompileWithWorkingFormatColorFilter(SkSpan<const sk_sp<PrecompileColorFilter>> childOptions,
const skcms_TransferFunction* tf,
const skcms_Matrix3x3* gamut,
const SkAlphaType* at)
: fChildOptions(childOptions.begin(), childOptions.end())
, fWorkingFormatCalculator(tf, gamut, at) {
PrecompileWithWorkingFormatColorFilter(SkSpan<const sk_sp<PrecompileColorFilter>> childOptions)
: fChildOptions(childOptions.begin(), childOptions.end()) {

fNumChildCombos = 0;
for (const auto& childOption : fChildOptions) {
fNumChildCombos += childOption->priv().numCombinations();
}
}

private:
inline static constexpr int kNumCombinations = 3;
inline static constexpr int kPremul = 2;
inline static constexpr int kSRGB = 1;
inline static constexpr int kGeneral = 0;

int numIntrinsicCombinations() const override { return kNumCombinations; }

int numChildCombinations() const override { return fNumChildCombos; }

void addToKey(const KeyContext& keyContext,
PaintParamsKeyBuilder* builder,
PipelineDataGatherer* gatherer,
int desiredCombination) const override {
SkASSERT(desiredCombination < fNumChildCombos);
SkASSERT(desiredCombination < this->numCombinations());

SkAlphaType unusedWorkingAT;
const sk_sp<SkColorSpace> dstCS = keyContext.dstColorInfo().colorSpace()
? keyContext.dstColorInfo().refColorSpace()
: SkColorSpace::MakeSRGB();
const sk_sp<SkColorSpace> workingCS =
fWorkingFormatCalculator.workingFormat(dstCS, &unusedWorkingAT);
const int colorSpaceCombo = desiredCombination / this->numChildCombinations();
const int childCombo = desiredCombination % this->numChildCombinations();

// The alpha type is unused for determining which color space transform block to use.
constexpr SkAlphaType kAlphaType = kPremul_SkAlphaType;
static sk_sp<SkColorSpace> srgbSpinColorSpace = sk_srgb_singleton()->makeColorSpin();
ColorSpaceTransformBlock::ColorSpaceTransformData csData =
colorSpaceCombo == kPremul
? ColorSpaceTransformBlock::ColorSpaceTransformData(
nullptr, kPremul_SkAlphaType,
nullptr, kUnpremul_SkAlphaType) :
colorSpaceCombo == kSRGB
? ColorSpaceTransformBlock::ColorSpaceTransformData(
sk_srgb_singleton(), kPremul_SkAlphaType,
srgbSpinColorSpace.get(), kPremul_SkAlphaType)
: ColorSpaceTransformBlock::ColorSpaceTransformData(
sk_srgb_singleton(), kPremul_SkAlphaType,
sk_srgb_linear_singleton(), kPremul_SkAlphaType);

// Use two nested compose blocks to chain (dst->working), child, and (working->dst) together
// while appearing as one block to the parent node.
Expand All @@ -417,38 +413,29 @@ class PrecompileWithWorkingFormatColorFilter : public PrecompileColorFilter {
Compose(keyContext, builder, gatherer,
/* addInnerToKey= */ [&]() -> void {
// Innermost (inner of inner compose)
ColorSpaceTransformBlock::ColorSpaceTransformData data1(
dstCS.get(), kAlphaType, workingCS.get(), kAlphaType);
ColorSpaceTransformBlock::AddBlock(keyContext, builder, gatherer,
data1);
csData);
},
/* addOuterToKey= */ [&]() -> void {
// Middle (outer of inner compose)
AddToKey<PrecompileColorFilter>(keyContext, builder, gatherer,
fChildOptions, desiredCombination);
fChildOptions, childCombo);
});
},
/* addOuterToKey= */ [&]() -> void {
// Outermost (outer of outer compose)
ColorSpaceTransformBlock::ColorSpaceTransformData data2(
workingCS.get(), kAlphaType, dstCS.get(), kAlphaType);
ColorSpaceTransformBlock::AddBlock(keyContext, builder, gatherer, data2);
ColorSpaceTransformBlock::AddBlock(keyContext, builder, gatherer, csData);
});
}

std::vector<sk_sp<PrecompileColorFilter>> fChildOptions;

int fNumChildCombos;

SkWorkingFormatCalculator fWorkingFormatCalculator;
};

sk_sp<PrecompileColorFilter> PrecompileColorFiltersPriv::WithWorkingFormat(
SkSpan<const sk_sp<PrecompileColorFilter>> childOptions,
const skcms_TransferFunction* tf,
const skcms_Matrix3x3* gamut,
const SkAlphaType* at) {
return sk_make_sp<PrecompileWithWorkingFormatColorFilter>(childOptions, tf, gamut, at);
SkSpan<const sk_sp<PrecompileColorFilter>> childOptions) {
return sk_make_sp<PrecompileWithWorkingFormatColorFilter>(childOptions);
}

} // namespace skgpu::graphite
8 changes: 2 additions & 6 deletions src/gpu/graphite/precompile/PrecompileColorFiltersPriv.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,10 @@ namespace PrecompileColorFiltersPriv {
// These three factories match those in src/core/SkColorFilterPriv.h
sk_sp<PrecompileColorFilter> Gaussian();

sk_sp<PrecompileColorFilter> ColorSpaceXform(SkSpan<const sk_sp<SkColorSpace>> src,
SkSpan<const sk_sp<SkColorSpace>> dst);
sk_sp<PrecompileColorFilter> ColorSpaceXform();

sk_sp<PrecompileColorFilter> WithWorkingFormat(
SkSpan<const sk_sp<PrecompileColorFilter>> childOptions,
const skcms_TransferFunction* tf,
const skcms_Matrix3x3* gamut,
const SkAlphaType* at);
SkSpan<const sk_sp<PrecompileColorFilter>> childOptions);

} // namespace PrecompileColorFiltersPriv

Expand Down
15 changes: 3 additions & 12 deletions tests/graphite/precompile/ChromePrecompileTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,28 +85,19 @@ PaintOptions lineargrad_srcover_dithered() {
// "RP(color: Dawn(f=23,s=1), resolve: {}, ds: Dawn(f=39,s=1), samples: 1, swizzle: rgba)"
// Single sampled BGRA w/ just depth
RenderPassProperties bgra_1_depth() {
return {DepthStencilFlags::kDepth,
kBGRA_8888_SkColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ false};
return { DepthStencilFlags::kDepth, kBGRA_8888_SkColorType, /* requiresMSAA= */ false };
}

// "RP(color: Dawn(f=23,s=4), resolve: Dawn(f=23,s=1), ds: Dawn(f=39,s=4), samples: 4, swizzle: rgba)"
// MSAA BGRA w/ just depth
RenderPassProperties bgra_4_depth() {
return {DepthStencilFlags::kDepth,
kBGRA_8888_SkColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ true};
return { DepthStencilFlags::kDepth, kBGRA_8888_SkColorType, /* requiresMSAA= */ true };
}

// "RP(color: Dawn(f=23,s=4), resolve: Dawn(f=23,s=1), ds: Dawn(f=41,s=4), samples: 4, swizzle: rgba)"
// MSAA BGRA w/ depth and stencil
RenderPassProperties bgra_4_depth_stencil() {
return {DepthStencilFlags::kDepthStencil,
kBGRA_8888_SkColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ true};
return { DepthStencilFlags::kDepthStencil, kBGRA_8888_SkColorType, /* requiresMSAA= */ true };
}

// Precompile with the provided paintOptions, drawType, and RenderPassSettings then verify that
Expand Down
4 changes: 2 additions & 2 deletions tests/graphite/precompile/CombinationBuilderTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ namespace {

// colorfilters
static constexpr int kExpectedBlendCFCombos = 15;
static constexpr int kExpectedColorSpaceCFCombos = 1;
static constexpr int kExpectedHighContrastCFCombos = 1;
static constexpr int kExpectedColorSpaceCFCombos = 3;
static constexpr int kExpectedHighContrastCFCombos = 3;
static constexpr int kExpectedLightingCFCombos = 1;
static constexpr int kExpectedLumaCFCombos = 1;
static constexpr int kExpectedMatrixCFCombos = 1;
Expand Down
25 changes: 10 additions & 15 deletions tests/graphite/precompile/PaintParamsKeyTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1094,10 +1094,9 @@ std::pair<sk_sp<SkColorFilter>, sk_sp<PrecompileColorFilter>> create_matrix_colo

std::pair<sk_sp<SkColorFilter>, sk_sp<PrecompileColorFilter>> create_color_space_colorfilter(
SkRandom* rand) {
sk_sp<SkColorSpace> src = random_colorspace(rand);
sk_sp<SkColorSpace> dst = random_colorspace(rand);
return { SkColorFilterPriv::MakeColorSpaceXform(src, dst),
PrecompileColorFiltersPriv::ColorSpaceXform({ src }, { dst }) };
return { SkColorFilterPriv::MakeColorSpaceXform(random_colorspace(rand),
random_colorspace(rand)),
PrecompileColorFiltersPriv::ColorSpaceXform() };
}

std::pair<sk_sp<SkColorFilter>, sk_sp<PrecompileColorFilter>> create_linear_to_srgb_colorfilter() {
Expand Down Expand Up @@ -1164,15 +1163,14 @@ std::pair<sk_sp<SkColorFilter>, sk_sp<PrecompileColorFilter>> create_workingform

SkASSERT(childCF && childO);

const skcms_TransferFunction* tf = rand->nextBool() ? &random_xfer_function(rand) : nullptr;
const skcms_Matrix3x3* gamut = rand->nextBool() ? &random_gamut(rand) : nullptr;
const SkAlphaType unpremul = kUnpremul_SkAlphaType;

sk_sp<SkColorFilter> cf =
SkColorFilterPriv::WithWorkingFormat(std::move(childCF), tf, gamut, &unpremul);
SkAlphaType unpremul = kUnpremul_SkAlphaType;
sk_sp<SkColorFilter> cf = SkColorFilterPriv::WithWorkingFormat(std::move(childCF),
&random_xfer_function(rand),
&random_gamut(rand),
&unpremul);

sk_sp<PrecompileColorFilter> o = PrecompileColorFiltersPriv::WithWorkingFormat(
{ std::move(childO) }, tf, gamut, &unpremul);
{ std::move(childO) });

return { std::move(cf), std::move(o) };
}
Expand Down Expand Up @@ -2011,11 +2009,9 @@ void precompile_vs_real_draws_subtest(skiatest::Reporter* reporter,

static const RenderPassProperties kDepth_Stencil_4 { DepthStencilFlags::kDepthStencil,
kColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ true };
static const RenderPassProperties kDepth_1 { DepthStencilFlags::kDepth,
kColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ false };

TextureInfo textureInfo = caps->getDefaultSampledTextureInfo(kColorType,
Expand Down Expand Up @@ -2043,8 +2039,7 @@ void precompile_vs_real_draws_subtest(skiatest::Reporter* reporter,
// The skp draws a rect w/ a default SkPaint and RGBA dst color type
PaintOptions skpPaintOptions;
Precompile(precompileContext, skpPaintOptions, DrawTypeFlags::kSimpleShape,
{ { kDepth_1.fDSFlags, kRGBA_8888_SkColorType, kDepth_1.fDstCS,
kDepth_1.fRequiresMSAA } });
{ { kDepth_1.fDSFlags, kRGBA_8888_SkColorType, kDepth_1.fRequiresMSAA } });
}
int after = globalCache->numGraphicsPipelines();

Expand Down
7 changes: 3 additions & 4 deletions tests/graphite/precompile/ThreadedPrecompileTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,9 @@ void precompile_gradients(std::unique_ptr<PrecompileContext> precompileContext,
std::shuffle(combos.begin(), combos.end(), g);
}

const RenderPassProperties kProps = { DepthStencilFlags::kDepth,
kBGRA_8888_SkColorType,
/* dstColorSpace= */ nullptr,
/* requiresMSAA= */ false };
constexpr RenderPassProperties kProps = { DepthStencilFlags::kDepth,
kBGRA_8888_SkColorType,
/* requiresMSAA= */ false };

for (auto c : combos) {
auto [_, paintOptions] = c.fCreateOptionsMtd(c.fNumStops);
Expand Down

0 comments on commit cca9328

Please sign in to comment.