Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007113008:
Add prescale option to deferred params (Closed)
DescriptionAdd prescale option to deferred params
Currently, Skia always uploads GPU textures at full resolution. This
change allows us to pass a pre-scale mip level to the
deferred texture image logic, which causes us to pre-scale the image
to the given mip level, and upload that mip level instead of the full
image.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2007113008
Committed: https://skia.googlesource.com/skia/+/b4da01d8f719f3c43d492e8f62a7e2c861e9ef27
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebase #Patch Set 3 : feedback #
Total comments: 1
Patch Set 4 : nit #Patch Set 5 : Check for GrContext to prevent crashes when GM test runs on a SW canvas. #
Total comments: 1
Patch Set 6 : feedback and build fixes #Patch Set 7 : build fix #
Messages
Total messages: 35 (19 generated)
Description was changed from ========== Add prescale option to deferred params Currently, Skia always uploads GPU textures at full resolution. This change allows us to pass a pre-scale mip level to the deferred texture image logic, which causes us to pre-scale the image to the given mip level, and upload that mip level instead of the full image. ========== to ========== Add prescale option to deferred params Currently, Skia always uploads GPU textures at full resolution. This change allows us to pass a pre-scale mip level to the deferred texture image logic, which causes us to pre-scale the image to the given mip level, and upload that mip level instead of the full image. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2007113008 ==========
ericrk@chromium.org changed reviewers: + bsalomon@google.com
Here's my first take at a pre-scale API, let me know how this looks. Thanks!
Maybe its time to add a GM test in addition to the unit test since we expect more churn on the implementation of this API. https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:362: const int scaleDivisor = powf(2.0f, lowestPreScaleMipLevel); 1 << lowestPrecaleMipLevel? Should we do some sort of sanity check on the requested levels? https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:364: const int scaledWidth = this->width() / scaleDivisor; I think this should probably be consistent with the way SkMipMap (and OpenGL) rounds level dimensions. Perhaps we should have a helper static function on that class to expose its rounding rules?
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Added a GM test as well. Thanks! https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:362: const int scaleDivisor = powf(2.0f, lowestPreScaleMipLevel); On 2016/05/26 13:12:54, bsalomon wrote: > 1 << lowestPrecaleMipLevel? > > Should we do some sort of sanity check on the requested levels? Using the helper function that now exists in SkMipMap https://codereview.chromium.org/2007113008/diff/1/src/image/SkImage_Gpu.cpp#n... src/image/SkImage_Gpu.cpp:364: const int scaledWidth = this->width() / scaleDivisor; On 2016/05/26 13:12:54, bsalomon wrote: > I think this should probably be consistent with the way SkMipMap (and OpenGL) > rounds level dimensions. Perhaps we should have a helper static function on that > class to expose its rounding rules? Cblume added one, using it now.
lgtm https://codereview.chromium.org/2007113008/diff/100001/gm/deferredtextureimag... File gm/deferredtextureimagedata.cpp (right): https://codereview.chromium.org/2007113008/diff/100001/gm/deferredtextureimag... gm/deferredtextureimagedata.cpp:18: bool DrawDeferredTextureImageData(SkCanvas* canvas, SkImage* image, nit, should be static bool draw_deferred_texture_image_data
The CQ bit was checked by ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2007113008/#ps120001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007113008/120001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by ericrk@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/2007113008/140001
Added a check so we don't try to use deferredtextureimagedata in a SW GM (only GPU GM runs).
https://codereview.chromium.org/2007113008/diff/140001/gm/deferredtextureimag... File gm/deferredtextureimagedata.cpp (right): https://codereview.chromium.org/2007113008/diff/140001/gm/deferredtextureimag... gm/deferredtextureimagedata.cpp:41: return; Can you make this do GM::DrawGpuOnlyMessage(canvas) before returning?
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 ericrk@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/2007113008/160001
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...)
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
The CQ bit was checked by ericrk@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/2007113008/200001
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 ericrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2007113008/#ps220001 (title: "build fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007113008/220001
Message was sent while issue was closed.
Description was changed from ========== Add prescale option to deferred params Currently, Skia always uploads GPU textures at full resolution. This change allows us to pass a pre-scale mip level to the deferred texture image logic, which causes us to pre-scale the image to the given mip level, and upload that mip level instead of the full image. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2007113008 ========== to ========== Add prescale option to deferred params Currently, Skia always uploads GPU textures at full resolution. This change allows us to pass a pre-scale mip level to the deferred texture image logic, which causes us to pre-scale the image to the given mip level, and upload that mip level instead of the full image. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2007113008 Committed: https://skia.googlesource.com/skia/+/b4da01d8f719f3c43d492e8f62a7e2c861e9ef27 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:220001) as https://skia.googlesource.com/skia/+/b4da01d8f719f3c43d492e8f62a7e2c861e9ef27 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
