Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018283002:
Add getting the size of a given mipmap level. (Closed)
DescriptionAdd getting the size of a given mipmap level.
When allocating memory for mipmaps, it would be very handy to know the
dimensions of a given mipmap level.
R=bsalomon@google.com
BUG=578304
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2018283002
Committed: https://skia.googlesource.com/skia/+/f95ff4a37aba42a07b0fed9ee760d1a238c6d291
Patch Set 1 #Patch Set 2 : Fixing missing include. #Patch Set 3 : Merged updates from base branch. #Patch Set 4 : Adding unit tests. #Patch Set 5 : Renaming the test. #Patch Set 6 : Fixing unit test. #Patch Set 7 : Using SkMipMap::ComputeLevel* functions within SkMipMap::Build. #
Total comments: 2
Patch Set 8 : Removing loop for getting the size of a given mipmap level. #
Total comments: 2
Patch Set 9 : Right shifting instead of dividing. #
Messages
Total messages: 54 (25 generated)
Description was changed from ========== Add getting the size of a given mipmap level. When allocating memory for mipmaps, it would be very handy to know the dimensions of a given mipmap level. R=bsalomon@google.com BUG=578304 ========== to ========== Add getting the size of a given mipmap level. When allocating memory for mipmaps, it would be very handy to know the dimensions of a given mipmap level. R=bsalomon@google.com BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2018283002 ==========
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/2018283002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/1
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)
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/2018283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
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/2018283002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
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/2018283002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2018283002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/60001
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/2018283002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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...)
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/2018283002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reed@google.com changed reviewers: + reed@google.com
1. Who is going to call this? Since its private, I would expect to see a caller inside skia. 2. Can we try to share code with the other site where we compute this (as we compute the sizes of all the levels? If not, let's at least try to make the code/technique more similar for readability. Also, if we can't share code, we could try calling this helper inside the existing loop (in debug code) to ensure that the two code snippets always compute the same value.
On 2016/05/31 12:48:09, reed1 wrote: > 1. Who is going to call this? Since its private, I would expect to see a caller > inside skia. The caller will indeed be inside Skia. I am working on CC's GPU image decode controller. In it, it calls getDeferredTextureImageData and MakeFromDeferredTextureImageData: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/i... Those calls create an opaque object to CC. But Skia knows what it is. We need to add mipmap support there. We can use SkMipMap to generate the data but not to store it -- CC allocates from ASHMEM so SkMipMap's destructor wouldn't get called. I am trying to break this into smaller chunks to make the CLs easier to review and to keep the ball rolling. Another CL related to this is here: https://codereview.chromium.org/2023573002/ Next I'll call into SkMipMap to generate and store those results. Then a fourth CL will add plumbing to Gr_____ to upload a pre-generated mipmap. > 2. Can we try to share code with the other site where we compute this (as we > compute the sizes of all the levels? If not, let's at least try to make the > code/technique more similar for readability. Also, if we can't share code, we > could try calling this helper inside the existing loop (in debug code) to ensure > that the two code snippets always compute the same value. I do agree with sharing code with ComputeLevelCount, if that is what you mean. Although perhaps you mean inside Build. Build needs to compute the size of each level. I think it would be wise for Build to call these.
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/2018283002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)
https://codereview.chromium.org/2018283002/diff/120001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/2018283002/diff/120001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:518: for (int currentMipLevel = 0; currentMipLevel <= level; currentMipLevel++) { Can we do this without a loop? width = max(1, baseWidth >> level);
https://codereview.chromium.org/2018283002/diff/120001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/2018283002/diff/120001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:518: for (int currentMipLevel = 0; currentMipLevel <= level; currentMipLevel++) { On 2016/06/01 12:55:56, reed1 wrote: > Can we do this without a loop? > > width = max(1, baseWidth >> level); Oh. Yeah. That's the whole benefit of this method over just >> 1 each loop iteration. Whoops. Fixed. :)
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/2018283002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nit about shifts https://codereview.chromium.org/2018283002/diff/140001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/2018283002/diff/140001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:516: int width = SkTMax(1, baseWidth / (1 << level)); why do we write this with a shift and divide, instead of just a shift? baseWidth >> level;
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/2018283002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/160001
https://codereview.chromium.org/2018283002/diff/140001/src/core/SkMipMap.cpp File src/core/SkMipMap.cpp (right): https://codereview.chromium.org/2018283002/diff/140001/src/core/SkMipMap.cpp#... src/core/SkMipMap.cpp:516: int width = SkTMax(1, baseWidth / (1 << level)); On 2016/06/01 19:36:17, reed1 wrote: > why do we write this with a shift and divide, instead of just a shift? > > baseWidth >> level; Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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/2018283002/#ps160001 (title: "Right shifting instead of dividing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018283002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018283002/160001
Message was sent while issue was closed.
Description was changed from ========== Add getting the size of a given mipmap level. When allocating memory for mipmaps, it would be very handy to know the dimensions of a given mipmap level. R=bsalomon@google.com BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2018283002 ========== to ========== Add getting the size of a given mipmap level. When allocating memory for mipmaps, it would be very handy to know the dimensions of a given mipmap level. R=bsalomon@google.com BUG=578304 GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2018283002 Committed: https://skia.googlesource.com/skia/+/f95ff4a37aba42a07b0fed9ee760d1a238c6d291 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://skia.googlesource.com/skia/+/f95ff4a37aba42a07b0fed9ee760d1a238c6d291 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
