Fix scanline decoding of rare RLE bmps

src/codec/SkBmpRLECodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkBmpRLECodec.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkStream.h"
 
 /*
  * Creates an instance of the decoder
  * Called only by NewFromStream
  */
 SkBmpRLECodec::SkBmpRLECodec(const SkImageInfo& info, SkStream* stream,
                              uint16_t bitsPerPixel, uint32_t numColors,
                              uint32_t bytesPerColor, uint32_t offset,
                              SkCodec::SkScanlineOrder rowOrder,
                              size_t RLEBytes)
     : INHERITED(info, stream, bitsPerPixel, rowOrder)
     , fColorTable(nullptr)
     , fNumColors(numColors)
     , fBytesPerColor(bytesPerColor)
     , fOffset(offset)
     , fStreamBuffer(new uint8_t[RLEBytes])
     , fRLEBytes(RLEBytes)
+    , fOrigRLEBytes(RLEBytes)
     , fCurrRLEByte(0)
     , fSampleX(1)
 {}
 
 /*
  * Initiates the bitmap decode
  */
 SkCodec::Result SkBmpRLECodec::onGetPixels(const SkImageInfo& dstInfo,
                                            void* dst, size_t dstRowBytes,
                                            const Options& opts,
@@ skipping 224 matching lines @@
         const SkCodec::Options& options, SkPMColor inputColorPtr[], int* inputColorCount) {
     // FIXME: Support subsets for scanline decodes.
     if (options.fSubset) {
         // Subsets are not supported.
         return kUnimplemented;
     }
 
     // Reset fSampleX. If it needs to be a value other than 1, it will get modified by
     // the sampler.
     fSampleX = 1;
+
+    // Scanline decodes allow the client to ask for a single scanline at a time.

[scroggo, 2016/02/11 15:56:52]

Maybe this comment should go in the header?

[msarett, 2016/02/11 16:03:51]

Done.

+    // This can be tricky when the RLE encoding instructs the decoder to jump down
+    // multiple lines.  This field keeps track of lines that need to be skipped
+    // on subsequent calls to getScanlines().
+    fLinesToSkip = 0;
+
     // Create the color table if necessary and prepare the stream for decode
     // Note that if it is non-NULL, inputColorCount will be modified
     if (!this->createColorTable(inputColorCount)) {
         SkCodecPrintf("Error: could not create color table.\n");
         return SkCodec::kInvalidInput;
     }
 
     // Copy the color table to the client if necessary
     copy_color_table(dstInfo, this->fColorTable, inputColorPtr, inputColorCount);
 
     // Initialize a buffer for encoded RLE data
+    fRLEBytes = fOrigRLEBytes;

[msarett, 2016/02/11 15:23:10]

On bmps where we need to "check for more data", we change the value of
fRLEBytes.

We need to reset this value to the original value after rewinding.

     if (!this->initializeStreamBuffer()) {
         SkCodecPrintf("Error: cannot initialize stream buffer.\n");
         return SkCodec::kInvalidConversion;
     }
 
     return SkCodec::kSuccess;
 }
 
 /*
  * Performs the bitmap decoding for RLE input format
  * RLE decoding is performed all at once, rather than a one row at a time
  */
 int SkBmpRLECodec::decodeRows(const SkImageInfo& info, void* dst, size_t dstRowBytes,
         const Options& opts) {
     // Set RLE flags
     static const uint8_t RLE_ESCAPE = 0;
     static const uint8_t RLE_EOL = 0;
     static const uint8_t RLE_EOF = 1;
     static const uint8_t RLE_DELTA = 2;
 
-    // Set constant values
     const int width = this->getInfo().width();
-    const int height = info.height();
+    int height = info.height();
 
     // Account for sampling.
     SkImageInfo dstInfo = info.makeWH(get_scaled_dimension(width, fSampleX), height);
 
-    // Destination parameters
-    int x = 0;
-    int y = 0;
-
     // Set the background as transparent.  Then, if the RLE code skips pixels,
     // the skipped pixels will be transparent.
     // Because of the need for transparent pixels, kN32 is the only color
     // type that makes sense for the destination format.
     SkASSERT(kN32_SkColorType == dstInfo.colorType());
     SkSampler::Fill(dstInfo, dst, dstRowBytes, SK_ColorTRANSPARENT, opts.fZeroInitialized);
 
+    // Adjust the height and the dst if the previous call to decodeRows() left us
+    // with lines that need to be skipped.
+    if (height > fLinesToSkip) {
+        height -= fLinesToSkip;
+        dst = SkTAddOffset<void>(dst, fLinesToSkip * dstRowBytes);
+        fLinesToSkip = 0;
+    } else {
+        fLinesToSkip -= height;
+        return height;
+    }
+
+    // Destination parameters
+    int x = 0;
+    int y = 0;
+
     while (true) {
         // If we have reached a row that is beyond the requested height, we have
         // succeeded.
         if (y >= height) {
             // It would be better to check for the EOF marker before indicating
             // success, but we may be performing a scanline decode, which
             // would require us to stop before decoding the full height.
             return height;
         }
 
@@ skipping 27 matching lines @@
                         SkCodecPrintf("Warning: might be incomplete RLE input.\n");
                         if (this->checkForMoreData() < 2) {
                             return y;
                         }
                     }
                     // Modify x and y
                     const uint8_t dx = fStreamBuffer.get()[fCurrRLEByte++];
                     const uint8_t dy = fStreamBuffer.get()[fCurrRLEByte++];
                     x += dx;
                     y += dy;
-                    if (x > width || y > height) {
+                    if (x > width) {
                         SkCodecPrintf("Warning: invalid RLE input.\n");

[scroggo, 2016/02/11 15:56:52]

Is this still correct if we consider a subset (in width) decode?

[msarett, 2016/02/11 16:03:51]

If width == originalWidth, this is fine.  But if width == subsetWidth, this is
wrong.

If we were ever to try to support bmp rle subsets, I think it might make sense
to use originalWidth in this function and then handle subsetting in the
setPixel() functions.  Similar to how we handle scaling.

[scroggo, 2016/02/11 16:08:47]

Ah, of course. Doesn't matter yet.

                         return y - dy;
+                    } else if (y > height) {
+                        fLinesToSkip = y - height;
+                        return height;
                     }
                     break;
                 }
                 default: {
                     // If task does not match any of the above signals, it
                     // indicates that we have a sequence of non-RLE pixels.
                     // Furthermore, the value of task is equal to the number
                     // of pixels to interpret.
                     uint8_t numPixels = task;
                     const size_t rowBytes = compute_row_bytes(numPixels,
@@ skipping 126 matching lines @@
         fSampler.reset(new SkBmpRLESampler(this));
     }
 
     return fSampler;
 }
 
 int SkBmpRLECodec::setSampleX(int sampleX){
     fSampleX = sampleX;
     return get_scaled_dimension(this->getInfo().width(), sampleX);
 }