Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1570173004:
This CL adds glTexStorage support. (Closed)
DescriptionThis CL adds glTexStorage support.
For us to take advantage of immutable texture storage, we would need to
know in advance that the texture will not be changing allocated size.
In some cases we cannot know this in advance: we will sometimes later
need mipmaps to be allocated and generated. However, in the cases where
we know in advance that we are allocating mipmaps we can take advantage
of immutable storage.
BUG=476416
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1570173004
Committed: https://skia.googlesource.com/skia/+/790d5132620d86813380d3df251e80dd2b41a409
Patch Set 1 #Patch Set 2 : Updating to latest mipmap code. #Patch Set 3 : Rebasing #Patch Set 4 : Fixing rebase mistake. #
Total comments: 4
Patch Set 5 : Moving check for TexStorage support inside allocate_and_populate_* calls. Changing the caps to indi… #
Total comments: 12
Patch Set 6 : Changing 0x16 -> 0x10 (facepalm), making isConfigTexStorageEnabled a GrGLCaps thing (not GrCaps), a… #Patch Set 7 : Adding TODO comment. #
Messages
Total messages: 31 (13 generated)
Description was changed from ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. BUG=skia: ========== to ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. 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
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/1570173004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570173004/60001
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-02-27 04:01 UTC
bsalomon@google.com changed reviewers: + bsalomon@google.com
https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp#... src/gpu/gl/GrGLGpu.cpp:925: // 565 is not a sized internal format on desktop GL. So on desktop with We now are making most decisions about configs in GrGLCaps. Can you store a bit in the config table there about whether a config is compatible with TextureStorage in GrGLCaps::initConfigTable? https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp#... src/gpu/gl/GrGLGpu.cpp:964: TexStorage2D(target, Won't this cause a problem if we later call glGenerateMipmaps? I was expecting that we'd only use TexStorage when the MIP chain was already available here.
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/1570173004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570173004/80001
https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp File src/gpu/gl/GrGLGpu.cpp (right): https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp#... src/gpu/gl/GrGLGpu.cpp:925: // 565 is not a sized internal format on desktop GL. So on desktop with On 2016/02/26 22:09:54, bsalomon wrote: > We now are making most decisions about configs in GrGLCaps. Can you store a bit > in the config table there about whether a config is compatible with > TextureStorage in GrGLCaps::initConfigTable? Done. https://codereview.chromium.org/1570173004/diff/60001/src/gpu/gl/GrGLGpu.cpp#... src/gpu/gl/GrGLGpu.cpp:964: TexStorage2D(target, On 2016/02/26 22:09:54, bsalomon wrote: > Won't this cause a problem if we later call glGenerateMipmaps? I was expecting > that we'd only use TexStorage when the MIP chain was already available here. The calling sites currently only have useTexStorage = true if the MIP chain is available. I will move the check to here.
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/1570173004/diff/80001/include/gpu/GrCaps.h File include/gpu/GrCaps.h (right): https://codereview.chromium.org/1570173004/diff/80001/include/gpu/GrCaps.h#ne... include/gpu/GrCaps.h:237: virtual bool isConfigTexSupportEnabled(GrPixelConfig config) const = 0; We shouldn't need to expose this at the GrCaps level, just GrGLCaps. Texture storage is a GL concept and the API-neutral layer of Ganesh need not know about it. https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1447: // only allows sized internal formats we disallow it. This logic is due for an update, as of 4.2 regular GL supports 565. I'll do that in a follow on patch, though. https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1637: fConfigTable[kIndex_8_GrPixelConfig].fFlags |= ConfigInfo::kCanUseTexStorage_Flag; The spec for this (at least the original extension against ES2) does not allow for CompressedTexSubImage. I didn't look up the other compressed formats we support. We can either check them all or just punt on texture storage for compressed formats for now. https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:486: kCanUseTexStorage_Flag = 0x16, 0x10?
https://codereview.chromium.org/1570173004/diff/80001/include/gpu/GrCaps.h File include/gpu/GrCaps.h (right): https://codereview.chromium.org/1570173004/diff/80001/include/gpu/GrCaps.h#ne... include/gpu/GrCaps.h:237: virtual bool isConfigTexSupportEnabled(GrPixelConfig config) const = 0; On 2016/02/27 14:18:54, bsalomon wrote: > We shouldn't need to expose this at the GrCaps level, just GrGLCaps. Texture > storage is a GL concept and the API-neutral layer of Ganesh need not know about > it. Done. https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1447: // only allows sized internal formats we disallow it. On 2016/02/27 14:18:54, bsalomon wrote: > This logic is due for an update, as of 4.2 regular GL supports 565. I'll do that > in a follow on patch, though. Want me to throw a TODO in here? https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1637: fConfigTable[kIndex_8_GrPixelConfig].fFlags |= ConfigInfo::kCanUseTexStorage_Flag; On 2016/02/27 14:18:54, bsalomon wrote: > The spec for this (at least the original extension against ES2) does not allow > for CompressedTexSubImage. I didn't look up the other compressed formats we > support. We can either check them all or just punt on texture storage for > compressed formats for now. If it is something I can just look up then I would prefer to do that and not punt. So I began doing that. But it looks like there may be extensions which allow this. This means we would need to also look up what extensions enable it. https://www.khronos.org/registry/gles/extensions/EXT/EXT_compressed_ETC1_RGB8... Should I go ahead and disable all compressed formats by default and as I come across any extensions, add checks for them to enable? https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.h File src/gpu/gl/GrGLCaps.h (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.h#n... src/gpu/gl/GrGLCaps.h:486: kCanUseTexStorage_Flag = 0x16, On 2016/02/27 14:18:54, bsalomon wrote: > 0x10? OMG. omg.
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/1570173004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570173004/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/1570173004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570173004/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
https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1447: // only allows sized internal formats we disallow it. On 2016/02/28 02:44:42, cblume wrote: > On 2016/02/27 14:18:54, bsalomon wrote: > > This logic is due for an update, as of 4.2 regular GL supports 565. I'll do > that > > in a follow on patch, though. > > Want me to throw a TODO in here? Sure https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1637: fConfigTable[kIndex_8_GrPixelConfig].fFlags |= ConfigInfo::kCanUseTexStorage_Flag; On 2016/02/28 02:44:42, cblume wrote: > On 2016/02/27 14:18:54, bsalomon wrote: > > The spec for this (at least the original extension against ES2) does not allow > > for CompressedTexSubImage. I didn't look up the other compressed formats we > > support. We can either check them all or just punt on texture storage for > > compressed formats for now. > > If it is something I can just look up then I would prefer to do that and not > punt. > So I began doing that. > But it looks like there may be extensions which allow this. This means we would > need to also look up what extensions enable it. > https://www.khronos.org/registry/gles/extensions/EXT/EXT_compressed_ETC1_RGB8... > Should I go ahead and disable all compressed formats by default and as I come > across any extensions, add checks for them to enable? Totally up to you... if you do wind up researching the various formats probably want to check both the ES and regular GL registries.
https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp File src/gpu/gl/GrGLCaps.cpp (right): https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1447: // only allows sized internal formats we disallow it. On 2016/02/29 14:42:28, bsalomon wrote: > On 2016/02/28 02:44:42, cblume wrote: > > On 2016/02/27 14:18:54, bsalomon wrote: > > > This logic is due for an update, as of 4.2 regular GL supports 565. I'll do > > that > > > in a follow on patch, though. > > > > Want me to throw a TODO in here? > > Sure Done. https://codereview.chromium.org/1570173004/diff/80001/src/gpu/gl/GrGLCaps.cpp... src/gpu/gl/GrGLCaps.cpp:1637: fConfigTable[kIndex_8_GrPixelConfig].fFlags |= ConfigInfo::kCanUseTexStorage_Flag; On 2016/02/29 14:42:28, bsalomon wrote: > On 2016/02/28 02:44:42, cblume wrote: > > On 2016/02/27 14:18:54, bsalomon wrote: > > > The spec for this (at least the original extension against ES2) does not > allow > > > for CompressedTexSubImage. I didn't look up the other compressed formats we > > > support. We can either check them all or just punt on texture storage for > > > compressed formats for now. > > > > If it is something I can just look up then I would prefer to do that and not > > punt. > > So I began doing that. > > But it looks like there may be extensions which allow this. This means we > would > > need to also look up what extensions enable it. > > > https://www.khronos.org/registry/gles/extensions/EXT/EXT_compressed_ETC1_RGB8... > > Should I go ahead and disable all compressed formats by default and as I come > > across any extensions, add checks for them to enable? > > Totally up to you... if you do wind up researching the various formats probably > want to check both the ES and regular GL registries. I went ahead and disabled all the compressed texture formats. On second thought, I don't think I want to enable with extensions until I have a way to test that code. So I'll leave them disabled for now.
lgtm
Description was changed from ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. BUG=476416 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
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/1570173004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1570173004/120001
Message was sent while issue was closed.
Description was changed from ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. BUG=476416 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== This CL adds glTexStorage support. For us to take advantage of immutable texture storage, we would need to know in advance that the texture will not be changing allocated size. In some cases we cannot know this in advance: we will sometimes later need mipmaps to be allocated and generated. However, in the cases where we know in advance that we are allocating mipmaps we can take advantage of immutable storage. BUG=476416 GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/790d5132620d86813380d3df251e80dd2b41a409 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/790d5132620d86813380d3df251e80dd2b41a409 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
