Fix BMP RLE bug: attempt 2

Fix BMP RLE bug: attempt 2

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/a37662937cb0a44b5d75a9240cfa9587b4d60e9c

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

msarett@google.com changed reviewers:
+ scroggo@google.com

[msarett, 2015-11-19 15:16:57]

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:352: return height;
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:52]

lgtm

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:352: return height;
On 2015/11/19 15:16:56, msarett wrote:
> 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.

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:30]

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp
File src/codec/SkBmpRLECodec.cpp (right):

https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp...
src/codec/SkBmpRLECodec.cpp:352: return height;
On 2015/11/19 15:55:51, scroggo wrote:
> On 2015/11/19 15:16:56, msarett wrote:
> > 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.
> 
> 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.

Will file a bug.

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

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.

[scroggo, 2015-11-19 16:45:33]

On 2015/11/19 16:23:30, msarett wrote:
> https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp
> File src/codec/SkBmpRLECodec.cpp (right):
> 
>
https://codereview.chromium.org/1457213003/diff/1/src/codec/SkBmpRLECodec.cpp...
> src/codec/SkBmpRLECodec.cpp:352: return height;
> On 2015/11/19 15:55:51, scroggo wrote:
> > On 2015/11/19 15:16:56, msarett wrote:
> > > 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.
> > 
> > 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.
> 
> Will file a bug.
> 
> > 
> > I'm confused why an incomplete image would hit this case though; won't an
> > incomplete image return without finding an EOF?
> 
> 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.

Ah, now I understand. LGTM

[msarett, 2015-11-19 16:46:05]

The CQ bit was checked by msarett@google.com

[commit-bot: I haz the power, 2015-11-19 16:46:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1457213003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1457213003/1

[commit-bot: I haz the power, 2015-11-19 16:59:15]

Committed patchset #1 (id:1) as
https://skia.googlesource.com/skia/+/a37662937cb0a44b5d75a9240cfa9587b4d60e9c