Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1119723003:
Add glCopyCompressedTextureCHROMIUM (Closed)
|
Created:
5 years, 7 months ago by christiank Modified:
5 years, 6 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, chromium-reviews, darin (slow to review), piman+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd glCopyCompressedTextureCHROMIUM
This CL adds a GPU command for copying compressed textures. It's
based on glCopyTextureCHROMIUM, but modified for use on
compressed textures. It only supports GLImage-based copying.
BUG=434699
Committed: https://crrev.com/8312009fd0fe42534b7c9eccedf4c34297e37d21
Cr-Commit-Position: refs/heads/master@{#334810}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Fix format validation #Patch Set 3 : Use glGetCompressedTexImage as a fallback #Patch Set 4 : Add tests #
Total comments: 2
Patch Set 5 : Fix memory buffer size and add extension documentation #Patch Set 6 : Fix comments in tests #
Total comments: 33
Patch Set 7 : Address issues #Patch Set 8 : Rename files to reflect upcoming extension name change #Patch Set 9 : Rename to glCompressedCopyTextureCHROMIUM #
Total comments: 4
Patch Set 10 : Remove glGetCompressedTexImage fallback #
Total comments: 10
Patch Set 11 : Remove internal_format and source_size parameters #Patch Set 12 : Rebase #
Total comments: 4
Patch Set 13 : Address issues #
Total comments: 28
Patch Set 14 : Address issues #
Total comments: 2
Patch Set 15 : Address issues #Patch Set 16 : Address issues #Patch Set 17 : Use ScopedVector for storing image format configurations #Messages
Total messages: 52 (9 generated)
christiank@opera.com changed reviewers: + jbauman@chromium.org, reveman@chromium.org
Hi, This is another support CL that's needed for the tile compression feature. It implements a new glCopyCompressedTextureCHROMIUM() command that will be used by ResourceProvider::CopyResource() when copying a compressed resource.
Please update gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_texture.txt or add a new extension description for this. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; hm, doesn't source format have to match destination format? https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); How do we handle this failure? CopyTexImage is allowed to fail at which time we need to implement some fallback. Or make sure this copy-compressed-image extension is not available unless it's supported by all available GLImage types. Could we use ARB_copy_image here if available?
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/04/30 13:36:14, reveman wrote: > hm, doesn't source format have to match destination format? Conceptually, maybe it should. However, it appears as if the source format is GL_RGBA when I am testing (using one-copy). The reason seems to be the following line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: ~ScopedWriteLockGpuMemoryBuffer(): gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), size_.width(), size_.height(), GL_RGBA); I don't think we want to change this format. I vaguely remember a discussion about this somewhere. I agree this doesn't look right, but I am not really sure what the right approach is? I have tried to find any instances where the other (non GL_RGBA) source_internal_formats are set, but I couldn't find any. The other formats have been added simply by following the behavior of ValidateCopyTextureCHROMIUM. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/04/30 13:36:14, reveman wrote: > How do we handle this failure? CopyTexImage is allowed to fail at which time we > need to implement some fallback. Or make sure this copy-compressed-image > extension is not available unless it's supported by all available GLImage types. > > Could we use ARB_copy_image here if available? Tile compression should only be enabled when using the GL_TEXTURE_EXTERNAL_OES target and we "should" be using a non-failing CopyTexImage implementation. But I see what you mean. Looking at glCopyCompressedTextureCHROMIUM in isolation, it should be able to handle other cases as well. ARB_copy_image could possibly work. I experimented with it a bit, but I could never get it to work after disabling CopyTexImage. It failed with GL_ILLEGAL_OPERATION, probably thanks to the source format being incompatible with the destination format (see the other discussion).
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/05/05 13:42:22, christiank wrote: > On 2015/04/30 13:36:14, reveman wrote: > > hm, doesn't source format have to match destination format? > > Conceptually, maybe it should. However, it appears as if the source format is > GL_RGBA when I am testing (using one-copy). The reason seems to be the following > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > ~ScopedWriteLockGpuMemoryBuffer(): > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > size_.width(), > size_.height(), > GL_RGBA); > > I don't think we want to change this format. I vaguely remember a discussion > about this somewhere. > > I agree this doesn't look right, but I am not really sure what the right > approach is? > > I have tried to find any instances where the other (non GL_RGBA) > source_internal_formats are set, but I couldn't find any. The other formats have > been added simply by following the behavior of ValidateCopyTextureCHROMIUM. This changed recently, CreateImageCHROMIUM now needs to specific a more exact internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. Are you using ToT? https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/05/05 13:42:22, christiank wrote: > On 2015/04/30 13:36:14, reveman wrote: > > How do we handle this failure? CopyTexImage is allowed to fail at which time > we > > need to implement some fallback. Or make sure this copy-compressed-image > > extension is not available unless it's supported by all available GLImage > types. > > > > Could we use ARB_copy_image here if available? > > Tile compression should only be enabled when using the GL_TEXTURE_EXTERNAL_OES > target and we "should" be using a non-failing CopyTexImage implementation. It's hard to see how this is enforced right now. If someone decides to add compressed texture format support to one of the native gpu-memory-buffer implementations then this is not going to work. I guess we need ARB_copy_image support before we can do that.. > But I > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in isolation, it > should be able to handle other cases as well. > > ARB_copy_image could possibly work. I experimented with it a bit, but I could > never get it to work after disabling CopyTexImage. It failed with > GL_ILLEGAL_OPERATION, probably thanks to the source format being incompatible > with the destination format (see the other discussion). OK, we can save ARB_copy_image support for a follow up. I think we should also be using that extension for normal CopyTexture if available. Can you log an error here instead of the DCHECK?
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/05/05 16:12:09, reveman wrote: > On 2015/05/05 13:42:22, christiank wrote: > > On 2015/04/30 13:36:14, reveman wrote: > > > hm, doesn't source format have to match destination format? > > > > Conceptually, maybe it should. However, it appears as if the source format is > > GL_RGBA when I am testing (using one-copy). The reason seems to be the > following > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > size_.width(), > > size_.height(), > > GL_RGBA); > > > > I don't think we want to change this format. I vaguely remember a discussion > > about this somewhere. > > > > I agree this doesn't look right, but I am not really sure what the right > > approach is? > > > > I have tried to find any instances where the other (non GL_RGBA) > > source_internal_formats are set, but I couldn't find any. The other formats > have > > been added simply by following the behavior of ValidateCopyTextureCHROMIUM. > > This changed recently, CreateImageCHROMIUM now needs to specific a more exact > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. Are > you using ToT? Ah, you're right. My Chromium revision was a little bit too old, I had to rebase. Rebasing fixed part of the problem. I also found that BindTexImage2DCHROMIUM causes a conflict of format. In GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with GL_RGBA. I am not sure why it always sets the format to GL_RGBA, could it be laziness? What do you think about adding a format parameter to BindTexImage2DCHROMIUM? That would fix the problem raised here. I could verify that the source and destination have the same compressed format. If my understanding is correct, it would also make the level information of the Texture object more accurate. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/05/05 16:12:09, reveman wrote: > On 2015/05/05 13:42:22, christiank wrote: > > On 2015/04/30 13:36:14, reveman wrote: > > > How do we handle this failure? CopyTexImage is allowed to fail at which time > > we > > > need to implement some fallback. Or make sure this copy-compressed-image > > > extension is not available unless it's supported by all available GLImage > > types. > > > > > > Could we use ARB_copy_image here if available? > > > > Tile compression should only be enabled when using the GL_TEXTURE_EXTERNAL_OES > > target and we "should" be using a non-failing CopyTexImage implementation. > > It's hard to see how this is enforced right now. If someone decides to add > compressed texture format support to one of the native gpu-memory-buffer > implementations then this is not going to work. I guess we need ARB_copy_image > support before we can do that.. Yes you're right. It certainly feels like a non-complete implementation. Then we have the case when ARB_copy_image isn't supported either. > > But I > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in isolation, it > > should be able to handle other cases as well. > > > > ARB_copy_image could possibly work. I experimented with it a bit, but I could > > never get it to work after disabling CopyTexImage. It failed with > > GL_ILLEGAL_OPERATION, probably thanks to the source format being incompatible > > with the destination format (see the other discussion). > > OK, we can save ARB_copy_image support for a follow up. I think we should also > be using that extension for normal CopyTexture if available. > > Can you log an error here instead of the DCHECK? Absolutely, I'll address this as soon as I know how to proceed with the validation issue.
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/05/07 12:47:00, christiank wrote: > On 2015/05/05 16:12:09, reveman wrote: > > On 2015/05/05 13:42:22, christiank wrote: > > > On 2015/04/30 13:36:14, reveman wrote: > > > > hm, doesn't source format have to match destination format? > > > > > > Conceptually, maybe it should. However, it appears as if the source format > is > > > GL_RGBA when I am testing (using one-copy). The reason seems to be the > > following > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > size_.width(), > > > size_.height(), > > > GL_RGBA); > > > > > > I don't think we want to change this format. I vaguely remember a discussion > > > about this somewhere. > > > > > > I agree this doesn't look right, but I am not really sure what the right > > > approach is? > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > source_internal_formats are set, but I couldn't find any. The other formats > > have > > > been added simply by following the behavior of ValidateCopyTextureCHROMIUM. > > > > This changed recently, CreateImageCHROMIUM now needs to specific a more exact > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. Are > > you using ToT? > > Ah, you're right. My Chromium revision was a little bit too old, I had to > rebase. Rebasing fixed part of the problem. I also found that > BindTexImage2DCHROMIUM causes a conflict of format. In > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with GL_RGBA. > > I am not sure why it always sets the format to GL_RGBA, could it be laziness? It used to be that GL_RGBA was the only supported format but I guess we forgot to fix this when adding support for other formats. Do you mind fixing this in a separate patch? > What do you think about adding a format parameter to BindTexImage2DCHROMIUM? I think it would be better to add a GetFormat function to the GLImage API. > That would fix the problem raised here. I could verify that the source and > destination have the same compressed format. If my understanding is correct, it > would also make the level information of the Texture object more accurate. Yes, sounds like a good plan. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/05/07 12:47:00, christiank wrote: > On 2015/05/05 16:12:09, reveman wrote: > > On 2015/05/05 13:42:22, christiank wrote: > > > On 2015/04/30 13:36:14, reveman wrote: > > > > How do we handle this failure? CopyTexImage is allowed to fail at which > time > > > we > > > > need to implement some fallback. Or make sure this copy-compressed-image > > > > extension is not available unless it's supported by all available GLImage > > > types. > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > Tile compression should only be enabled when using the > GL_TEXTURE_EXTERNAL_OES > > > target and we "should" be using a non-failing CopyTexImage implementation. > > > > It's hard to see how this is enforced right now. If someone decides to add > > compressed texture format support to one of the native gpu-memory-buffer > > implementations then this is not going to work. I guess we need ARB_copy_image > > support before we can do that.. > > Yes you're right. It certainly feels like a non-complete implementation. Then we > have the case when ARB_copy_image isn't supported either. Ideally we'd be able to say that CopyCompressedTexture is only supported when ARB_copy_image is present but it looks like we need this to be supported using GpuMemoryBuffers in cases where ARB_copy_image is unavailable. What if we fallback to GetCompressedTexImage+CompressedTexImage2D when ARB_copy_image is not supported? > > > > But I > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in isolation, > it > > > should be able to handle other cases as well. > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, but I > could > > > never get it to work after disabling CopyTexImage. It failed with > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > incompatible > > > with the destination format (see the other discussion). > > > > OK, we can save ARB_copy_image support for a follow up. I think we should also > > be using that extension for normal CopyTexture if available. > > > > Can you log an error here instead of the DCHECK? > > > Absolutely, I'll address this as soon as I know how to proceed with the > validation issue.
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/05/07 18:35:49, reveman wrote: > On 2015/05/07 12:47:00, christiank wrote: > > On 2015/05/05 16:12:09, reveman wrote: > > > On 2015/05/05 13:42:22, christiank wrote: > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > hm, doesn't source format have to match destination format? > > > > > > > > Conceptually, maybe it should. However, it appears as if the source format > > is > > > > GL_RGBA when I am testing (using one-copy). The reason seems to be the > > > following > > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > > size_.width(), > > > > size_.height(), > > > > GL_RGBA); > > > > > > > > I don't think we want to change this format. I vaguely remember a > discussion > > > > about this somewhere. > > > > > > > > I agree this doesn't look right, but I am not really sure what the right > > > > approach is? > > > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > > source_internal_formats are set, but I couldn't find any. The other > formats > > > have > > > > been added simply by following the behavior of > ValidateCopyTextureCHROMIUM. > > > > > > This changed recently, CreateImageCHROMIUM now needs to specific a more > exact > > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. > Are > > > you using ToT? > > > > Ah, you're right. My Chromium revision was a little bit too old, I had to > > rebase. Rebasing fixed part of the problem. I also found that > > BindTexImage2DCHROMIUM causes a conflict of format. In > > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with GL_RGBA. > > > > I am not sure why it always sets the format to GL_RGBA, could it be laziness? > > It used to be that GL_RGBA was the only supported format but I guess we forgot > to fix this when adding support for other formats. Do you mind fixing this in a > separate patch? > > > What do you think about adding a format parameter to BindTexImage2DCHROMIUM? > > I think it would be better to add a GetFormat function to the GLImage API. > > > That would fix the problem raised here. I could verify that the source and > > destination have the same compressed format. If my understanding is correct, > it > > would also make the level information of the Texture object more accurate. > > Yes, sounds like a good plan. Sure, I'll address it in a separate patch. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/05/07 18:35:49, reveman wrote: > On 2015/05/07 12:47:00, christiank wrote: > > On 2015/05/05 16:12:09, reveman wrote: > > > On 2015/05/05 13:42:22, christiank wrote: > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > How do we handle this failure? CopyTexImage is allowed to fail at which > > time > > > > we > > > > > need to implement some fallback. Or make sure this copy-compressed-image > > > > > extension is not available unless it's supported by all available > GLImage > > > > types. > > > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > > > Tile compression should only be enabled when using the > > GL_TEXTURE_EXTERNAL_OES > > > > target and we "should" be using a non-failing CopyTexImage implementation. > > > > > > It's hard to see how this is enforced right now. If someone decides to add > > > compressed texture format support to one of the native gpu-memory-buffer > > > implementations then this is not going to work. I guess we need > ARB_copy_image > > > support before we can do that.. > > > > Yes you're right. It certainly feels like a non-complete implementation. Then > we > > have the case when ARB_copy_image isn't supported either. > > Ideally we'd be able to say that CopyCompressedTexture is only supported when > ARB_copy_image is present but it looks like we need this to be supported using > GpuMemoryBuffers in cases where ARB_copy_image is unavailable. > > What if we fallback to GetCompressedTexImage+CompressedTexImage2D when > ARB_copy_image is not supported? > > > > > > > But I > > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in > isolation, > > it > > > > should be able to handle other cases as well. > > > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, but I > > could > > > > never get it to work after disabling CopyTexImage. It failed with > > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > > incompatible > > > > with the destination format (see the other discussion). > > > > > > OK, we can save ARB_copy_image support for a follow up. I think we should > also > > > be using that extension for normal CopyTexture if available. > > > > > > Can you log an error here instead of the DCHECK? > > > > > > Absolutely, I'll address this as soon as I know how to proceed with the > > validation issue. > Sure, I imagine GetCompressedTexImage + CompressedTexImage2D could work as a fallback. GetCompressedTexImage is OpenGL 3+ as far as I can tell though. I'll give it a shot.
https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; On 2015/05/08 13:04:23, christiank wrote: > On 2015/05/07 18:35:49, reveman wrote: > > On 2015/05/07 12:47:00, christiank wrote: > > > On 2015/05/05 16:12:09, reveman wrote: > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > hm, doesn't source format have to match destination format? > > > > > > > > > > Conceptually, maybe it should. However, it appears as if the source > format > > > is > > > > > GL_RGBA when I am testing (using one-copy). The reason seems to be the > > > > following > > > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > > > size_.width(), > > > > > size_.height(), > > > > > GL_RGBA); > > > > > > > > > > I don't think we want to change this format. I vaguely remember a > > discussion > > > > > about this somewhere. > > > > > > > > > > I agree this doesn't look right, but I am not really sure what the right > > > > > approach is? > > > > > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > > > source_internal_formats are set, but I couldn't find any. The other > > formats > > > > have > > > > > been added simply by following the behavior of > > ValidateCopyTextureCHROMIUM. > > > > > > > > This changed recently, CreateImageCHROMIUM now needs to specific a more > > exact > > > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. > > Are > > > > you using ToT? > > > > > > Ah, you're right. My Chromium revision was a little bit too old, I had to > > > rebase. Rebasing fixed part of the problem. I also found that > > > BindTexImage2DCHROMIUM causes a conflict of format. In > > > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with GL_RGBA. > > > > > > I am not sure why it always sets the format to GL_RGBA, could it be > laziness? > > > > It used to be that GL_RGBA was the only supported format but I guess we forgot > > to fix this when adding support for other formats. Do you mind fixing this in > a > > separate patch? > > > > > What do you think about adding a format parameter to BindTexImage2DCHROMIUM? > > > > I think it would be better to add a GetFormat function to the GLImage API. > > > > > That would fix the problem raised here. I could verify that the source and > > > destination have the same compressed format. If my understanding is correct, > > it > > > would also make the level information of the Texture object more accurate. > > > > Yes, sounds like a good plan. > > Sure, I'll address it in a separate patch. I have now fixed the format validation. https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); On 2015/05/08 13:04:23, christiank wrote: > On 2015/05/07 18:35:49, reveman wrote: > > On 2015/05/07 12:47:00, christiank wrote: > > > On 2015/05/05 16:12:09, reveman wrote: > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > How do we handle this failure? CopyTexImage is allowed to fail at > which > > > time > > > > > we > > > > > > need to implement some fallback. Or make sure this > copy-compressed-image > > > > > > extension is not available unless it's supported by all available > > GLImage > > > > > types. > > > > > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > > > > > Tile compression should only be enabled when using the > > > GL_TEXTURE_EXTERNAL_OES > > > > > target and we "should" be using a non-failing CopyTexImage > implementation. > > > > > > > > It's hard to see how this is enforced right now. If someone decides to add > > > > compressed texture format support to one of the native gpu-memory-buffer > > > > implementations then this is not going to work. I guess we need > > ARB_copy_image > > > > support before we can do that.. > > > > > > Yes you're right. It certainly feels like a non-complete implementation. > Then > > we > > > have the case when ARB_copy_image isn't supported either. > > > > Ideally we'd be able to say that CopyCompressedTexture is only supported when > > ARB_copy_image is present but it looks like we need this to be supported using > > GpuMemoryBuffers in cases where ARB_copy_image is unavailable. > > > > What if we fallback to GetCompressedTexImage+CompressedTexImage2D when > > ARB_copy_image is not supported? > > > > > > > > > > But I > > > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in > > isolation, > > > it > > > > > should be able to handle other cases as well. > > > > > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, but I > > > could > > > > > never get it to work after disabling CopyTexImage. It failed with > > > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > > > incompatible > > > > > with the destination format (see the other discussion). > > > > > > > > OK, we can save ARB_copy_image support for a follow up. I think we should > > also > > > > be using that extension for normal CopyTexture if available. > > > > > > > > Can you log an error here instead of the DCHECK? > > > > > > > > > Absolutely, I'll address this as soon as I know how to proceed with the > > > validation issue. > > > > Sure, I imagine GetCompressedTexImage + CompressedTexImage2D could work as a > fallback. GetCompressedTexImage is OpenGL 3+ as far as I can tell though. I'll > give it a shot. Is it okay if I log an error for now? I'd rather address the GetCompressedTexImage and CopyTexImage fallbacks separately if possible.
On 2015/05/12 at 11:08:59, christiank wrote: > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format == GL_BGRA_EXT; > On 2015/05/08 13:04:23, christiank wrote: > > On 2015/05/07 18:35:49, reveman wrote: > > > On 2015/05/07 12:47:00, christiank wrote: > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > hm, doesn't source format have to match destination format? > > > > > > > > > > > > Conceptually, maybe it should. However, it appears as if the source > > format > > > > is > > > > > > GL_RGBA when I am testing (using one-copy). The reason seems to be the > > > > > following > > > > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > > > > size_.width(), > > > > > > size_.height(), > > > > > > GL_RGBA); > > > > > > > > > > > > I don't think we want to change this format. I vaguely remember a > > > discussion > > > > > > about this somewhere. > > > > > > > > > > > > I agree this doesn't look right, but I am not really sure what the right > > > > > > approach is? > > > > > > > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > > > > source_internal_formats are set, but I couldn't find any. The other > > > formats > > > > > have > > > > > > been added simply by following the behavior of > > > ValidateCopyTextureCHROMIUM. > > > > > > > > > > This changed recently, CreateImageCHROMIUM now needs to specific a more > > > exact > > > > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, etc. > > > Are > > > > > you using ToT? > > > > > > > > Ah, you're right. My Chromium revision was a little bit too old, I had to > > > > rebase. Rebasing fixed part of the problem. I also found that > > > > BindTexImage2DCHROMIUM causes a conflict of format. In > > > > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with GL_RGBA. > > > > > > > > I am not sure why it always sets the format to GL_RGBA, could it be > > laziness? > > > > > > It used to be that GL_RGBA was the only supported format but I guess we forgot > > > to fix this when adding support for other formats. Do you mind fixing this in > > a > > > separate patch? > > > > > > > What do you think about adding a format parameter to BindTexImage2DCHROMIUM? > > > > > > I think it would be better to add a GetFormat function to the GLImage API. > > > > > > > That would fix the problem raised here. I could verify that the source and > > > > destination have the same compressed format. If my understanding is correct, > > > it > > > > would also make the level information of the Texture object more accurate. > > > > > > Yes, sounds like a good plan. > > > > Sure, I'll address it in a separate patch. > > I have now fixed the format validation. > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); > On 2015/05/08 13:04:23, christiank wrote: > > On 2015/05/07 18:35:49, reveman wrote: > > > On 2015/05/07 12:47:00, christiank wrote: > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > How do we handle this failure? CopyTexImage is allowed to fail at > > which > > > > time > > > > > > we > > > > > > > need to implement some fallback. Or make sure this > > copy-compressed-image > > > > > > > extension is not available unless it's supported by all available > > > GLImage > > > > > > types. > > > > > > > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > > > > > > > Tile compression should only be enabled when using the > > > > GL_TEXTURE_EXTERNAL_OES > > > > > > target and we "should" be using a non-failing CopyTexImage > > implementation. > > > > > > > > > > It's hard to see how this is enforced right now. If someone decides to add > > > > > compressed texture format support to one of the native gpu-memory-buffer > > > > > implementations then this is not going to work. I guess we need > > > ARB_copy_image > > > > > support before we can do that.. > > > > > > > > Yes you're right. It certainly feels like a non-complete implementation. > > Then > > > we > > > > have the case when ARB_copy_image isn't supported either. > > > > > > Ideally we'd be able to say that CopyCompressedTexture is only supported when > > > ARB_copy_image is present but it looks like we need this to be supported using > > > GpuMemoryBuffers in cases where ARB_copy_image is unavailable. > > > > > > What if we fallback to GetCompressedTexImage+CompressedTexImage2D when > > > ARB_copy_image is not supported? > > > > > > > > > > > > > But I > > > > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in > > > isolation, > > > > it > > > > > > should be able to handle other cases as well. > > > > > > > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, but I > > > > could > > > > > > never get it to work after disabling CopyTexImage. It failed with > > > > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > > > > incompatible > > > > > > with the destination format (see the other discussion). > > > > > > > > > > OK, we can save ARB_copy_image support for a follow up. I think we should > > > also > > > > > be using that extension for normal CopyTexture if available. > > > > > > > > > > Can you log an error here instead of the DCHECK? > > > > > > > > > > > > Absolutely, I'll address this as soon as I know how to proceed with the > > > > validation issue. > > > > > > > Sure, I imagine GetCompressedTexImage + CompressedTexImage2D could work as a > > fallback. GetCompressedTexImage is OpenGL 3+ as far as I can tell though. I'll > > give it a shot. > > Is it okay if I log an error for now? I'd rather address the GetCompressedTexImage and CopyTexImage fallbacks separately if possible. The problem is that there's no way for the compositor to know if this will succeed or not. What if the GLImage implementation doesn't support CopyTexImage?
On 2015/05/12 15:59:40, reveman wrote: > On 2015/05/12 at 11:08:59, christiank wrote: > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format > == GL_BGRA_EXT; > > On 2015/05/08 13:04:23, christiank wrote: > > > On 2015/05/07 18:35:49, reveman wrote: > > > > On 2015/05/07 12:47:00, christiank wrote: > > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > > hm, doesn't source format have to match destination format? > > > > > > > > > > > > > > Conceptually, maybe it should. However, it appears as if the source > > > format > > > > > is > > > > > > > GL_RGBA when I am testing (using one-copy). The reason seems to be > the > > > > > > following > > > > > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > > > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > > > > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > > > > > size_.width(), > > > > > > > size_.height(), > > > > > > > GL_RGBA); > > > > > > > > > > > > > > I don't think we want to change this format. I vaguely remember a > > > > discussion > > > > > > > about this somewhere. > > > > > > > > > > > > > > I agree this doesn't look right, but I am not really sure what the > right > > > > > > > approach is? > > > > > > > > > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > > > > > source_internal_formats are set, but I couldn't find any. The other > > > > formats > > > > > > have > > > > > > > been added simply by following the behavior of > > > > ValidateCopyTextureCHROMIUM. > > > > > > > > > > > > This changed recently, CreateImageCHROMIUM now needs to specific a > more > > > > exact > > > > > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, > etc. > > > > Are > > > > > > you using ToT? > > > > > > > > > > Ah, you're right. My Chromium revision was a little bit too old, I had > to > > > > > rebase. Rebasing fixed part of the problem. I also found that > > > > > BindTexImage2DCHROMIUM causes a conflict of format. In > > > > > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with > GL_RGBA. > > > > > > > > > > I am not sure why it always sets the format to GL_RGBA, could it be > > > laziness? > > > > > > > > It used to be that GL_RGBA was the only supported format but I guess we > forgot > > > > to fix this when adding support for other formats. Do you mind fixing this > in > > > a > > > > separate patch? > > > > > > > > > What do you think about adding a format parameter to > BindTexImage2DCHROMIUM? > > > > > > > > I think it would be better to add a GetFormat function to the GLImage API. > > > > > > > > > That would fix the problem raised here. I could verify that the source > and > > > > > destination have the same compressed format. If my understanding is > correct, > > > > it > > > > > would also make the level information of the Texture object more > accurate. > > > > > > > > Yes, sounds like a good plan. > > > > > > Sure, I'll address it in a separate patch. > > > > I have now fixed the format validation. > > > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); > > On 2015/05/08 13:04:23, christiank wrote: > > > On 2015/05/07 18:35:49, reveman wrote: > > > > On 2015/05/07 12:47:00, christiank wrote: > > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > > How do we handle this failure? CopyTexImage is allowed to fail at > > > which > > > > > time > > > > > > > we > > > > > > > > need to implement some fallback. Or make sure this > > > copy-compressed-image > > > > > > > > extension is not available unless it's supported by all available > > > > GLImage > > > > > > > types. > > > > > > > > > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > > > > > > > > > Tile compression should only be enabled when using the > > > > > GL_TEXTURE_EXTERNAL_OES > > > > > > > target and we "should" be using a non-failing CopyTexImage > > > implementation. > > > > > > > > > > > > It's hard to see how this is enforced right now. If someone decides to > add > > > > > > compressed texture format support to one of the native > gpu-memory-buffer > > > > > > implementations then this is not going to work. I guess we need > > > > ARB_copy_image > > > > > > support before we can do that.. > > > > > > > > > > Yes you're right. It certainly feels like a non-complete implementation. > > > Then > > > > we > > > > > have the case when ARB_copy_image isn't supported either. > > > > > > > > Ideally we'd be able to say that CopyCompressedTexture is only supported > when > > > > ARB_copy_image is present but it looks like we need this to be supported > using > > > > GpuMemoryBuffers in cases where ARB_copy_image is unavailable. > > > > > > > > What if we fallback to GetCompressedTexImage+CompressedTexImage2D when > > > > ARB_copy_image is not supported? > > > > > > > > > > > > > > > > But I > > > > > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in > > > > isolation, > > > > > it > > > > > > > should be able to handle other cases as well. > > > > > > > > > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, > but I > > > > > could > > > > > > > never get it to work after disabling CopyTexImage. It failed with > > > > > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > > > > > incompatible > > > > > > > with the destination format (see the other discussion). > > > > > > > > > > > > OK, we can save ARB_copy_image support for a follow up. I think we > should > > > > also > > > > > > be using that extension for normal CopyTexture if available. > > > > > > > > > > > > Can you log an error here instead of the DCHECK? > > > > > > > > > > > > > > > Absolutely, I'll address this as soon as I know how to proceed with the > > > > > validation issue. > > > > > > > > > > Sure, I imagine GetCompressedTexImage + CompressedTexImage2D could work as a > > > fallback. GetCompressedTexImage is OpenGL 3+ as far as I can tell though. > I'll > > > give it a shot. > > > > Is it okay if I log an error for now? I'd rather address the > GetCompressedTexImage and CopyTexImage fallbacks separately if possible. > > The problem is that there's no way for the compositor to know if this will > succeed or not. What if the GLImage implementation doesn't support CopyTexImage? I have now added glGetCompressedTexImage as a fallback. When neither GLImage::CopyTexImage or glGetCompressedTexImage is available the copy operation returns GL_INVALID_OPERATION. Do you think this will work? I can investigate ARB_copy_image later on to further improve the functionality of the copy operation.
On 2015/05/27 at 14:49:40, christiank wrote: > On 2015/05/12 15:59:40, reveman wrote: > > On 2015/05/12 at 11:08:59, christiank wrote: > > > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:11588: source_internal_format > > == GL_BGRA_EXT; > > > On 2015/05/08 13:04:23, christiank wrote: > > > > On 2015/05/07 18:35:49, reveman wrote: > > > > > On 2015/05/07 12:47:00, christiank wrote: > > > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > > > hm, doesn't source format have to match destination format? > > > > > > > > > > > > > > > > Conceptually, maybe it should. However, it appears as if the source > > > > format > > > > > > is > > > > > > > > GL_RGBA when I am testing (using one-copy). The reason seems to be > > the > > > > > > > following > > > > > > > > line in ResourceProvider::ScopedWriteLockGpuMemoryBuffer:: > > > > > > > > ~ScopedWriteLockGpuMemoryBuffer(): > > > > > > > > > > > > > > > > gl->CreateImageCHROMIUM(gpu_memory_buffer_->AsClientBuffer(), > > > > > > > > size_.width(), > > > > > > > > size_.height(), > > > > > > > > GL_RGBA); > > > > > > > > > > > > > > > > I don't think we want to change this format. I vaguely remember a > > > > > discussion > > > > > > > > about this somewhere. > > > > > > > > > > > > > > > > I agree this doesn't look right, but I am not really sure what the > > right > > > > > > > > approach is? > > > > > > > > > > > > > > > > I have tried to find any instances where the other (non GL_RGBA) > > > > > > > > source_internal_formats are set, but I couldn't find any. The other > > > > > formats > > > > > > > have > > > > > > > > been added simply by following the behavior of > > > > > ValidateCopyTextureCHROMIUM. > > > > > > > > > > > > > > This changed recently, CreateImageCHROMIUM now needs to specific a > > more > > > > > exact > > > > > > > internal format. e.g. GL_RGBA, GL_BGRA_EXT, GL_R8, GL_ETC1_RGB8_OES, > > etc. > > > > > Are > > > > > > > you using ToT? > > > > > > > > > > > > Ah, you're right. My Chromium revision was a little bit too old, I had > > to > > > > > > rebase. Rebasing fixed part of the problem. I also found that > > > > > > BindTexImage2DCHROMIUM causes a conflict of format. In > > > > > > GLES2DecoderImpl::DoBindTexImage2DCHROMIUM calls SetLevelInfo with > > GL_RGBA. > > > > > > > > > > > > I am not sure why it always sets the format to GL_RGBA, could it be > > > > laziness? > > > > > > > > > > It used to be that GL_RGBA was the only supported format but I guess we > > forgot > > > > > to fix this when adding support for other formats. Do you mind fixing this > > in > > > > a > > > > > separate patch? > > > > > > > > > > > What do you think about adding a format parameter to > > BindTexImage2DCHROMIUM? > > > > > > > > > > I think it would be better to add a GetFormat function to the GLImage API. > > > > > > > > > > > That would fix the problem raised here. I could verify that the source > > and > > > > > > destination have the same compressed format. If my understanding is > > correct, > > > > > it > > > > > > would also make the level information of the Texture object more > > accurate. > > > > > > > > > > Yes, sounds like a good plan. > > > > > > > > Sure, I'll address it in a separate patch. > > > > > > I have now fixed the format validation. > > > > > > > > https://codereview.chromium.org/1119723003/diff/1/gpu/command_buffer/service/... > > > gpu/command_buffer/service/gles2_cmd_decoder.cc:11989: DCHECK(copy_result); > > > On 2015/05/08 13:04:23, christiank wrote: > > > > On 2015/05/07 18:35:49, reveman wrote: > > > > > On 2015/05/07 12:47:00, christiank wrote: > > > > > > On 2015/05/05 16:12:09, reveman wrote: > > > > > > > On 2015/05/05 13:42:22, christiank wrote: > > > > > > > > On 2015/04/30 13:36:14, reveman wrote: > > > > > > > > > How do we handle this failure? CopyTexImage is allowed to fail at > > > > which > > > > > > time > > > > > > > > we > > > > > > > > > need to implement some fallback. Or make sure this > > > > copy-compressed-image > > > > > > > > > extension is not available unless it's supported by all available > > > > > GLImage > > > > > > > > types. > > > > > > > > > > > > > > > > > > Could we use ARB_copy_image here if available? > > > > > > > > > > > > > > > > Tile compression should only be enabled when using the > > > > > > GL_TEXTURE_EXTERNAL_OES > > > > > > > > target and we "should" be using a non-failing CopyTexImage > > > > implementation. > > > > > > > > > > > > > > It's hard to see how this is enforced right now. If someone decides to > > add > > > > > > > compressed texture format support to one of the native > > gpu-memory-buffer > > > > > > > implementations then this is not going to work. I guess we need > > > > > ARB_copy_image > > > > > > > support before we can do that.. > > > > > > > > > > > > Yes you're right. It certainly feels like a non-complete implementation. > > > > Then > > > > > we > > > > > > have the case when ARB_copy_image isn't supported either. > > > > > > > > > > Ideally we'd be able to say that CopyCompressedTexture is only supported > > when > > > > > ARB_copy_image is present but it looks like we need this to be supported > > using > > > > > GpuMemoryBuffers in cases where ARB_copy_image is unavailable. > > > > > > > > > > What if we fallback to GetCompressedTexImage+CompressedTexImage2D when > > > > > ARB_copy_image is not supported? > > > > > > > > > > > > > > > > > > > But I > > > > > > > > see what you mean. Looking at glCopyCompressedTextureCHROMIUM in > > > > > isolation, > > > > > > it > > > > > > > > should be able to handle other cases as well. > > > > > > > > > > > > > > > > ARB_copy_image could possibly work. I experimented with it a bit, > > but I > > > > > > could > > > > > > > > never get it to work after disabling CopyTexImage. It failed with > > > > > > > > GL_ILLEGAL_OPERATION, probably thanks to the source format being > > > > > > incompatible > > > > > > > > with the destination format (see the other discussion). > > > > > > > > > > > > > > OK, we can save ARB_copy_image support for a follow up. I think we > > should > > > > > also > > > > > > > be using that extension for normal CopyTexture if available. > > > > > > > > > > > > > > Can you log an error here instead of the DCHECK? > > > > > > > > > > > > > > > > > > Absolutely, I'll address this as soon as I know how to proceed with the > > > > > > validation issue. > > > > > > > > > > > > > Sure, I imagine GetCompressedTexImage + CompressedTexImage2D could work as a > > > > fallback. GetCompressedTexImage is OpenGL 3+ as far as I can tell though. > > I'll > > > > give it a shot. > > > > > > Is it okay if I log an error for now? I'd rather address the > > GetCompressedTexImage and CopyTexImage fallbacks separately if possible. > > > > The problem is that there's no way for the compositor to know if this will > > succeed or not. What if the GLImage implementation doesn't support CopyTexImage? > > I have now added glGetCompressedTexImage as a fallback. When neither > GLImage::CopyTexImage or glGetCompressedTexImage is available the copy operation > returns GL_INVALID_OPERATION. Do you think this will work? Yes. We just need to expose this as a command buffer extension so the client can know if it's supported or not. CHROMIUM_copy_compressed_texture? Please add it to gpu/GLES2/extensions/CHROMIUM/. > > I can investigate ARB_copy_image later on to further improve the functionality of > the copy operation. Sounds great. thanks!
https://codereview.chromium.org/1119723003/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12014: GLbyte image_data[GL_TEXTURE_COMPRESSED_IMAGE_SIZE]; This doesn't work. You need to query the size using glGetTexLevelParameter and GL_TEXTURE_COMPRESSED_IMAGE_SIZE. Or maybe even better, compute if yourself as I assume that we already have to code that does that.
I have now documented the extension in CHROMIUM_copy_compressed_texture.txt. Not completely sure how to document the current limitations, but I made an attempt. https://codereview.chromium.org/1119723003/diff/60001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/60001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:12014: GLbyte image_data[GL_TEXTURE_COMPRESSED_IMAGE_SIZE]; On 2015/05/27 15:39:35, reveman wrote: > This doesn't work. You need to query the size using glGetTexLevelParameter and > GL_TEXTURE_COMPRESSED_IMAGE_SIZE. Or maybe even better, compute if yourself as I > assume that we already have to code that does that. Ah right, now I feel silly :) Actually, we already have the size in the source_size parameter. Should now be fixed.
christiank@opera.com changed reviewers: + piman@chromium.org - jbauman@chromium.org
Adding piman to review.
https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:30: GL_UNPACK_UNPREMULTIPLY_ALPHA_CHROMIUM. think that's implicit by not mentioning them. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:39: will fallback to using glGetCompressedTexImage. I think that's too implementation specific to be mentioned here. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:43: INVALID_OPERATION. Please make sure the requirements to always guarantee that glCopyCompressedTextureCHROMIUM succeeds are in the Dependencies section. If this extension is available, then valid usage as described by this spec should never fail. If the required dependencies are missing, then this extension should not be available and usage of it should never succeed. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:53: GLsizei source_size) glCompressedCopyTextureCHROMIUM? to match the CompressedTexImage naming convention.. if you rebase this in ToT you'll see that we also need a CompressedCopySubTextureCHROMIUM as part of this extension. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:70: copying compressed textures. I think that should be handled by the availability of this extension instead. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12011: if (!feature_info_->gl_version_info().is_es) { can you early out at the top of this function if this is not available? this function should not modify any state unless it's supported. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12016: glGetCompressedTexImage(GL_TEXTURE_2D, 0, image_data.get()); can you add a TRACE_EVENT around this somehow? I understand that the usage we're interested in will not result in this being called. A trace event here will make it easier to notice if this is ever being called by accident.
https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:43: INVALID_OPERATION. On 2015/05/28 14:06:48, reveman wrote: > Please make sure the requirements to always guarantee that > glCopyCompressedTextureCHROMIUM succeeds are in the Dependencies section. If > this extension is available, then valid usage as described by this spec should > never fail. If the required dependencies are missing, then this extension should > not be available and usage of it should never succeed. Do you have any suggestion on how I can formally specify that GLImage is required for OpenGL ES? GLImage is an implementation detail, but is it part of any feature that I can write as a dependency? https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:53: GLsizei source_size) On 2015/05/28 14:06:48, reveman wrote: > glCompressedCopyTextureCHROMIUM? to match the CompressedTexImage naming > convention.. > > if you rebase this in ToT you'll see that we also need a > CompressedCopySubTextureCHROMIUM as part of this extension. Ahh I see. Should I perhaps rename this document to CHROMIUM_compressed_copy_texture.txt as well? https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12011: if (!feature_info_->gl_version_info().is_es) { On 2015/05/28 14:06:48, reveman wrote: > can you early out at the top of this function if this is not available? this > function should not modify any state unless it's supported. We still want to use GLImage::CopyTexImage on OpenGL ES platforms where we don't have the glGetCompressedTexImage fallback. So ideally, if I want to early out I would need to know if GLImage::CopyTexImage will work or not. I could make an early check like this: if (!image && feature_info_->gl_version_info().is_es) { return... Would that be sufficient? GLImage::CopyTexImage could still fail, but so could other GL commands as well I suppose.
https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:43: INVALID_OPERATION. On 2015/05/28 at 14:40:29, christiank wrote: > On 2015/05/28 14:06:48, reveman wrote: > > Please make sure the requirements to always guarantee that > > glCopyCompressedTextureCHROMIUM succeeds are in the Dependencies section. If > > this extension is available, then valid usage as described by this spec should > > never fail. If the required dependencies are missing, then this extension should > > not be available and usage of it should never succeed. > > Do you have any suggestion on how I can formally specify that GLImage is required for OpenGL ES? GLImage is an implementation detail, but is it part of any feature that I can write as a dependency? I don't think we can have such a dependency. We can depend on CHROMIUM_image but that doesn't really solve the problem. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:53: GLsizei source_size) On 2015/05/28 at 14:40:29, christiank wrote: > On 2015/05/28 14:06:48, reveman wrote: > > glCompressedCopyTextureCHROMIUM? to match the CompressedTexImage naming > > convention.. > > > > if you rebase this in ToT you'll see that we also need a > > CompressedCopySubTextureCHROMIUM as part of this extension. > > Ahh I see. Should I perhaps rename this document to CHROMIUM_compressed_copy_texture.txt as well? Sounds good. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12011: if (!feature_info_->gl_version_info().is_es) { On 2015/05/28 at 14:40:29, christiank wrote: > On 2015/05/28 14:06:48, reveman wrote: > > can you early out at the top of this function if this is not available? this > > function should not modify any state unless it's supported. > > We still want to use GLImage::CopyTexImage on OpenGL ES platforms where we don't have the glGetCompressedTexImage fallback. So ideally, if I want to early out I would need to know if GLImage::CopyTexImage will work or not. Ok, that's a problem. If a texture is backed by an image is not something the client should need to be aware of and more importantly- if the image implementation can handle the copy operation or not is definitely not something the client should have to be aware of. We shouldn't have commands that the client can't predict if they will succeed or not. If the extension is available then proper usage of it should always work (disregarding oom errors and such). > > I could make an early check like this: > if (!image && feature_info_->gl_version_info().is_es) { > return... > > Would that be sufficient? GLImage::CopyTexImage could still fail, but so could other GL commands as well I suppose. We need to make this predictable from the clients pov. How important is it to support this when GetCompressedTexImage is not available? If that's critical then maybe we can adjust the spec to say that CompressedCopyTextureCHROMIUM might result in the destination texture being uncompressed. Not sure how to deal with that in the CompressedCopySubTextureCHROMIUM case though..
https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:26: to glCopyTexImage2D, but for compressed textures. You mean, similar to glCopyTextureCHROMIUM? glCopyTexImage2D copies from framebuffer to texture. glCopyTextureCHROMIUM copies from texture to texture. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:52: GLenum dest_type, What is dest_type? Compressed texture data doesn't have a "type" in the "format+type" sense that applies to uncompressed data. See glCompressedTexImage2D. Do we need to specify internal_format? I'm pretty sure we can't do conversion (at least not in the general case), so we can just use the internal_format of the source? https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:53: GLsizei source_size) Similarly, I don't think there's a good argument for having source_size here. It doesn't generally make sense to take a subset of the data, and either way we have to validate the size against the size passed at glCompressedTexImage2D time. Since they have to match, might as well omit it. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11995: true); We should only do that after we successfully filled the texture. Same with the SetLevelInfo above, we can't say it's cleared yet. In any case, no state should be modified if an error is generated. So in command handlers we typically do all validation first, generate errors/early return and only then modify state and do the GL calls. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12001: DCHECK(!(unpack_premultiply_alpha_ ^ unpack_unpremultiply_alpha_)); These can't be DCHECKed, we can't trust the client. We need to generate an error in this case instead. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12014: scoped_ptr<GLbyte[]> image_data(new GLbyte[source_size]); We can't trust source_size from the client. Instead we'd need to keep the size passed to glCompressedTexImage2D on the source image and use that here. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12022: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); Why do we do these? I don't think they belong.
https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12011: if (!feature_info_->gl_version_info().is_es) { On 2015/05/28 16:33:08, reveman wrote: > On 2015/05/28 at 14:40:29, christiank wrote: > > On 2015/05/28 14:06:48, reveman wrote: > > > can you early out at the top of this function if this is not available? this > > > function should not modify any state unless it's supported. > > > > We still want to use GLImage::CopyTexImage on OpenGL ES platforms where we > don't have the glGetCompressedTexImage fallback. So ideally, if I want to early > out I would need to know if GLImage::CopyTexImage will work or not. > > Ok, that's a problem. If a texture is backed by an image is not something the > client should need to be aware of and more importantly- if the image > implementation can handle the copy operation or not is definitely not something > the client should have to be aware of. > > We shouldn't have commands that the client can't predict if they will succeed or > not. If the extension is available then proper usage of it should always work > (disregarding oom errors and such). > > > > > I could make an early check like this: > > if (!image && feature_info_->gl_version_info().is_es) { > > return... > > > > Would that be sufficient? GLImage::CopyTexImage could still fail, but so could > other GL commands as well I suppose. > > We need to make this predictable from the clients pov. > > How important is it to support this when GetCompressedTexImage is not available? > > > If that's critical then maybe we can adjust the spec to say that > CompressedCopyTextureCHROMIUM might result in the destination texture being > uncompressed. Not sure how to deal with that in the > CompressedCopySubTextureCHROMIUM case though.. It's rather important as Android is my primary driver for the tile compression feature. I considered switching to using CopyImageSubData (EXT_copy_image) instead. It's supported on some OpenGL ES devices, but unfortunately very few so the problem still remains. It would not rule out OpenGL ES completely however. I can't think of another way to solve this than changing the spec, so I'll try that. I imagine that I'll have to render the compressed texture to a framebuffer, then read that data out and re-upload it as a non-compressed texture. Is that what you had in mind?
On 2015/06/01 at 12:14:16, christiank wrote: > https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:12011: if (!feature_info_->gl_version_info().is_es) { > On 2015/05/28 16:33:08, reveman wrote: > > On 2015/05/28 at 14:40:29, christiank wrote: > > > On 2015/05/28 14:06:48, reveman wrote: > > > > can you early out at the top of this function if this is not available? this > > > > function should not modify any state unless it's supported. > > > > > > We still want to use GLImage::CopyTexImage on OpenGL ES platforms where we > > don't have the glGetCompressedTexImage fallback. So ideally, if I want to early > > out I would need to know if GLImage::CopyTexImage will work or not. > > > > Ok, that's a problem. If a texture is backed by an image is not something the > > client should need to be aware of and more importantly- if the image > > implementation can handle the copy operation or not is definitely not something > > the client should have to be aware of. > > > > We shouldn't have commands that the client can't predict if they will succeed or > > not. If the extension is available then proper usage of it should always work > > (disregarding oom errors and such). > > > > > > > > I could make an early check like this: > > > if (!image && feature_info_->gl_version_info().is_es) { > > > return... > > > > > > Would that be sufficient? GLImage::CopyTexImage could still fail, but so could > > other GL commands as well I suppose. > > > > We need to make this predictable from the clients pov. > > > > How important is it to support this when GetCompressedTexImage is not available? > > > > > > If that's critical then maybe we can adjust the spec to say that > > CompressedCopyTextureCHROMIUM might result in the destination texture being > > uncompressed. Not sure how to deal with that in the > > CompressedCopySubTextureCHROMIUM case though.. > > It's rather important as Android is my primary driver for the tile compression feature. I considered switching to using CopyImageSubData (EXT_copy_image) instead. It's supported on some OpenGL ES devices, but unfortunately very few so the problem still remains. It would not rule out OpenGL ES completely however. > > I can't think of another way to solve this than changing the spec, so I'll try that. I imagine that I'll have to render the compressed texture to a framebuffer, then read that data out and re-upload it as a non-compressed texture. Is that what you had in mind? I was thinking you can just draw the compressed texture to the destination as our standard CopyTextureCHROMIUM implementation does. You can probably reuse most of that code to do it as well.
Addressed many issues, but not everything. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:26: to glCopyTexImage2D, but for compressed textures. On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > You mean, similar to glCopyTextureCHROMIUM? > glCopyTexImage2D copies from framebuffer to texture. glCopyTextureCHROMIUM > copies from texture to texture. Yes, maybe that's a better reference. I was thinking that glCopyTexImage2D was a better reference as glCopyTextureCHROMIUM also respects the pixel storage modifiers. Anyhow, I'll change the text to "similarily to glCopyTextureCHROMIUM". https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:30: GL_UNPACK_UNPREMULTIPLY_ALPHA_CHROMIUM. On 2015/05/28 14:06:48, reveman wrote: > think that's implicit by not mentioning them. Done. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:39: will fallback to using glGetCompressedTexImage. On 2015/05/28 14:06:48, reveman wrote: > I think that's too implementation specific to be mentioned here. Done. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:43: INVALID_OPERATION. On 2015/05/28 16:33:08, reveman wrote: > On 2015/05/28 at 14:40:29, christiank wrote: > > On 2015/05/28 14:06:48, reveman wrote: > > > Please make sure the requirements to always guarantee that > > > glCopyCompressedTextureCHROMIUM succeeds are in the Dependencies section. If > > > this extension is available, then valid usage as described by this spec > should > > > never fail. If the required dependencies are missing, then this extension > should > > > not be available and usage of it should never succeed. > > > > Do you have any suggestion on how I can formally specify that GLImage is > required for OpenGL ES? GLImage is an implementation detail, but is it part of > any feature that I can write as a dependency? > > I don't think we can have such a dependency. We can depend on CHROMIUM_image but > that doesn't really solve the problem. This should now be addressed since the operation will always succeed, falling back to copying into a non-compressed texture. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:52: GLenum dest_type, On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > What is dest_type? Compressed texture data doesn't have a "type" in the > "format+type" sense that applies to uncompressed data. See > glCompressedTexImage2D. > > Do we need to specify internal_format? I'm pretty sure we can't do conversion > (at least not in the general case), so we can just use the internal_format of > the source? It was included because I modeled this function of glCopyTextureChromium. But you're right, it can be removed. Fixed. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:53: GLsizei source_size) On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > Similarly, I don't think there's a good argument for having source_size here. It > doesn't generally make sense to take a subset of the data, and either way we > have to validate the size against the size passed at glCompressedTexImage2D > time. Since they have to match, might as well omit it. Alright, I'll try to get rid of the source_size parameter. See my discussion in a different thread. https://codereview.chromium.org/1119723003/diff/100001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_copy_compressed_texture.txt:70: copying compressed textures. On 2015/05/28 14:06:48, reveman wrote: > I think that should be handled by the availability of this extension instead. Done. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11995: true); On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > We should only do that after we successfully filled the texture. > > Same with the SetLevelInfo above, we can't say it's cleared yet. > > In any case, no state should be modified if an error is generated. > So in command handlers we typically do all validation first, generate > errors/early return and only then modify state and do the GL calls. Can I keep this code as is, now that I have updated the copy operation with a fallback mode to always succeed? https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12001: DCHECK(!(unpack_premultiply_alpha_ ^ unpack_unpremultiply_alpha_)); On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > These can't be DCHECKed, we can't trust the client. We need to generate an error > in this case instead. Fixed. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12014: scoped_ptr<GLbyte[]> image_data(new GLbyte[source_size]); On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > We can't trust source_size from the client. Instead we'd need to keep the size > passed to glCompressedTexImage2D on the source image and use that here. I see. What's the best way of storing the byte size? I looked at using estimated_size_ in the Texture object. I could update this member from DoCompressedTexImage2D, but that's insufficient for the GLImage case. Since GLImage may perform texture uploads I need to update the Texture object estimated_size_ member on other places as well. Maybe DoBindTexImage2DCHROMIUM is sufficient to cover that case? Alternatively I could compute the byte size from the texture width, height and format. Assuming those values can be trusted? https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12016: glGetCompressedTexImage(GL_TEXTURE_2D, 0, image_data.get()); On 2015/05/28 14:06:49, reveman wrote: > can you add a TRACE_EVENT around this somehow? I understand that the usage we're > interested in will not result in this being called. A trace event here will make > it easier to notice if this is ever being called by accident. I added an event to trace both the inefficient glGetCompressedTexImage and the non-compressed fallback case. https://codereview.chromium.org/1119723003/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12022: glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_NEAREST); On 2015/05/28 20:34:27, piman (Very slow to review) wrote: > Why do we do these? I don't think they belong. We don't, you're right. Wrong thinking on my part.
https://codereview.chromium.org/1119723003/diff/160001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/160001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:42: void glCompressedCopyTextureCHROMIUM (GLenum target, GLenum source_id, We'll need a CompressedCopySubTexture function too as that's what the compositor is now using. https://codereview.chromium.org/1119723003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12022: if (!feature_info_->gl_version_info().is_es) { I don't think we need this fallback now that we have the copy to RGBA fallback. ARB_copy_image in a follow up still makes sense as that is the ideal solution for native GMBs and standard textures.
https://codereview.chromium.org/1119723003/diff/160001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/160001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:42: void glCompressedCopyTextureCHROMIUM (GLenum target, GLenum source_id, On 2015/06/05 14:09:36, reveman wrote: > We'll need a CompressedCopySubTexture function too as that's what the compositor > is now using. Sure, I'll add that one after I have figured out how to best remove the source_size parameter. https://codereview.chromium.org/1119723003/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12022: if (!feature_info_->gl_version_info().is_es) { On 2015/06/05 14:09:36, reveman wrote: > I don't think we need this fallback now that we have the copy to RGBA fallback. > ARB_copy_image in a follow up still makes sense as that is the ideal solution > for native GMBs and standard textures. Alright, I have removed the glGetCompressedTexImage fallback.
https://codereview.chromium.org/1119723003/diff/180001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/180001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:45: GLsizei source_size) nit: can we remove internal_format and source_size? They provide no value since they have to match the source texture anyway. https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11903: if (image) { Do we need these 2 separate paths? Presumably the image size and the texture level size match (since the texture level size is set to the image size in DoBindTexImage2DCHROMIUM). https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11924: source_width, source_height, 1)) { Is this needed? The validity should have been tested on the source texture when it was set up? https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11978: dest_height != source_height) { What if it was defined, but not as a compressed texture? https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11983: source_height, 0, source_size, NULL); Really, this is only useful if we have an image. If we don't have an image, we'll do a glTexImage2D. So that means you only need source_size if you have an image. I assume the image would have to keep track of the size, so there goes. If you were to keep the GetCompressedTexImage path, you would need to keep track of the source size on the Texture levels in DoCompressedTexImage*, and you could pull it out of there.
reveman, regarding a glCompressedCopySubTextureCHROMIUM extesion. Is the "new" glCopySubTextureCHROMIUM supposed to retire the old glCopyTextureCHROMIUM extension? I am thinking that maybe I should evolve glCompressedCopyTextureCHROMIUM into glCompressedCopySubTextureCHROMIUM rather than having both coexist. https://codereview.chromium.org/1119723003/diff/180001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/180001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:45: GLsizei source_size) On 2015/06/09 00:24:19, piman (Very slow to review) wrote: > nit: can we remove internal_format and source_size? They provide no value since > they have to match the source texture anyway. Done. https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11903: if (image) { On 2015/06/09 00:24:19, piman (Very slow to review) wrote: > Do we need these 2 separate paths? Presumably the image size and the texture > level size match (since the texture level size is set to the image size in > DoBindTexImage2DCHROMIUM). I think you might be right. However, CopyTextureCHROMIUM having both paths like this makes me slightly hesitant https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11924: source_width, source_height, 1)) { On 2015/06/09 00:24:19, piman (Very slow to review) wrote: > Is this needed? The validity should have been tested on the source texture when > it was set up? I don't think so. But same thing here, CopyTextureCHROMIUM has this so I didn't dare changing it. I don't mind removing this and correcting the double paths as you noticed above, but I wanted someone else's opinion first. Should I go ahead? https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11978: dest_height != source_height) { On 2015/06/09 00:24:19, piman (Very slow to review) wrote: > What if it was defined, but not as a compressed texture? Done. https://codereview.chromium.org/1119723003/diff/180001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:11983: source_height, 0, source_size, NULL); On 2015/06/09 00:24:19, piman (Very slow to review) wrote: > Really, this is only useful if we have an image. If we don't have an image, > we'll do a glTexImage2D. > > So that means you only need source_size if you have an image. I assume the image > would have to keep track of the size, so there goes. > > > If you were to keep the GetCompressedTexImage path, you would need to keep track > of the source size on the Texture levels in DoCompressedTexImage*, and you could > pull it out of there. Done.
On 2015/06/11 at 07:31:57, christiank wrote: > reveman, regarding a glCompressedCopySubTextureCHROMIUM extesion. Is the "new" glCopySubTextureCHROMIUM supposed to retire the old glCopyTextureCHROMIUM extension? > > I am thinking that maybe I should evolve glCompressedCopyTextureCHROMIUM into glCompressedCopySubTextureCHROMIUM rather than having both coexist. Copy is different from CopySub in that the latter will fail if the storage has not already been defined. I think we need both but CopySub is what the compositor uses. The fallback is a bit more complicated in the CopySub case as we can't assume the destination is not a compressed format. We probably have to copy the contents of the old destination texture into a new destination texture before performing the sub rect update.
https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:229: if (!gfx::GLImageMemory::StrideInBytes( hm, it's a bit awkward to depend on GLImageMemory specific code here. Can you instead refactor the GLES2DecoderImpl::ValidateCompressedTexFuncData code so that you can use that same logic to compute the image size? https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12422: if (!dest_level_defined || dest_width != source_width || I understand that this is not needed in the "!image" case right now but maybe move it above the "if (image)" scope anyhow to make it clear that it's needed when we have a better fallback. e.g. ARB_copy_image.
Ah okay, I'll add a separate glCompressedCopySubTexture then. https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:229: if (!gfx::GLImageMemory::StrideInBytes( On 2015/06/11 22:40:42, reveman wrote: > hm, it's a bit awkward to depend on GLImageMemory specific code here. Can you > instead refactor the GLES2DecoderImpl::ValidateCompressedTexFuncData code so > that you can use that same logic to compute the image size? Done. https://codereview.chromium.org/1119723003/diff/220001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12422: if (!dest_level_defined || dest_width != source_width || On 2015/06/11 22:40:42, reveman wrote: > I understand that this is not needed in the "!image" case right now but maybe > move it above the "if (image)" scope anyhow to make it clear that it's needed > when we have a better fallback. e.g. ARB_copy_image. Done.
assuming that you'll add CopySub support in a follow up patch, lgtm % piman's review https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/gl2extchromi... File gpu/GLES2/gl2extchromium.h (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/gl2extchromi... gpu/GLES2/gl2extchromium.h:417: GL_APICALL void GL_APIENTRY glCompressedCopyTextureCHROMIUM( nit: move this to a separate "#ifndef GL_CHROMIUM_compressed copy_texture" section as it's a different extension https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:68: #include "ui/gl/gl_image_memory.h" nit: no longer needed
https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:52: compressed, and they must match. The format must be one of the following "The internal format of the source and destination texture must be compressed, and they must match" That's not what's implemented in the code, and that's not consistent with the level image-creating functions (e.g. glCopyTexImage2D, glCopyTextureCHROMIUM, glCompressedTexImage2D etc.). Typically, what you want, and this is what's implemented, is that the call will create/replace the level image with a new image that has the same internal format as the source. You don't want to have to call glCompressedTexImage2D before calling this - that would be more appropriate for a glCompressedCopySubTextureCHROMIUM (which doesn't create the image). https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:59: INVALID_OPERATION is generated if internal format of source and destination "source and destination" -> "source" per the above. https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:63: texture doesn't match. Remove, per the above. https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:63: texture doesn't match. Please add the INVALID_OPERATION error case if the destination texture is immutable. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9124: *size_in_bytes = bytes_required.ValueOrDie(); nit: ValueOrDefault(0). No need to generate code+strings for the assert, we tested validity above. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12412: dest_internal_format != source_internal_format) { Can we make sure we only go through here is we have an image? If we take the fallback this is all redundant. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12417: return; This can't happen, can it? The only reason it could fail after we validated the internal format above, is if we overflow the size. However that would have also failed before when we tried to create the image in the source texture. So can we replace this with a DCHECK? https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12448: } This should be before we do GL calls (e.g. right after the ValidateCompressedCopyTextureCHROMIUM), and return in the error case. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12454: gfx::Rect(0, 0, source_width, source_height))) { Need to RestoreCurrentTextureBindings here. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12467: glBindTexture(GL_TEXTURE_2D, dest_texture->service_id()); We bind the dest_texture up to 3 times. Can we instead bind it once and for all before l.12409 ? https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12499: DoDidUseTexImageIfNeeded(source_texture, source_texture->target()); RestoreCurrentTextureBindings here. Maybe this calls for a scoped thing.
https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... File gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:52: compressed, and they must match. The format must be one of the following On 2015/06/12 19:50:29, piman (Very slow to review) wrote: > "The internal format of the source and destination texture must be compressed, > and they must match" > > That's not what's implemented in the code, and that's not consistent with the > level image-creating functions (e.g. glCopyTexImage2D, glCopyTextureCHROMIUM, > glCompressedTexImage2D etc.). > Typically, what you want, and this is what's implemented, is that the call will > create/replace the level image with a new image that has the same internal > format as the source. > > You don't want to have to call glCompressedTexImage2D before calling this - that > would be more appropriate for a glCompressedCopySubTextureCHROMIUM (which > doesn't create the image). Ah yes you're right. I have now rephrased the text a bit. https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:59: INVALID_OPERATION is generated if internal format of source and destination On 2015/06/12 19:50:29, piman (Very slow to review) wrote: > "source and destination" -> "source" per the above. Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/extensions/C... gpu/GLES2/extensions/CHROMIUM/CHROMIUM_compressed_copy_texture.txt:63: texture doesn't match. On 2015/06/12 19:50:29, piman (Very slow to review) wrote: > Please add the INVALID_OPERATION error case if the destination texture is > immutable. Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/gl2extchromi... File gpu/GLES2/gl2extchromium.h (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/GLES2/gl2extchromi... gpu/GLES2/gl2extchromium.h:417: GL_APICALL void GL_APIENTRY glCompressedCopyTextureCHROMIUM( On 2015/06/12 14:23:34, reveman wrote: > nit: move this to a separate "#ifndef GL_CHROMIUM_compressed copy_texture" > section as it's a different extension Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9124: *size_in_bytes = bytes_required.ValueOrDie(); On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > nit: ValueOrDefault(0). No need to generate code+strings for the assert, we > tested validity above. Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12412: dest_internal_format != source_internal_format) { On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > Can we make sure we only go through here is we have an image? If we take the > fallback this is all redundant. reveman actually suggested me to keep it like this. I will add a different fallback in a later review that will need this piece of code. I'll cite reveman from patch set 12: "I understand that this is not needed in the "!image" case right now but maybe move it above the "if (image)" scope anyhow to make it clear that it's needed when we have a better fallback. e.g. ARB_copy_image." https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12417: return; On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > This can't happen, can it? > The only reason it could fail after we validated the internal format above, is > if we overflow the size. However that would have also failed before when we > tried to create the image in the source texture. > > So can we replace this with a DCHECK? What if a malicious renderer creates an image of one size and then specifies different dimensions for the copy operation? Isn't that a possible scenario we need to catch in release builds? https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12448: } On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > This should be before we do GL calls (e.g. right after the > ValidateCompressedCopyTextureCHROMIUM), and return in the error case. Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12454: gfx::Rect(0, 0, source_width, source_height))) { On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > Need to RestoreCurrentTextureBindings here. Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12467: glBindTexture(GL_TEXTURE_2D, dest_texture->service_id()); On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > We bind the dest_texture up to 3 times. Can we instead bind it once and for all > before l.12409 ? Done. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12499: DoDidUseTexImageIfNeeded(source_texture, source_texture->target()); On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > RestoreCurrentTextureBindings here. > > Maybe this calls for a scoped thing. Done.
https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12412: dest_internal_format != source_internal_format) { On 2015/06/15 at 09:42:42, christiank wrote: > On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > > Can we make sure we only go through here is we have an image? If we take the > > fallback this is all redundant. > > reveman actually suggested me to keep it like this. I will add a different fallback in a later review that will need this piece of code. > > I'll cite reveman from patch set 12: > "I understand that this is not needed in the "!image" case right now but maybe move it above the "if (image)" scope anyhow to make it clear that it's needed when we have a better fallback. e.g. ARB_copy_image." Either way is fine with me. This just seemed a bit more consistent with CopyTextureCHROMIUM and well aligned with future improvements. I don't think we care about the performance of the fallback and doing some extra work in that case. ARB_copy_image support is going to be needed before we can use this with native GpuMemoryBuffers.
https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12417: return; On 2015/06/15 09:42:42, christiank wrote: > On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > > This can't happen, can it? > > The only reason it could fail after we validated the internal format above, is > > if we overflow the size. However that would have also failed before when we > > tried to create the image in the source texture. > > > > So can we replace this with a DCHECK? > > What if a malicious renderer creates an image of one size and then specifies > different dimensions for the copy operation? Isn't that a possible scenario we > need to catch in release builds? source_* come from the source image/texture, they've been validated before and they're not changing here.... am I missing something?
https://codereview.chromium.org/1119723003/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12373: "pixel-storage multipliers are not supported"); And return.
https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12412: dest_internal_format != source_internal_format) { On 2015/06/15 14:44:56, reveman wrote: > On 2015/06/15 at 09:42:42, christiank wrote: > > On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > > > Can we make sure we only go through here is we have an image? If we take the > > > fallback this is all redundant. > > > > reveman actually suggested me to keep it like this. I will add a different > fallback in a later review that will need this piece of code. > > > > I'll cite reveman from patch set 12: > > "I understand that this is not needed in the "!image" case right now but maybe > move it above the "if (image)" scope anyhow to make it clear that it's needed > when we have a better fallback. e.g. ARB_copy_image." > > Either way is fine with me. This just seemed a bit more consistent with > CopyTextureCHROMIUM and well aligned with future improvements. I don't think we > care about the performance of the fallback and doing some extra work in that > case. ARB_copy_image support is going to be needed before we can use this with > native GpuMemoryBuffers. I have moved it into the if (image) clause for now. I'll move it out later when I have to. https://codereview.chromium.org/1119723003/diff/240001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12417: return; On 2015/06/15 18:35:03, piman (Very slow to review) wrote: > On 2015/06/15 09:42:42, christiank wrote: > > On 2015/06/12 19:50:30, piman (Very slow to review) wrote: > > > This can't happen, can it? > > > The only reason it could fail after we validated the internal format above, > is > > > if we overflow the size. However that would have also failed before when we > > > tried to create the image in the source texture. > > > > > > So can we replace this with a DCHECK? > > > > What if a malicious renderer creates an image of one size and then specifies > > different dimensions for the copy operation? Isn't that a possible scenario we > > need to catch in release builds? > > source_* come from the source image/texture, they've been validated before and > they're not changing here.... am I missing something? No, you're right. For some reason I was in the mindset that the source_* came from parameters. Fixed. https://codereview.chromium.org/1119723003/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1119723003/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:12373: "pixel-storage multipliers are not supported"); On 2015/06/15 18:37:47, piman (Very slow to review) wrote: > And return. Done.
LGTM, thanks. I think it reads more clearly this way. We can move things around if we need to when we add another path.
The CQ bit was checked by christiank@opera.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1119723003/#ps300001 (title: "Address issues")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119723003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by christiank@opera.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1119723003/#ps320001 (title: "Use ScopedVector for storing image format configurations")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119723003/320001
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 christiank@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1119723003/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/8312009fd0fe42534b7c9eccedf4c34297e37d21 Cr-Commit-Position: refs/heads/master@{#334810} |
