Move all knowledge of X sampling into SkScaledCodec

Move all knowledge of X sampling into SkScaledCodec

Prior to this CL, each SkCodec subclass that allows sampling does an
extra check in onStartScanlineDecode to determine whether the X dimension
is supported for sampling. Remove this check, and provide a way for
SkScaledCodec to directly access the SkSwizzler, and update it to do
sampling. This way, the SkCodec knows nothing of sampling, but we can
still save the extra step of sampling afterwards.

FIXME: SkBmpRLECodec still calls SkScaledCodec::DimensionsSupported. It
seems like it could directly support sampling, rather than dealing with
SkScaledCodec (partially).

Add a new base class for SkSwizzler. It allows updating the swizzler
after it was created, which is done by SkScaledCodec. Modify SkSwizzler's
constructor/factory function to stop taking any info about sampling,
assume the sample size is one, and move modifying that into a virtual
function overridden from the base class.

BUG=skia:4284
Committed: https://skia.googlesource.com/skia/+/e7fc14b55bb8c41ba054abf0bfa09cdd6ec84671

[scroggo, 2015-10-01 15:33:13]

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

[scroggo, 2015-10-01 15:33:14]

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:244: // FIXME: This check breaks down when we sample
without letting SkCodec
The problem shows up when we want to scale down an image using SkScaledCodec.
(Particularly when using an interlaced PNG, since it uses a countLines > 1.)
fDstInfo has the width of the entire image, while the SkScaledCodec is using the
scaled width.

I have a couple of ideas here:
- We could skip the check entirely, which allows smart clients to work
(slightly) faster, and forces dumb clients to figure out why they're failing
(keeping in mind that we may end up being dumb clients...)
- We could provide some kind of function that tells the minimum row bytes, which
computes it based on a sampler.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:288: // Set up a fake decompress struct in order to
use libjpeg to calculate output dimensions
The old code crashed for me when I attempted to call both getScaledDimensions
*and* onDimensionsSupported. This surprised me, and maybe I had some other
error, but I think we talked about doing this anyway for some other reason.

[msarett, 2015-10-01 19:34:34]

I think this is another really good change.  It'll be interesting to integrate
this with scaled subsetting.

I have a lot of comments and a few suggestions.

https://codereview.chromium.org/1372973002/diff/80001/include/codec/SkScaledC...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1372973002/diff/80001/include/codec/SkScaledC...
include/codec/SkScaledCodec.h:25: static void ComputeSampleSize(const SkISize&
dstDim, const SkISize& srcDim,
+1 for this change.  I have a CL somewhere that makes this same change as
well...

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

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.cpp:47: // scaling could be supported?
Yes sampling in the x dimension is already implemented.  We could consider
implementing sampling in the y dimension so that this class supports scaling
natively.

I'm still not quite sure that this is best though.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:155: } else if (options->fSubset) {
I'm guessing that, for some reason (maybe the kSubset_Mode in DM), it is
necessary to return kUnimplemented for a subset decode before returning
kInvalidScale for a badly scaled decode.  So you have added this to make sure
that the checks are still in the right order?

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:164: // FIXME: Support subsets somehow? Note that this
works for SkWebpCodec
There's a couple ways we could go about this, I don't think either of them is
too tricky.

We may eventually want to combine dimensionsSupported() and options->fSubset
into one check.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:244: // FIXME: This check breaks down when we sample
without letting SkCodec
On 2015/10/01 15:33:14, scroggo wrote:
> The problem shows up when we want to scale down an image using SkScaledCodec.
> (Particularly when using an interlaced PNG, since it uses a countLines > 1.)
> fDstInfo has the width of the entire image, while the SkScaledCodec is using
the
> scaled width.
> 
> I have a couple of ideas here:
> - We could skip the check entirely, which allows smart clients to work
> (slightly) faster, and forces dumb clients to figure out why they're failing
> (keeping in mind that we may end up being dumb clients...)
> - We could provide some kind of function that tells the minimum row bytes,
which
> computes it based on a sampler.

I've deleted this check in my scaled subsetting CL.  Turns out it is also
annoying when we only want to decode part of a scanline.

So yeah, I'm in favor of getting rid of it.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libgi...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:589: const SkImageInfo subsetDstInfo =
dstInfo.makeWH(fFrameDims.width(), fFrameDims.height());
Glad we don't have to do that any more.  Do you have/want a fFrameIsSubset gif
to test on?

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
File src/codec/SkCodec_libico.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
src/codec/SkCodec_libico.cpp:229: bool SkIcoCodec::onDimensionsSupported(const
SkISize& dim) {
This is a bit redundant which I think is unfortunate.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
src/codec/SkCodec_libico.cpp:295: SkCodecPrintf("Error: No matching candidate
image in ico.\n");
Should we assert that this is never reached?

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:160: dinfo->num_components = 0;
This line belongs in onGetScaledDimensions(), I think.  It is a hack that makes
sure jpeg_calc_output_dimensions() does not give an error on our fake dinfo
struct (we tell it there are zero components, so there are no components to
iterate through and verify things about).

I don't think we want to be modifying this value if someone calls this function
with a real dinfo struct.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:288: // Set up a fake decompress struct in order to
use libjpeg to calculate output dimensions
On 2015/10/01 15:33:14, scroggo wrote:
> The old code crashed for me when I attempted to call both getScaledDimensions
> *and* onDimensionsSupported. This surprised me, and maybe I had some other
> error, but I think we talked about doing this anyway for some other reason.

That's surprising.  Hopefully it's because of the num_components thing I
mentioned above?  Let me know...if not I'd like to track down what was happening
at some point.

It seems a bit weird to me to not directly modify fDecoderMgr->dinfo(), I'd lean
towards doing it that way.

But I'd guess this implementation has identical performance and behavior, so I'm
happy either way.  Especially if the old approach is still crashing.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:443: fStorage.free();
I'm a little confused here...

Why can we do this/Why do we want to do this?

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:205: return fCodec->getPixels(requestedInfo, dst,
rowBytes, &options, ctable, ctableCount);
Unfortunately, I think I might change this back to scanline decoding when we add
subsets here.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:213: return kInvalidScale;
I think we should probably assert false here, since we've already checked that
native scaling or sampling is supported.

Alternatively, we could just return true in onDimensionsSupported(), since we
are going to redo the checks in here anyway.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:234: return kInvalidScale;
I think it should be unimplemented.

[scroggo, 2015-10-01 21:16:58]

https://codereview.chromium.org/1372973002/diff/80001/include/codec/SkScaledC...
File include/codec/SkScaledCodec.h (right):

https://codereview.chromium.org/1372973002/diff/80001/include/codec/SkScaledC...
include/codec/SkScaledCodec.h:25: static void ComputeSampleSize(const SkISize&
dstDim, const SkISize& srcDim,
On 2015/10/01 19:34:33, msarett wrote:
> +1 for this change.  I have a CL somewhere that makes this same change as
> well...

Acknowledged.

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

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.cpp:47: // scaling could be supported?
On 2015/10/01 19:34:33, msarett wrote:
> Yes sampling in the x dimension is already implemented.  We could consider
> implementing sampling in the y dimension so that this class supports scaling
> natively.
> 
> I'm still not quite sure that this is best though.

What do you think is the right way to handle it? In my patch, RLE will no longer
work with SkScaledCodec, which has to be able get a sampler to update. If this
class handles sampling itself, we don't need to worry about that.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:155: } else if (options->fSubset) {
On 2015/10/01 19:34:33, msarett wrote:
> I'm guessing that, for some reason (maybe the kSubset_Mode in DM), it is
> necessary to return kUnimplemented for a subset decode before returning
> kInvalidScale for a badly scaled decode.  So you have added this to make sure
> that the checks are still in the right order?

That is correct. I don't think this is best long term solution necessarily, but
I think this is good enough until we have subsets in SkScaledCodec.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:164: // FIXME: Support subsets somehow? Note that this
works for SkWebpCodec
On 2015/10/01 19:34:33, msarett wrote:
> There's a couple ways we could go about this, I don't think either of them is
> too tricky.
> 
> We may eventually want to combine dimensionsSupported() and options->fSubset
> into one check.

Yeah, I considered that. I decided to punt that until SkScaledCodec supports
subsets.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:244: // FIXME: This check breaks down when we sample
without letting SkCodec
On 2015/10/01 19:34:33, msarett wrote:
> On 2015/10/01 15:33:14, scroggo wrote:
> > The problem shows up when we want to scale down an image using
SkScaledCodec.
> > (Particularly when using an interlaced PNG, since it uses a countLines > 1.)
> > fDstInfo has the width of the entire image, while the SkScaledCodec is using
> the
> > scaled width.
> > 
> > I have a couple of ideas here:
> > - We could skip the check entirely, which allows smart clients to work
> > (slightly) faster, and forces dumb clients to figure out why they're failing
> > (keeping in mind that we may end up being dumb clients...)
> > - We could provide some kind of function that tells the minimum row bytes,
> which
> > computes it based on a sampler.
> 
> I've deleted this check in my scaled subsetting CL.  Turns out it is also
> annoying when we only want to decode part of a scanline.
> 
> So yeah, I'm in favor of getting rid of it.

Done.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libgi...
File src/codec/SkCodec_libgif.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libgi...
src/codec/SkCodec_libgif.cpp:589: const SkImageInfo subsetDstInfo =
dstInfo.makeWH(fFrameDims.width(), fFrameDims.height());
On 2015/10/01 19:34:33, msarett wrote:
> Glad we don't have to do that any more.  Do you have/want a fFrameIsSubset gif
> to test on?

Yes, I want one. I have not made or found one though.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
File src/codec/SkCodec_libico.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
src/codec/SkCodec_libico.cpp:229: bool SkIcoCodec::onDimensionsSupported(const
SkISize& dim) {
On 2015/10/01 19:34:33, msarett wrote:
> This is a bit redundant which I think is unfortunate.

Agreed. Added a fixme.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec_libic...
src/codec/SkCodec_libico.cpp:295: SkCodecPrintf("Error: No matching candidate
image in ico.\n");
On 2015/10/01 19:34:34, msarett wrote:
> Should we assert that this is never reached?

Done.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:160: dinfo->num_components = 0;
On 2015/10/01 19:34:34, msarett wrote:
> This line belongs in onGetScaledDimensions(), I think.  It is a hack that
makes
> sure jpeg_calc_output_dimensions() does not give an error on our fake dinfo
> struct (we tell it there are zero components, so there are no components to
> iterate through and verify things about).
> 
> I don't think we want to be modifying this value if someone calls this
function
> with a real dinfo struct.

This function is only ever called on a fake dinfo struct, though. Maybe add a
comment?

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:288: // Set up a fake decompress struct in order to
use libjpeg to calculate output dimensions
On 2015/10/01 19:34:34, msarett wrote:
> On 2015/10/01 15:33:14, scroggo wrote:
> > The old code crashed for me when I attempted to call both
getScaledDimensions
> > *and* onDimensionsSupported. This surprised me, and maybe I had some other
> > error, but I think we talked about doing this anyway for some other reason.
> 
> That's surprising.  Hopefully it's because of the num_components thing I
> mentioned above?  Let me know...if not I'd like to track down what was
happening
> at some point.
> 
> It seems a bit weird to me to not directly modify fDecoderMgr->dinfo(), I'd
lean
> towards doing it that way.
> 
> But I'd guess this implementation has identical performance and behavior, so
I'm
> happy either way.  Especially if the old approach is still crashing.

I agree that it's worth figuring out why the other version doesn't work (I tried
leaving out the modification of num_components; that didn't work, but I can keep
trying.)

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:443: fStorage.free();
On 2015/10/01 19:34:34, msarett wrote:
> I'm a little confused here...
> 
> Why can we do this/Why do we want to do this?

Because onGetScanlines changes its behavior based on whether there is an
fSwizzler. If Someone calls start again, we assume that they do not want to use
the same sampling. Maybe this is the wrong assumption, but it seems like it's
the safest.

We could leave the storage (not sure if you meant just that line or all three).
Leaving the storage means we'll be hanging on to memory that may not be needed.
Also, when we ask for sampling again we could be asking for a different color
type, meaning the size needed is different.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
File src/codec/SkScaledCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:205: return fCodec->getPixels(requestedInfo, dst,
rowBytes, &options, ctable, ctableCount);
On 2015/10/01 19:34:34, msarett wrote:
> Unfortunately, I think I might change this back to scanline decoding when we
add
> subsets here.

Well, it will have to do scanline decoding if there's a subset that is not
supported by fCodec, but otherwise I think we should call fCodec->getPixels.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:213: return kInvalidScale;
On 2015/10/01 19:34:34, msarett wrote:
> I think we should probably assert false here, since we've already checked that
> native scaling or sampling is supported.

Done.
> 
> Alternatively, we could just return true in onDimensionsSupported(), since we
> are going to redo the checks in here anyway.

As another option, we could store fSampleX and fSampleY in
onDimensionsSupported, and never need to call it here.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkScaledCodec...
src/codec/SkScaledCodec.cpp:234: return kInvalidScale;
On 2015/10/01 19:34:34, msarett wrote:
> I think it should be unimplemented.

Done.

[msarett, 2015-10-01 22:53:07]

lgtm

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

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkBmpRLECodec...
src/codec/SkBmpRLECodec.cpp:47: // scaling could be supported?
On 2015/10/01 21:16:57, scroggo wrote:
> On 2015/10/01 19:34:33, msarett wrote:
> > Yes sampling in the x dimension is already implemented.  We could consider
> > implementing sampling in the y dimension so that this class supports scaling
> > natively.
> > 
> > I'm still not quite sure that this is best though.
> 
> What do you think is the right way to handle it? In my patch, RLE will no
longer
> work with SkScaledCodec, which has to be able get a sampler to update. If this
> class handles sampling itself, we don't need to worry about that.

I haven't given this enough thought yet.

In my mind, it would be ideal for SkScaledCodec to handle it, but I'm in favor
of doing it natively if its more efficient to do it natively (which it is, based
on the ideas we've discussed so far).

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp
File src/codec/SkCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkCodec.cpp#n...
src/codec/SkCodec.cpp:164: // FIXME: Support subsets somehow? Note that this
works for SkWebpCodec
On 2015/10/01 21:16:57, scroggo wrote:
> On 2015/10/01 19:34:33, msarett wrote:
> > There's a couple ways we could go about this, I don't think either of them
is
> > too tricky.
> > 
> > We may eventually want to combine dimensionsSupported() and options->fSubset
> > into one check.
> 
> Yeah, I considered that. I decided to punt that until SkScaledCodec supports
> subsets.

That makes perfect sense.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.cpp
File src/codec/SkJpegCodec.cpp (right):

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:160: dinfo->num_components = 0;
On 2015/10/01 21:16:57, scroggo wrote:
> On 2015/10/01 19:34:34, msarett wrote:
> > This line belongs in onGetScaledDimensions(), I think.  It is a hack that
> makes
> > sure jpeg_calc_output_dimensions() does not give an error on our fake dinfo
> > struct (we tell it there are zero components, so there are no components to
> > iterate through and verify things about).
> > 
> > I don't think we want to be modifying this value if someone calls this
> function
> > with a real dinfo struct.
> 
> This function is only ever called on a fake dinfo struct, though. Maybe add a
> comment?

Comment works for me.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:288: // Set up a fake decompress struct in order to
use libjpeg to calculate output dimensions
On 2015/10/01 21:16:57, scroggo wrote:
> On 2015/10/01 19:34:34, msarett wrote:
> > On 2015/10/01 15:33:14, scroggo wrote:
> > > The old code crashed for me when I attempted to call both
> getScaledDimensions
> > > *and* onDimensionsSupported. This surprised me, and maybe I had some other
> > > error, but I think we talked about doing this anyway for some other
reason.
> > 
> > That's surprising.  Hopefully it's because of the num_components thing I
> > mentioned above?  Let me know...if not I'd like to track down what was
> happening
> > at some point.
> > 
> > It seems a bit weird to me to not directly modify fDecoderMgr->dinfo(), I'd
> lean
> > towards doing it that way.
> > 
> > But I'd guess this implementation has identical performance and behavior, so
> I'm
> > happy either way.  Especially if the old approach is still crashing.
> 
> I agree that it's worth figuring out why the other version doesn't work (I
tried
> leaving out the modification of num_components; that didn't work, but I can
keep
> trying.)

I'll file a bug on myself.

https://codereview.chromium.org/1372973002/diff/80001/src/codec/SkJpegCodec.c...
src/codec/SkJpegCodec.cpp:443: fStorage.free();
On 2015/10/01 21:16:57, scroggo wrote:
> On 2015/10/01 19:34:34, msarett wrote:
> > I'm a little confused here...
> > 
> > Why can we do this/Why do we want to do this?
> 
> Because onGetScanlines changes its behavior based on whether there is an
> fSwizzler. If Someone calls start again, we assume that they do not want to
use
> the same sampling. Maybe this is the wrong assumption, but it seems like it's
> the safest.
> 
> We could leave the storage (not sure if you meant just that line or all
three).
> Leaving the storage means we'll be hanging on to memory that may not be
needed.
> Also, when we ask for sampling again we could be asking for a different color
> type, meaning the size needed is different.

Thanks!

[scroggo, 2015-10-02 15:11:52]

The latest patch set (8) attempts to fix SkBmpRLECodec. It does not address
sampling without using the SkScaledCodec (though I think we should consider
that), but prior to this patch set it turned previously working BMP rle images
(e.g. rle4-height-negative.bmp) into failures, since SkBmpRLECodec::getSampler()
returned null.

In patch set 8, it returns a sampler which just forwards sampleX to the
SkBmpRLECodec. I cannot tell that it does anything interestingly different from
before (I think fSampleX is the only thing that used to change when scanline
sampling one), but it's apparently stomping on memory - I'm getting
__malloc_assert when I run. Can you help me look at SkBmpRLECodec to try to
figure out what's wrong? (I'm also running an ASAN trybot to help...)

[scroggo, 2015-10-02 15:30:21]

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

[scroggo, 2015-10-02 15:30:22]

RLE is fixed (thanks, Matt!).

Derek, can you take a look at the API?

[scroggo, 2015-10-02 15:50:30]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 15:50:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372973002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372973002/160001

[commit-bot: I haz the power, 2015-10-02 15:52:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-02 15:52:48]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[djsollen, 2015-10-02 17:55:25]

api lgtm

[scroggo, 2015-10-02 18:54:03]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-10-02 18:54:03]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1372973002/#ps200001
(title: "Fix final BMP subclass")

[scroggo, 2015-10-02 18:54:16]

The CQ bit was unchecked by scroggo@google.com

[commit-bot: I haz the power, 2015-10-02 18:54:16]

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

[scroggo, 2015-10-02 18:54:27]

The CQ bit was checked by scroggo@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-10-02 18:54:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372973002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372973002/200001

[scroggo, 2015-10-02 18:55:26]

The CQ bit was unchecked by scroggo@google.com

[scroggo, 2015-10-02 20:07:36]

The CQ bit was checked by scroggo@google.com

[scroggo, 2015-10-02 20:07:36]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com,
djsollen@google.com
Link to the patchset: https://codereview.chromium.org/1372973002/#ps220001
(title: "Attempt to fix RLE overflow")

[commit-bot: I haz the power, 2015-10-02 20:07:53]

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

[commit-bot: I haz the power, 2015-10-02 20:14:53]

Committed patchset #12 (id:220001) as
https://skia.googlesource.com/skia/+/e7fc14b55bb8c41ba054abf0bfa09cdd6ec84671