Revert "Revert "Add SkImage::makeColorTypeAndColorSpace""

This reverts commit 9a97c96c9c.

This works like makeColorSpace, but allows changing color type as well.
Added a GM to test, although the GM demonstrates several ways this can
fail (especially when using this on lazy images).

For simple use-cases (8888 <-> F16), everything should be fine.

For the reland, add logic to the GM to guard against context abandon
failures, and to ensure that lazy images can be decoded (by calling
makeRasterImage) before trying to draw them. That prevents the DDL
recorder from seeing them.

Bug: skia:
Change-Id: Ibc7b07c3399979b1a44d85a38424e5487e606607
Reviewed-on: https://skia-review.googlesource.com/c/183800
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>

gm/makecolorspace.cpp

@@ -8,6 +8,7 @@
 #include "gm.h"
 #include "Resources.h"
 #include "SkCodec.h"
+#include "SkColorSpace.h"
 #include "SkImage.h"
 #include "SkImagePriv.h"
 
@@ -82,3 +83,50 @@ private:
 };
 
 DEF_GM(return new MakeCSGM;)
+
+DEF_SIMPLE_GM_BG(makecolortypeandspace, canvas, 128 * 3, 128 * 4, SK_ColorWHITE) {
+    sk_sp<SkImage> images[] = {
+        GetResourceAsImage("images/mandrill_128.png"),
+        GetResourceAsImage("images/color_wheel.png"),
+    };
+    auto rec2020 = SkColorSpace::MakeRGB(SkNamedTransferFn::kSRGB, SkNamedGamut::kRec2020);
+
+    // Use the lazy images on the first iteration, and concrete (raster/GPU) images on the second
+    for (bool lazy : {true, false}) {
+        for (int j = 0; j < 2; ++j) {
+            const SkImage* image = images[j].get();
+            if (!image) {
+                // Can happen on bots that abandon the GPU context
+                continue;
+            }
+
+            // Unmodified
+            canvas->drawImage(image, 0, 0);
+
+            // Change the color type/space of the image in a couple ways. In both cases, codec
+            // may fail, because we refude to decode transparent sources to opaque color types.
+            // Guard against that, to avoid cascading failures in DDL.
+
+            // 565 in a wide color space (should be visibly quantized). Fails with the color_wheel,
+            // because of the codec issues mentioned above.
+            auto image565 = image->makeColorTypeAndColorSpace(kRGB_565_SkColorType, rec2020);
+            if (!lazy || image565->makeRasterImage()) {
+                canvas->drawImage(image565, 128, 0);
+            }
+
+            // Grayscale in the original color space. This fails in even more cases, due to the
+            // above opaque issue, and because Ganesh doesn't support drawing to gray, at all.
+            auto imageGray = image->makeColorTypeAndColorSpace(kGray_8_SkColorType,
+                                                               image->refColorSpace());
+            if (!lazy || imageGray->makeRasterImage()) {
+                canvas->drawImage(imageGray, 256, 0);
+            }
+
+            images[j] = canvas->getGrContext()
+                    ? image->makeTextureImage(canvas->getGrContext(), nullptr)
+                    : image->makeRasterImage();
+
+            canvas->translate(0, 128);
+        }
+    }
+}

include/core/SkImage.h

@@ -1020,6 +1020,19 @@ public:
     */
     sk_sp<SkImage> makeColorSpace(sk_sp<SkColorSpace> target) const;
 
+    /** Experimental.
+        Creates SkImage in target SkColorType and SkColorSpace.
+        Returns nullptr if SkImage could not be created.
+
+        Returns original SkImage if it is in target SkColorType and SkColorSpace.
+
+        @param targetColorType  SkColorType of returned SkImage
+        @param targetColorSpace SkColorSpace of returned SkImage
+        @return                 created SkImage in target SkColorType and SkColorSpace
+    */
+    sk_sp<SkImage> makeColorTypeAndColorSpace(SkColorType targetColorType,
+                                              sk_sp<SkColorSpace> targetColorSpace) const;
+
 private:
     SkImage(int width, int height, uint32_t uniqueID);
     friend class SkImage_Base;

src/image/SkImage.cpp

@@ -313,12 +313,30 @@ sk_sp<SkImage> SkImage::makeColorSpace(sk_sp<SkColorSpace> target) const {
     if (!colorSpace) {
         colorSpace = sk_srgb_singleton();
     }
-    if (SkColorSpace::Equals(colorSpace, target.get()) ||
+    if (SkColorSpace::Equals(colorSpace, target.get()) || this->isAlphaOnly()) {
-            kAlpha_8_SkColorType == as_IB(this)->onImageInfo().colorType()) {
         return sk_ref_sp(const_cast<SkImage*>(this));
     }
 
-    return as_IB(this)->onMakeColorSpace(std::move(target));
+    return as_IB(this)->onMakeColorTypeAndColorSpace(this->colorType(), std::move(target));
+}
+
+sk_sp<SkImage> SkImage::makeColorTypeAndColorSpace(SkColorType targetColorType,
+                                                   sk_sp<SkColorSpace> targetColorSpace) const {
+    if (kUnknown_SkColorType == targetColorType || !targetColorSpace) {
+        return nullptr;
+    }
+
+    SkColorType colorType = this->colorType();
+    SkColorSpace* colorSpace = this->colorSpace();
+    if (!colorSpace) {
+        colorSpace = sk_srgb_singleton();
+    }
+    if (colorType == targetColorType &&
+        (SkColorSpace::Equals(colorSpace, targetColorSpace.get()) || this->isAlphaOnly())) {
+        return sk_ref_sp(const_cast<SkImage*>(this));
+    }
+
+    return as_IB(this)->onMakeColorTypeAndColorSpace(targetColorType, std::move(targetColorSpace));
 }
 
 sk_sp<SkImage> SkImage::makeNonTextureImage() const {

src/image/SkImage_Base.h

@@ -96,7 +96,7 @@ public:
     virtual bool onPinAsTexture(GrContext*) const { return false; }
     virtual void onUnpinAsTexture(GrContext*) const {}
 
-    virtual sk_sp<SkImage> onMakeColorSpace(sk_sp<SkColorSpace>) const = 0;
+    virtual sk_sp<SkImage> onMakeColorTypeAndColorSpace(SkColorType, sk_sp<SkColorSpace>) const = 0;
 protected:
     SkImage_Base(int width, int height, uint32_t uniqueID);
 

src/image/SkImage_Gpu.cpp

@@ -60,10 +60,11 @@ SkImageInfo SkImage_Gpu::onImageInfo() const {
     return SkImageInfo::Make(fProxy->width(), fProxy->height(), colorType, fAlphaType, fColorSpace);
 }
 
-sk_sp<SkImage> SkImage_Gpu::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
+sk_sp<SkImage> SkImage_Gpu::onMakeColorTypeAndColorSpace(SkColorType targetCT,
+                                                         sk_sp<SkColorSpace> targetCS) const {
     auto xform = GrColorSpaceXformEffect::Make(fColorSpace.get(), fAlphaType,
-                                               target.get(), fAlphaType);
+                                               targetCS.get(), fAlphaType);
-    SkASSERT(xform);
+    SkASSERT(xform || targetCT != this->colorType());
 
     sk_sp<GrTextureProxy> proxy = this->asTextureProxyRef();
 
@@ -75,7 +76,7 @@ sk_sp<SkImage> SkImage_Gpu::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
     sk_sp<GrRenderTargetContext> renderTargetContext(
         fContext->contextPriv().makeDeferredRenderTargetContextWithFallback(
             format, SkBackingFit::kExact, this->width(), this->height(),
-            proxy->config(), nullptr));
+            SkColorType2GrPixelConfig(targetCT), nullptr));
     if (!renderTargetContext) {
         return nullptr;
     }
@@ -83,7 +84,9 @@ sk_sp<SkImage> SkImage_Gpu::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
     GrPaint paint;
     paint.setPorterDuffXPFactory(SkBlendMode::kSrc);
     paint.addColorTextureProcessor(std::move(proxy), SkMatrix::I());
-    paint.addColorFragmentProcessor(std::move(xform));
+    if (xform) {
+        paint.addColorFragmentProcessor(std::move(xform));
+    }
 
     renderTargetContext->drawRect(GrNoClip(), std::move(paint), GrAA::kNo, SkMatrix::I(),
                                   SkRect::MakeIWH(this->width(), this->height()));
@@ -93,7 +96,7 @@ sk_sp<SkImage> SkImage_Gpu::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
 
     // MDB: this call is okay bc we know 'renderTargetContext' was exact
     return sk_make_sp<SkImage_Gpu>(fContext, kNeedNewImageUniqueID, fAlphaType,
-                                   renderTargetContext->asTextureProxyRef(), std::move(target));
+                                   renderTargetContext->asTextureProxyRef(), std::move(targetCS));
 }
 
 ///////////////////////////////////////////////////////////////////////////////////////////////////

src/image/SkImage_Gpu.h

@@ -37,7 +37,7 @@ public:
 
     virtual bool onIsTextureBacked() const override { return SkToBool(fProxy.get()); }
 
-    sk_sp<SkImage> onMakeColorSpace(sk_sp<SkColorSpace>) const final;
+    sk_sp<SkImage> onMakeColorTypeAndColorSpace(SkColorType, sk_sp<SkColorSpace>) const final;
 
     /**
         Create a new SkImage that is very similar to an SkImage created by MakeFromTexture. The main

src/image/SkImage_GpuYUVA.cpp

@@ -143,15 +143,18 @@ sk_sp<GrTextureProxy> SkImage_GpuYUVA::asMippedTextureProxyRef() const {
 
 //////////////////////////////////////////////////////////////////////////////////////////////////
 
-sk_sp<SkImage> SkImage_GpuYUVA::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
+sk_sp<SkImage> SkImage_GpuYUVA::onMakeColorTypeAndColorSpace(SkColorType,
+                                                             sk_sp<SkColorSpace> targetCS) const {
+    // We explicitly ignore color type changes, for now.
+
     // we may need a mutex here but for now we expect usage to be in a single thread
     if (fOnMakeColorSpaceTarget &&
-        SkColorSpace::Equals(target.get(), fOnMakeColorSpaceTarget.get())) {
+        SkColorSpace::Equals(targetCS.get(), fOnMakeColorSpaceTarget.get())) {
         return fOnMakeColorSpaceResult;
     }
-    sk_sp<SkImage> result = sk_sp<SkImage>(new SkImage_GpuYUVA(this, target));
+    sk_sp<SkImage> result = sk_sp<SkImage>(new SkImage_GpuYUVA(this, targetCS));
     if (result) {
-        fOnMakeColorSpaceTarget = target;
+        fOnMakeColorSpaceTarget = targetCS;
         fOnMakeColorSpaceResult = result;
     }
     return result;

src/image/SkImage_GpuYUVA.h

@@ -36,7 +36,7 @@ public:
 
     virtual bool onIsTextureBacked() const override { return SkToBool(fProxies[0].get()); }
 
-    sk_sp<SkImage> onMakeColorSpace(sk_sp<SkColorSpace>) const final;
+    sk_sp<SkImage> onMakeColorTypeAndColorSpace(SkColorType, sk_sp<SkColorSpace>) const final;
 
     virtual bool isYUVA() const override { return true; }
     virtual bool asYUVATextureProxiesRef(sk_sp<GrTextureProxy> proxies[4],

src/image/SkImage_Lazy.cpp

@@ -53,7 +53,7 @@ private:
 ///////////////////////////////////////////////////////////////////////////////
 
 SkImage_Lazy::Validator::Validator(sk_sp<SharedGenerator> gen, const SkIRect* subset,
-                                   sk_sp<SkColorSpace> colorSpace)
+                                   const SkColorType* colorType, sk_sp<SkColorSpace> colorSpace)
         : fSharedGenerator(std::move(gen)) {
     if (!fSharedGenerator) {
         return;
@@ -84,8 +84,13 @@ SkImage_Lazy::Validator::Validator(sk_sp<SharedGenerator> gen, const SkIRect* su
 
     fInfo   = info.makeWH(subset->width(), subset->height());
     fOrigin = SkIPoint::Make(subset->x(), subset->y());
-    if (colorSpace) {
+    if (colorType || colorSpace) {
-        fInfo = fInfo.makeColorSpace(colorSpace);
+        if (colorType) {
+            fInfo = fInfo.makeColorType(*colorType);
+        }
+        if (colorSpace) {
+            fInfo = fInfo.makeColorSpace(colorSpace);
+        }
         fUniqueID = SkNextID::ImageID();
     }
 }
@@ -249,30 +254,33 @@ sk_sp<SkImage> SkImage_Lazy::onMakeSubset(const SkIRect& subset) const {
     SkASSERT(fInfo.bounds() != subset);
 
     const SkIRect generatorSubset = subset.makeOffset(fOrigin.x(), fOrigin.y());
-    Validator validator(fSharedGenerator, &generatorSubset, fInfo.refColorSpace());
+    const SkColorType colorType = fInfo.colorType();
+    Validator validator(fSharedGenerator, &generatorSubset, &colorType, fInfo.refColorSpace());
     return validator ? sk_sp<SkImage>(new SkImage_Lazy(&validator)) : nullptr;
 }
 
-sk_sp<SkImage> SkImage_Lazy::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
+sk_sp<SkImage> SkImage_Lazy::onMakeColorTypeAndColorSpace(SkColorType targetCT,
-    SkAutoExclusive autoAquire(fOnMakeColorSpaceMutex);
+                                                          sk_sp<SkColorSpace> targetCS) const {
-    if (fOnMakeColorSpaceTarget &&
+    SkAutoExclusive autoAquire(fOnMakeColorTypeAndSpaceMutex);
-        SkColorSpace::Equals(target.get(), fOnMakeColorSpaceTarget.get())) {
+    if (fOnMakeColorTypeAndSpaceResult &&
-        return fOnMakeColorSpaceResult;
+        targetCT == fOnMakeColorTypeAndSpaceResult->colorType() &&
+        SkColorSpace::Equals(targetCS.get(), fOnMakeColorTypeAndSpaceResult->colorSpace())) {
+        return fOnMakeColorTypeAndSpaceResult;
     }
     const SkIRect generatorSubset =
             SkIRect::MakeXYWH(fOrigin.x(), fOrigin.y(), fInfo.width(), fInfo.height());
-    Validator validator(fSharedGenerator, &generatorSubset, target);
+    Validator validator(fSharedGenerator, &generatorSubset, &targetCT, targetCS);
     sk_sp<SkImage> result = validator ? sk_sp<SkImage>(new SkImage_Lazy(&validator)) : nullptr;
     if (result) {
-        fOnMakeColorSpaceTarget = target;
+        fOnMakeColorTypeAndSpaceResult = result;
-        fOnMakeColorSpaceResult = result;
     }
     return result;
 }
 
 sk_sp<SkImage> SkImage::MakeFromGenerator(std::unique_ptr<SkImageGenerator> generator,
                                           const SkIRect* subset) {
-    SkImage_Lazy::Validator validator(SharedGenerator::Make(std::move(generator)), subset, nullptr);
+    SkImage_Lazy::Validator
+            validator(SharedGenerator::Make(std::move(generator)), subset, nullptr, nullptr);
 
     return validator ? sk_make_sp<SkImage_Lazy>(&validator) : nullptr;
 }
@@ -427,9 +435,10 @@ sk_sp<GrTextureProxy> SkImage_Lazy::lockTextureProxy(
         ScopedGenerator generator(fSharedGenerator);
         Generator_GrYUVProvider provider(generator);
 
-        // The pixels in the texture will be in the generator's color space. If onMakeColorSpace
+        // The pixels in the texture will be in the generator's color space.
-        // has been called then this will not match this image's color space. To correct this, apply
+        // If onMakeColorTypeAndColorSpace has been called then this will not match this image's
-        // a color space conversion from the generator's color space to this image's color space.
+        // color space. To correct this, apply a color space conversion from the generator's color
+        // space to this image's color space.
         SkColorSpace* generatorColorSpace = fSharedGenerator->fGenerator->getInfo().colorSpace();
         SkColorSpace* thisColorSpace = fInfo.colorSpace();
 

src/image/SkImage_Lazy.h

@@ -20,7 +20,8 @@ class SharedGenerator;
 class SkImage_Lazy : public SkImage_Base {
 public:
     struct Validator {
-        Validator(sk_sp<SharedGenerator>, const SkIRect* subset, sk_sp<SkColorSpace> colorSpace);
+        Validator(sk_sp<SharedGenerator>, const SkIRect* subset, const SkColorType* colorType,
+                  sk_sp<SkColorSpace> colorSpace);
 
         operator bool() const { return fSharedGenerator.get(); }
 
@@ -55,7 +56,7 @@ public:
     sk_sp<SkImage> onMakeSubset(const SkIRect&) const override;
     bool getROPixels(SkBitmap*, CachingHint) const override;
     bool onIsLazyGenerated() const override { return true; }
-    sk_sp<SkImage> onMakeColorSpace(sk_sp<SkColorSpace>) const override;
+    sk_sp<SkImage> onMakeColorTypeAndColorSpace(SkColorType, sk_sp<SkColorSpace>) const override;
 
     bool onIsValid(GrContext*) const override;
 
@@ -77,17 +78,16 @@ private:
 
     sk_sp<SharedGenerator> fSharedGenerator;
     // Note that fInfo is not necessarily the info from the generator. It may be cropped by
-    // onMakeSubset and its color space may be changed by onMakeColorSpace.
+    // onMakeSubset and its color type/space may be changed by onMakeColorTypeAndColorSpace.
     const SkImageInfo      fInfo;
     const SkIPoint         fOrigin;
 
     uint32_t fUniqueID;
 
-    // Repeated calls to onMakeColorSpace will result in a proliferation of unique IDs and
+    // Repeated calls to onMakeColorTypeAndColorSpace will result in a proliferation of unique IDs
-    // SkImage_Lazy instances. Cache the result of the last successful onMakeColorSpace call.
+    // and SkImage_Lazy instances. Cache the result of the last successful call.
-    mutable SkMutex             fOnMakeColorSpaceMutex;
+    mutable SkMutex             fOnMakeColorTypeAndSpaceMutex;
-    mutable sk_sp<SkColorSpace> fOnMakeColorSpaceTarget;
+    mutable sk_sp<SkImage>      fOnMakeColorTypeAndSpaceResult;
-    mutable sk_sp<SkImage>      fOnMakeColorSpaceResult;
 
 #if SK_SUPPORT_GPU
     // When the SkImage_Lazy goes away, we will iterate over all the unique keys we've used and

src/image/SkImage_Raster.cpp

@@ -101,7 +101,7 @@ public:
         SkASSERT(bitmapMayBeMutable || fBitmap.isImmutable());
     }
 
-    sk_sp<SkImage> onMakeColorSpace(sk_sp<SkColorSpace>) const override;
+    sk_sp<SkImage> onMakeColorTypeAndColorSpace(SkColorType, sk_sp<SkColorSpace>) const override;
 
     bool onIsValid(GrContext* context) const override { return true; }
     void notifyAddedToRasterCache() const override {
@@ -337,12 +337,13 @@ bool SkImage_Raster::onAsLegacyBitmap(SkBitmap* bitmap) const {
 
 ///////////////////////////////////////////////////////////////////////////////
 
-sk_sp<SkImage> SkImage_Raster::onMakeColorSpace(sk_sp<SkColorSpace> target) const {
+sk_sp<SkImage> SkImage_Raster::onMakeColorTypeAndColorSpace(SkColorType targetCT,
+                                                            sk_sp<SkColorSpace> targetCS) const {
     SkPixmap src;
     SkAssertResult(fBitmap.peekPixels(&src));
 
     SkBitmap dst;
-    dst.allocPixels(fBitmap.info().makeColorSpace(target));
+    dst.allocPixels(fBitmap.info().makeColorType(targetCT).makeColorSpace(targetCS));
 
     SkAssertResult(dst.writePixels(src));
     dst.setImmutable();