Bmp Image Decoding

dm/DMSrcSink.cpp

Index: dm/DMSrcSink.cpp
diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index a615b78e7aa34b5296e8332f5ea4036531800a39..efd3e703879ffa90287c0ec6f1b5d397d580adc7 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -45,6 +45,7 @@ Error ImageSrc::draw(SkCanvas* canvas) const {
         return SkStringPrintf("Couldn't read %s.", fPath.c_str());
     }
     const SkColorType dstColorType = canvas->imageInfo().colorType();
+    SkString error;
     if (fDivisor == 0) {
         // Decode the full image.
         SkBitmap bitmap;
@@ -66,18 +67,24 @@ Error ImageSrc::draw(SkCanvas* canvas) const {
             SkAutoLockPixels alp(bitmap);
             const SkImageGenerator::Result result = codec->getPixels(info, bitmap.getPixels(),
                                                                      bitmap.rowBytes());
-            if (result != SkImageGenerator::kSuccess) {
+            if (result == SkImageGenerator::kIncompleteInput) {

[scroggo, 2015/03/04 17:10:29]

I think this would read better as a switch statement.

That said, I'm not sure what the result of drawing to the bitmap and reporting
an error is (as opposed to one or the other).

[msarett, 2015/03/05 23:13:17]

Doesn't feel to me like a switch statement is necessary here now that we've
revised the behavior to be simpler again.  Let me know if you feel otherwise.

+                error = SkStringPrintf("Warning incomplete getPixels %s.", fPath.c_str());

[scroggo, 2015/03/04 17:10:29]

I'm not sure what the right behavior is here. Note that DM does not distinguish
between errors and warnings. So these will be reported as failures. If we expect
to run our tests on incomplete images, this will mean we'll turn the bot red
(and make people think there is a problem with the code).

On the other hand, these are potentially worth investigating, since the decoder
*could* have a bug leading to only partially decoding. It may just be enough to
look at the results in Gold, and if the image is incorrectly incomplete, that
tells us we have a problem.

[msarett, 2015/03/05 23:13:17]

The decision here is to accept kSuccess or kIncompleteInput and draw without
signaling an error.  The decoder should print a warning before returning
kIncompleteInput.  Other return values will signal an error.

It might be worth noting that a few paths in the bmp decoder are able to decode
partial images before hitting an invalid input that forces the decode to halt
(ex: invalid RLE code).  I still return kIncompleteInput here (instead of
kInvalidInput) because we want to signal that there is a valid partially decoded
image.  Let me know if this is confusing.

+            } else if (result != SkImageGenerator::kSuccess) {
                 return SkStringPrintf("Couldn't getPixels %s.", fPath.c_str());
+            } else {
+                error = "";

[scroggo, 2015/03/04 17:10:29]

Maybe set error to "" initially?

             }
         } else {
             if (!SkImageDecoder::DecodeMemory(encoded->data(), encoded->size(), &bitmap,
                                               dstColorType, SkImageDecoder::kDecodePixels_Mode)) {
                 return SkStringPrintf("Couldn't decode %s.", fPath.c_str());
+            } else {
+                error = "";
             }
         }
         encoded.reset((SkData*)NULL);  // Might as well drop this when we're done with it.
         canvas->drawBitmap(bitmap, 0,0);
-        return "";
+        return error;
     }
     // Decode subsets.  This is a little involved.
     SkAutoTDelete<SkMemoryStream> stream(new SkMemoryStream(encoded));
@@ -114,15 +121,27 @@ Error ImageSrc::draw(SkCanvas* canvas) const {
 
 SkISize ImageSrc::size() const {
     SkAutoTUnref<SkData> encoded(SkData::NewFromFileName(fPath.c_str()));
-    SkBitmap bitmap;
-    if (!encoded || !SkImageDecoder::DecodeMemory(encoded->data(),
-                                                  encoded->size(),
-                                                  &bitmap,
-                                                  kUnknown_SkColorType,
-                                                  SkImageDecoder::kDecodeBounds_Mode)) {
-        return SkISize::Make(0,0);
+    if (FLAGS_codec) {
+        SkAutoTDelete<SkCodec> codec(SkCodec::NewFromData(encoded));
+        if (!codec) {
+            return SkISize::Make(0,0);
+        }
+        SkImageInfo info;
+        if (!codec->getInfo(&info)) {
+            return SkISize::Make(0,0);
+        }
+        return info.dimensions();
+    } else {
+        SkBitmap bitmap;
+        if (!encoded || !SkImageDecoder::DecodeMemory(encoded->data(),
+                                                      encoded->size(),
+                                                      &bitmap,
+                                                      kUnknown_SkColorType,
+                                                      SkImageDecoder::kDecodeBounds_Mode)) {
+            return SkISize::Make(0,0);
+        }
+        return bitmap.dimensions();
     }
-    return bitmap.dimensions();
 }
 
 Name ImageSrc::name() const {