Fix makeColorSpace on YUV images
This is a second attempt at https://skia-review.googlesource.com/c/skia/+/182816
This version ensures that SkImage::colorSpace() returns the target after
makeColorSpace has been called (to match user expectations, and match
behavior with lazy images). Given that, the xform is baked into the FP
within the maker, rather than externally (again, this matches the lazy
image behavior).
Additionally, the target color space needs to be taken into account when
flattening into the RGB proxy, and some base-class methods need to use
this->colorSpace() rather than fColorSpace to tag the output.
Added a GM that tests quite a few different scenarios. All images have
makeColorSpace() applied:
- Raster image (for reference)
- yuvImage
- yuvImage->makeSubset()
- yuvImage->makeNonTextureImage()
- readPixels(yuvImage)
All images should look the same as the top row. Verified that they do
match, in both untagged (gl) and tagged (glsrgb) configs.
I think there may still be some cases where we transform too many or too
few times, or incorrectly tag the result of an image operation, but this
is much more correct than before, and should (I hope) address Chrome's
immediate needs.
Bug: skia:8740
Change-Id: I5d501879866861a5ba91240f688d3f95711f7595
Reviewed-on: https://skia-review.googlesource.com/c/189494
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
diff --git a/src/gpu/GrImageTextureMaker.cpp b/src/gpu/GrImageTextureMaker.cpp
index fa87037..5e821f5 100644
--- a/src/gpu/GrImageTextureMaker.cpp
+++ b/src/gpu/GrImageTextureMaker.cpp
@@ -5,6 +5,7 @@
* found in the LICENSE file.
*/
+#include "GrColorSpaceXform.h"
#include "GrImageTextureMaker.h"
#include "SkGr.h"
#include "SkImage_GpuYUVA.h"
@@ -81,9 +82,6 @@
SkColorSpace* GrYUVAImageTextureMaker::colorSpace() const {
return fImage->colorSpace();
}
-SkColorSpace* GrYUVAImageTextureMaker::targetColorSpace() const {
- return fImage->targetColorSpace();
-}
std::unique_ptr<GrFragmentProcessor> GrYUVAImageTextureMaker::createFragmentProcessor(
const SkMatrix& textureMatrix,
@@ -111,7 +109,11 @@
filter = GrSamplerState::Filter::kBilerp;
}
- return GrYUVtoRGBEffect::Make(fImage->fProxies, fImage->fYUVAIndices,
- fImage->fYUVColorSpace, filter);
-
+ auto fp = GrYUVtoRGBEffect::Make(fImage->fProxies, fImage->fYUVAIndices,
+ fImage->fYUVColorSpace, filter);
+ if (fImage->fTargetColorSpace) {
+ fp = GrColorSpaceXformEffect::Make(std::move(fp), fImage->fColorSpace.get(),
+ fImage->alphaType(), fImage->fTargetColorSpace.get());
+ }
+ return fp;
}
diff --git a/src/gpu/GrImageTextureMaker.h b/src/gpu/GrImageTextureMaker.h
index 8024579..544705f 100644
--- a/src/gpu/GrImageTextureMaker.h
+++ b/src/gpu/GrImageTextureMaker.h
@@ -65,7 +65,6 @@
SkAlphaType alphaType() const override;
SkColorSpace* colorSpace() const override;
- SkColorSpace* targetColorSpace() const override;
private:
const SkImage_GpuYUVA* fImage;
diff --git a/src/gpu/GrTextureProducer.h b/src/gpu/GrTextureProducer.h
index 68c1b47..1994f8b 100644
--- a/src/gpu/GrTextureProducer.h
+++ b/src/gpu/GrTextureProducer.h
@@ -109,7 +109,6 @@
bool isAlphaOnly() const { return fIsAlphaOnly; }
virtual SkAlphaType alphaType() const = 0;
virtual SkColorSpace* colorSpace() const = 0;
- virtual SkColorSpace* targetColorSpace() const { return nullptr; }
protected:
friend class GrTextureProducer_TestAccess;
diff --git a/src/gpu/SkGpuDevice_drawTexture.cpp b/src/gpu/SkGpuDevice_drawTexture.cpp
index 63ab4f9..39385e6 100644
--- a/src/gpu/SkGpuDevice_drawTexture.cpp
+++ b/src/gpu/SkGpuDevice_drawTexture.cpp
@@ -289,11 +289,8 @@
}
auto fp = producer->createFragmentProcessor(*textureMatrix, clippedSrcRect, constraintMode,
coordsAllInsideSrcRect, filterMode);
- SkColorSpace* rtColorSpace = fRenderTargetContext->colorSpaceInfo().colorSpace();
- SkColorSpace* targetColorSpace = producer->targetColorSpace();
- SkColorSpace* dstColorSpace = SkToBool(rtColorSpace) ? rtColorSpace : targetColorSpace;
fp = GrColorSpaceXformEffect::Make(std::move(fp), producer->colorSpace(), producer->alphaType(),
- dstColorSpace);
+ fRenderTargetContext->colorSpaceInfo().colorSpace());
if (!fp) {
return;
}
diff --git a/src/image/SkImage_Gpu.cpp b/src/image/SkImage_Gpu.cpp
index 9b91bc8..1e0829d 100644
--- a/src/image/SkImage_Gpu.cpp
+++ b/src/image/SkImage_Gpu.cpp
@@ -201,7 +201,7 @@
}
const SkRect rect = SkRect::MakeIWH(size.width(), size.height());
- if (!RenderYUVAToRGBA(ctx, renderTargetContext, rect, yuvColorSpace,
+ if (!RenderYUVAToRGBA(ctx, renderTargetContext, rect, yuvColorSpace, nullptr,
tempTextureProxies, yuvaIndices)) {
return nullptr;
}
diff --git a/src/image/SkImage_GpuBase.cpp b/src/image/SkImage_GpuBase.cpp
index 9c86d29..35d5a2e 100644
--- a/src/image/SkImage_GpuBase.cpp
+++ b/src/image/SkImage_GpuBase.cpp
@@ -135,7 +135,7 @@
// MDB: this call is okay bc we know 'sContext' was kExact
return sk_make_sp<SkImage_Gpu>(fContext, kNeedNewImageUniqueID, fAlphaType,
- sContext->asTextureProxyRef(), fColorSpace);
+ sContext->asTextureProxyRef(), this->refColorSpace());
}
static void apply_premul(const SkImageInfo& info, void* pixels, size_t rowBytes) {
@@ -185,7 +185,7 @@
}
sk_sp<GrSurfaceContext> sContext = fContext->priv().makeWrappedSurfaceContext(
- this->asTextureProxyRef(), fColorSpace);
+ this->asTextureProxyRef(), this->refColorSpace());
if (!sContext) {
return false;
}
@@ -343,6 +343,7 @@
bool SkImage_GpuBase::RenderYUVAToRGBA(GrContext* ctx, GrRenderTargetContext* renderTargetContext,
const SkRect& rect, SkYUVColorSpace yuvColorSpace,
+ sk_sp<GrColorSpaceXform> colorSpaceXform,
const sk_sp<GrTextureProxy> proxies[4],
const SkYUVAIndex yuvaIndices[4]) {
SkASSERT(renderTargetContext);
@@ -353,9 +354,12 @@
GrPaint paint;
paint.setPorterDuffXPFactory(SkBlendMode::kSrc);
- paint.addColorFragmentProcessor(GrYUVtoRGBEffect::Make(proxies, yuvaIndices,
- yuvColorSpace,
- GrSamplerState::Filter::kNearest));
+ auto fp = GrYUVtoRGBEffect::Make(proxies, yuvaIndices, yuvColorSpace,
+ GrSamplerState::Filter::kNearest);
+ if (colorSpaceXform) {
+ fp = GrColorSpaceXformEffect::Make(std::move(fp), std::move(colorSpaceXform));
+ }
+ paint.addColorFragmentProcessor(std::move(fp));
renderTargetContext->drawRect(GrNoClip(), std::move(paint), GrAA::kNo, SkMatrix::I(), rect);
diff --git a/src/image/SkImage_GpuBase.h b/src/image/SkImage_GpuBase.h
index e9501ac..871fa59 100644
--- a/src/image/SkImage_GpuBase.h
+++ b/src/image/SkImage_GpuBase.h
@@ -15,6 +15,7 @@
#include "SkImage_Base.h"
#include "SkYUVAIndex.h"
+class GrColorSpaceXform;
class SkColorSpace;
class SkImage_GpuBase : public SkImage_Base {
@@ -89,6 +90,7 @@
static bool RenderYUVAToRGBA(GrContext* ctx, GrRenderTargetContext* renderTargetContext,
const SkRect& rect, SkYUVColorSpace yuvColorSpace,
+ sk_sp<GrColorSpaceXform> colorSpaceXform,
const sk_sp<GrTextureProxy> proxies[4],
const SkYUVAIndex yuvaIndices[4]);
diff --git a/src/image/SkImage_GpuYUVA.cpp b/src/image/SkImage_GpuYUVA.cpp
index 3c4fda4..c4898d1 100644
--- a/src/image/SkImage_GpuYUVA.cpp
+++ b/src/image/SkImage_GpuYUVA.cpp
@@ -75,7 +75,7 @@
SkImageInfo SkImage_GpuYUVA::onImageInfo() const {
// Note: this is the imageInfo for the flattened image, not the YUV planes
return SkImageInfo::Make(this->width(), this->height(), kRGBA_8888_SkColorType,
- fAlphaType, fColorSpace);
+ fAlphaType, fTargetColorSpace ? fTargetColorSpace : fColorSpace);
}
bool SkImage_GpuYUVA::setupMipmapsForPlanes() const {
@@ -112,9 +112,11 @@
return nullptr;
}
+ auto colorSpaceXform = GrColorSpaceXform::Make(fColorSpace.get(), fAlphaType,
+ fTargetColorSpace.get(), fAlphaType);
const SkRect rect = SkRect::MakeIWH(this->width(), this->height());
if (!RenderYUVAToRGBA(fContext.get(), renderTargetContext.get(), rect, fYUVColorSpace,
- fProxies, fYUVAIndices)) {
+ std::move(colorSpaceXform), fProxies, fYUVAIndices)) {
return nullptr;
}
diff --git a/src/image/SkImage_GpuYUVA.h b/src/image/SkImage_GpuYUVA.h
index 3916911..a428761 100644
--- a/src/image/SkImage_GpuYUVA.h
+++ b/src/image/SkImage_GpuYUVA.h
@@ -55,8 +55,6 @@
// Returns a ref-ed texture proxy with miplevels
sk_sp<GrTextureProxy> asMippedTextureProxyRef() const;
- SkColorSpace* targetColorSpace() const { return fTargetColorSpace.get(); }
-
/**
* This is the implementation of SkDeferredDisplayListRecorder::makeYUVAPromiseTexture.
*/