Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2042843005:
SkMipMap::ComputeLevelSize should only cover SkMipMap's levels. (Closed)
DescriptionSkMipMap::ComputeLevelSize should only cover SkMipMap's levels.
SkMipMap only deals with the levels it generates.
That is to day, it deals with mipmap levels 1-x, not 0-x.
Other functions reflect thing when indexing.
They go from 0 to x-1 (giving the index into SkMipMap's contents).
ComputeLevelSize should also follow that same indexing.
BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2042843005
Committed: https://skia.googlesource.com/skia/+/609623b47f5722a4d3a4dbb5103736fe70ae1878
Patch Set 1 #Patch Set 2 : Parens around rhs of >> #Patch Set 3 : Fixing test #Patch Set 4 : Adding more comments. #
Total comments: 2
Patch Set 5 : Adding asserts to the unit tests. #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== SkMipMap::ComputeLevelSize should only cover SkMipMap's levels. SkMipMap only deals with the levels it generates. That is to day, it deals with mipmap levels 1-x, not 0-x. Other functions reflect thing when indexing. They go from 0 to x-1 (giving the index into SkMipMap's contents). ComputeLevelSize should also follow that same indexing. BUG=578304 ========== to ========== SkMipMap::ComputeLevelSize should only cover SkMipMap's levels. SkMipMap only deals with the levels it generates. That is to day, it deals with mipmap levels 1-x, not 0-x. Other functions reflect thing when indexing. They go from 0 to x-1 (giving the index into SkMipMap's contents). ComputeLevelSize should also follow that same indexing. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2042843005 ==========
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/2042843005/1
cblume@chromium.org changed reviewers: + reed@google.com
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-ShuttleA-GPU...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
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/2042843005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
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/2042843005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
trying to understand this...
On 2016/06/08 20:21:32, reed1 wrote: > trying to understand this... Oh sorry, I'll try to explain better. Many SkMipMap functions are based on only the levels that SkMipMap generates. An example of this is ComputeLevelCount. It computes the number of levels that SkMipMap will generate. So if I have an image which is 8x8, I'll end up generating: 1.) 4x4 2.) 2x2 3.) 1x1 ComputeLevelCount will return 3. But there are actually 4 levels total. 8x8 is the base level. Since SkMipMap is not generating/dealing with that level, it is not included. All of SkMipMap's access is based around this. Normally, the base level is index 0. But for SkMipMap, index 0 is the first generated mipmap level (level 1). I had written ComputeLevelSize to treat level 0 as the base level, not as the first generated mipmap level. This means it didn't really align with all the rest of how SkMipMap indexed. So you'll see changes where I am treating index 0 as level 1.
Lets update the comment on the .h to make this clearer.
+ add dox for getLevel() so we see that both functions follow the same (0-based) convention.
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/2042843005/60001
I added documentation in the header that I hope is concise and useful.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2042843005/diff/60001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/2042843005/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:72: Here would be an interesting place to call ComputeLevelSize, to assert that it matches the level returned by getLevel().
The CQ bit was checked by cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com Link to the patchset: https://codereview.chromium.org/2042843005/#ps80001 (title: "Adding asserts to the unit tests.")
https://codereview.chromium.org/2042843005/diff/60001/tests/MipMapTest.cpp File tests/MipMapTest.cpp (right): https://codereview.chromium.org/2042843005/diff/60001/tests/MipMapTest.cpp#ne... tests/MipMapTest.cpp:72: On 2016/06/09 10:31:26, reed1 wrote: > Here would be an interesting place to call ComputeLevelSize, to assert that it > matches the level returned by getLevel(). Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2042843005/80001
Message was sent while issue was closed.
Description was changed from ========== SkMipMap::ComputeLevelSize should only cover SkMipMap's levels. SkMipMap only deals with the levels it generates. That is to day, it deals with mipmap levels 1-x, not 0-x. Other functions reflect thing when indexing. They go from 0 to x-1 (giving the index into SkMipMap's contents). ComputeLevelSize should also follow that same indexing. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2042843005 ========== to ========== SkMipMap::ComputeLevelSize should only cover SkMipMap's levels. SkMipMap only deals with the levels it generates. That is to day, it deals with mipmap levels 1-x, not 0-x. Other functions reflect thing when indexing. They go from 0 to x-1 (giving the index into SkMipMap's contents). ComputeLevelSize should also follow that same indexing. BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2042843005 Committed: https://skia.googlesource.com/skia/+/609623b47f5722a4d3a4dbb5103736fe70ae1878 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/609623b47f5722a4d3a4dbb5103736fe70ae1878 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
