Adding direct access to SkMipMap levels.

tests/MipMapTest.cpp

Index: tests/MipMapTest.cpp
diff --git a/tests/MipMapTest.cpp b/tests/MipMapTest.cpp
index 11021e6ebf960ef674c6a9b4f35bfde0275d30e9..981edd27adedfab1f4bbfc43991c5446a4f15be8 100644
--- a/tests/MipMapTest.cpp
+++ b/tests/MipMapTest.cpp
@@ -53,3 +53,184 @@ DEF_TEST(MipMap, reporter) {
         }
     }
 }
+
+DEF_TEST(MipMap_DirectLevelAccess, reporter) {

[reed1, 2016/01/28 02:06:03]

Looking above, perhaps we just could augment the existing test to also compare
the level-count to the new method ComputeLevelCount?

[cblume, 2016/02/01 09:31:07]

Sure, I'll do that. Good idea.

In fact, I would like to do a bit more as well with your permission.

In addition to positive, expected outcomes you also want your unit tests to
catch common mistakes like checking edge cases, such as -1, 0, 1, INT_MIN,
INT_MAX. In the case of SkMipMap, 2 is an edge for a dimension (the behavior
changes between 1 and 2). Also in the case of SkMipMap, maybe INT_MAX for a
dimension doesn't make sense.

The above test currently prevents testing -1, 0, and 1 since the call to
make_bitmap adds 2 to rand.nextU(). But it also isn't testing 2. I wrote a quick
bit of code to see if a default-constructed SkRandom contains -1, 0, 1, or 2 in
the first 1000 calls to nextU() (1000 since we loop 500 times but call it twice
per loop). It does not.

In fact, using RNG at all makes it feel much more like fuzz testing than unit
testing. But the default-constructed SkRandom currently seeds with 0 so it isn't
a fuzz test. However, these tests don't assert that SkRandom will be consistent
between runs so they could start failing when SkRandom changes. For example, if
someone changes SkRandom to use hardware RNG where available, suddenly
SkMipMap's tests may become flaky. None of us want to be the person to have to
debug that one.

Really, a handful of chosen positive tests should be fine. I.E. A small mipmap,
some common mipmaps, a large mipmap.
(Then add to that more-than-a-handful negative tests.)

+    // create mipmap with invalid size
+    {
+        // SkMipMap current requires the dimensions be greater than 2x2
+        SkBitmap bm;
+        bm.allocN32Pixels(1, 1);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+        REPORTER_ASSERT(reporter, mm->countLevels() == 0);
+    }
+
+
+    // check small mipmap's count and levels

[reed1, 2016/01/28 02:06:03]

can this block be combined with the one below it, since they seem to just diff
by the initial dimensions?

[cblume, 2016/02/01 09:31:07]

Yes, it can. Initial dimensions, the numerator for the size calculation (which
can be computed form the initial dimensions), and expected mip level count.

I can refactor it out.

+    {
+        SkBitmap bm;
+        bm.allocN32Pixels(32, 32);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+
+        const int mipLevelCount = mm->countLevels();
+        // There should be 5 mipmap levels generated:
+        // 16x16, 8x8, 4x4, 2x2, 1x1
+        REPORTER_ASSERT(reporter, mipLevelCount == 5);
+        for (int i = 0; i < mipLevelCount; ++i) {
+            SkMipMap::Level level;
+            mm->getLevel(i, &level);
+            // Make sure the mipmaps contain valid data and that the sizes are
+            // correct
+            REPORTER_ASSERT(reporter, level.fPixmap.addr() != nullptr);
+
+            // Add 1 because we are checking mip levels 1-6.
+            // The bitmap is the 0th mipmap level.
+            int twoToTheMipLevel = 1 << (i + 1);
+            int size = 16 / twoToTheMipLevel;
+            REPORTER_ASSERT(reporter, level.fPixmap.width() == size);
+            REPORTER_ASSERT(reporter, level.fPixmap.height() == size);
+        }
+    }
+
+
+    // check large mipmap's count and levels
+    {
+        SkBitmap bm;
+        bm.allocN32Pixels(1000, 1000);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+
+        const int mipLevelCount = mm->countLevels();
+        // There should be 9 mipmap levels generated:
+        // 500x500, 250x250, 125x125, 62x62, 31x31, 15x15, 7x7, 3x3. 1x1
+        REPORTER_ASSERT(reporter, mipLevelCount == 9);
+        for (int i = 0; i < mipLevelCount; ++i) {
+            SkMipMap::Level level;
+            mm->getLevel(i, &level);
+            // Make sure the mipmaps contain valid data and that the sizes are
+            // correct
+            REPORTER_ASSERT(reporter, level.fPixmap.addr() != nullptr);
+
+            // Add 1 because we are checking mip levels 1-9.
+            // The bitmap is the 0th mipmap level.
+            int twoToTheMipLevel = 1 << (i + 1);
+            int size = 500 / twoToTheMipLevel;
+            REPORTER_ASSERT(reporter, level.fPixmap.width() == size);
+            REPORTER_ASSERT(reporter, level.fPixmap.height() == size);
+        }
+    }
+}
+
+DEF_TEST(MipMap_ComputeLevelCount, reporter) {
+    // Test mipmaps with negative sizes
+    int negativeWidthResult = SkMipMap::ComputeLevelCount(-100, 100);
+    REPORTER_ASSERT(reporter, negativeWidthResult == 0);
+
+    int negativeHeightResult = SkMipMap::ComputeLevelCount(100, -100);
+    REPORTER_ASSERT(reporter, negativeHeightResult == 0);
+
+    int negativeBothResult = SkMipMap::ComputeLevelCount(-100, -100);
+    REPORTER_ASSERT(reporter, negativeBothResult == 0);
+
+
+    // Test mipmaps with 0, 1, 2 as dimensions
+    // (SkMipMap::Build requires a min size of 2)
+    //
+    // 0
+    int zeroWidthResult = SkMipMap::ComputeLevelCount(0, 100);

[reed1, 2016/01/28 02:06:03]

There is a fair amount of copy/paste here. I wonder if we can condense this a
lot with a table and/or programmatic generation of test cases.

e.g.

repeat (10000) {
    w = rand.nextU() % 1000;
    h = rand.nextU() %1000;
    n0 == ComputeLevelCount(w, h);
    bm.allocPixels(...);
    mip = Build(bm);
    assert(n0 == mip->countLevels());
}

[cblume, 2016/02/01 09:31:07]

I agree that there is a lot of copy/paste/minor change here. But remember this
is test code, which has different goals from non-test code.

I just wrote some code to make it a table and tested what the result looks like
when a test failed.

FAILURE: ../../tests/MipMapTest.cpp:208 values[i] == results[i]

Compare that to what this patchset currently has:

FAILURE: ../../tests/MipMapTest.cpp:202 smallestAxisIsSixtyFourResult == 6

In test coding, you want a failed test to tell you exactly what failed and what
was expected. In this case, the thing which failed is the
"smallestAxisIsSixtyFour" case. And the expected value was 6. (It's actually 7,
but I changed it to 6 to make this test fail.)

As a result of this unique-to-test-code need, it is pretty standard for test
code to feel very copy/paste. The tests should be as isolated and concise as
possible.

So I recommend leaving these the way they are. Each one is testing something
specific and clearly describes what is being tested and what is expected.


That said, Skia's DEF_TEST is a tad interesting. There doesn't seem to be a test
suite. So DEF_TEST is sort of the test suite, which does the setup and teardown.
Then each REPORTER_ASSERT is sort of its own test. Just a side thought to help
make it fit.

In a scenario where you have a test suite you would have a failed test look like
this:

Inside SkMipMap_ComputeLevelCount test suite,
test "smallestAxisIsSixtyFour" failed...
ASSERT( SkMipMap::ComputeLevelCount(64, 129) == 7 )

The reason I broke these into two lines instead of REPORTER_ASSERT(reporter,
SkMipMap::ComputeLevelCount(64, 129) == 7) is because then there is no
description of what was being tested. In this case, we specifically wanted to
test when one axis is smaller, and that smaller axis is 64. When it is broken
into two lines there is a sort of test name included in the print out.

+    REPORTER_ASSERT(reporter, zeroWidthResult == 0);
+
+    int zeroHeightResult = SkMipMap::ComputeLevelCount(100, 0);
+    REPORTER_ASSERT(reporter, zeroHeightResult == 0);
+
+    int zeroBothResult = SkMipMap::ComputeLevelCount(0, 0);
+    REPORTER_ASSERT(reporter, zeroBothResult == 0);
+
+    // 1
+    int oneWidthResult = SkMipMap::ComputeLevelCount(1, 100);
+    REPORTER_ASSERT(reporter, oneWidthResult == 0);
+
+    int oneHeightResult = SkMipMap::ComputeLevelCount(100, 1);
+    REPORTER_ASSERT(reporter, oneHeightResult == 0);
+
+    int oneBothResult = SkMipMap::ComputeLevelCount(1, 1);
+    REPORTER_ASSERT(reporter, oneBothResult == 0);
+
+    // 2
+    int twoWidthResult = SkMipMap::ComputeLevelCount(2, 100);
+    REPORTER_ASSERT(reporter, twoWidthResult == 2);
+
+    int twoHeightResult = SkMipMap::ComputeLevelCount(100, 2);
+    REPORTER_ASSERT(reporter, twoHeightResult == 2);
+
+    int twoBothResult = SkMipMap::ComputeLevelCount(2, 2);
+    REPORTER_ASSERT(reporter, twoBothResult == 2);
+
+
+    // Test a handful of boundaries such as 63x63 and 64x64
+    int sixtyThreeResult = SkMipMap::ComputeLevelCount(63, 63);
+    REPORTER_ASSERT(reporter, sixtyThreeResult == 6);
+
+    int sixtyFourResult = SkMipMap::ComputeLevelCount(64, 64);
+    REPORTER_ASSERT(reporter, sixtyFourResult == 7);
+
+    int oneHundredTwentySevenResult = SkMipMap::ComputeLevelCount(127, 127);
+    REPORTER_ASSERT(reporter, oneHundredTwentySevenResult == 7);
+
+    int oneHundredTwentyEightResult = SkMipMap::ComputeLevelCount(128, 128);
+    REPORTER_ASSERT(reporter, oneHundredTwentyEightResult == 8);
+
+    int twoHundredFiftyFiveResult = SkMipMap::ComputeLevelCount(255, 255);
+    REPORTER_ASSERT(reporter, twoHundredFiftyFiveResult == 8);
+
+    int twoHundredFiftySixResult = SkMipMap::ComputeLevelCount(256, 256);
+    REPORTER_ASSERT(reporter, twoHundredFiftySixResult == 9);
+
+
+    // Test different dimensions, such as 256x64
+    int smallestAxisIsSixtyFourResult = SkMipMap::ComputeLevelCount(64, 129);
+    REPORTER_ASSERT(reporter, smallestAxisIsSixtyFourResult == 7);
+
+    int smallestAxisIsThirtyTwoResult = SkMipMap::ComputeLevelCount(255, 32);
+    REPORTER_ASSERT(reporter, smallestAxisIsThirtyTwoResult == 6);
+
+    int smallestAxisIsFiveHundredResult = SkMipMap::ComputeLevelCount(500, 1000);
+    REPORTER_ASSERT(reporter, smallestAxisIsFiveHundredResult == 9);
+
+    // Test a few calls to SkMipMap::Build to make sure it matches
+    // SkMipMap::ComputeLevelCount
+    //
+    // Add 1 to mm->countLevels() because SkMipMap does not contain the base
+    // mipmap level.
+    // It contains mipmap levels 1-x, not 0-x.
+    {
+        SkBitmap bm;
+        bm.allocN32Pixels(255, 255);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+        REPORTER_ASSERT(reporter, mm->countLevels() + 1 == SkMipMap::ComputeLevelCount(255, 255));
+    }
+
+    {
+        SkBitmap bm;
+        bm.allocN32Pixels(32, 32);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+        REPORTER_ASSERT(reporter, mm->countLevels() + 1 == SkMipMap::ComputeLevelCount(32, 32));
+    }
+
+    {
+        SkBitmap bm;
+        bm.allocN32Pixels(1000, 1000);
+        bm.eraseColor(SK_ColorWHITE);
+        SkAutoTUnref<SkMipMap> mm(SkMipMap::Build(bm, nullptr));
+
+        REPORTER_ASSERT(reporter, mm->countLevels() + 1 == SkMipMap::ComputeLevelCount(1000, 1000));
+    }
+}