Make SkAndroidCodec support ico

src/codec/SkBmpStandardCodec.cpp

Index: src/codec/SkBmpStandardCodec.cpp
diff --git a/src/codec/SkBmpStandardCodec.cpp b/src/codec/SkBmpStandardCodec.cpp
index fd4d6d18bc2d3df2ea6366554dc3bf56ec72700e..adf391b03b0ffeb52dc4ad9a6b6b851f9ed549a7 100644
--- a/src/codec/SkBmpStandardCodec.cpp
+++ b/src/codec/SkBmpStandardCodec.cpp
@@ -60,9 +60,6 @@ SkCodec::Result SkBmpStandardCodec::onGetPixels(const SkImageInfo& dstInfo,
         *rowsDecoded = rows;
         return kIncompleteInput;
     }
-    if (fInIco) {
-        return this->decodeIcoMask(dstInfo, dst, dstRowBytes);
-    }
     return kSuccess;
 }
 
@@ -227,9 +224,8 @@ SkCodec::Result SkBmpStandardCodec::prepareToDecode(const SkImageInfo& dstInfo,
 /*
  * Performs the bitmap decoding for standard input format
  */
-int SkBmpStandardCodec::decodeRows(const SkImageInfo& dstInfo,
-                                               void* dst, size_t dstRowBytes,
-                                               const Options& opts) {
+int SkBmpStandardCodec::decodeRows(const SkImageInfo& dstInfo, void* dst, size_t dstRowBytes,
+        const Options& opts) {
     // Iterate over rows of the image
     const int height = dstInfo.height();
     for (int y = 0; y < height; y++) {
@@ -246,29 +242,63 @@ int SkBmpStandardCodec::decodeRows(const SkImageInfo& dstInfo,
         fSwizzler->swizzle(dstRow, fSrcBuffer.get());
     }
 
-    // Finished decoding the entire image
+    if (fInIco) {
+        const int startScanline = this->currScanline();
+        const size_t andMaskRowBytes = SkAlign4(compute_row_bytes(this->getInfo().width(), 1));

[scroggo, 2015/12/02 16:27:03]

IIUC, compute_row_bytes will return width * 8. Isn't that already SkAlign4?

[msarett, 2015/12/03 19:19:16]

This will actually give us width / 8 (because there are 8 pixelsPerByte), which
isn't necessarily aligned.  I accidentally removed this at one point and it
caused some failures.

+        if (startScanline < 0) {
+            // We are not performing a scanline decode.
+            // Just decode the entire ICO mask and return.
+            decodeIcoMask(dstInfo, dst, dstRowBytes, andMaskRowBytes);
+            return height;
+        }
+
+        // In order to perform a scanline ICO decode, we must be able
+        // to skip ahead in the stream in order to apply the AND mask
+        // to the requested scanlines.  We also must be able to return
+        // the stream to its original position in order to decode the
+        // next scanline on a subsequent request.
+        // We will do this by taking advantage of the fact that
+        // SkIcoCodec always uses a SkMemoryStream as its underlying
+        // representation of the stream.
+        SkMemoryStream* stream = (SkMemoryStream*) this->stream();

[scroggo, 2015/12/02 16:27:03]

I think this is the correct assumption to make, but the wrong way to make it. If
the assumption was wrong for some reason (i.e. we change the implementation
elsewhere), I think this will fail mysteriously. (We would be treating some
other kind of stream as an SkMemoryStream... actually, it looks like you still
call only virtual functions on SkStream, so the cast is unnecessary?)

I think a better way to handle this would be to call SkStream::getMemoryBase()
and ASSERT that it returns a reasonable value (and maybe even ASSERT at creation
if fInIco). You can then read that as an SkMemoryStream, so you'll never need to
modify this->stream(). (This would be cleaner if/when we convert getMemoryBase
to be a method returning an SkData, as proposed in the comments. Last I looked
at that I think it was a big messy change to make...)

It might be fine to just use this->stream() directly, but make sure you look at
the return values of calls that might fail if the assumption is wrong. seek
could fail, for example. So we should assert that seek returns true.

[msarett, 2015/12/03 19:19:16]

I think you're right - the clearest thing is probably to get the memory base
(and assert that it succeeds) and then operate on the memory.

In addition, I actually decided to wrap the memory base in another stream, just
because it is convenient to call stream->read() rather than check the length of
the stream every time we access memory.

+        const size_t startPosition = stream->getPosition();
+
+        const int remainingScanlines = this->getInfo().height() - startScanline - height;
+        const size_t bytesToSkip = remainingScanlines * fSrcRowBytes +
+                startScanline * andMaskRowBytes;
+        if (bytesToSkip != stream->skip(bytesToSkip)) {
+            // FIXME: How can we indicate that this decode was actually incomplete?
+            stream->seek(startPosition);
+            return height;
+        }
+        // FIXME: If decodeIcoMask does not succeed, is there a way that we can

[scroggo, 2015/12/02 16:27:03]

You removed the return value from decodeIcoMask, which seems like it would be
helpful. The tricky thing, of course, is that you indicate incomplete here by
using the height. But the height is weird - we have partially decoded the entire
height, but did not decode the mask for those lines.

Furthermore, I think the height is used to tell SkCodec to fill the remaining
lines. Do we want to fill those lines? We cannot really separate the two ideas
(filling versus incomplete) as the code is written.

What does Blink's decoder do? I'm guessing that it just stops when it runs out
of data. Not sure that is the best behavior here, but something to consider.

[msarett, 2015/12/03 19:19:16]

Yeah the return value from decodeIcoMask() could tell us incomplete.  The reason
I removed the return value is that I don't think we want to use it here.

If we return less than the full height from decodeIcoMask() (and then return it
here), we will fill some rows of pixels.

I think the behavior we want is to just leave those pixels unmasked but not
filled (though I guess it's arbitrary).
Yes I think this is the problem.  We want to indicate that all of the lines are
filled, but return kIncomplete.  SkBmpRLECodec has a slightly similar problem
with scanline decodes (skbug.com/4582).
AFAICT blink's decoder will leave the pixels unmasked (same as we do) since it
will just stop when there is no more data.

+        //        indicate the decode was incomplete?
+        decodeIcoMask(dstInfo, dst, dstRowBytes, andMaskRowBytes);
+        stream->seek(startPosition);
+    }
+
     return height;
 }
 
-// TODO (msarett): This function will need to be modified in order to perform row by row decodes
-//                 when the Ico scanline decoder is implemented.
-SkCodec::Result SkBmpStandardCodec::decodeIcoMask(const SkImageInfo& dstInfo,
-        void* dst, size_t dstRowBytes) {
+void SkBmpStandardCodec::decodeIcoMask(const SkImageInfo& dstInfo,
+        void* dst, size_t dstRowBytes, size_t srcRowBytes) {
     // BMP in ICO have transparency, so this cannot be 565, and this mask
     // prevents us from using kIndex8. The below code depends on the output
     // being an SkPMColor.
     SkASSERT(dstInfo.colorType() == kN32_SkColorType);
 
-    // The AND mask is always 1 bit per pixel
-    const int width = this->getInfo().width();
-    const size_t rowBytes = SkAlign4(compute_row_bytes(width, 1));
+    // If we are sampling, make sure that we only mask the sampled pixels.
+    // We do not need to worry about sampling in the y-dimension because that
+    // should be handled by SkSampledCodec.
+    int sampleX = fSwizzler->sampleX();
+    int startX = get_start_coord(sampleX);
 
     SkPMColor* dstPtr = (SkPMColor*) dst;
     for (int y = 0; y < dstInfo.height(); y++) {
         // The srcBuffer will at least be large enough
-        if (stream()->read(fSrcBuffer.get(), rowBytes) != rowBytes) {
+        if (this->stream()->read(fSrcBuffer.get(), srcRowBytes) != srcRowBytes) {

[scroggo, 2015/12/02 16:27:03]

Should this be a private method? (I assume we do not want to calculate the
rowBytes for each call; maybe store it as a const member variable?)

It seems weird that we need to pass the srcRowBytes to this method

[msarett, 2015/12/03 19:19:16]

Agreed.  I've made fAndMaskRowBytes a field.

Given the latest change, I'm not sure it makes sense to make this a private
method.  We could have potentially created a method also used by decodeRows()
(and passed rowBytes as a parameter).  But it's a little messier now that I am
passing in a stream to decodeIcoMask().

             SkCodecPrintf("Warning: incomplete AND mask for bmp-in-ico.\n");
-            return kIncompleteInput;
+            return;
         }
 
         int row = this->getDstRow(y, dstInfo.height());
@@ -276,17 +306,15 @@ SkCodec::Result SkBmpStandardCodec::decodeIcoMask(const SkImageInfo& dstInfo,
         SkPMColor* dstRow =
                 SkTAddOffset<SkPMColor>(dstPtr, row * dstRowBytes);
 
-        for (int x = 0; x < width; x++) {
+        for (int x = startX; x < this->getInfo().width(); x += sampleX) {
             int quotient;
             int modulus;
             SkTDivMod(x, 8, &quotient, &modulus);
             uint32_t shift = 7 - modulus;
-            uint32_t alphaBit =
-                    (fSrcBuffer.get()[quotient] >> shift) & 0x1;
-            dstRow[x] &= alphaBit - 1;
+            uint32_t alphaBit = (fSrcBuffer.get()[quotient] >> shift) & 0x1;
+            dstRow[get_dst_coord(x, sampleX)] &= alphaBit - 1;
         }
     }
-    return kSuccess;
 }
 
 uint32_t SkBmpStandardCodec::onGetFillValue(SkColorType colorType, SkAlphaType alphaType) const {