Reland "Fix genID cloning bugs."

tests/GpuBitmapCopyTest.cpp

 
 /*
  * Copyright 2012 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #if SK_SUPPORT_GPU
 
@@ skipping 22 matching lines @@
     const char*         fValid;
 };
 
 /**
  *  Check to ensure that copying a GPU-backed SkBitmap behaved as expected.
  *  @param reporter Used to report failures.
  *  @param desiredConfig Config being copied to. If the copy succeeded, dst must have this Config.
  *  @param success True if the copy succeeded.
  *  @param src A GPU-backed SkBitmap that had copyTo or deepCopyTo called on it.
  *  @param dst SkBitmap that was copied to.
- *  @param deepCopy True if deepCopyTo was used; false if copyTo was used.
+ *  @param expectChangedGenID Whether the genIDs should be different if success is true.
  */
 static void TestIndividualCopy(skiatest::Reporter* reporter, const SkBitmap::Config desiredConfig,
                                const bool success, const SkBitmap& src, const SkBitmap& dst,
-                               const bool deepCopy = true) {
+                               const bool expectChangedGenID) {

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

I think I might find this easier to follow inverted as expectSameGenID, but I
can go with the flow here.

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

Agreed. Inverted.

     if (success) {
         REPORTER_ASSERT(reporter, src.width() == dst.width());
         REPORTER_ASSERT(reporter, src.height() == dst.height());
         REPORTER_ASSERT(reporter, dst.config() == desiredConfig);
         if (src.config() == dst.config()) {
-            // FIXME: When calling copyTo (so deepCopy is false here), sometimes we copy the pixels
-            // exactly, in which case the IDs should be the same, but sometimes we do a bitmap draw,
-            // in which case the IDs should not be the same. Is there any way to determine which is
-            // the case at this point?
-            if (deepCopy) {
+            if (expectChangedGenID) {

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

Don't know if you find this clearer or not, but I think you can write this as

REPORTER_ASSERT(reporter, (src.getGenerationID() != dst.getGenerationID()) ==
expectChangedGenID));

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

I find it clever but less clear.

+                REPORTER_ASSERT(reporter, src.getGenerationID() != dst.getGenerationID());
+            } else {
                 REPORTER_ASSERT(reporter, src.getGenerationID() == dst.getGenerationID());
             }
             REPORTER_ASSERT(reporter, src.pixelRef() != NULL && dst.pixelRef() != NULL);
 
             // Do read backs and make sure that the two are the same.
             SkBitmap srcReadBack, dstReadBack;
             {
                 SkASSERT(src.getTexture() != NULL);
-                bool readBack = src.pixelRef()->readPixels(&srcReadBack);
+                const SkIPoint origin = src.pixelRefOrigin();
+                const SkIRect subset = SkIRect::MakeXYWH(origin.fX, origin.fY,
+                                                         src.width(), src.height());
+                bool readBack = src.pixelRef()->readPixels(&srcReadBack, &subset);
                 REPORTER_ASSERT(reporter, readBack);
             }
             if (dst.getTexture() != NULL) {
-                bool readBack = dst.pixelRef()->readPixels(&dstReadBack);
+                const SkIPoint origin = dst.pixelRefOrigin();
+                const SkIRect subset = SkIRect::MakeXYWH(origin.fX, origin.fY,
+                                                         dst.width(), dst.height());
+                bool readBack = dst.pixelRef()->readPixels(&dstReadBack, &subset);
                 REPORTER_ASSERT(reporter, readBack);
             } else {
                 // If dst is not a texture, do a copy instead, to the same config as srcReadBack.
                 bool copy = dst.copyTo(&dstReadBack, srcReadBack.config());
                 REPORTER_ASSERT(reporter, copy);
             }
 
             SkAutoLockPixels srcLock(srcReadBack);
             SkAutoLockPixels dstLock(dstReadBack);
             REPORTER_ASSERT(reporter, srcReadBack.readyToDraw() && dstReadBack.readyToDraw());
@@ skipping 79 matching lines @@
                 bool canSucceed = src.canCopyTo(gPairs[j].fConfig);
                 if (success != canSucceed) {
                     SkString str;
                     str.printf("SkBitmap::deepCopyTo from %s to %s returned %s,"
                                "but canCopyTo returned %s",
                                gConfigName[i], gConfigName[j], boolStr(success),
                                boolStr(canSucceed));
                     reporter->reportFailed(str);
                 }
 
-                TestIndividualCopy(reporter, gPairs[j].fConfig, success, src, dst);
+                TestIndividualCopy(reporter, gPairs[j].fConfig, success, src, dst, false);
 
                 // Test copying the subset bitmap, using both copyTo and deepCopyTo.
                 if (extracted) {
                     SkBitmap subsetCopy;
                     success = subset.copyTo(&subsetCopy, gPairs[j].fConfig);
                     REPORTER_ASSERT(reporter, success == expected);
                     REPORTER_ASSERT(reporter, success == canSucceed);
                     TestIndividualCopy(reporter, gPairs[j].fConfig, success, subset, subsetCopy,
                                        false);
 
                     // Reset the bitmap so that a failed copyTo will leave it in the expected state.
                     subsetCopy.reset();
                     success = subset.deepCopyTo(&subsetCopy, gPairs[j].fConfig);
                     REPORTER_ASSERT(reporter, success == expected);
                     REPORTER_ASSERT(reporter, success == canSucceed);
                     TestIndividualCopy(reporter, gPairs[j].fConfig, success, subset, subsetCopy,
+                                       false);
+
+                    // Now set a bitmap to be a subset that will share the same pixelref.
+                    // This allows testing another case of cloning the genID. When calling copyTo
+                    // on a bitmap representing a subset of its pixelref, the resulting pixelref
+                    // should not share the genID, since we only copied the subset.
+                    SkBitmap trueSubset;
+                    // FIXME: Once https://codereview.chromium.org/109023008/ lands, call
+                    // trueSubset.installPixelRef(src.pixelRef(), subset);
+                    trueSubset.setConfig(gPairs[i].fConfig, W/2, H/2);
+                    trueSubset.setPixelRef(src.pixelRef(), W/2, H/2);
+
+                    subsetCopy.reset();
+                    success = trueSubset.copyTo(&subsetCopy, gPairs[j].fConfig);
+                    REPORTER_ASSERT(reporter, success == expected);
+                    REPORTER_ASSERT(reporter, success == canSucceed);
+                    TestIndividualCopy(reporter, gPairs[j].fConfig, success, trueSubset, subsetCopy,
                                        true);
+
+                    // deepCopyTo copies the entire pixelref, even if the bitmap only represents
+                    // a subset. Therefore, the result should share the same genID.
+                    subsetCopy.reset();
+                    success = trueSubset.deepCopyTo(&subsetCopy, gPairs[j].fConfig);
+                    REPORTER_ASSERT(reporter, success == expected);
+                    REPORTER_ASSERT(reporter, success == canSucceed);
+                    TestIndividualCopy(reporter, gPairs[j].fConfig, success, trueSubset, subsetCopy,
+                                       false);
                 }
             } // for (size_t j = ...
         } // for (size_t i = ...
     } // GrContextFactory::GLContextType
 }
 
 #include "TestClassDef.h"
 DEFINE_GPUTESTCLASS("GpuBitmapCopy", TestGpuBitmapCopyClass, TestGpuBitmapCopy)
 
 #endif