Scanline decoding for wbmp

src/codec/SkCodec.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 "SkBmpCodec.h"
 #include "SkCodec.h"
 #include "SkData.h"
@@ skipping 116 matching lines @@
     const Result result = this->onGetPixels(info, pixels, rowBytes, *options, ctable, ctableCount);
 
     if ((kIncompleteInput == result || kSuccess == result) && ctableCount) {
         SkASSERT(*ctableCount >= 0 && *ctableCount <= 256);
     }
     return result;
 }
 
 SkCodec::Result SkCodec::getPixels(const SkImageInfo& info, void* pixels, size_t rowBytes) {
     SkASSERT(kIndex_8_SkColorType != info.colorType());
     if (kIndex_8_SkColorType == info.colorType()) {

[scroggo, 2015/08/05 14:36:20]

For consistency, I think you should also remove this check.

(I see that in the full version of getPixels, we return kInvalidParameters
already, as we should.)

[emmaleer, 2015/08/05 15:06:55]

Why would we need this check, if we assert above that info.colorType() !=
kIndex_8_SkColorType?

[scroggo, 2015/08/05 15:23:16]

For running in release. ASSERTs only happen in debug mode, so if we want to
gracefully handle bad input, we would need to do a separate check.

That said, in some cases we are willing to do the wrong thing and/or crash.

When deciding whether to do both, I think you need to ask the following
questions:
- Is this a crucial piece of code that needs to go fast, so we should skip the
check in release?
- If we do not fail gracefully, is there a security risk?

On a related note, I can imagine (at least) two flavors of assert:
1. Internal code - we are the only clients, so we *know* that we'll never hit
the assert
2. APIs (like this case) - the assert could fire if the client called with bad
parameters - in this case the assert gives the client testing their code clear
feedback that they've done something wrong, and the check makes sure we handle
it gracefully if the client ignores the assert - or never runs in debug mode,
like on android :(.

[msarett, 2015/08/05 15:29:42]

I'm wondering if this is an argument for me to add the assert back in...

[msarett, 2015/08/05 15:29:42]

Done.

[scroggo, 2015/08/05 17:55:37]

Yes, I think the assert here would be good.

[msarett, 2015/08/05 18:50:33]

Per our discussion in person, I'm not adding an assert.  This would make our
kInvalidParameter test in CodexTest.cpp fail.

[scroggo, 2015/08/05 19:35:38]

An alternative I just thought of - we could have the assert and only run the
test in release. I do not know if that is a good idea or a terrible one
though...

         return kInvalidConversion;
     }
     return this->getPixels(info, pixels, rowBytes, NULL, NULL, NULL);
 }
-