Reland "Add SkImageInfoValidConversion() and SkImageInfoIsValid"
The original is at:
https://skia-review.googlesource.com/c/6887/
The only change to the original is to temporarily comment out
a check in SkImageInfoPriv.h until a Chrome unit test can
be fixed.
The idea is share these standards for the following:
SkImage::readPixels()
SkCanvas::readPixels()
SkCanvas::writePixels()
SkBitmap::readPixels()
SkPixmap::readPixels()
On the raster side, SkPixmap::readPixels() is the right
place to check, because all raster calls go through
there eventually. Then at lower levels (ex: SkPixelInfo),
we can assert.
There's not really a unifying location for gpu calls,
so I've added this in multiple places. I haven't really
dug into the gpu code to SkASSERT() on invalid cases
that we will have already caught.
Follow-up work:
Similar refactor for SkReadPixelRec::trim().
Code cleanup in SkPixelInfo::CopyPixels()
BUG=skia:6021
Change-Id: I6a16f9479bc09e3c87e10c72b0378579f1a70866
Reviewed-on: https://skia-review.googlesource.com/7104
Reviewed-by: Matt Sarett <msarett@google.com>
Commit-Queue: Matt Sarett <msarett@google.com>
diff --git a/gm/showmiplevels.cpp b/gm/showmiplevels.cpp
index a4fd164..0394b57 100644
--- a/gm/showmiplevels.cpp
+++ b/gm/showmiplevels.cpp
@@ -216,6 +216,46 @@
///////////////////////////////////////////////////////////////////////////////////////////////////
+static void copy_32_to_g8(void* dst, size_t dstRB, const void* src, const SkImageInfo& srcInfo,
+ size_t srcRB) {
+ uint8_t* dst8 = (uint8_t*)dst;
+ const uint32_t* src32 = (const uint32_t*)src;
+
+ const int w = srcInfo.width();
+ const int h = srcInfo.height();
+ const bool isBGRA = (kBGRA_8888_SkColorType == srcInfo.colorType());
+
+ for (int y = 0; y < h; ++y) {
+ if (isBGRA) {
+ // BGRA
+ for (int x = 0; x < w; ++x) {
+ uint32_t s = src32[x];
+ dst8[x] = SkComputeLuminance((s >> 16) & 0xFF, (s >> 8) & 0xFF, s & 0xFF);
+ }
+ } else {
+ // RGBA
+ for (int x = 0; x < w; ++x) {
+ uint32_t s = src32[x];
+ dst8[x] = SkComputeLuminance(s & 0xFF, (s >> 8) & 0xFF, (s >> 16) & 0xFF);
+ }
+ }
+ src32 = (const uint32_t*)((const char*)src32 + srcRB);
+ dst8 += dstRB;
+ }
+}
+
+void copy_to(SkBitmap* dst, SkColorType dstColorType, const SkBitmap& src) {
+ if (kGray_8_SkColorType == dstColorType) {
+ SkImageInfo grayInfo = src.info().makeColorType(kGray_8_SkColorType);
+ dst->allocPixels(grayInfo);
+ copy_32_to_g8(dst->getPixels(), dst->rowBytes(), src.getPixels(), src.info(),
+ src.rowBytes());
+ return;
+ }
+
+ src.copyTo(dst, dstColorType);
+}
+
/**
* Show mip levels that were built, for all supported colortypes
*/
@@ -283,7 +323,7 @@
for (auto ctype : ctypes) {
SkBitmap bm;
- orig.copyTo(&bm, ctype);
+ copy_to(&bm, ctype, orig);
drawLevels(canvas, bm);
canvas->translate(orig.width()/2 + 8.0f, 0);
}
diff --git a/src/core/SkBitmap.cpp b/src/core/SkBitmap.cpp
index e7c75b2..00b2f8e 100644
--- a/src/core/SkBitmap.cpp
+++ b/src/core/SkBitmap.cpp
@@ -681,6 +681,7 @@
case kRGBA_8888_SkColorType:
case kBGRA_8888_SkColorType:
break;
+ case kGray_8_SkColorType:
case kIndex_8_SkColorType:
if (!sameConfigs) {
return false;
@@ -688,16 +689,6 @@
break;
case kARGB_4444_SkColorType:
return sameConfigs || kN32_SkColorType == srcCT || kIndex_8_SkColorType == srcCT;
- case kGray_8_SkColorType:
- switch (srcCT) {
- case kGray_8_SkColorType:
- case kRGBA_8888_SkColorType:
- case kBGRA_8888_SkColorType:
- return true;
- default:
- break;
- }
- return false;
default:
return false;
}
@@ -774,7 +765,13 @@
if (!src->requestLock(&srcUnlocker)) {
return false;
}
- const SkPixmap& srcPM = srcUnlocker.pixmap();
+ SkPixmap srcPM = srcUnlocker.pixmap();
+ if (kRGB_565_SkColorType == dstColorType && kOpaque_SkAlphaType != srcPM.alphaType()) {
+ // copyTo() is not strict on alpha type. Here we set the src to opaque to allow
+ // the call to readPixels() to succeed and preserve this lenient behavior.
+ srcPM = SkPixmap(srcPM.info().makeAlphaType(kOpaque_SkAlphaType), srcPM.addr(),
+ srcPM.rowBytes(), srcPM.ctable());
+ }
const SkImageInfo dstInfo = srcPM.info().makeColorType(dstColorType);
SkBitmap tmpDst;
diff --git a/src/core/SkConfig8888.cpp b/src/core/SkConfig8888.cpp
index b882146..98e5b7a 100644
--- a/src/core/SkConfig8888.cpp
+++ b/src/core/SkConfig8888.cpp
@@ -12,6 +12,7 @@
#include "SkConfig8888.h"
#include "SkColorPriv.h"
#include "SkDither.h"
+#include "SkImageInfoPriv.h"
#include "SkMathPriv.h"
#include "SkPM4fPriv.h"
#include "SkRasterPipeline.h"
@@ -173,9 +174,7 @@
}
bool SkSrcPixelInfo::convertPixelsTo(SkDstPixelInfo* dst, int width, int height) const {
- if (width <= 0 || height <= 0) {
- return false;
- }
+ SkASSERT(width > 0 && height > 0);
if (!is_32bit_colortype(fColorType) || !is_32bit_colortype(dst->fColorType)) {
return false;
@@ -237,34 +236,6 @@
}
}
-static void copy_32_to_g8(void* dst, size_t dstRB, const void* src, size_t srcRB,
- const SkImageInfo& srcInfo) {
- uint8_t* dst8 = (uint8_t*)dst;
- const uint32_t* src32 = (const uint32_t*)src;
-
- const int w = srcInfo.width();
- const int h = srcInfo.height();
- const bool isBGRA = (kBGRA_8888_SkColorType == srcInfo.colorType());
-
- for (int y = 0; y < h; ++y) {
- if (isBGRA) {
- // BGRA
- for (int x = 0; x < w; ++x) {
- uint32_t s = src32[x];
- dst8[x] = SkComputeLuminance((s >> 16) & 0xFF, (s >> 8) & 0xFF, s & 0xFF);
- }
- } else {
- // RGBA
- for (int x = 0; x < w; ++x) {
- uint32_t s = src32[x];
- dst8[x] = SkComputeLuminance(s & 0xFF, (s >> 8) & 0xFF, (s >> 16) & 0xFF);
- }
- }
- src32 = (const uint32_t*)((const char*)src32 + srcRB);
- dst8 += dstRB;
- }
-}
-
static bool extract_alpha(void* dst, size_t dstRB, const void* src, size_t srcRB,
const SkImageInfo& srcInfo, SkColorTable* ctable) {
uint8_t* SK_RESTRICT dst8 = (uint8_t*)dst;
@@ -393,38 +364,14 @@
bool SkPixelInfo::CopyPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
const SkImageInfo& srcInfo, const void* srcPixels, size_t srcRB,
SkColorTable* ctable) {
- if (srcInfo.dimensions() != dstInfo.dimensions()) {
- return false;
- }
-
- if (srcInfo.colorType() == kAlpha_8_SkColorType &&
- dstInfo.colorType() != kAlpha_8_SkColorType)
- {
- return false; // can't convert from alpha to non-alpha
- }
-
- if (dstInfo.colorSpace() &&
- SkColorSpace_Base::Type::kXYZ != as_CSB(dstInfo.colorSpace())->type())
- {
- return false; // unsupported dst space
- }
-
- const bool srcIsF16 = kRGBA_F16_SkColorType == srcInfo.colorType();
- const bool dstIsF16 = kRGBA_F16_SkColorType == dstInfo.colorType();
- if (srcIsF16 || dstIsF16) {
- if (!srcInfo.colorSpace() || !dstInfo.colorSpace() ||
- (srcIsF16 && !srcInfo.colorSpace()->gammaIsLinear()) ||
- (dstIsF16 && !dstInfo.colorSpace()->gammaIsLinear()))
- {
- return false;
- }
- }
+ SkASSERT(dstInfo.dimensions() == srcInfo.dimensions());
+ SkASSERT(SkImageInfoValidConversion(dstInfo, srcInfo));
const int width = srcInfo.width();
const int height = srcInfo.height();
// Do the easiest one first : both configs are equal
- if ((srcInfo == dstInfo) && !ctable) {
+ if (srcInfo == dstInfo && kIndex_8_SkColorType != srcInfo.colorType()) {
size_t bytes = width * srcInfo.bytesPerPixel();
for (int y = 0; y < height; ++y) {
memcpy(dstPixels, srcPixels, bytes);
@@ -491,10 +438,6 @@
copy_g8_to_32(dstPixels, dstRB, srcPixels, srcRB, width, height);
return true;
}
- if (kGray_8_SkColorType == dstInfo.colorType() && 4 == srcInfo.bytesPerPixel()) {
- copy_32_to_g8(dstPixels, dstRB, srcPixels, srcRB, srcInfo);
- return true;
- }
if (kAlpha_8_SkColorType == dstInfo.colorType() &&
extract_alpha(dstPixels, dstRB, srcPixels, srcRB, srcInfo, ctable)) {
@@ -517,9 +460,7 @@
const SkPMColor* table = nullptr;
if (kIndex_8_SkColorType == srcInfo.colorType()) {
- if (nullptr == ctable) {
- return false;
- }
+ SkASSERT(ctable);
table = ctable->readColors();
}
diff --git a/src/core/SkImageInfo.cpp b/src/core/SkImageInfo.cpp
index db796b3..0453c7b 100644
--- a/src/core/SkImageInfo.cpp
+++ b/src/core/SkImageInfo.cpp
@@ -98,17 +98,13 @@
#include "SkReadPixelsRec.h"
bool SkReadPixelsRec::trim(int srcWidth, int srcHeight) {
- switch (fInfo.colorType()) {
- case kUnknown_SkColorType:
- case kIndex_8_SkColorType:
- return false;
- default:
- break;
+ if (kIndex_8_SkColorType == fInfo.colorType()) {
+ return false;
}
if (nullptr == fPixels || fRowBytes < fInfo.minRowBytes()) {
return false;
}
- if (0 == fInfo.width() || 0 == fInfo.height()) {
+ if (0 >= fInfo.width() || 0 >= fInfo.height()) {
return false;
}
diff --git a/src/core/SkImageInfoPriv.h b/src/core/SkImageInfoPriv.h
new file mode 100644
index 0000000..029aa93
--- /dev/null
+++ b/src/core/SkImageInfoPriv.h
@@ -0,0 +1,79 @@
+/*
+ * Copyright 2017 Google Inc.
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+/**
+ * Returns true if |info| contains a valid combination of width, height, colorType, alphaType,
+ * colorSpace. Returns false otherwise.
+ */
+static inline bool SkImageInfoIsValid(const SkImageInfo& info) {
+ if (info.width() <= 0 || info.height() <= 0) {
+ return false;
+ }
+
+ if (kUnknown_SkColorType == info.colorType() || kUnknown_SkAlphaType == info.alphaType()) {
+ return false;
+ }
+
+ if (kOpaque_SkAlphaType != info.alphaType() &&
+ (kRGB_565_SkColorType == info.colorType() || kGray_8_SkColorType == info.colorType())) {
+ return false;
+ }
+
+ if (kRGBA_F16_SkColorType == info.colorType() &&
+ (!info.colorSpace() || !info.colorSpace()->gammaIsLinear())) {
+ return false;
+ }
+
+ if (info.colorSpace() &&
+ (!info.colorSpace()->gammaCloseToSRGB() && !info.colorSpace()->gammaIsLinear())) {
+ return false;
+ }
+
+ return true;
+}
+
+/**
+ * Returns true if Skia has defined a pixel conversion from the |src| to the |dst|.
+ * Returns false otherwise. Some discussion of false cases:
+ * We will not convert to kIndex8 unless |src| is kIndex8. This is possible only
+ * in some cases and likley inefficient.
+ * We do not convert to kGray8 when the |src| is not kGray8. We may add this
+ * feature - it just requires some work to convert to luminance while handling color
+ * spaces correctly. Currently no one is asking for this.
+ * We will not convert from kAlpha8 when the |dst| is not kAlpha8. This would require
+ * inventing color information.
+ * We will not convert to kOpaque when the |src| is not kOpaque. This could be
+ * implemented to set all the alpha values to 1, but there is still some ambiguity -
+ * should we use kPremul or kUnpremul color values with the opaque alphas? Or should
+ * we just use whatever the |src| alpha is? In the future, we could choose to clearly
+ * define this, but currently no one is asking for this feature.
+ */
+static inline bool SkImageInfoValidConversion(const SkImageInfo& dst, const SkImageInfo& src) {
+ if (!SkImageInfoIsValid(dst) || !SkImageInfoIsValid(src)) {
+ return false;
+ }
+
+ if (kIndex_8_SkColorType == dst.colorType() && kIndex_8_SkColorType != src.colorType()) {
+ return false;
+ }
+
+ if (kGray_8_SkColorType == dst.colorType() && kGray_8_SkColorType != src.colorType()) {
+ return false;
+ }
+
+ if (kAlpha_8_SkColorType != dst.colorType() && kAlpha_8_SkColorType == src.colorType()) {
+ return false;
+ }
+
+ // FIXME (msarett): This is commented out until a fix to Chrome's gfx_unittest lands.
+ // In those tests, they write kPremul pixels to a kOpaque canvas.
+ //if (kOpaque_SkAlphaType == dst.alphaType() && kOpaque_SkAlphaType != src.alphaType()) {
+ // return false;
+ //}
+
+ return true;
+}
diff --git a/src/core/SkPixmap.cpp b/src/core/SkPixmap.cpp
index 3e89af6..1e24b93 100644
--- a/src/core/SkPixmap.cpp
+++ b/src/core/SkPixmap.cpp
@@ -10,6 +10,7 @@
#include "SkColorPriv.h"
#include "SkConfig8888.h"
#include "SkData.h"
+#include "SkImageInfoPriv.h"
#include "SkHalf.h"
#include "SkMask.h"
#include "SkNx.h"
@@ -84,15 +85,13 @@
bool SkPixmap::readPixels(const SkImageInfo& requestedDstInfo, void* dstPixels, size_t dstRB,
int x, int y) const {
- if (kUnknown_SkColorType == requestedDstInfo.colorType()) {
+ if (!SkImageInfoValidConversion(requestedDstInfo, fInfo)) {
return false;
}
+
if (nullptr == dstPixels || dstRB < requestedDstInfo.minRowBytes()) {
return false;
}
- if (0 == requestedDstInfo.width() || 0 == requestedDstInfo.height()) {
- return false;
- }
SkIRect srcR = SkIRect::MakeXYWH(x, y, requestedDstInfo.width(), requestedDstInfo.height());
if (!srcR.intersect(0, 0, this->width(), this->height())) {
diff --git a/src/gpu/SkGpuDevice.cpp b/src/gpu/SkGpuDevice.cpp
index 258713c..1dec0f6 100644
--- a/src/gpu/SkGpuDevice.cpp
+++ b/src/gpu/SkGpuDevice.cpp
@@ -26,6 +26,7 @@
#include "SkImageCacherator.h"
#include "SkImageFilter.h"
#include "SkImageFilterCache.h"
+#include "SkImageInfoPriv.h"
#include "SkImage_Base.h"
#include "SkLatticeIter.h"
#include "SkMaskFilter.h"
@@ -194,6 +195,10 @@
int x, int y) {
ASSERT_SINGLE_OWNER
+ if (!SkImageInfoValidConversion(dstInfo, this->imageInfo())) {
+ return false;
+ }
+
return fRenderTargetContext->readPixels(dstInfo, dstPixels, dstRowBytes, x, y);
}
@@ -201,6 +206,10 @@
size_t srcRowBytes, int x, int y) {
ASSERT_SINGLE_OWNER
+ if (!SkImageInfoValidConversion(this->imageInfo(), srcInfo)) {
+ return false;
+ }
+
return fRenderTargetContext->writePixels(srcInfo, srcPixels, srcRowBytes, x, y);
}
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index faccf8d..b56b1ba 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -24,6 +24,7 @@
#include "SkGrPriv.h"
#include "SkImage_Gpu.h"
#include "SkImageCacherator.h"
+#include "SkImageInfoPriv.h"
#include "SkMipMap.h"
#include "SkPixelRef.h"
@@ -124,6 +125,10 @@
bool SkImage_Gpu::onReadPixels(const SkImageInfo& info, void* pixels, size_t rowBytes,
int srcX, int srcY, CachingHint) const {
+ if (!SkImageInfoValidConversion(info, this->onImageInfo())) {
+ return false;
+ }
+
GrPixelConfig config = SkImageInfo2GrPixelConfig(info, *fTexture->getContext()->caps());
uint32_t flags = 0;
if (kUnpremul_SkAlphaType == info.alphaType() && kPremul_SkAlphaType == fAlphaType) {
diff --git a/tests/SpecialImageTest.cpp b/tests/SpecialImageTest.cpp
index b0915f0..33f3948 100644
--- a/tests/SpecialImageTest.cpp
+++ b/tests/SpecialImageTest.cpp
@@ -88,7 +88,7 @@
// Test that draw restricts itself to the subset
SkImageFilter::OutputProperties outProps(img->getColorSpace());
sk_sp<SkSpecialSurface> surf(img->makeSurface(outProps, SkISize::Make(kFullSize, kFullSize),
- kOpaque_SkAlphaType));
+ kPremul_SkAlphaType));
SkCanvas* canvas = surf->getCanvas();
@@ -96,7 +96,7 @@
img->draw(canvas, SkIntToScalar(kPad), SkIntToScalar(kPad), nullptr);
SkBitmap bm;
- bm.allocN32Pixels(kFullSize, kFullSize, true);
+ bm.allocN32Pixels(kFullSize, kFullSize, false);
bool result = canvas->readPixels(bm.info(), bm.getPixels(), bm.rowBytes(), 0, 0);
SkASSERT_RELEASE(result);