Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2337833002:
Implement WEBGL_compressed_texture_s3tc_srgb (Closed)
|
Created:
4 years, 3 months ago by Kai Ninomiya Modified:
4 years, 3 months ago CC:
chromium-reviews, blink-reviews, piman+watch_chromium.org, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement WEBGL_compressed_texture_s3tc_srgb
Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra).
This is tested by WebGL conformance test
conformance/extensions/webgl-compressed-texture-s3tc-srgb.html
BUG=630498
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
TBR=haraken@chromium.org
Committed: https://crrev.com/692194d1c8295d88a60230f24fee887026dae424
Cr-Commit-Position: refs/heads/master@{#419333}
Patch Set 1 : Implement WEBGL_compressed_texture_s3tc_srgb #
Total comments: 10
Patch Set 2 : address comments #Patch Set 3 : re-rebase #Patch Set 4 : replace NV_sRGB_formats with (draft) EXT_texture_compression_s3tc_srgb #
Total comments: 2
Patch Set 5 : a TODO + short copyright headers #Patch Set 6 : add test and fix feature detection (enabled on webgl2, android/tegra) #Messages
Total messages: 47 (25 generated)
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ==========
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Patchset #1 (id:1) has been deleted
kainino@chromium.org changed reviewers: + haraken@chromium.org, jbauman@chromium.org, kbr@chromium.org, vmiura@chromium.org
jbauman@chromium.org, vmiura@chromium.org: Please review changes in gpu/command_buffer/ kbr@chromium.org: Please review changes in webgl/ haraken@chromium.org: Please review changes in third_party/WebKit/Source/modules/modules_idl_files.gni
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || (enable_dxt1 && enable_dxt3 && enable_dxt5)) && have_srgb) { Will this actually guarantee that these SRGB formats are supported? It looks to me like this works with GL_EXT_texture_compression_s3tc and GL_EXT_texture_sRGB, but is it defined with other combinations of extensions? https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:632: GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT); I guess we're not going to bother with GL_SLUMINANCE_NV? If so, we should at least put a note in that we're not supporting the complete extension. https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:636: //validators_.texture_unsized_internal_format.AddValue(_); Unneeded?
The WebGL-side changes look perfect. LGTM It would be good to mention in the commit message that this is tested by the conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html . https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:632: GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT); On 2016/09/15 00:14:25, jbauman wrote: > I guess we're not going to bother with GL_SLUMINANCE_NV? If so, we should at > least put a note in that we're not supporting the complete extension. Agreed. Should we consider exposing the extension string EXT_texture_compression_s3tc_srgb instead, even though it's still a draft OpenGL ES extension? Or add a TODO before the AddExtensionString call above indicating that it should be changed once that extension is published? https://codereview.chromium.org/2337833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp (right): https://codereview.chromium.org/2337833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp:58: return extensionsUtil->supportsExtension("GL_NV_sRGB_formats"); Let's add a TODO here, at least, about changing this to GL_EXT_texture_compression_s3tc_srgb once it's published.
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || (enable_dxt1 && enable_dxt3 && enable_dxt5)) && have_srgb) { On 2016/09/15 00:14:25, jbauman wrote: > Will this actually guarantee that these SRGB formats are supported? > > It looks to me like this works with GL_EXT_texture_compression_s3tc and > GL_EXT_texture_sRGB, but is it defined with other combinations of extensions? You're right. It should be enabled if the desktop GL context has both EXT_texture_compression_s3tc and EXT_texture_sRGB. But I'm not sure whether (have_s3tc && have_srgb) would be correct either, since these extension checks are at the ES level. In ES, EXT_sRGB plus EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:632: GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT); On 2016/09/15 00:50:35, Ken Russell wrote: > On 2016/09/15 00:14:25, jbauman wrote: > > I guess we're not going to bother with GL_SLUMINANCE_NV? If so, we should at > > least put a note in that we're not supporting the complete extension. > > Agreed. > > Should we consider exposing the extension string > EXT_texture_compression_s3tc_srgb instead, even though it's still a draft OpenGL > ES extension? Or add a TODO before the AddExtensionString call above indicating > that it should be changed once that extension is published? I have tried replacing GL_NV_sRGB_formats with GL_EXT_texture_compression_s3tc_srgb in both places, but that disables the feature in WebGL. I'm guessing this is being queried somewhere else? https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:636: //validators_.texture_unsized_internal_format.AddValue(_); On 2016/09/15 00:14:25, jbauman wrote: > Unneeded? Done. https://codereview.chromium.org/2337833002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp (right): https://codereview.chromium.org/2337833002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp:58: return extensionsUtil->supportsExtension("GL_NV_sRGB_formats"); On 2016/09/15 00:50:35, Ken Russell wrote: > Let's add a TODO here, at least, about changing this to > GL_EXT_texture_compression_s3tc_srgb once it's published. Done.
Hang on, bad rebase.
On 2016/09/15 20:47:50, Kai Ninomiya wrote: > Hang on, bad rebase. Never mind, it's fine.
https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/feature_info.cc:632: GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_EXT); On 2016/09/15 20:45:35, Kai Ninomiya wrote: > On 2016/09/15 00:50:35, Ken Russell wrote: > > On 2016/09/15 00:14:25, jbauman wrote: > > > I guess we're not going to bother with GL_SLUMINANCE_NV? If so, we should at > > > least put a note in that we're not supporting the complete extension. > > > > Agreed. > > > > Should we consider exposing the extension string > > EXT_texture_compression_s3tc_srgb instead, even though it's still a draft > OpenGL > > ES extension? Or add a TODO before the AddExtensionString call above > indicating > > that it should be changed once that extension is published? > > I have tried replacing GL_NV_sRGB_formats with > GL_EXT_texture_compression_s3tc_srgb in both places, but that disables the > feature in WebGL. I'm guessing this is being queried somewhere else? Never mind, it actually works fine. We decided offline to use the currently-draft extension GL_EXT_texture_compression_s3tc_srgb since it's what we're actually doing. https://codereview.chromium.org/2337833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp (right): https://codereview.chromium.org/2337833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp:38: context->addCompressedTextureFormat(GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_NV); Should these be left as _NV, or should we define the _EXT versions somewhere? The values are of course the same.
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb (adds support on non-ANGLE (mac/linux) platforms). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb (adds support on non-ANGLE (mac/linux) platforms). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb (Enables support on non-ANGLE desktop platforms (Mac/Linux).) This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Thanks, still LGTM. https://codereview.chromium.org/2337833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp (right): https://codereview.chromium.org/2337833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/webgl/WebGLCompressedTextureS3TCsRGB.cpp:38: context->addCompressedTextureFormat(GL_COMPRESSED_SRGB_ALPHA_S3TC_DXT5_NV); On 2016/09/15 21:54:08, Kai Ninomiya wrote: > Should these be left as _NV, or should we define the _EXT versions somewhere? > The values are of course the same. We'd need to modify src/third_party/khronos/GLES2/gl2ext.h to use the _EXT versions -- probably best to wait until GL_EXT_compressed_texture_s3tc_srgb is ratified by Khronos, and just rev our copy of the Khronos headers at that point. I suggest adding a comment: // TODO(kainino): update these with _EXT versions once GL_EXT_compressed_texture_s3tc_srgb is ratified
Could you also add a test to feature_info_unittest.cc? Other than that (and the extension checking change), lgtm. On 2016/09/15 20:45:35, Kai Ninomiya wrote: > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > File gpu/command_buffer/service/feature_info.cc (right): > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || (enable_dxt1 > && enable_dxt3 && enable_dxt5)) && have_srgb) { > On 2016/09/15 00:14:25, jbauman wrote: > > Will this actually guarantee that these SRGB formats are supported? > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc and > > GL_EXT_texture_sRGB, but is it defined with other combinations of extensions? > > You're right. It should be enabled if the desktop GL context has both > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > But I'm not sure whether (have_s3tc && have_srgb) would be correct either, since > these extension checks are at the ES level. In ES, EXT_sRGB plus > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > Probably we should check "feature_flags_.desktop_srgb_support && IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we get ANGLE to support it then we might have a better idea of what to do for ES2.
On 2016/09/16 00:25:29, jbauman wrote: > Could you also add a test to feature_info_unittest.cc? > > Other than that (and the extension checking change), lgtm. > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || (enable_dxt1 > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > On 2016/09/15 00:14:25, jbauman wrote: > > > Will this actually guarantee that these SRGB formats are supported? > > > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc and > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > extensions? > > > > You're right. It should be enabled if the desktop GL context has both > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct either, > since > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > > > Probably we should check "feature_flags_.desktop_srgb_support && > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we get ANGLE > to support it then we might have a better idea of what to do for ES2. That would be problematic -- I'm pretty sure Unity wants to use these compressed texture formats in a WebGL 2.0 context.
On 2016/09/16 00:31:14, Ken Russell wrote: > On 2016/09/16 00:25:29, jbauman wrote: > > Could you also add a test to feature_info_unittest.cc? > > > > Other than that (and the extension checking change), lgtm. > > > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || > (enable_dxt1 > > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > > On 2016/09/15 00:14:25, jbauman wrote: > > > > Will this actually guarantee that these SRGB formats are supported? > > > > > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc and > > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > > extensions? > > > > > > You're right. It should be enabled if the desktop GL context has both > > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct either, > > since > > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > > > > > Probably we should check "feature_flags_.desktop_srgb_support && > > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we get > ANGLE > > to support it then we might have a better idea of what to do for ES2. > > That would be problematic -- I'm pretty sure Unity wants to use these compressed > texture formats in a WebGL 2.0 context. It looks like the block that's currently setting have_srgb also checks for IsWebGL1OrES2Context(), though I admit I can't quite see why. We could probably remove the check in this case, assuming the new extension is specced correctly.
On 2016/09/16 00:37:16, jbauman wrote: > On 2016/09/16 00:31:14, Ken Russell wrote: > > On 2016/09/16 00:25:29, jbauman wrote: > > > Could you also add a test to feature_info_unittest.cc? > > > > > > Other than that (and the extension checking change), lgtm. > > > > > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || > > (enable_dxt1 > > > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > > > On 2016/09/15 00:14:25, jbauman wrote: > > > > > Will this actually guarantee that these SRGB formats are supported? > > > > > > > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc and > > > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > > > extensions? > > > > > > > > You're right. It should be enabled if the desktop GL context has both > > > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct either, > > > since > > > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > > > > > > > Probably we should check "feature_flags_.desktop_srgb_support && > > > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we get > > ANGLE > > > to support it then we might have a better idea of what to do for ES2. > > > > That would be problematic -- I'm pretty sure Unity wants to use these > compressed > > texture formats in a WebGL 2.0 context. > > It looks like the block that's currently setting have_srgb also checks for > IsWebGL1OrES2Context(), though I admit I can't quite see why. We could probably > remove the check in this case, assuming the new extension is specced correctly. I think this is because that check is for enabling GL_EXT_sRGB, which isn't necessary in ES 3 since it's core. (In which case, setting `have_srgb` only there is an error.)
On 2016/09/16 00:49:51, Kai Ninomiya wrote: > On 2016/09/16 00:37:16, jbauman wrote: > > On 2016/09/16 00:31:14, Ken Russell wrote: > > > On 2016/09/16 00:25:29, jbauman wrote: > > > > Could you also add a test to feature_info_unittest.cc? > > > > > > > > Other than that (and the extension checking change), lgtm. > > > > > > > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || > > > (enable_dxt1 > > > > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > > > > On 2016/09/15 00:14:25, jbauman wrote: > > > > > > Will this actually guarantee that these SRGB formats are supported? > > > > > > > > > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc > and > > > > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > > > > extensions? > > > > > > > > > > You're right. It should be enabled if the desktop GL context has both > > > > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > > > > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct > either, > > > > since > > > > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > > > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > > > > > > > > > Probably we should check "feature_flags_.desktop_srgb_support && > > > > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we get > > > ANGLE > > > > to support it then we might have a better idea of what to do for ES2. > > > > > > That would be problematic -- I'm pretty sure Unity wants to use these > > compressed > > > texture formats in a WebGL 2.0 context. > > > > It looks like the block that's currently setting have_srgb also checks for > > IsWebGL1OrES2Context(), though I admit I can't quite see why. We could > probably > > remove the check in this case, assuming the new extension is specced > correctly. > > I think this is because that check is for enabling GL_EXT_sRGB, which isn't > necessary in ES 3 since it's core. > > (In which case, setting `have_srgb` only there is an error.) (And there should be a test for this on WebGL 2 contexts.)
On 2016/09/16 00:52:52, Kai Ninomiya wrote: > On 2016/09/16 00:49:51, Kai Ninomiya wrote: > > On 2016/09/16 00:37:16, jbauman wrote: > > > On 2016/09/16 00:31:14, Ken Russell wrote: > > > > On 2016/09/16 00:25:29, jbauman wrote: > > > > > Could you also add a test to feature_info_unittest.cc? > > > > > > > > > > Other than that (and the extension checking change), lgtm. > > > > > > > > > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || > > > > (enable_dxt1 > > > > > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > > > > > On 2016/09/15 00:14:25, jbauman wrote: > > > > > > > Will this actually guarantee that these SRGB formats are supported? > > > > > > > > > > > > > > It looks to me like this works with GL_EXT_texture_compression_s3tc > > and > > > > > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > > > > > extensions? > > > > > > > > > > > > You're right. It should be enabled if the desktop GL context has both > > > > > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > > > > > > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct > > either, > > > > > since > > > > > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > > > > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you know? > > > > > > > > > > > Probably we should check "feature_flags_.desktop_srgb_support && > > > > > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we > get > > > > ANGLE > > > > > to support it then we might have a better idea of what to do for ES2. > > > > > > > > That would be problematic -- I'm pretty sure Unity wants to use these > > > compressed > > > > texture formats in a WebGL 2.0 context. > > > > > > It looks like the block that's currently setting have_srgb also checks for > > > IsWebGL1OrES2Context(), though I admit I can't quite see why. We could > > probably > > > remove the check in this case, assuming the new extension is specced > > correctly. > > > > I think this is because that check is for enabling GL_EXT_sRGB, which isn't > > necessary in ES 3 since it's core. > > > > (In which case, setting `have_srgb` only there is an error.) > > (And there should be a test for this on WebGL 2 contexts.) Your new conformance/extensions/webgl-compressed-texture-s3tc-srgb.html test should run against a WebGL 2.0 context if that's the context type selected in the harness.
On 2016/09/16 01:23:09, Ken Russell wrote: > On 2016/09/16 00:52:52, Kai Ninomiya wrote: > > On 2016/09/16 00:49:51, Kai Ninomiya wrote: > > > On 2016/09/16 00:37:16, jbauman wrote: > > > > On 2016/09/16 00:31:14, Ken Russell wrote: > > > > > On 2016/09/16 00:25:29, jbauman wrote: > > > > > > Could you also add a test to feature_info_unittest.cc? > > > > > > > > > > > > Other than that (and the extension checking change), lgtm. > > > > > > > > > > > > On 2016/09/15 20:45:35, Kai Ninomiya wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > > > File gpu/command_buffer/service/feature_info.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2337833002/diff/20001/gpu/command_buffer/serv... > > > > > > > gpu/command_buffer/service/feature_info.cc:613: if ((have_s3tc || > > > > > (enable_dxt1 > > > > > > > && enable_dxt3 && enable_dxt5)) && have_srgb) { > > > > > > > On 2016/09/15 00:14:25, jbauman wrote: > > > > > > > > Will this actually guarantee that these SRGB formats are > supported? > > > > > > > > > > > > > > > > It looks to me like this works with > GL_EXT_texture_compression_s3tc > > > and > > > > > > > > GL_EXT_texture_sRGB, but is it defined with other combinations of > > > > > > extensions? > > > > > > > > > > > > > > You're right. It should be enabled if the desktop GL context has > both > > > > > > > EXT_texture_compression_s3tc and EXT_texture_sRGB. > > > > > > > > > > > > > > But I'm not sure whether (have_s3tc && have_srgb) would be correct > > > either, > > > > > > since > > > > > > > these extension checks are at the ES level. In ES, EXT_sRGB plus > > > > > > > EXT_texture_compression_s3tc does not provide s3tc-srgb. Do you > know? > > > > > > > > > > > > > Probably we should check "feature_flags_.desktop_srgb_support && > > > > > > IsWebGL1OrES2Context()" instead of have_srgb, just to be safe. Once we > > get > > > > > ANGLE > > > > > > to support it then we might have a better idea of what to do for ES2. > > > > > > > > > > That would be problematic -- I'm pretty sure Unity wants to use these > > > > compressed > > > > > texture formats in a WebGL 2.0 context. > > > > > > > > It looks like the block that's currently setting have_srgb also checks for > > > > IsWebGL1OrES2Context(), though I admit I can't quite see why. We could > > > probably > > > > remove the check in this case, assuming the new extension is specced > > > correctly. > > > > > > I think this is because that check is for enabling GL_EXT_sRGB, which isn't > > > necessary in ES 3 since it's core. > > > > > > (In which case, setting `have_srgb` only there is an error.) > > > > (And there should be a test for this on WebGL 2 contexts.) > > Your new conformance/extensions/webgl-compressed-texture-s3tc-srgb.html test > should run against a WebGL 2.0 context if that's the context type selected in > the harness. Ah, yes, I just tried that and it does. Of course, it skips since the extension is unavailable.
The CQ bit was checked by kainino@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb (Enables support on non-ANGLE desktop platforms (Mac/Linux).) This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and Android platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and Android platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
jbauman, I updated the extension checking and added tests. How does this look to you?
On 2016/09/16 23:45:58, Kai Ninomiya wrote: > jbauman, I updated the extension checking and added tests. How does this look to > you? lgtm
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=haraken@chromium.org ==========
TBRing haraken for change to modules_idl_files.gni
The CQ bit was checked by kainino@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2337833002/#ps120001 (title: "add test and fix feature detection (enabled on webgl2, android/tegra)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=haraken@chromium.org ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=haraken@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=haraken@chromium.org ========== to ========== Implement WEBGL_compressed_texture_s3tc_srgb Enables support on non-ANGLE desktop platforms (Mac/Linux) and GLES platforms supporting NV_sRGB_formats (Tegra). This is tested by WebGL conformance test conformance/extensions/webgl-compressed-texture-s3tc-srgb.html BUG=630498 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel TBR=haraken@chromium.org Committed: https://crrev.com/692194d1c8295d88a60230f24fee887026dae424 Cr-Commit-Position: refs/heads/master@{#419333} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/692194d1c8295d88a60230f24fee887026dae424 Cr-Commit-Position: refs/heads/master@{#419333} |
