Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2034933003:
Store mipmap levels in deferred texture image (Closed)
DescriptionStore mipmap levels in deferred texture image
When creating the deferred texture image, detect if using medium / high
quality. If so, generate and store mipmaps in the deferred texture
image.
When creating a texture from that be sure to read it back out.
BUG=578304
R=bsalomon@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2034933003
Committed: https://skia.googlesource.com/skia/+/b3105190a6e02d37f1d7f07a3a8bdd368ec7f157
Patch Set 1 #Patch Set 2 : Keeping support for non-mipmapped images the same. #
Total comments: 6
Patch Set 3 : Code review comments & fixing build on VC. #
Total comments: 10
Patch Set 4 : Addressing CR comments. #
Total comments: 8
Patch Set 5 : Fixing comments. #Patch Set 6 : Adding gm test. #Patch Set 7 : Adding the ability to respect gamma. #Patch Set 8 : Making the new parameter a default so the roll into Chrome won't break. #Patch Set 9 : Fixing Windows build. #
Total comments: 9
Patch Set 10 : Code review comments -- style #Patch Set 11 : Rebasing and refactoring. #
Total comments: 2
Patch Set 12 : Storing the gamma treatment in the deferred texture image. #
Total comments: 5
Patch Set 13 : Using scaled size. #Patch Set 14 : Another scaled image. #Patch Set 15 : Using scaled size. #
Messages
Total messages: 92 (34 generated)
Description was changed from ========== Store mipmap levels in deferred texture image When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 R=bsalomon@google.com ========== to ========== Store mipmap levels in deferred texture image When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 R=bsalomon@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2034933003 ==========
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/2034933003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2034933003/1
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
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/2034933003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2034933003/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...)
https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:398: sk_sp<SkImage> MakeTextureFromMipMap(GrContext*, const SkImageInfo&, const GrMipLevel* texels, This is from the other CL, right? https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:415: minScaleFactor != -1.f && minScaleFactor < 1.f) { Do we not use mip maps when the matrix has perspective? https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:476: memcpy(reinterpret_cast<void*>(mipLevelPtr), currentMipMapLevel.fPixmap.addr(), It's too bad we have to memcpy here. Should we extend the mip builder to allow building in client provided memory?
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/2034933003/40001
https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage.cpp File src/image/SkImage.cpp (right): https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage.cpp#n... src/image/SkImage.cpp:398: sk_sp<SkImage> MakeTextureFromMipMap(GrContext*, const SkImageInfo&, const GrMipLevel* texels, On 2016/06/03 14:43:33, bsalomon wrote: > This is from the other CL, right? Yeah. Whoops. https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:415: minScaleFactor != -1.f && minScaleFactor < 1.f) { On 2016/06/03 14:43:33, bsalomon wrote: > Do we not use mip maps when the matrix has perspective? Hrmmm I think any time perspective is involved it is likely being used in a regular 3d sense, which means mipmaps would be fitting imho. I'll add that. https://codereview.chromium.org/2034933003/diff/20001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:476: memcpy(reinterpret_cast<void*>(mipLevelPtr), currentMipMapLevel.fPixmap.addr(), On 2016/06/03 14:43:33, bsalomon wrote: > It's too bad we have to memcpy here. Should we extend the mip builder to allow > building in client provided memory? I agree @ memcpy. I would love to add a CL that modifies SkMipMap allowing user-specified memory. I'll do that in a follow-up CL then have a second follow-up CL which modifies this call site.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ericrk@chromium.org changed reviewers: + ericrk@chromium.org
https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:396: int mipMapLevelCount = 1; This is a bit misleading - if we are using mips, mipMapLevelCount is equal to the number of levels excluding the base level, if we are not using mips, mipMapLevelCount is equal to 1 (the number of levels including the base level). Why not make this uniform? https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:397: for (int currentParamIndex = paramCnt; currentParamIndex > 0; currentParamIndex--) { I'm going to need to extract things from params (and I expect others will in the future) - might be nicer to put this at the top and extract all relevant values for params - especially if we need these in both branches - we could even separate into a helper function (but feel free to do that later, or I can in my patch). https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:398: const SkMatrix& currentMatrix = params[currentParamIndex].fMatrix; won't this index out of bounds and skip the 0th param - I expect params to be zero indexed and to go up to paramCnt-1. Why not just for (int currentParamIndex = 0; currentParamIndex < paramCnt; ++currentParamIndex)? https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:400: if (currentMatrix.hasPerspective() || Can you add a comment explaining the logic here (it seems correct, but a comment would be nice for future reference). Also, it *might* be nicer to break out some of this into individual bools rather than having one complex if, but up to you. https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:456: for (int currentMipMapLevelIndex = 1; currentMipMapLevelIndex < mipMapLevelCount; Wont this skip the last mip level? when computed by SkMipMap::ComputeLevelCount, this number excludes the base level, so souldn't you really want to generate levels 1 through mipLevelCount inclusive?
Seems like we should have a GM test that creates a SkImage using this API and then draws it in ways that will trigger showing all the various MIP levels. Perhaps with a normally created SkImage side-by-side so we can see that they draw the same.
On 2016/06/03 18:49:37, bsalomon wrote: > Seems like we should have a GM test that creates a SkImage using this API and > then draws it in ways that will trigger showing all the various MIP levels. > Perhaps with a normally created SkImage side-by-side so we can see that they > draw the same. I think this is a great idea. Where can I look for GM tests so I can add this? https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:396: int mipMapLevelCount = 1; On 2016/06/03 18:05:45, ericrk wrote: > This is a bit misleading - if we are using mips, mipMapLevelCount is equal to > the number of levels excluding the base level, if we are not using mips, > mipMapLevelCount is equal to 1 (the number of levels including the base level). > Why not make this uniform? Hrmmm perhaps I made a mistake. The way it *should* be is that there is always 1 level (index 0). Levels indexed 1-x only exist if there are mipmaps. These are the ones generated by SkMipMap. I'll double check myself and fix it if I did this wrong. It should be uniform. https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:397: for (int currentParamIndex = paramCnt; currentParamIndex > 0; currentParamIndex--) { On 2016/06/03 18:05:45, ericrk wrote: > I'm going to need to extract things from params (and I expect others will in the > future) - might be nicer to put this at the top and extract all relevant values > for params - especially if we need these in both branches - we could even > separate into a helper function (but feel free to do that later, or I can in my > patch). Good idea. I'll put this at the top and see how it looks. I may then do a follow-up patch to refactor it if I think of a lovely way. https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:398: const SkMatrix& currentMatrix = params[currentParamIndex].fMatrix; On 2016/06/03 18:05:45, ericrk wrote: > won't this index out of bounds and skip the 0th param - I expect params to be > zero indexed and to go up to paramCnt-1. Why not just > > for (int currentParamIndex = 0; currentParamIndex < paramCnt; > ++currentParamIndex)? Hrmm it will indeed skip the 0th. It should be for(int currentParamIndex = paramCnt - 1; currentParamIndex >= 0, ...). When the code is simple enough, I prefer to have comparisons against 0. 0 is a special value for CPUs almost always, resulting in more simple machine code. It is unlikely to matter here as this isn't exactly a performance-critical area. But it is a good general habit if it makes no difference to readability. If you think it is less readable I'm happy to iterate up. https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:400: if (currentMatrix.hasPerspective() || On 2016/06/03 18:05:45, ericrk wrote: > Can you add a comment explaining the logic here (it seems correct, but a comment > would be nice for future reference). Also, it *might* be nicer to break out some > of this into individual bools rather than having one complex if, but up to you. I agree 100% on the individual bools. That might actually make it more readable anyway to the point where a comment is no longer helpful (just restating what the code clearly says). I'll do that. https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:456: for (int currentMipMapLevelIndex = 1; currentMipMapLevelIndex < mipMapLevelCount; On 2016/06/03 18:05:45, ericrk wrote: > Wont this skip the last mip level? when computed by SkMipMap::ComputeLevelCount, > this number excludes the base level, so souldn't you really want to generate > levels 1 through mipLevelCount inclusive? Good catch.
On 2016/06/03 18:57:31, cblume wrote: > On 2016/06/03 18:49:37, bsalomon wrote: > > Seems like we should have a GM test that creates a SkImage using this API and > > then draws it in ways that will trigger showing all the various MIP levels. > > Perhaps with a normally created SkImage side-by-side so we can see that they > > draw the same. > > I think this is a great idea. > Where can I look for GM tests so I can add this? Under gm/ in the Skia root. > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.cpp > File src/image/SkImage_Gpu.cpp (right): > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... > src/image/SkImage_Gpu.cpp:396: int mipMapLevelCount = 1; > On 2016/06/03 18:05:45, ericrk wrote: > > This is a bit misleading - if we are using mips, mipMapLevelCount is equal to > > the number of levels excluding the base level, if we are not using mips, > > mipMapLevelCount is equal to 1 (the number of levels including the base > level). > > Why not make this uniform? > > Hrmmm perhaps I made a mistake. > The way it *should* be is that there is always 1 level (index 0). > Levels indexed 1-x only exist if there are mipmaps. These are the ones generated > by SkMipMap. > > I'll double check myself and fix it if I did this wrong. It should be uniform. > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... > src/image/SkImage_Gpu.cpp:397: for (int currentParamIndex = paramCnt; > currentParamIndex > 0; currentParamIndex--) { > On 2016/06/03 18:05:45, ericrk wrote: > > I'm going to need to extract things from params (and I expect others will in > the > > future) - might be nicer to put this at the top and extract all relevant > values > > for params - especially if we need these in both branches - we could even > > separate into a helper function (but feel free to do that later, or I can in > my > > patch). > > Good idea. I'll put this at the top and see how it looks. I may then do a > follow-up patch to refactor it if I think of a lovely way. > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... > src/image/SkImage_Gpu.cpp:398: const SkMatrix& currentMatrix = > params[currentParamIndex].fMatrix; > On 2016/06/03 18:05:45, ericrk wrote: > > won't this index out of bounds and skip the 0th param - I expect params to be > > zero indexed and to go up to paramCnt-1. Why not just > > > > for (int currentParamIndex = 0; currentParamIndex < paramCnt; > > ++currentParamIndex)? > > Hrmm it will indeed skip the 0th. It should be for(int currentParamIndex = > paramCnt - 1; currentParamIndex >= 0, ...). > > When the code is simple enough, I prefer to have comparisons against 0. 0 is a > special value for CPUs almost always, resulting in more simple machine code. It > is unlikely to matter here as this isn't exactly a performance-critical area. > But it is a good general habit if it makes no difference to readability. If you > think it is less readable I'm happy to iterate up. > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... > src/image/SkImage_Gpu.cpp:400: if (currentMatrix.hasPerspective() || > On 2016/06/03 18:05:45, ericrk wrote: > > Can you add a comment explaining the logic here (it seems correct, but a > comment > > would be nice for future reference). Also, it *might* be nicer to break out > some > > of this into individual bools rather than having one complex if, but up to > you. > > I agree 100% on the individual bools. That might actually make it more readable > anyway to the point where a comment is no longer helpful (just restating what > the code clearly says). I'll do that. > > https://codereview.chromium.org/2034933003/diff/40001/src/image/SkImage_Gpu.c... > src/image/SkImage_Gpu.cpp:456: for (int currentMipMapLevelIndex = 1; > currentMipMapLevelIndex < mipMapLevelCount; > On 2016/06/03 18:05:45, ericrk wrote: > > Wont this skip the last mip level? when computed by > SkMipMap::ComputeLevelCount, > > this number excludes the base level, so souldn't you really want to generate > > levels 1 through mipLevelCount inclusive? > > Good catch.
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/2034933003/60001
I believe I have addressed the comments. I will add a GM test in a separate CL. I am trying to keep these CLs as small and independent as I can. If we want, I can wrap the mip-gen portion if a #if 0 until the GM test is ready.
On 2016/06/06 22:44:00, cblume wrote: > I believe I have addressed the comments. > > I will add a GM test in a separate CL. I am trying to keep these CLs as small > and independent as I can. > > If we want, I can wrap the mip-gen portion if a #if 0 until the GM test is > ready. I'd really prefer we land tests/code at the same time - this prevents landing code that doesn't work. Keeping CLs small is a good goal, but I wouldn't really count test code size in the "size" of a CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for fixing things up. Looks really close! https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:368: if (minAxisScale != -1.f) { nit - just && this and the following if to reduce nesting? https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:420: mipMapLevelCount = SkMipMap::ComputeLevelCount(width(), height()) + 1; nit: thanks for fixing this up - please add a comment explaining the +1 https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:422: // SkMipMap will generate nit: comment isn't finished? https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:427: pixelSize += SkAlign8(SkAutoPixmapStorage::AllocSize(mipInfo, nullptr)); This will add in the size of level 0... isn't level 0 already accounted for int he above if/else (line 393/408) - should this loop be > 0?
bsalomon - I looked through a handful of the tests in gm/ . Do you happen to know if there is one similar that I can use as a starting point? I think I'll also add some regular unit tests. https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:368: if (minAxisScale != -1.f) { On 2016/06/06 22:57:40, ericrk wrote: > nit - just && this and the following if to reduce nesting? Done. https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:420: mipMapLevelCount = SkMipMap::ComputeLevelCount(width(), height()) + 1; On 2016/06/06 22:57:40, ericrk wrote: > nit: thanks for fixing this up - please add a comment explaining the +1 Done. https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:422: // SkMipMap will generate On 2016/06/06 22:57:40, ericrk wrote: > nit: comment isn't finished? Done. https://codereview.chromium.org/2034933003/diff/60001/src/image/SkImage_Gpu.c... src/image/SkImage_Gpu.cpp:427: pixelSize += SkAlign8(SkAutoPixmapStorage::AllocSize(mipInfo, nullptr)); On 2016/06/06 22:57:40, ericrk wrote: > This will add in the size of level 0... isn't level 0 already accounted for int > he above if/else (line 393/408) - should this loop be > 0? We need both, I believe. 393/408 gets the size of the base level. This loop gets the size of the generated (non-base) levels. The 0 here refers to the index 0 of SkMipMap's generated stuff. I originally wrote SkMipMap code which used 1-based indexing because all uses I can think of will include the base level as index 0. But it is awkward to have one special class with a special rule, so we got rid of it. I believe my unfinished comment above (whoops) was supposed to describe that.
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/2034933003/80001
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-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 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/2034933003/120001
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/2034933003/140001
GM test added. I will merge it with Eric's patch once that lands, since I expect his will land first. Also, SkMipMap just gained a parameter for respecting gamma. I'm including that parameter here. I am giving it a default value so it can land in Chrome without breaking Chrome. I can then later remove the default after Chrome had a chance to update the params it passes in.
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/2034933003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
bsalomon@google.com changed reviewers: + brianosman@google.com
https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:2: * Copyright 2015 Google Inc. 2016 https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:42: params[0].fQuality =kMedium_SkFilterQuality; space after = https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... include/core/SkImage.h:383: SkSourceGammaTreatment treatment = Should this be here or part of DeferredTextureImageUsageParams? Also, I think the default you want for Chrome is kIgnore not kRepect. +brianosman https://codereview.chromium.org/2034933003/diff/160001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/160001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:459: SkISize mipSize = SkMipMap::ComputeLevelSize(width(), height(), currentMipMapLevelIndex); nit: wrap at 100col and this->width()/this->height()
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/2034933003/180001
https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... File gm/deferredtextureimage.cpp (right): https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:2: * Copyright 2015 Google Inc. On 2016/06/13 14:11:39, bsalomon wrote: > 2016 Done. https://codereview.chromium.org/2034933003/diff/160001/gm/deferredtextureimag... gm/deferredtextureimage.cpp:42: params[0].fQuality =kMedium_SkFilterQuality; On 2016/06/13 14:11:39, bsalomon wrote: > space after = Done. https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... include/core/SkImage.h:383: SkSourceGammaTreatment treatment = On 2016/06/13 14:11:39, bsalomon wrote: > Should this be here or part of DeferredTextureImageUsageParams? Also, I think > the default you want for Chrome is kIgnore not kRepect. +brianosman Sounds good. I'll wait to hear from brianosman@. For the time being, I changed it to kIgnore https://codereview.chromium.org/2034933003/diff/160001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/160001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:459: SkISize mipSize = SkMipMap::ComputeLevelSize(width(), height(), currentMipMapLevelIndex); On 2016/06/13 14:11:39, bsalomon wrote: > nit: wrap at 100col and this->width()/this->height() Done.
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-Release-Shared-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h File include/core/SkImage.h (right): https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... include/core/SkImage.h:383: SkSourceGammaTreatment treatment = On 2016/06/13 14:11:39, bsalomon wrote: > Should this be here or part of DeferredTextureImageUsageParams? Also, I think > the default you want for Chrome is kIgnore not kRepect. +brianosman Yes, kIgnore will match existing (legacy) behavior.
On 2016/06/13 18:22:09, Brian Osman wrote: > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h > File include/core/SkImage.h (right): > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... > include/core/SkImage.h:383: SkSourceGammaTreatment treatment = > On 2016/06/13 14:11:39, bsalomon wrote: > > Should this be here or part of DeferredTextureImageUsageParams? Also, I think > > the default you want for Chrome is kIgnore not kRepect. +brianosman > > Yes, kIgnore will match existing (legacy) behavior. Do you think it should be part of DeferredTextureImageUsageParams? I'm thinking it shouldn't. I cannot think of a situation where we want the image sometimes to be gamma corrected and sometimes not. As I understand it, either everything is completely gamma corrected or everything is completely not gamma corrected.
On 2016/06/13 20:39:48, cblume wrote: > On 2016/06/13 18:22:09, Brian Osman wrote: > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h > > File include/core/SkImage.h (right): > > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... > > include/core/SkImage.h:383: SkSourceGammaTreatment treatment = > > On 2016/06/13 14:11:39, bsalomon wrote: > > > Should this be here or part of DeferredTextureImageUsageParams? Also, I > think > > > the default you want for Chrome is kIgnore not kRepect. +brianosman > > > > Yes, kIgnore will match existing (legacy) behavior. > > Do you think it should be part of DeferredTextureImageUsageParams? > > I'm thinking it shouldn't. I cannot think of a situation where we want the image > sometimes to be gamma corrected and sometimes not. As I understand it, either > everything is completely gamma corrected or everything is completely not gamma > corrected. Oh also, I am making this a defaulted parameter right now so when it lands in Chrome it doesn't break the build. I plan to then land a patch in Chrome to set the param value, and then to remove the default value. But if there are other people using Skia and calling the deferred texture image functions I will break them. Do we know if maybe Android is calling these so we can prevent breaking their build? Anyone else? Thanks!
On 2016/06/13 20:39:48, cblume wrote: > On 2016/06/13 18:22:09, Brian Osman wrote: > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h > > File include/core/SkImage.h (right): > > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... > > include/core/SkImage.h:383: SkSourceGammaTreatment treatment = > > On 2016/06/13 14:11:39, bsalomon wrote: > > > Should this be here or part of DeferredTextureImageUsageParams? Also, I > think > > > the default you want for Chrome is kIgnore not kRepect. +brianosman > > > > Yes, kIgnore will match existing (legacy) behavior. > > Do you think it should be part of DeferredTextureImageUsageParams? > > I'm thinking it shouldn't. I cannot think of a situation where we want the image > sometimes to be gamma corrected and sometimes not. As I understand it, either > everything is completely gamma corrected or everything is completely not gamma > corrected. When drawing to a "legacy", non-gamma correct, SkCanvas we ignore the srgb-ness of the src image. If we didn't ignore it we would wind up decoding from srgb to linear when reading from the texture but not re-encoding to srgb before writing to the canvas's backing store. So in legacy mode we ignore the srgb tagging of the input image. That's why kIgnore is the right default as Chrome doesn't (yet) render to gamma-correct SkCanvases. The case where you'd need different values would be when drawing the same image to two canvases: one srgb-correct and one not. Perhaps its not an important case to consider.
On 2016/06/13 20:47:16, bsalomon wrote: > On 2016/06/13 20:39:48, cblume wrote: > > On 2016/06/13 18:22:09, Brian Osman wrote: > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h > > > File include/core/SkImage.h (right): > > > > > > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... > > > include/core/SkImage.h:383: SkSourceGammaTreatment treatment = > > > On 2016/06/13 14:11:39, bsalomon wrote: > > > > Should this be here or part of DeferredTextureImageUsageParams? Also, I > > think > > > > the default you want for Chrome is kIgnore not kRepect. +brianosman > > > > > > Yes, kIgnore will match existing (legacy) behavior. > > > > Do you think it should be part of DeferredTextureImageUsageParams? > > > > I'm thinking it shouldn't. I cannot think of a situation where we want the > image > > sometimes to be gamma corrected and sometimes not. As I understand it, either > > everything is completely gamma corrected or everything is completely not gamma > > corrected. > > When drawing to a "legacy", non-gamma correct, SkCanvas we ignore the srgb-ness > of the src image. If we didn't ignore it we would wind up decoding from srgb to > linear when reading from the texture but not re-encoding to srgb before writing > to the canvas's backing store. So in legacy mode we ignore the srgb tagging of > the input image. That's why kIgnore is the right default as Chrome doesn't (yet) > render to gamma-correct SkCanvases. > > The case where you'd need different values would be when drawing the same image > to two canvases: one srgb-correct and one not. Perhaps its not an important case > to consider. Okay, I see what you are saying. So from the perspective of Chrome we won't have two canvases, I think. But from the perspective of Skia as a library it is possible. I'll add it to the deferred texture params. And I'll give it a default value in the constructor.
On 2016/06/13 20:54:27, cblume wrote: > On 2016/06/13 20:47:16, bsalomon wrote: > > On 2016/06/13 20:39:48, cblume wrote: > > > On 2016/06/13 18:22:09, Brian Osman wrote: > > > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h > > > > File include/core/SkImage.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2034933003/diff/160001/include/core/SkImage.h... > > > > include/core/SkImage.h:383: SkSourceGammaTreatment treatment = > > > > On 2016/06/13 14:11:39, bsalomon wrote: > > > > > Should this be here or part of DeferredTextureImageUsageParams? Also, I > > > think > > > > > the default you want for Chrome is kIgnore not kRepect. +brianosman > > > > > > > > Yes, kIgnore will match existing (legacy) behavior. > > > > > > Do you think it should be part of DeferredTextureImageUsageParams? > > > > > > I'm thinking it shouldn't. I cannot think of a situation where we want the > > image > > > sometimes to be gamma corrected and sometimes not. As I understand it, > either > > > everything is completely gamma corrected or everything is completely not > gamma > > > corrected. > > > > When drawing to a "legacy", non-gamma correct, SkCanvas we ignore the > srgb-ness > > of the src image. If we didn't ignore it we would wind up decoding from srgb > to > > linear when reading from the texture but not re-encoding to srgb before > writing > > to the canvas's backing store. So in legacy mode we ignore the srgb tagging of > > the input image. That's why kIgnore is the right default as Chrome doesn't > (yet) > > render to gamma-correct SkCanvases. > > > > The case where you'd need different values would be when drawing the same > image > > to two canvases: one srgb-correct and one not. Perhaps its not an important > case > > to consider. > > Okay, I see what you are saying. > So from the perspective of Chrome we won't have two canvases, I think. But from > the perspective of Skia as a library it is possible. > I'll add it to the deferred texture params. And I'll give it a default value in > the constructor. Yes. The big piece that may not be obvious here: Skia handles this by re-building the mips (on the thread that's doing drawing), pipelined with GPU work, whenever we detect that the mip-maps we last uploaded are incorrect for the upcoming draw. So: You could add it to the params, and build the mips according to the first value you see, but for any sequence of draws that contains both values, we're still going to end up doing work on the GPU/driver to re-build them. In practice, this isn't really going to happen, so you can still get the benefits of pre-building the mips, but it may be best to just make the gamma treatment a parameter of the function, rather than per-usage struct, because we can't really honor it at that time anyway.
> Yes. The big piece that may not be obvious here: Skia handles this by > re-building > the mips (on the thread that's doing drawing), pipelined with GPU work, whenever > we detect that the mip-maps we last uploaded are incorrect for the upcoming > draw. > > So: You could add it to the params, and build the mips according to the first > value you see, but for any sequence of draws that contains both values, we're > still going to end up doing work on the GPU/driver to re-build them. > > In practice, this isn't really going to happen, so you can still get the > benefits > of pre-building the mips, but it may be best to just make the gamma treatment > a parameter of the function, rather than per-usage struct, because we can't > really > honor it at that time anyway. Oh boy. So the choice is either: 1.) have an incorrect API (function param), or 2.) have a correct API but an incorrect behavior. It sounds like the only option to have correct behavior would be for multiple textures to be generated and stored in the SkImage. I don't know if we would want that. Given the two options above, I'm inclined to have a not-perfect API that more accurately represents behavior. Later, if we generate multiple textures, the better API makes more sense.
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/2034933003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL) Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_TIMED_OUT, no build URL)
https://codereview.chromium.org/2034933003/diff/200001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/200001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:629: return sk_make_sp<SkImage_Gpu>(texture->width(), texture->height(), kNeedNewImageUniqueID, FYI: Ganesh needs to know the gamma treatment value that was used to build the mips (or it will assume that it's kIgnore by default). So you probably want to store it in the DeferredTextureImage, and then plumb it to here from MakeFromDeferredTextureImageData. Then just throw in: texture->texturePriv().setGammaTreatment(treatment)
https://codereview.chromium.org/2034933003/diff/200001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/200001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:629: return sk_make_sp<SkImage_Gpu>(texture->width(), texture->height(), kNeedNewImageUniqueID, On 2016/06/14 12:10:51, Brian Osman wrote: > FYI: Ganesh needs to know the gamma treatment value that was used to build the > mips (or it will assume that it's kIgnore by default). So you probably want to > store it in the DeferredTextureImage, and then plumb it to here from > MakeFromDeferredTextureImageData. Then just throw in: > > texture->texturePriv().setGammaTreatment(treatment) Done. Should we also be thinking about gamma treatment for the non-mipmap cases? Maybe in a separate CL. Or maybe when the time is right?
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/2034933003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ping
One comment, otherwise LGTM. You'll need LGTM from Skia team as well. https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: mipMapLevelCount = SkMipMap::ComputeLevelCount(width(), height()) + 1; This should be using scaledSize, not full image size. Same issue below. https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:554: SkISize mipSize = SkMipMap::ComputeLevelSize(width(), height(), Should use scaledSize (or just use pixmap.width(), pixmap.height()).
On 2016/06/17 22:41:54, ericrk wrote: > One comment, otherwise LGTM. > > You'll need LGTM from Skia team as well. > > https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.cpp > File src/image/SkImage_Gpu.cpp (right): > > https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... > src/image/SkImage_Gpu.cpp:493: mipMapLevelCount = > SkMipMap::ComputeLevelCount(width(), height()) + 1; > This should be using scaledSize, not full image size. Same issue below. > > https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... > src/image/SkImage_Gpu.cpp:554: SkISize mipSize = > SkMipMap::ComputeLevelSize(width(), height(), > Should use scaledSize (or just use pixmap.width(), pixmap.height()). Other than Eric's comments, lgtm.
https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: mipMapLevelCount = SkMipMap::ComputeLevelCount(width(), height()) + 1; On 2016/06/17 22:41:54, ericrk wrote: > This should be using scaledSize, not full image size. Same issue below. Done. https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:554: SkISize mipSize = SkMipMap::ComputeLevelSize(width(), height(), On 2016/06/17 22:41:54, ericrk wrote: > Should use scaledSize (or just use pixmap.width(), pixmap.height()). 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/2034933003/240001
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 bsalomon@google.com, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2034933003/#ps260001 (title: "Another scaled image.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034933003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.cpp File src/image/SkImage_Gpu.cpp (right): https://codereview.chromium.org/2034933003/diff/220001/src/image/SkImage_Gpu.... src/image/SkImage_Gpu.cpp:493: mipMapLevelCount = SkMipMap::ComputeLevelCount(width(), height()) + 1; On 2016/06/20 18:51:02, cblume wrote: > On 2016/06/17 22:41:54, ericrk wrote: > > This should be using scaledSize, not full image size. Same issue below. > > Done. Actually, this seems to be causing the current problems with the crash. pixmap.height() and .width() are returning 0 while height() and width() are returning 8. Was that supposed to not be 0? Is there a scaledSize you recommend me use somewhere?
The CQ bit was checked by cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com, ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/2034933003/#ps280001 (title: "Using scaled size.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034933003/280001
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...) 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
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2034933003/280001
Message was sent while issue was closed.
Description was changed from ========== Store mipmap levels in deferred texture image When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 R=bsalomon@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2034933003 ========== to ========== Store mipmap levels in deferred texture image When creating the deferred texture image, detect if using medium / high quality. If so, generate and store mipmaps in the deferred texture image. When creating a texture from that be sure to read it back out. BUG=578304 R=bsalomon@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2034933003 Committed: https://skia.googlesource.com/skia/+/b3105190a6e02d37f1d7f07a3a8bdd368ec7f157 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://skia.googlesource.com/skia/+/b3105190a6e02d37f1d7f07a3a8bdd368ec7f157
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:280001) has been created in https://codereview.chromium.org/2083393002/ by brianosman@google.com. The reason for reverting is: Crashes on a few different bots (including ASAN). Examples: https://build.chromium.org/p/client.skia.android/builders/Test-Android-GCC-Ne... https://build.chromium.org/p/client.skia/builders/Test-iOS-Clang-iPad4-GPU-SG... https://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-Golo-GPU-GT.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
