Reland "Fix genID cloning bugs."

tests/BitmapCopyTest.cpp

-
 /*
  * Copyright 2011 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 #include "Test.h"
 #include "TestClassDef.h"
 #include "SkBitmap.h"
 #include "SkRect.h"
@@ skipping 204 matching lines @@
     };
 
     static const bool isExtracted[] = {
         false, true
     };
 
     const int W = 20;
     const int H = 33;
 
     for (size_t i = 0; i < SK_ARRAY_COUNT(gPairs); i++) {
+        SkBitmap srcOpaque, srcPremul;
+        {
+            SkColorTable* ctOpaque = NULL;
+            SkColorTable* ctPremul = NULL;
+
+            srcOpaque.setConfig(gPairs[i].fConfig, W, H, 0, kOpaque_SkAlphaType);
+            srcPremul.setConfig(gPairs[i].fConfig, W, H, 0, kPremul_SkAlphaType);
+            if (SkBitmap::kIndex8_Config == gPairs[i].fConfig) {
+                ctOpaque = init_ctable(kOpaque_SkAlphaType);
+                ctPremul = init_ctable(kPremul_SkAlphaType);
+            }
+            srcOpaque.allocPixels(ctOpaque);
+            srcPremul.allocPixels(ctPremul);
+            SkSafeUnref(ctOpaque);
+            SkSafeUnref(ctPremul);
+        }
+        init_src(srcOpaque);
+        init_src(srcPremul);
+
+        // test extractSubset

[mtklein, 2014/01/09 15:58:07]

A block like this makes me think you might want to extract a new
DEF_TEST(BitmapCopy_extractSubset, reporter) { ... }

[scroggo, 2014/01/09 20:45:23]

Why so, for clarity? Or is there an advantage I'm not seeing? To me it makes
sense here because it shares some of the same code (setting up the bitmaps,
iterating through the configs) and it is testing copying the extracted subset,
but I could go either way.

[mtklein, 2014/01/09 21:29:18]

Yeah, clarity.  It's nearly-independent from the code below and it's a feature
that could fail independently of the other things we test below it.  If nothing
else, it'd help readability of that code below, in that we wouldn't have to
think about anything in the extractSubset block setting up state or
preconditions the test depends on.  Same reason we break up code into separate
functions even if they're only called from one place.

That shared setup code itself could be pulled out as a function too:
  static void setup_src_bitmaps(SkBitmap* opaque, SkBitmap* premul) { ... }

My rule of thumb is that, nearly any time you write a freestanding {} block and
it doesn't involve some sort of RAII scope trick, it wants to be its own
function, especially if it's got a comment like // test extractSubset above it. 
It's a major  step up in readability if you can get your functions to fit
entirely within a reasonable screen height, and I find it helps me get a better
handle on data dependencies too when I'm forced to think about which functions
need what.

[scroggo, 2014/01/09 21:47:29]

Thanks for the justification - you have me convinced! I have moved extractSubset
to its own test.

+        {
+            SkBitmap bitmap(srcOpaque);
+            SkBitmap subset;
+            SkIRect r;
+            // Extract a subset which has the same width as the original. This
+            // catches a bug where we cloned the genID incorrectly.
+            r.set(0, 1, W, 3);
+            bitmap.setIsVolatile(true);
+            if (bitmap.extractSubset(&subset, r)) {
+                REPORTER_ASSERT(reporter, subset.width() == W);
+                REPORTER_ASSERT(reporter, subset.height() == 2);
+                REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+                REPORTER_ASSERT(reporter, subset.isVolatile() == true);
+
+                // Test copying an extracted subset.
+                for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
+                    SkBitmap copy;
+                    bool success = subset.copyTo(&copy, gPairs[j].fConfig);
+                    if (!success) {
+                        // Skip checking that success matches fValid, which is redundant
+                        // with the code below.
+                        REPORTER_ASSERT(reporter, gPairs[i].fConfig != gPairs[j].fConfig);
+                        continue;
+                    }
+
+                    // When performing a copy of an extracted subset, the gen id should
+                    // change.
+                    REPORTER_ASSERT(reporter, copy.getGenerationID() != subset.getGenerationID());
+
+                    REPORTER_ASSERT(reporter, copy.width() == W);
+                    REPORTER_ASSERT(reporter, copy.height() == 2);
+
+                    if (gPairs[i].fConfig == gPairs[j].fConfig) {
+                        SkAutoLockPixels alp0(subset);
+                        SkAutoLockPixels alp1(copy);
+                        // they should both have, or both not-have, a colortable
+                        bool hasCT = subset.getColorTable() != NULL;
+                        REPORTER_ASSERT(reporter, (copy.getColorTable() != NULL) == hasCT);
+                    }
+                }
+            }
+
+            bitmap = srcPremul;
+            bitmap.setIsVolatile(false);
+            if (bitmap.extractSubset(&subset, r)) {
+                REPORTER_ASSERT(reporter, subset.alphaType() == bitmap.alphaType());
+                REPORTER_ASSERT(reporter, subset.isVolatile() == false);
+            }
+        }
+
         for (size_t j = 0; j < SK_ARRAY_COUNT(gPairs); j++) {
-            SkBitmap srcOpaque, srcPremul, dst;
-
-            {
-                SkColorTable* ctOpaque = NULL;
-                SkColorTable* ctPremul = NULL;
-
-                srcOpaque.setConfig(gPairs[i].fConfig, W, H, 0, kOpaque_SkAlphaType);
-                srcPremul.setConfig(gPairs[i].fConfig, W, H, 0, kPremul_SkAlphaType);
-                if (SkBitmap::kIndex8_Config == gPairs[i].fConfig) {
-                    ctOpaque = init_ctable(kOpaque_SkAlphaType);
-                    ctPremul = init_ctable(kPremul_SkAlphaType);
-                }
-                srcOpaque.allocPixels(ctOpaque);
-                srcPremul.allocPixels(ctPremul);
-                SkSafeUnref(ctOpaque);
-                SkSafeUnref(ctPremul);
-            }
-            init_src(srcOpaque);
-            init_src(srcPremul);
+            SkBitmap dst;
 
             bool success = srcPremul.copyTo(&dst, gPairs[j].fConfig);
             bool expected = gPairs[i].fValid[j] != '0';
             if (success != expected) {
                 SkString str;
                 str.printf("SkBitmap::copyTo from %s to %s. expected %s returned %s",
                            gConfigName[i], gConfigName[j], boolStr(expected),
                            boolStr(success));
                 reporter->reportFailed(str);
             }
@@ skipping 19 matching lines @@
                     REPORTER_ASSERT(reporter, dst.readyToDraw());
                     const char* srcP = (const char*)srcPremul.getAddr(0, 0);
                     const char* dstP = (const char*)dst.getAddr(0, 0);
                     REPORTER_ASSERT(reporter, srcP != dstP);
                     REPORTER_ASSERT(reporter, !memcmp(srcP, dstP,
                                                       srcPremul.getSize()));
                     REPORTER_ASSERT(reporter, srcPremul.getGenerationID() == dst.getGenerationID());
                 } else {
                     REPORTER_ASSERT(reporter, srcPremul.getGenerationID() != dst.getGenerationID());
                 }
-                // test extractSubset
-                {
-                    SkBitmap bitmap(srcOpaque);
-                    SkBitmap subset;
-                    SkIRect r;
-                    r.set(1, 1, 2, 2);
-                    bitmap.setIsVolatile(true);
-                    if (bitmap.extractSubset(&subset, r)) {
-                        REPORTER_ASSERT(reporter, subset.width() == 1);
-                        REPORTER_ASSERT(reporter, subset.height() == 1);
-                        REPORTER_ASSERT(reporter,
-                                        subset.alphaType() == bitmap.alphaType());
-                        REPORTER_ASSERT(reporter,
-                                        subset.isVolatile() == true);
-
-                        SkBitmap copy;
-                        REPORTER_ASSERT(reporter,
-                                        subset.copyTo(&copy, subset.config()));
-                        REPORTER_ASSERT(reporter, copy.width() == 1);
-                        REPORTER_ASSERT(reporter, copy.height() == 1);
-                        REPORTER_ASSERT(reporter, copy.rowBytes() <= 4);
-
-                        SkAutoLockPixels alp0(subset);
-                        SkAutoLockPixels alp1(copy);
-                        // they should both have, or both not-have, a colortable
-                        bool hasCT = subset.getColorTable() != NULL;
-                        REPORTER_ASSERT(reporter,
-                                    (copy.getColorTable() != NULL) == hasCT);
-                    }
-                    bitmap = srcPremul;
-                    bitmap.setIsVolatile(false);
-                    if (bitmap.extractSubset(&subset, r)) {
-                        REPORTER_ASSERT(reporter,
-                                        subset.alphaType() == bitmap.alphaType());
-                        REPORTER_ASSERT(reporter,
-                                        subset.isVolatile() == false);
-                    }
-                }
             } else {
                 // dst should be unchanged from its initial state
                 REPORTER_ASSERT(reporter, dst.config() == SkBitmap::kNo_Config);
                 REPORTER_ASSERT(reporter, dst.width() == 0);
                 REPORTER_ASSERT(reporter, dst.height() == 0);
             }
         } // for (size_t j = ...
 
         // Tests for getSafeSize(), getSafeSize64(), copyPixelsTo(),
         // copyPixelsFrom().
@@ skipping 211 matching lines @@
                 // for the transfer.
                 REPORTER_ASSERT(reporter,
                     subset.copyPixelsFrom(buf, 1, subset.rowBytes()) ==
                         false);
 
 #endif
             }
         } // for (size_t copyCase ...
     }
 }