Skip memcpy() swizzles in SkPngCodec

src/codec/SkPngCodec.cpp

Index: src/codec/SkPngCodec.cpp
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index 232373c04407c63c2b391c148a7053db9e9d62e9..e12690d4d79dd7c34d1235dd8e9d48dfe64454a0 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -373,10 +373,16 @@ void SkPngCodec::destroyReadStruct() {
 // Getting the pixels
 ///////////////////////////////////////////////////////////////////////////////
 
-SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
-                                               const Options& options,
-                                               SkPMColor ctable[],
-                                               int* ctableCount) {
+void SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo, const Options& options) {
+    const SkPMColor* colors = get_color_ptr(fColorTable.get());
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, colors, requestedInfo, options));
+    SkASSERT(fSwizzler);
+}
+
+SkCodec::Result SkPngCodec::prepareToDecode(const SkImageInfo& requestedInfo,
+                                            const Options& options,
+                                            SkPMColor ctable[],
+                                            int* ctableCount) {
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(fPng_ptr))) {
@@ -388,10 +394,16 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
     // suggestedColorType was determined in read_header() based on the encodedColorType
     const SkColorType suggestedColorType = this->getInfo().colorType();
 
+    // If the conversion provided by the swizzler would be a no-op, we may be able
+    // to skip the swizzle step.
+    bool skipSwizzle = false;
+
     switch (suggestedColorType) {
         case kIndex_8_SkColorType:
-            //decode palette to Skia format
             fSrcConfig = SkSwizzler::kIndex;
+            skipSwizzle = (kIndex_8_SkColorType == requestedInfo.colorType());
+
+            // Decode palette to Skia format
             if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(),
                     ctableCount)) {
                 return kInvalidInput;
@@ -399,6 +411,7 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
             break;
         case kGray_8_SkColorType:
             fSrcConfig = SkSwizzler::kGray;
+            skipSwizzle = (kGray_8_SkColorType == requestedInfo.colorType());
             break;
         case kN32_SkColorType: {
             const uint8_t encodedColorType = png_get_color_type(fPng_ptr, fInfo_ptr);
@@ -417,6 +430,9 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
                     fSrcConfig = SkSwizzler::kRGB;
                 } else {
                     fSrcConfig = SkSwizzler::kRGBA;
+#ifdef SK_PMCOLOR_IS_RGBA
+                    skipSwizzle = (kUnpremul_SkAlphaType == this->getInfo().alphaType());
+#endif
                 }
             }
             break;
@@ -429,14 +445,27 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
     // Copy the color table to the client if they request kIndex8 mode
     copy_color_table(requestedInfo, fColorTable, ctable, ctableCount);
 
+    // If this is a subset decode, we will need a swizzler.
+    if (options.fSubset) {
+        skipSwizzle = false;
+    }
+
     // Create the swizzler.  SkPngCodec retains ownership of the color table.
-    const SkPMColor* colors = get_color_ptr(fColorTable.get());
-    fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, colors, requestedInfo, options));
-    SkASSERT(fSwizzler);
+    if (!skipSwizzle) {
+        this->initializeSwizzler(requestedInfo, options);
+    }
 
     return kSuccess;
 }
 
+SkSampler* SkPngCodec::getSampler(bool createIfNecessary) {
+    if (!createIfNecessary || fSwizzler) {
+        return fSwizzler;
+    }
+
+    this->initializeSwizzler(this->dstInfo(), this->options());
+    return fSwizzler;
+}
 
 bool SkPngCodec::onRewind() {
     // This sets fPng_ptr and fInfo_ptr to nullptr. If read_header
@@ -471,10 +500,15 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     }
 
     // Note that ctable and ctableCount may be modified if there is a color table
-    const Result result = this->initializeSwizzler(requestedInfo, options, ctable, ctableCount);
+    const Result result = this->prepareToDecode(requestedInfo, options, ctable, ctableCount);
     if (result != kSuccess) {
         return result;
     }
+
+    const int width = requestedInfo.width();
+    const int height = requestedInfo.height();
+    const int bpp = SkSwizzler::BytesPerPixel(fSrcConfig);
+
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     int row = 0;
@@ -484,7 +518,7 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
         // Assume that any error that occurs while reading rows is caused by an incomplete input.
         if (fNumberPasses > 1) {
             // FIXME (msarett): Handle incomplete interlaced pngs.
-            return kInvalidInput;
+            return row == height ? kSuccess : kInvalidInput;
         }
         // FIXME: We do a poor job on incomplete pngs compared to other decoders (ex: Chromium,
         // Ubuntu Image Viewer).  This is because we use the default buffer size in libpng (8192
@@ -498,50 +532,43 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
         // it have on regular decode performance?  Should we investigate using a different API
         // instead of png_read_row(s)?  Chromium uses png_process_data.
         *rowsDecoded = row;
-        return kIncompleteInput;
+        return row == height ? kSuccess : kIncompleteInput;
     }
 
     // FIXME: We could split these out based on subclass.
-    void* dstRow = dst;
     if (fNumberPasses > 1) {
-        const int width = requestedInfo.width();
-        const int height = requestedInfo.height();
-        const int bpp = SkSwizzler::BytesPerPixel(fSrcConfig);
         const size_t srcRowBytes = width * bpp;
-
-        storage.reset(width * height * bpp);
-        uint8_t* const base = storage.get();
-
+        const size_t rowBytes = fSwizzler ? srcRowBytes : dstRowBytes;
+        uint8_t* const base = fSwizzler ? storage.reset(width * height * bpp) : (uint8_t*) dst;
         for (int i = 0; i < fNumberPasses; i++) {
-            uint8_t* srcRow = base;
+            uint8_t* rowPtr = base;
             for (int y = 0; y < height; y++) {
-                uint8_t* bmRow = srcRow;
-                png_read_rows(fPng_ptr, &bmRow, nullptr, 1);
-                srcRow += srcRowBytes;
+                png_read_rows(fPng_ptr, &rowPtr, nullptr, 1);
+                rowPtr += rowBytes;
             }
         }
 
-        // Now swizzle it.
-        uint8_t* srcRow = base;
-        for (int y = 0; y < height; y++) {
-            fSwizzler->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-            srcRow += srcRowBytes;
+        // Swizzle if necessary.
+        if (fSwizzler) {
+            uint8_t* srcRow = base;
+            for (int y = 0; y < height; y++) {
+                fSwizzler->swizzle(dst, srcRow);
+                dst = SkTAddOffset<void>(dst, dstRowBytes);
+                srcRow += srcRowBytes;
+            }
         }
     } else {
-        storage.reset(requestedInfo.width() * SkSwizzler::BytesPerPixel(fSrcConfig));
-        uint8_t* srcRow = storage.get();
+        uint8_t* rowPtr = fSwizzler ? storage.reset(width * bpp) : (uint8_t*) dst;
         for (; row < requestedInfo.height(); row++) {
-            png_read_rows(fPng_ptr, &srcRow, nullptr, 1);
-            // FIXME: Only call IsOpaque once, outside the loop. Same for onGetScanlines.
-            fSwizzler->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-        }
-    }
+            png_read_rows(fPng_ptr, &rowPtr, nullptr, 1);

[scroggo, 2016/02/05 21:36:52]

if (!swizzler), would it make sense to call this with an array of row pointers
and height for the last parameter, instead of looping?

[msarett, 2016/02/05 22:14:16]

This looks like it makes things another half percent faster...

 
-    if (setjmp(png_jmpbuf(fPng_ptr))) {
-        // We've already read all the scanlines. This is a success.
-        return kSuccess;
+            if (fSwizzler) {
+                fSwizzler->swizzle(dst, rowPtr);
+                dst = SkTAddOffset<void>(dst, dstRowBytes);
+            } else {
+                rowPtr += dstRowBytes;
+            }
+        }
     }
 
     // read rest of file, and get additional comment and time chunks in info_ptr
@@ -573,8 +600,7 @@ public:
             return kInvalidConversion;
         }
 
-        const Result result = this->initializeSwizzler(dstInfo, options, ctable,
-                                                       ctableCount);
+        const Result result = this->prepareToDecode(dstInfo, options, ctable, ctableCount);
         if (result != kSuccess) {
             return result;
         }
@@ -593,11 +619,23 @@ public:
             return row;
         }
 
-        void* dstRow = dst;
+        uint8_t* rowPtr;
+        if (this->swizzler()) {
+            rowPtr = fSrcRow;
+        } else {
+            // Write decoded pixels directly to dst.
+            rowPtr = (uint8_t*) dst;
+        }
+
         for (; row < count; row++) {
-            png_read_rows(this->png_ptr(), &fSrcRow, nullptr, 1);
-            this->swizzler()->swizzle(dstRow, fSrcRow);
-            dstRow = SkTAddOffset<void>(dstRow, rowBytes);
+            png_read_rows(this->png_ptr(), &rowPtr, nullptr, 1);

[scroggo, 2016/02/05 21:36:52]

Again, instead of looping when there's no swizzler, would it make sense to call
this once with an array of row pointers?

I guess this means a separate code path and we still need to set up the
pointers.

+
+            if (this->swizzler()) {
+                this->swizzler()->swizzle(dst, rowPtr);
+                dst = SkTAddOffset<void>(dst, rowBytes);
+            } else {
+                rowPtr += rowBytes;
+            }
         }
 
         return row;
@@ -644,8 +682,7 @@ public:
             return kInvalidConversion;
         }
 
-        const Result result = this->initializeSwizzler(dstInfo, options, ctable,
-                                                       ctableCount);
+        const Result result = this->prepareToDecode(dstInfo, options, ctable, ctableCount);
         if (result != kSuccess) {
             return result;
         }
@@ -690,9 +727,10 @@ public:
             // fail on the first pass, we can still report than some scanlines are initialized.
             return 0;
         }
-        SkAutoTMalloc<uint8_t> storage(count * fSrcRowBytes);
-        uint8_t* storagePtr = storage.get();
-        uint8_t* srcRow;
+        SkAutoTMalloc<uint8_t> storage;
+        uint8_t* const base = this->swizzler() ?
+                storage.reset(count * fSrcRowBytes) : (uint8_t*) dst;
+        const size_t rowBytes = this->swizzler() ? fSrcRowBytes : dstRowBytes;
         const int startRow = this->nextScanline();
         for (int i = 0; i < this->numberPasses(); i++) {
             // read rows we planned to skip into garbage row
@@ -700,23 +738,26 @@ public:
                 png_read_rows(this->png_ptr(), &fGarbageRowPtr, nullptr, 1);
             }
             // read rows we care about into buffer
-            srcRow = storagePtr;
+            uint8_t* rowPtr = base;
             for (int y = 0; y < count; y++) {
-                png_read_rows(this->png_ptr(), &srcRow, nullptr, 1);
-                srcRow += fSrcRowBytes;
+                png_read_rows(this->png_ptr(), &rowPtr, nullptr, 1);
+                rowPtr += rowBytes;
             }
             // read rows we don't want into garbage buffer
             for (int y = 0; y < fHeight - startRow - count; y++) {
                 png_read_rows(this->png_ptr(), &fGarbageRowPtr, nullptr, 1);
             }
         }
-        //swizzle the rows we care about
-        srcRow = storagePtr;
-        void* dstRow = dst;
-        for (int y = 0; y < count; y++) {
-            this->swizzler()->swizzle(dstRow, srcRow);
-            dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
-            srcRow += fSrcRowBytes;
+
+        // Swizzle if necessary
+        if (this->swizzler()) {
+            uint8_t* srcRow = storage.get();
+            void* dstRow = dst;
+            for (int y = 0; y < count; y++) {
+                this->swizzler()->swizzle(dstRow, srcRow);
+                dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
+                srcRow += fSrcRowBytes;
+            }
         }
 
         return count;