Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1639693002:
Adding direct access to SkMipMap levels. (Closed)
DescriptionAdding direct access to SkMipMap levels.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1639693002
Committed: https://skia.googlesource.com/skia/+/e2412d5738e4ee37a897cc51996431a4dfb10436
Patch Set 1 #
Total comments: 2
Patch Set 2 : Making the function a static member. #
Total comments: 2
Patch Set 3 : Restricting mipmap level count to isotropic space. #Patch Set 4 : Adding unit tests. Restricting ComputeLevelCount to a minimum size of 2x2, like SkMipMap. Fixing a … #
Total comments: 8
Patch Set 5 : Refactoring MipMap tests. #Patch Set 6 : Fixing an assert. #Patch Set 7 : Adding printf debugging because the buildbot is seeing something different from what I am seeing.p #Patch Set 8 : No longer asserting in cases where the mipmap didn't build. #Patch Set 9 : A test was failing because >= became < without flipping operands. #Patch Set 10 : Is the 1x1 SkMipMap the problem? #Patch Set 11 : The size of the levels is not always what I expect them to be. #Patch Set 12 : Fixing first problem in unit test. #Patch Set 13 : Uncommenting 1x1 test to see if it now causes problems. #Patch Set 14 : Making negative test assert SkMipMap::Build returned null #
Total comments: 4
Patch Set 15 : ComputeLevelCount now returns a number matching the levels of SkMipMap as opposed to the number of … #Patch Set 16 : Rebasing. #Patch Set 17 : Fixing spelling mistake and rebase mistake. #
Total comments: 2
Patch Set 18 : Making the mipmap test less verbose. #
Total comments: 2
Patch Set 19 : Removing comparisons of ComputeLevelCoutn and Build's level coutn, since that comparison is already… #
Messages
Total messages: 66 (25 generated)
Description was changed from ========== Adding direct access to SkMipMap levels. BUG=skia: ========== to ========== Adding direct access to SkMipMap levels. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
cblume@chromium.org changed reviewers: + bsalomon@chromium.org, reed@chromium.org
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-01-26 08:27 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
reed@google.com changed reviewers: + fmalita@chromium.org, reed@google.com
florin, did your fix where we store 2 scale values (x,y) land? https://codereview.chromium.org/1639693002/diff/1/src/core/SkMipMap.h File src/core/SkMipMap.h (right): https://codereview.chromium.org/1639693002/diff/1/src/core/SkMipMap.h#newcode55 src/core/SkMipMap.h:55: int SkGetMipMapLevelCount(int baseWidth, int baseHeight); This should be a static method on SkMipMap.. int SkMipMap::ComputeLevelCount(int w, int h);
On 2016/01/26 14:25:55, reed1 wrote: > florin, did your fix where we store 2 scale values (x,y) land? Yes, Level::fScale is now an SkSize.
https://codereview.chromium.org/1639693002/diff/1/src/core/SkMipMap.h File src/core/SkMipMap.h (right): https://codereview.chromium.org/1639693002/diff/1/src/core/SkMipMap.h#newcode55 src/core/SkMipMap.h:55: int SkGetMipMapLevelCount(int baseWidth, int baseHeight); On 2016/01/26 14:25:55, reed1 wrote: > This should be a static method on SkMipMap.. > > int SkMipMap::ComputeLevelCount(int w, int h); Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
We will want a unittest to exercise the new functions (unittests can access private headers, so that is not a problem). https://codereview.chromium.org/1639693002/diff/20001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/20001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:378: // OpenGL's spec requires that each mipmap level have height/width equal to This is not what we do today, in terms of the number of levels. We do follow the floor() math, but we stop sooner than this comment implies. i.e. calling this function will not necessarily give the same answer as calling mip->countLevels();
On 2016/01/26 18:15:42, reed1 wrote: > This is not what we do today, in terms of the number of levels. We do follow the > floor() math, but we stop sooner than this comment implies. i.e. calling this > function will not necessarily give the same answer as calling > mip->countLevels(); Oh, I see. Yeah, if one axis hits the end we stop, rather than keep going until the other axis also hits the end. This is a behavioral difference between SkMipMap and what OpenGL requires. This may be why I saw bad dimensions earlier and why I moved away from SkMipMap.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
On 2016/01/26 18:15:42, reed1 wrote: > We will want a unittest to exercise the new functions (unittests can access > private headers, so that is not a problem). Done. https://codereview.chromium.org/1639693002/diff/20001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/20001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:378: // OpenGL's spec requires that each mipmap level have height/width equal to On 2016/01/26 18:15:42, reed1 wrote: > This is not what we do today, in terms of the number of levels. We do follow the > floor() math, but we stop sooner than this comment implies. i.e. calling this > function will not necessarily give the same answer as calling > mip->countLevels(); Done. @reed and I spoke and agreed that this should restrict to isotropic space to match what the rest of Skia does. It is a full stack of mipmaps as far as isotropic space is concerned. Once an axis hits size 1 we stop generating mipmaps and do not continue into anisotropic space.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1639693002/diff/60001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/60001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:377: int SkMipMap::ComputeLevelCount(int baseWidth, int baseHeight) { Its a little odd that the Build code doesn't call or share code with ComputeLevelCount. Perhaps they could somehow share code to help ensure they don't get out-of-sync. Or at least, we could assert that the count computed in Build is the same as calling this new method.. perhaps after line 249? https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:57: DEF_TEST(MipMap_DirectLevelAccess, reporter) { Looking above, perhaps we just could augment the existing test to also compare the level-count to the new method ComputeLevelCount? https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:70: // check small mipmap's count and levels can this block be combined with the one below it, since they seem to just diff by the initial dimensions? https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:144: int zeroWidthResult = SkMipMap::ComputeLevelCount(0, 100); 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()); }
https://codereview.chromium.org/1639693002/diff/60001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/60001/src/core/SkMipMap.cpp#n... src/core/SkMipMap.cpp:377: int SkMipMap::ComputeLevelCount(int baseWidth, int baseHeight) { On 2016/01/28 02:06:03, reed1 wrote: > Its a little odd that the Build code doesn't call or share code with > ComputeLevelCount. Perhaps they could somehow share code to help ensure they > don't get out-of-sync. Or at least, we could assert that the count computed in > Build is the same as calling this new method.. perhaps after line 249? I agree. But when I was writing this I realized they have different goals. One wants to find the dimensions of a given level, generating levels as it goes. The other wants to know how many levels it would generate in total. So it sort of makes sense that they wouldn't share much code. I added a few tests to make sure their results are the same. You can see it at https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... on line 205. I also added (in the next patch set) an assert inside Build, since it maintains countLevels as it generates them. Once the count is complete I assert that they match. https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:57: DEF_TEST(MipMap_DirectLevelAccess, reporter) { On 2016/01/28 02:06:03, reed1 wrote: > Looking above, perhaps we just could augment the existing test to also compare > the level-count to the new method ComputeLevelCount? 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.) https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:70: // check small mipmap's count and levels On 2016/01/28 02:06:03, reed1 wrote: > can this block be combined with the one below it, since they seem to just diff > by the initial dimensions? 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. https://codereview.chromium.org/1639693002/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:144: int zeroWidthResult = SkMipMap::ComputeLevelCount(0, 100); On 2016/01/28 02:06:03, reed1 wrote: > 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()); > } 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.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:413: if (NULL == fLevels) { I think we should either assert or return a bool (false) if we get a request we can't fulfill (e.g. index out of range) https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.h File src/core/SkMipMap.h (right): https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.h#ne... src/core/SkMipMap.h:28: static int ComputeLevelCount(int baseWidth, int baseHeight); Its odd that this returns a different value (+1) from countLevels(). Can we make them the same?
https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:413: if (NULL == fLevels) { On 2016/02/05 22:37:11, reed1 wrote: > I think we should either assert or return a bool (false) if we get a request we > can't fulfill (e.g. index out of range) Done. https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.h File src/core/SkMipMap.h (right): https://codereview.chromium.org/1639693002/diff/260001/src/core/SkMipMap.h#ne... src/core/SkMipMap.h:28: static int ComputeLevelCount(int baseWidth, int baseHeight); On 2016/02/05 22:37:11, reed1 wrote: > Its odd that this returns a different value (+1) from countLevels(). Can we make > them the same? Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a full Skia committer
Are there any more changes I should make to this CL?
sorry for the delay. the code/header look good, thanks. just a comment about making the unit-test more easily read/modified. https://codereview.chromium.org/1639693002/diff/320001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/320001/tests/MipMapTest.cpp#n... tests/MipMapTest.cpp:123: int zeroWidthResult = SkMipMap::ComputeLevelCount(0, 100); I bet there is a more succinct way to express this table of expected values, one that will make it easier for the next maintainer to see what is going on, and add/more tests. e.g. const struct { int fWidth; int fHeight; int fExpectedLevelCount; } recs[] = { // negative dimensions should always fail { -100, 100, 0 } , { 100, -100, 0 }, { -100, -100, 0 }, // smallest dim < 2 also fails { 0, 100, 0 }, { 100, 0, 0 }, { 1, 100, 0 }, { 100, 1, 0 }, { 2, 100, 1 }, ... }; for (auto rec : recs) { int levelCount = SkMipMap::ComputeLevelCount(rec.fWidth, rec.fHeight); REPORTER_ASSERT(reporter, rec.fExpectedLevelCount == levelCount); }
https://codereview.chromium.org/1639693002/diff/320001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/320001/tests/MipMapTest.cpp#n... tests/MipMapTest.cpp:123: int zeroWidthResult = SkMipMap::ComputeLevelCount(0, 100); On 2016/02/16 15:53:45, reed1 wrote: > I bet there is a more succinct way to express this table of expected values, one > that will make it easier for the next maintainer to see what is going on, and > add/more tests. > > e.g. > > const struct { > int fWidth; > int fHeight; > int fExpectedLevelCount; > } recs[] = { > // negative dimensions should always fail > { -100, 100, 0 } , > { 100, -100, 0 }, > { -100, -100, 0 }, > > // smallest dim < 2 also fails > { 0, 100, 0 }, > { 100, 0, 0 }, > { 1, 100, 0 }, > { 100, 1, 0 }, > > { 2, 100, 1 }, > ... > }; > > for (auto rec : recs) { > int levelCount = SkMipMap::ComputeLevelCount(rec.fWidth, rec.fHeight); > REPORTER_ASSERT(reporter, rec.fExpectedLevelCount == levelCount); > } Done.
thanks. one last suggestion, to remove more copy/paste code. https://codereview.chromium.org/1639693002/diff/340001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/340001/tests/MipMapTest.cpp#n... tests/MipMapTest.cpp:162: REPORTER_ASSERT(reporter, mm->countLevels() == SkMipMap::ComputeLevelCount(255, 255)); nit: - seems like these instances of creating a bitmap could be rolled into the first auto-loop as well. - they will appear even more similar (correctly) if we use the bitmap to pass the sizes to ComputeLevelCount... SkMipMap::ComputeLevelCount(bm.width(), bm.height()) That will make it pretty clear that each block is identical modulo the width/height used (which you can get from your tests[] table.
https://codereview.chromium.org/1639693002/diff/340001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/1639693002/diff/340001/tests/MipMapTest.cpp#n... tests/MipMapTest.cpp:162: REPORTER_ASSERT(reporter, mm->countLevels() == SkMipMap::ComputeLevelCount(255, 255)); On 2016/02/17 14:29:11, reed1 wrote: > nit: > > - seems like these instances of creating a bitmap could be rolled into the first > auto-loop as well. > - they will appear even more similar (correctly) if we use the bitmap to pass > the sizes to ComputeLevelCount... > > SkMipMap::ComputeLevelCount(bm.width(), bm.height()) > > That will make it pretty clear that each block is identical modulo the > width/height used (which you can get from your tests[] table. I spoke with @reed1 for clarification and we agreed that this section, which compares the result of SkMipMap::Build to SkMipMap::ComputeLevelCount, is already being covered by line 30. So we're removing this section.
lgtm
The CQ bit was checked by cblume@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1639693002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1639693002/360001
Message was sent while issue was closed.
Description was changed from ========== Adding direct access to SkMipMap levels. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Adding direct access to SkMipMap levels. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e2412d5738e4ee37a897cc51996431a4dfb10436 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://skia.googlesource.com/skia/+/e2412d5738e4ee37a897cc51996431a4dfb10436 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
