Fix BMP RLE bug: attempt 2

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"
@@ skipping 331 matching lines @@
         const uint8_t task = fStreamBuffer.get()[fCurrRLEByte++];
 
         // Perform decoding
         if (RLE_ESCAPE == flag) {
             switch (task) {
                 case RLE_EOL:
                     x = 0;
                     y++;
                     break;
                 case RLE_EOF:
-                    return y;
+                    return height;

[msarett, 2015/11/19 15:16:56]

We will often hit the EOF marker when x=width, y=height-1.  This should indicate
that we have finished a successful decode.

If we return "y" here, it will indicate that the decode was incomplete (since y
< height), which is not correct.

One fix to this might be:
if (x >= width && y < height)
  return y + 1;
else
  return y

I'm not sure whether this is more correct than just returning "height".  RLE is
allowed to skip/ignore pixels.  So no matter when we hit an EOF, I think we can
conclude "decode completed successfully" and not worry about any pixels that we
haven't reached.  This is what I've decided to do.

BTW, all of this is coming up because of another bmp related "bug". 
SkBmpRLECodec::onGetPixels() (correctly) never fills on incomplete images
because we start the RLE decode by filling the memory.

Unfortunately, SkBmpRLECodec::getScanlines() does (incorrectly) fill on
incomplete images because the code is shared in SkCodec::getScanlines().  So, on
incomplete scanline decodes, some memory will get filled twice.  I'm not sure
how bad this is.  It has allowed us to catch this bug because we are filling
over pixels that we successfully decoded.

[scroggo, 2015/11/19 15:55:51]

Maybe file a bug and/or add a FIXME? It seems unfortunate (we could be faster),
but not terrible. I think BMP is relatively rare (and RLE even more so - the old
implementation did not even support it, IIRC), and incomplete ones should be
even more rare.

I'm confused why an incomplete image would hit this case though; won't an
incomplete image return without finding an EOF?

[msarett, 2015/11/19 16:23:29]

Will file a bug.
That's exactly the goal of this CL.  If we hit an EOF marker, we should never
consider the image incomplete.  That's why we always want to return height.  If
we return some y that is less than height, the image will be interpreted as
incomplete.

The buggy case that still exists is double filling actual incomplete RLE bmps.

                 case RLE_DELTA: {
                     // Two bytes are needed to specify delta
                     if ((int) fRLEBytes - fCurrRLEByte < 2) {
                         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++];
@@ skipping 142 matching lines @@
         fSampler.reset(new SkBmpRLESampler(this));
     }
 
     return fSampler;
 }
 
 int SkBmpRLECodec::setSampleX(int sampleX){
     fSampleX = sampleX;
     return get_scaled_dimension(this->getInfo().width(), sampleX);
 }