Remove directGeneratePixels, and do some follow-up refactoring

This caused visually different results depending on what you passed for
cacheHint *and* whether or not the image had already been cached. That
nondeterminism is not worth the slight performance boost.

With that path gone, things are much simpler, and getROPixels and
lockAsBitmap can be folded together.

Bug: skia:
Change-Id: I9535764a56cef57feb241fd8c86c6c96ef89c142
Reviewed-on: https://skia-review.googlesource.com/c/164040
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Mike Klein <mtklein@google.com>

2 files changed, 22 insertions, 91 deletions

diff --git a/src/image/SkImage_Lazy.cpp b/src/image/SkImage_Lazy.cpp
index 6ac7e7d3a6..fa88db1ba9 100644
--- a/src/image/SkImage_Lazy.cpp
+++ b/src/image/SkImage_Lazy.cpp
@@ -135,36 +135,6 @@ SkImage_Lazy::~SkImage_Lazy() {
 
 //////////////////////////////////////////////////////////////////////////////////////////////////
 
-static bool check_output_bitmap(const SkBitmap& bitmap, const SkImageInfo& info) {
-    SkASSERT(bitmap.isImmutable());
-    SkASSERT(bitmap.getPixels());
-    SkASSERT(bitmap.colorType() == info.colorType());
-    SkASSERT(SkColorSpace::Equals(bitmap.colorSpace(), info.colorSpace()));
-    return true;
-}
-
-bool SkImage_Lazy::directGeneratePixels(const SkImageInfo& info, void* pixels, size_t rb,
-                                        int srcX, int srcY) const {
-    ScopedGenerator generator(fSharedGenerator);
-    const SkImageInfo& genInfo = generator->getInfo();
-    // Currently generators do not natively handle subsets, so check that first.
-    if (srcX || srcY || genInfo.width() != info.width() || genInfo.height() != info.height()) {
-        return false;
-    }
-
-    return generator->getPixels(info, pixels, rb);
-}
-
-//////////////////////////////////////////////////////////////////////////////////////////////////
-
-bool SkImage_Lazy::lockAsBitmapOnlyIfAlreadyCached(SkBitmap* bitmap,
-                                                   const SkImageInfo& dstInfo) const {
-    auto desc = SkBitmapCacheDesc::Make(fUniqueID, dstInfo.colorType(), dstInfo.colorSpace(),
-                                        SkIRect::MakeSize(fInfo.dimensions()));
-    return SkBitmapCache::Find(desc, bitmap) &&
-           check_output_bitmap(*bitmap, dstInfo);
-}
-
 static bool generate_pixels(SkImageGenerator* gen, const SkPixmap& pmap, int originX, int originY) {
     const int genW = gen->getInfo().width();
     const int genH = gen->getInfo().height();
@@ -201,45 +171,39 @@ static bool generate_pixels(SkImageGenerator* gen, const SkPixmap& pmap, int ori
     return true;
 }
 
-bool SkImage_Lazy::lockAsBitmap(SkBitmap* bitmap, SkImage::CachingHint chint,
-                                const SkImageInfo& info) const {
-    if (this->lockAsBitmapOnlyIfAlreadyCached(bitmap, info)) {
+bool SkImage_Lazy::getROPixels(SkBitmap* bitmap, SkImage::CachingHint chint) const {
+    auto check_output_bitmap = [bitmap]() {
+        SkASSERT(bitmap->isImmutable());
+        SkASSERT(bitmap->getPixels());
+        (void)bitmap;
+    };
+
+    auto desc = SkBitmapCacheDesc::Make(this);
+    if (SkBitmapCache::Find(desc, bitmap)) {
+        check_output_bitmap();
         return true;
     }
 
-    SkBitmap tmpBitmap;
-    SkBitmapCache::RecPtr cacheRec;
-    SkPixmap pmap;
     if (SkImage::kAllow_CachingHint == chint) {
-        auto desc = SkBitmapCacheDesc::Make(fUniqueID, info.colorType(), info.colorSpace(),
-                                            SkIRect::MakeSize(info.dimensions()));
-        cacheRec = SkBitmapCache::Alloc(desc, info, &pmap);
-        if (!cacheRec) {
-            return false;
-        }
-    } else {
-        if (!tmpBitmap.tryAllocPixels(info)) {
+        SkPixmap pmap;
+        SkBitmapCache::RecPtr cacheRec = SkBitmapCache::Alloc(desc, fInfo, &pmap);
+        if (!cacheRec ||
+            !generate_pixels(ScopedGenerator(fSharedGenerator), pmap,
+                             fOrigin.x(), fOrigin.y())) {
             return false;
         }
-        if (!tmpBitmap.peekPixels(&pmap)) {
-            return false;
-        }
-    }
-
-    ScopedGenerator generator(fSharedGenerator);
-    if (!generate_pixels(generator, pmap, fOrigin.x(), fOrigin.y())) {
-        return false;
-    }
-
-    if (cacheRec) {
         SkBitmapCache::Add(std::move(cacheRec), bitmap);
         this->notifyAddedToRasterCache();
     } else {
-        *bitmap = tmpBitmap;
+        if (!bitmap->tryAllocPixels(fInfo) ||
+            !generate_pixels(ScopedGenerator(fSharedGenerator), bitmap->pixmap(),
+                             fOrigin.x(), fOrigin.y())) {
+            return false;
+        }
         bitmap->setImmutable();
     }
 
-    check_output_bitmap(*bitmap, info);
+    check_output_bitmap();
     return true;
 }
 
@@ -248,22 +212,6 @@ bool SkImage_Lazy::lockAsBitmap(SkBitmap* bitmap, SkImage::CachingHint chint,
 bool SkImage_Lazy::onReadPixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
                                 int srcX, int srcY, CachingHint chint) const {
     SkBitmap bm;
-#ifdef SK_USE_LEGACY_LAZY_IMAGE_DECODE
-    if (kDisallow_CachingHint == chint) {
-        if (this->lockAsBitmapOnlyIfAlreadyCached(&bm, dstInfo)) {
-            return bm.readPixels(dstInfo, dstPixels, dstRB, srcX, srcY);
-        } else {
-            // Try passing the caller's buffer directly down to the generator. If this fails we
-            // may still succeed in the general case, as the generator may prefer some other
-            // config, which we could then convert via SkBitmap::readPixels.
-            if (this->directGeneratePixels(dstInfo, dstPixels, dstRB, srcX, srcY)) {
-                return true;
-            }
-            // else fall through
-        }
-    }
-#endif
-
     if (this->getROPixels(&bm, chint)) {
         return bm.readPixels(dstInfo, dstPixels, dstRB, srcX, srcY);
     }
@@ -275,10 +223,6 @@ sk_sp<SkData> SkImage_Lazy::onRefEncoded() const {
     return generator->refEncodedData();
 }
 
-bool SkImage_Lazy::getROPixels(SkBitmap* bitmap, CachingHint chint) const {
-    return this->lockAsBitmap(bitmap, chint, fInfo);
-}
-
 bool SkImage_Lazy::onIsValid(GrContext* context) const {
     ScopedGenerator generator(fSharedGenerator);
     return generator->isValid(context);
@@ -498,7 +442,7 @@ sk_sp<GrTextureProxy> SkImage_Lazy::lockTextureProxy(
 
     // 4. Ask the generator to return RGB(A) data, which the GPU can convert
     SkBitmap bitmap;
-    if (!proxy && this->lockAsBitmap(&bitmap, chint, fInfo)) {
+    if (!proxy && this->getROPixels(&bitmap, chint)) {
         if (willBeMipped) {
             proxy = proxyProvider->createMipMapProxyFromBitmap(bitmap);
         }
diff --git a/src/image/SkImage_Lazy.h b/src/image/SkImage_Lazy.h
index b9bfe91245..780098aacb 100644
--- a/src/image/SkImage_Lazy.h
+++ b/src/image/SkImage_Lazy.h
@@ -59,12 +59,6 @@ public:
 
     bool onIsValid(GrContext*) const override;
 
-    // Only return true if the generate has already been cached, in a format that matches the info.
-    bool lockAsBitmapOnlyIfAlreadyCached(SkBitmap*, const SkImageInfo&) const;
-    // Call the underlying generator directly
-    bool directGeneratePixels(const SkImageInfo& dstInfo, void* dstPixels, size_t dstRB,
-                              int srcX, int srcY) const;
-
 #if SK_SUPPORT_GPU
     // Returns the texture proxy. If we're going to generate and cache the texture, we should use
     // the passed in key (if the key is valid). If genType is AllowedTexGenType::kCheap and the
@@ -82,13 +76,6 @@ public:
 private:
     class ScopedGenerator;
 
-    /**
-     *  On success (true), bitmap will point to the pixels for this generator. If this returns
-     *  false, the bitmap will be reset to empty.
-     *  TODO: Pass in dstColorSpace to ensure bitmap is compatible?
-     */
-    bool lockAsBitmap(SkBitmap*, SkImage::CachingHint, const SkImageInfo&) const;
-
     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.