Reland "clamp gamut if needed in SkConvertPixels"
This is a reland of b8fef7c9869816c93a9e4fe9547d8ce4b82f2166
readpixels is now disabled on serialize-8888.
The image encoding means it doesn't quite round trip,
though the image looks fine to the eye.
Original change's description:
> clamp gamut if needed in SkConvertPixels
>
> We need to clamp here for all the same reasons we need to clamp when
> blitting. I've centralized the clamp's implementation to help that.
>
> I've allowed any --config to run this GM. --config 565 was actually
> pointing out this problem by asserting, and now looks fine.
>
> Change-Id: Icc066792fc53747b161302d200abdd8dc4669f7f
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
> Reviewed-on: https://skia-review.googlesource.com/158721
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: Mike Klein <mtklein@google.com>
Change-Id: I07601149e1d1e070bf96361f5303569b6a7b4e2a
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://skia-review.googlesource.com/c/159001
Reviewed-by: Mike Klein <mtklein@google.com>
Commit-Queue: Mike Klein <mtklein@google.com>
diff --git a/gm/readpixels.cpp b/gm/readpixels.cpp
index 9defff4..36ff7ed 100644
--- a/gm/readpixels.cpp
+++ b/gm/readpixels.cpp
@@ -124,11 +124,6 @@
}
void onDraw(SkCanvas* canvas) override {
- if (!canvas->imageInfo().colorSpace()) {
- // This gm is only interesting in color correct modes.
- return;
- }
-
const SkAlphaType alphaTypes[] = {
kUnpremul_SkAlphaType,
kPremul_SkAlphaType,
diff --git a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-ASAN.json b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-ASAN.json
index bb854e2..cbe8c06 100644
--- a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-ASAN.json
+++ b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-ASAN.json
@@ -361,6 +361,10 @@
"serialize-8888",
"gm",
"_",
+ "readpixels",
+ "serialize-8888",
+ "gm",
+ "_",
"analytic_antialias_convex",
"serialize-8888",
"gm",
diff --git a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-BonusConfigs.json b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-BonusConfigs.json
index 148aefb..c6f6efb 100644
--- a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-BonusConfigs.json
+++ b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-BonusConfigs.json
@@ -437,6 +437,10 @@
"serialize-8888",
"gm",
"_",
+ "readpixels",
+ "serialize-8888",
+ "gm",
+ "_",
"analytic_antialias_convex",
"serialize-8888",
"gm",
diff --git a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-MSAN.json b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-MSAN.json
index 44cf63f..26e6bd5 100644
--- a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-MSAN.json
+++ b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Debug-All-MSAN.json
@@ -355,6 +355,10 @@
"serialize-8888",
"gm",
"_",
+ "readpixels",
+ "serialize-8888",
+ "gm",
+ "_",
"analytic_antialias_convex",
"serialize-8888",
"gm",
diff --git a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-TSAN.json b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-TSAN.json
index 2211b22..aa28bf4 100644
--- a/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-TSAN.json
+++ b/infra/bots/recipes/test.expected/Test-Debian9-Clang-GCE-CPU-AVX2-x86_64-Release-All-TSAN.json
@@ -356,6 +356,10 @@
"serialize-8888",
"gm",
"_",
+ "readpixels",
+ "serialize-8888",
+ "gm",
+ "_",
"analytic_antialias_convex",
"serialize-8888",
"gm",
diff --git a/infra/bots/recipes/test.py b/infra/bots/recipes/test.py
index 2ccf78c..98f23e9 100644
--- a/infra/bots/recipes/test.py
+++ b/infra/bots/recipes/test.py
@@ -468,6 +468,7 @@
# Not expected to round trip encoding/decoding.
bad_serialize_gms.append('all_bitmap_configs')
bad_serialize_gms.append('makecolorspace')
+ bad_serialize_gms.append('readpixels')
# This GM forces a path to be convex. That property doesn't survive
# serialization.
diff --git a/src/core/SkConvertPixels.cpp b/src/core/SkConvertPixels.cpp
index a8a5c0e..f0de61c 100644
--- a/src/core/SkConvertPixels.cpp
+++ b/src/core/SkConvertPixels.cpp
@@ -177,6 +177,8 @@
pipeline.append_load(srcInfo.colorType(), &src);
steps.apply(&pipeline);
+ pipeline.append_gamut_clamp_if_normalized(dstInfo);
+
// We'll dither if we're decreasing precision below 32-bit.
float dither_rate = 0.0f;
if (srcInfo.bytesPerPixel() > dstInfo.bytesPerPixel()) {
diff --git a/src/core/SkRasterPipeline.cpp b/src/core/SkRasterPipeline.cpp
index 7fe722f..6654f28 100644
--- a/src/core/SkRasterPipeline.cpp
+++ b/src/core/SkRasterPipeline.cpp
@@ -23,6 +23,7 @@
SkASSERT(stage != unbounded_uniform_color); // Please use append_constant_color().
SkASSERT(stage != set_rgb); // Please use append_set_rgb().
SkASSERT(stage != unbounded_set_rgb); // Please use append_set_rgb().
+ SkASSERT(stage != clamp_gamut); // Please use append_gamut_clamp_if_normalized().
this->unchecked_append(stage, ctx);
}
void SkRasterPipeline::unchecked_append(StockStage stage, void* ctx) {
@@ -266,3 +267,12 @@
break;
}
}
+
+void SkRasterPipeline::append_gamut_clamp_if_normalized(const SkImageInfo& dstInfo) {
+ if (dstInfo.colorType() != kRGBA_F16_SkColorType &&
+ dstInfo.colorType() != kRGBA_F32_SkColorType &&
+ dstInfo.alphaType() == kPremul_SkAlphaType)
+ {
+ this->unchecked_append(SkRasterPipeline::clamp_gamut, nullptr);
+ }
+}
diff --git a/src/core/SkRasterPipeline.h b/src/core/SkRasterPipeline.h
index 4650581..21e97b0 100644
--- a/src/core/SkRasterPipeline.h
+++ b/src/core/SkRasterPipeline.h
@@ -156,6 +156,8 @@
void append_load_dst(SkColorType, const SkJumper_MemoryCtx*);
void append_store (SkColorType, const SkJumper_MemoryCtx*);
+ void append_gamut_clamp_if_normalized(const SkImageInfo&);
+
bool empty() const { return fStages == nullptr; }
diff --git a/src/core/SkRasterPipelineBlitter.cpp b/src/core/SkRasterPipelineBlitter.cpp
index 036043b..6627a84 100644
--- a/src/core/SkRasterPipelineBlitter.cpp
+++ b/src/core/SkRasterPipelineBlitter.cpp
@@ -86,17 +86,6 @@
typedef SkBlitter INHERITED;
};
-static void append_gamut_clamp(SkRasterPipeline* p, SkImageInfo dstInfo) {
- // TODO: can we refine this condition further to avoid clamps when we're known in-gamut?
- // When opaque we could _probably_ get away without a clamp, but for consistency we keep it.
- if (dstInfo.colorType() != kRGBA_F16_SkColorType &&
- dstInfo.colorType() != kRGBA_F32_SkColorType &&
- dstInfo.alphaType() == kPremul_SkAlphaType)
- {
- p->append(SkRasterPipeline::clamp_gamut);
- }
-}
-
SkBlitter* SkCreateRasterPipelineBlitter(const SkPixmap& dst,
const SkPaint& paint,
const SkMatrix& ctm,
@@ -214,7 +203,7 @@
if (is_constant) {
SkColor4f constantColor;
SkJumper_MemoryCtx constantColorPtr = { &constantColor, 0 };
- append_gamut_clamp(colorPipeline, dst.info());
+ colorPipeline->append_gamut_clamp_if_normalized(dst.info());
colorPipeline->append(SkRasterPipeline::store_f32, &constantColorPtr);
colorPipeline->run(0,0,1,1);
colorPipeline->reset();
@@ -236,7 +225,7 @@
// Not all blits can memset, so we need to keep colorPipeline too.
SkRasterPipeline_<256> p;
p.extend(*colorPipeline);
- append_gamut_clamp(&p, dst.info());
+ p.append_gamut_clamp_if_normalized(dst.info());
blitter->fDstPtr = SkJumper_MemoryCtx{&blitter->fMemsetColor, 0};
blitter->append_store(&p);
p.run(0,0,1,1);
@@ -301,7 +290,7 @@
if (!fBlitRect) {
SkRasterPipeline p(fAlloc);
p.extend(fColorPipeline);
- append_gamut_clamp(&p, fDst.info());
+ p.append_gamut_clamp_if_normalized(fDst.info());
if (fBlend == SkBlendMode::kSrcOver
&& (fDst.info().colorType() == kRGBA_8888_SkColorType ||
fDst.info().colorType() == kBGRA_8888_SkColorType)
@@ -338,7 +327,7 @@
if (!fBlitAntiH) {
SkRasterPipeline p(fAlloc);
p.extend(fColorPipeline);
- append_gamut_clamp(&p, fDst.info());
+ p.append_gamut_clamp_if_normalized(fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/false)) {
p.append(SkRasterPipeline::scale_1_float, &fCurrentCoverage);
this->append_load_dst(&p);
@@ -423,7 +412,7 @@
if (effectiveMaskFormat == SkMask::kA8_Format && !fBlitMaskA8) {
SkRasterPipeline p(fAlloc);
p.extend(fColorPipeline);
- append_gamut_clamp(&p, fDst.info());
+ p.append_gamut_clamp_if_normalized(fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/false)) {
p.append(SkRasterPipeline::scale_u8, &fMaskPtr);
this->append_load_dst(&p);
@@ -439,7 +428,7 @@
if (effectiveMaskFormat == SkMask::kLCD16_Format && !fBlitMaskLCD16) {
SkRasterPipeline p(fAlloc);
p.extend(fColorPipeline);
- append_gamut_clamp(&p, fDst.info());
+ p.append_gamut_clamp_if_normalized(fDst.info());
if (SkBlendMode_ShouldPreScaleCoverage(fBlend, /*rgb_coverage=*/true)) {
// Somewhat unusually, scale_565 needs dst loaded first.
this->append_load_dst(&p);