Swizzler changes Index8 and 565

dm/DM.cpp

Index: dm/DM.cpp
diff --git a/dm/DM.cpp b/dm/DM.cpp
index 5d85e22b3a8e989d30ff358ead3f1098df882c37..fdf952cfeb00abd686e3709b77c12099d750fa48 100644
--- a/dm/DM.cpp
+++ b/dm/DM.cpp
@@ -178,6 +178,47 @@ static void push_src(const char* tag, const char* options, Src* s) {
     }
 }
 
+static void push_codec_srcs(Path path) {
+    // These are the extensions for images that might be natively kIndex8.  For these images,
+    // we should test decoding to kIndex8 and also to the canvas color type.
+    // All gifs are natively kIndex8 compatible.  Only some bmps and rarely icos (when they
+    // contain a kIndex8 bmp) can be decoded to kIndex8.  In the case that we try to decode
+    // incompatible images to kIndex8, we will fail gracefully in CodecSrc::draw().
+    static const char* const indexExts[] = {
+        "bmp", "gif", "ico",
+        "BMP", "GIF", "ICO",
+    };
+
+    // The same applies to natively kGray images.
+    static const char* const grayExts[] = {
+        "wbmp", "WBMP",
+    };
+
+    // Push sources that we want to try to decode to kIndex8
+    for (uint32_t i = 0; i < SK_ARRAY_COUNT(indexExts); i++) {
+        if (path.endsWith(indexExts[i])) {

[scroggo, 2015/04/07 21:15:34]

Here's how I imagined this working:

SkAutoTDelete<SkCodec> codec(NewFromStream(...));
switch(codec->getInfo().colorType()) {
  case kIndex8:
    push_src(...., kIndex8Always);
    break;
  case kGrayscale:
    push_src(...., kGrayAlways);
    break;
  default:
    // do nothing
    break;
}

// normal calls to push_src

The advantage to my approach is that if we make another decoder (e.g. png, jpeg)
support one of these types, we won't need to update this code to look for those
types as well. (Case in point: looking at this code reminded me to update
codec_supported in my change to implement a webp codec.) Our PNG decoder
*should* actually support both Index8 and Grayscale, depending on the data (I
partially kept the code to decode to grayscale - but it is disguised as Alpha8,
since we didn't have grayscale - we need to update it to use grayscale and test
it, although that can be a separate change). I also think it's generally
simpler.

One downside to my approach is that it adds an extra SkCodec creation into the
setup (which is not multi-threaded), although it will remove some extraneous
tasks (where we created Index8 CodecSrcs for BMPs etc which are not in fact
Index8).

I would lean towards my approach since I think it's more maintainable.

A downside to both of our approaches is there is no way to test e.g. kIndex8 for
an SkCodec that claimed to be grayscale. Our Wbmp codec supports this. (Related
issue: should it? Is there any reason for a client to prefer kIndex8 over
grayscale? Android may not know what to do with a grayscale bitmap; for kIndex8
their hardware renderer copies to N32 before uploading, and they may need to
update to do the same thing for grayscale, at the very least.) If we do support
this (I think Hal added support because it's easy), we should test it somehow.

[msarett, 2015/04/08 13:59:10]

What you've described is how I originally thought you intended it.  But then I
ran into the problem of not having the codec and went the file type route.

I have felt similar pain in adding file types to those lists, and I am noticing
now that I forgot png on both lists.  Pretty good evidence that creating the
SkCodec earlier would serve us better.

How do you feel about making the SkCodec a field in CodecSrc and only paying the
price of creating it once?  Maybe this is what you intended to begin with?  I
think its worth seeing how it looks - it would hopefully negate the cost of
creating the codec if we don't have to do it again later.

I was aware of the wbmp problem but decided not to handle it.  FWIW, the current
approach would probably be better in this case, given that we could just
describe custom behavior when the file type is a wbmp.  But I don't think this
is a convincing argument to stick with this code.  I see benefits in always
supporting kN32 and also supporting the "best" native format.  I'm not sure that
we want to work to support all of the obscure formats just for android to
convert them to kN32 anyway?  But this seems like it's worth discussing.

[scroggo, 2015/04/08 17:21:26]

That seems okay, although it seems like you call create_codec for each creation
of a CodecSrc? How is that different from letting CodecSrc create it?

It seems like it *might* be nice to create the SkCodec once, but you cannot use
it with multiple CodecSrcs because it is stateful. Instead, you could create the
SkData once, and share it with each CodecSrc. This means we read from disk less.
I'm not sure how much that will improve our testing speed overall, but it might
be worth it. (I could imagine making it possible to copy an SkCodec, but that
would be essentially free for one backed by an SkData, but much more difficult
with an arbitrary stream.)
Definitely worth discussing, and it can inform our decision here, but I don't
know that we need to hold this up for that.

If we think that a gray scale image will always be supported by index8 (it seems
like it could be easily), we could make the grayscale case fall through to add
index 8 as well. And even if it doesn't support index8, no harm done; we'll just
return Error::Nonfatal. So I'm leaning towards this approach.

[msarett, 2015/04/08 19:35:39]

That was my disappointment with this approach.  We save the creation of just one
SkCodec (If we are running four tests, we create four SkCodecs here.  Where
before, if we ran four tests we would create one SkCodec here, and then four
more in the four calls to draw.).

As we discussed let's keep the code as simple as possible.

+            push_src("image", "codec kIndex8", new CodecSrc(
+                    path, CodecSrc::kNormal_Mode, CodecSrc::kIndex8_Always_DstColorType));
+            break;
+        }
+    }
+
+    // Push sources that we want to try to decode to kGray8
+    for (uint32_t i = 0; i < SK_ARRAY_COUNT(grayExts); i++) {
+        if (path.endsWith(grayExts[i])) {
+            push_src("image", "codec kGray8", new CodecSrc(
+                    path, CodecSrc::kNormal_Mode, CodecSrc::kGrayscale_Always_DstColorType));
+            break;
+        }
+    }
+
+    // Decode all sources to the canvas color type
+    push_src("image", "codec", new CodecSrc(
+            path, CodecSrc::kNormal_Mode, CodecSrc::kGetFromCanvas_DstColorType));
+    push_src("image", "scanline", new CodecSrc(
+            path, CodecSrc::kScanline_Mode, CodecSrc::kGetFromCanvas_DstColorType));
+}
+
 static bool codec_supported(const char* ext) {
     // FIXME: Once other versions of SkCodec are available, we can add them to this
     // list (and eventually we can remove this check once they are all supported).
@@ -223,8 +264,7 @@ static void gather_srcs() {
                     push_src("image", "decode", new ImageSrc(path)); // Decode entire image
                     push_src("image", "subset", new ImageSrc(path, 2)); // Decode into 2x2 subsets
                     if (codec_supported(exts[j])) {
-                        push_src("image", "codec", new CodecSrc(path, CodecSrc::kNormal_Mode));
-                        push_src("image", "scanline", new CodecSrc(path, CodecSrc::kScanline_Mode));
+                        push_codec_srcs(path);
                     }
                 }
             }
@@ -232,8 +272,7 @@ static void gather_srcs() {
             // assume that FLAGS_images[i] is a valid image if it is a file.
             push_src("image", "decode", new ImageSrc(flag)); // Decode entire image.
             push_src("image", "subset", new ImageSrc(flag, 2)); // Decode into 2 x 2 subsets
-            push_src("image", "codec", new CodecSrc(flag, CodecSrc::kNormal_Mode));
-            push_src("image", "scanline", new CodecSrc(flag, CodecSrc::kScanline_Mode));
+            push_codec_srcs(flag);
         }
     }
 }