Revert "replace SkColorSpaceXformSteps::Required()"
This reverts commit 122eda78775301bb28f467d30ad5edb95dad8df2.
Reason for revert: breaking up into parts to help diagnose a possible performance regression.
Original change's description:
> replace SkColorSpaceXformSteps::Required()
>
> It's error-prone to ask if a color space transform is required without
> explicitly mentioning alpha types. I think the whole method can be
> replaced by a friendlier constructor and direct use of flags/mask:
>
> SkColorSpaceXformSteps::Required(src.colorSpace(), dst.colorSpace())
>
> ~~~>
>
> 0 != SkColorSpaceXformSteps(src, dst).flags.mask()
>
> This removes the unexpected gray square from the fiddle in skia:9662.
>
> Bug: skia:9662
> Change-Id: Id54cae534023ab29c94dcc149e6b67fc9c166665
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/257399
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
TBR=mtklein@google.com,brianosman@google.com
Change-Id: I4073f45c52e77f02fe3ecb75c20fc29873080c20
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: skia:9662
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/257652
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
diff --git a/src/core/SkBlitter_Sprite.cpp b/src/core/SkBlitter_Sprite.cpp
index 701607e..89b107b 100644
--- a/src/core/SkBlitter_Sprite.cpp
+++ b/src/core/SkBlitter_Sprite.cpp
@@ -56,7 +56,8 @@
class SkSpriteBlitter_Memcpy final : public SkSpriteBlitter {
public:
static bool Supports(const SkPixmap& dst, const SkPixmap& src, const SkPaint& paint) {
- SkASSERT(0 == SkColorSpaceXformSteps(src, dst).flags.mask());
+ // the caller has already inspected the colorspace on src and dst
+ SkASSERT(!SkColorSpaceXformSteps::Required(src.colorSpace(), dst.colorSpace()));
if (dst.colorType() != src.colorType()) {
return false;
@@ -177,14 +178,13 @@
*/
SkASSERT(allocator != nullptr);
- // TODO: in principle SkRasterPipelineSpriteBlitter could be made to handle this.
if (source.alphaType() == kUnpremul_SkAlphaType) {
return nullptr;
}
SkSpriteBlitter* blitter = nullptr;
- if (0 == SkColorSpaceXformSteps(source,dst).flags.mask()) {
+ if (!SkColorSpaceXformSteps::Required(source.colorSpace(), dst.colorSpace())) {
if (!blitter && SkSpriteBlitter_Memcpy::Supports(dst, source, paint)) {
blitter = allocator->make<SkSpriteBlitter_Memcpy>(source);
}
diff --git a/src/core/SkColorSpaceXformSteps.cpp b/src/core/SkColorSpaceXformSteps.cpp
index 691ecaf..c15fb9a 100644
--- a/src/core/SkColorSpaceXformSteps.cpp
+++ b/src/core/SkColorSpaceXformSteps.cpp
@@ -10,6 +10,16 @@
#include "src/core/SkColorSpaceXformSteps.h"
#include "src/core/SkRasterPipeline.h"
+// TODO(mtklein): explain the logic of this file
+
+bool SkColorSpaceXformSteps::Required(SkColorSpace* src, SkColorSpace* dst) {
+ // Any SkAlphaType will work fine here as long as we use the same one.
+ SkAlphaType at = kPremul_SkAlphaType;
+ return 0 != SkColorSpaceXformSteps(src, at,
+ dst, at).flags.mask();
+ // TODO(mtklein): quicker impl. that doesn't construct an SkColorSpaceXformSteps?
+}
+
SkColorSpaceXformSteps::SkColorSpaceXformSteps(SkColorSpace* src, SkAlphaType srcAT,
SkColorSpace* dst, SkAlphaType dstAT) {
// Opaque outputs are treated as the same alpha type as the source input.
diff --git a/src/core/SkColorSpaceXformSteps.h b/src/core/SkColorSpaceXformSteps.h
index e470885..0b7a4f9 100644
--- a/src/core/SkColorSpaceXformSteps.h
+++ b/src/core/SkColorSpaceXformSteps.h
@@ -15,6 +15,9 @@
class SkRasterPipeline;
struct SkColorSpaceXformSteps {
+ // Returns true if SkColorSpaceXformSteps must be applied
+ // to draw content in `src` into a destination in `dst`.
+ static bool Required(SkColorSpace* src, SkColorSpace* dst);
struct Flags {
bool unpremul = false;
@@ -35,11 +38,6 @@
SkColorSpaceXformSteps(SkColorSpace* src, SkAlphaType srcAT,
SkColorSpace* dst, SkAlphaType dstAT);
- template <typename S, typename D>
- SkColorSpaceXformSteps(const S& src, const D& dst)
- : SkColorSpaceXformSteps(src.colorSpace(), src.alphaType(),
- dst.colorSpace(), dst.alphaType()) {}
-
void apply(float rgba[4]) const;
void apply(SkRasterPipeline*, bool src_is_normalized) const;
diff --git a/src/gpu/GrSurfaceContext.cpp b/src/gpu/GrSurfaceContext.cpp
index e742b7c..f8f2399 100644
--- a/src/gpu/GrSurfaceContext.cpp
+++ b/src/gpu/GrSurfaceContext.cpp
@@ -105,12 +105,13 @@
// Our tight row bytes may have been changed by clipping.
tightRowBytes = dstInfo.minRowBytes();
+ bool premul = this->colorInfo().alphaType() == kUnpremul_SkAlphaType &&
+ dstInfo.alphaType() == kPremul_SkAlphaType;
+ bool unpremul = this->colorInfo().alphaType() == kPremul_SkAlphaType &&
+ dstInfo.alphaType() == kUnpremul_SkAlphaType;
- SkColorSpaceXformSteps::Flags flags = SkColorSpaceXformSteps{this->colorInfo(), dstInfo}.flags;
- bool unpremul = flags.unpremul,
- needColorConversion = flags.linearize || flags.gamut_transform || flags.encode,
- premul = flags.premul;
-
+ bool needColorConversion =
+ SkColorSpaceXformSteps::Required(this->colorInfo().colorSpace(), dstInfo.colorSpace());
const GrCaps* caps = direct->priv().caps();
// This is the getImageData equivalent to the canvas2D putImageData fast path. We probably don't
@@ -263,10 +264,13 @@
// Our tight row bytes may have been changed by clipping.
tightRowBytes = srcInfo.minRowBytes();
- SkColorSpaceXformSteps::Flags flags = SkColorSpaceXformSteps{srcInfo, this->colorInfo()}.flags;
- bool unpremul = flags.unpremul,
- needColorConversion = flags.linearize || flags.gamut_transform || flags.encode,
- premul = flags.premul;
+ bool premul = this->colorInfo().alphaType() == kPremul_SkAlphaType &&
+ srcInfo.alphaType() == kUnpremul_SkAlphaType;
+ bool unpremul = this->colorInfo().alphaType() == kUnpremul_SkAlphaType &&
+ srcInfo.alphaType() == kPremul_SkAlphaType;
+
+ bool needColorConversion =
+ SkColorSpaceXformSteps::Required(srcInfo.colorSpace(), this->colorInfo().colorSpace());
const GrCaps* caps = direct->priv().caps();
diff --git a/src/shaders/SkShader.cpp b/src/shaders/SkShader.cpp
index 7455488..79520b1 100644
--- a/src/shaders/SkShader.cpp
+++ b/src/shaders/SkShader.cpp
@@ -111,12 +111,7 @@
SkShaderBase::Context::~Context() {}
bool SkShaderBase::ContextRec::isLegacyCompatible(SkColorSpace* shaderColorSpace) const {
- // In legacy pipelines, shaders always produce premul (or opaque) and the destination is also
- // always premul (or opaque). (And those "or opaque" caveats won't make any difference here.)
- SkAlphaType shaderAT = kPremul_SkAlphaType,
- dstAT = kPremul_SkAlphaType;
- return 0 == SkColorSpaceXformSteps{shaderColorSpace, shaderAT,
- fDstColorSpace, dstAT}.flags.mask();
+ return !SkColorSpaceXformSteps::Required(shaderColorSpace, fDstColorSpace);
}
SkImage* SkShader::isAImage(SkMatrix* localMatrix, SkTileMode xy[2]) const {
