Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2609393002:
Add CopyTexture gl_tests for level == 0 (Closed)
DescriptionAdd CopyTexture gl_tests for level == 0
CopyTextureLevel test is added for both ES2 and ES3. Move testing code
to a common RunCopyTexture function.
BUG=612542
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
Review-Url: https://codereview.chromium.org/2609393002
Cr-Commit-Position: refs/heads/master@{#441818}
Committed: https://chromium.googlesource.com/chromium/src/+/ae0ddfaace8dd20347b3898655f0209583ad8b96
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add TODO to support level > 0 #Messages
Total messages: 13 (6 generated)
Description was changed from ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to an common RunCopyTexture function. BUG=612542 ========== to ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to an common RunCopyTexture function. BUG=612542 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 ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to an common RunCopyTexture function. BUG=612542 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 ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to a common RunCopyTexture function. BUG=612542 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 ==========
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, yunchao.he@intel.com, zmo@chromium.org
PTAL. Some preliminary work to support level > 0. I will try to make each CL not very big for easy review. Also try to reduce risk when a large patch is reverted.
https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:328: glCopyTextureCHROMIUM(textures_[0], textures_[1], Could you please add a comment here that this function will be upgraded to also take in tex level information? Otherwise this code doesn't make sense because you only set up a certain level but it's not reflected in the actual copying. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:337: glCopySubTextureCHROMIUM(textures_[0], textures_[1], 0, 0, 0, 0, width_, Same here. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:353: GLTestHelper::DrawTextureQuad( Again, the tex level information isn't reflected here, so this function also needs to be upgraded. Could you add a comment? Otherwise this code is very confusing. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:724: // Source level must be 0 in ES2 context. Even this can be lifted if you sample the source texture through a shader and write to the dest texture - this path is needed anyway.
https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:328: glCopyTextureCHROMIUM(textures_[0], textures_[1], On 2017/01/04 18:51:21, Zhenyao Mo wrote: > Could you please add a comment here that this function will be upgraded to also > take in tex level information? Otherwise this code doesn't make sense because > you only set up a certain level but it's not reflected in the actual copying. Done. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:337: glCopySubTextureCHROMIUM(textures_[0], textures_[1], 0, 0, 0, 0, width_, On 2017/01/04 18:51:21, Zhenyao Mo wrote: > Same here. Done. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:353: GLTestHelper::DrawTextureQuad( On 2017/01/04 18:51:21, Zhenyao Mo wrote: > Again, the tex level information isn't reflected here, so this function also > needs to be upgraded. Could you add a comment? Otherwise this code is very > confusing. Done. https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:724: // Source level must be 0 in ES2 context. On 2017/01/04 18:51:22, Zhenyao Mo wrote: > Even this can be lifted if you sample the source texture through a shader and > write to the dest texture - this path is needed anyway. We cannot do this if the source texture is not complete. Source texture may only specify only one level and this level may be greater than 0. We need to set minification filter to NEAREST_MIPMAP_NEAREST for level > 0 if we want to sample from that level. In such situation, source texture is not mipmap complete. Sampling from level > 0 of incomplete source texture will return (0, 0, 0, 1) in ES2. See page 87, ES2.0 spec in https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf. " Calling a sampler from a fragment shader will return (R, G, B, A) = (0, 0, 0, 1) if any of the following conditions are true: • A two-dimensional sampler is called, the minification filter is one that requires a mipmap (neither NEAREST nor LINEAR), and the sampler’s associated texture object is not complete. " Similar description in ES 3.0 spec: " The minification filter requires a mipmap (is neither NEAREST nor LINEAR), and the texture is not mipmap complete. " But we can do this for ES3. We can set base level to source_level then restore base level after glCopyTextureCHROMIUM is done.
On 2017/01/05 03:07:45, qiankun wrote: > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:328: > glCopyTextureCHROMIUM(textures_[0], textures_[1], > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > Could you please add a comment here that this function will be upgraded to > also > > take in tex level information? Otherwise this code doesn't make sense because > > you only set up a certain level but it's not reflected in the actual copying. > > Done. > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:337: > glCopySubTextureCHROMIUM(textures_[0], textures_[1], 0, 0, 0, 0, width_, > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > Same here. > > Done. > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:353: > GLTestHelper::DrawTextureQuad( > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > Again, the tex level information isn't reflected here, so this function also > > needs to be upgraded. Could you add a comment? Otherwise this code is very > > confusing. > > Done. > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:724: // Source > level must be 0 in ES2 context. > On 2017/01/04 18:51:22, Zhenyao Mo wrote: > > Even this can be lifted if you sample the source texture through a shader and > > write to the dest texture - this path is needed anyway. > > We cannot do this if the source texture is not complete. Source texture may only > specify only one level and this level may be greater than 0. We need to set > minification filter to NEAREST_MIPMAP_NEAREST for level > 0 if we want to sample > from that level. In such situation, source texture is not mipmap complete. > Sampling from level > 0 of incomplete source texture will return (0, 0, 0, 1) in > ES2. See page 87, ES2.0 spec in > https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf. > " > Calling a sampler from a fragment shader will return (R, G, B, A) = > (0, 0, 0, 1) if any of the following conditions are true: > • A two-dimensional sampler is called, the minification filter is one that > requires > a mipmap (neither NEAREST nor LINEAR), and the sampler’s associated texture > object is not complete. > " > > Similar description in ES 3.0 spec: > " > The minification filter requires a mipmap (is neither NEAREST nor LINEAR), > and the texture is not mipmap complete. > " > > But we can do this for ES3. We can set base level to source_level then restore > base level after glCopyTextureCHROMIUM is done. Thanks for explaining.
On 2017/01/05 18:00:10, Zhenyao Mo wrote: > On 2017/01/05 03:07:45, qiankun wrote: > > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > > File gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc (right): > > > > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:328: > > glCopyTextureCHROMIUM(textures_[0], textures_[1], > > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > > Could you please add a comment here that this function will be upgraded to > > also > > > take in tex level information? Otherwise this code doesn't make sense > because > > > you only set up a certain level but it's not reflected in the actual > copying. > > > > Done. > > > > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:337: > > glCopySubTextureCHROMIUM(textures_[0], textures_[1], 0, 0, 0, 0, width_, > > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > > Same here. > > > > Done. > > > > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:353: > > GLTestHelper::DrawTextureQuad( > > On 2017/01/04 18:51:21, Zhenyao Mo wrote: > > > Again, the tex level information isn't reflected here, so this function also > > > needs to be upgraded. Could you add a comment? Otherwise this code is very > > > confusing. > > > > Done. > > > > > https://codereview.chromium.org/2609393002/diff/1/gpu/command_buffer/tests/gl... > > gpu/command_buffer/tests/gl_copy_texture_CHROMIUM_unittest.cc:724: // Source > > level must be 0 in ES2 context. > > On 2017/01/04 18:51:22, Zhenyao Mo wrote: > > > Even this can be lifted if you sample the source texture through a shader > and > > > write to the dest texture - this path is needed anyway. > > > > We cannot do this if the source texture is not complete. Source texture may > only > > specify only one level and this level may be greater than 0. We need to set > > minification filter to NEAREST_MIPMAP_NEAREST for level > 0 if we want to > sample > > from that level. In such situation, source texture is not mipmap complete. > > Sampling from level > 0 of incomplete source texture will return (0, 0, 0, 1) > in > > ES2. See page 87, ES2.0 spec in > > https://www.khronos.org/registry/gles/specs/2.0/es_full_spec_2.0.25.pdf. > > " > > Calling a sampler from a fragment shader will return (R, G, B, A) = > > (0, 0, 0, 1) if any of the following conditions are true: > > • A two-dimensional sampler is called, the minification filter is one that > > requires > > a mipmap (neither NEAREST nor LINEAR), and the sampler’s associated texture > > object is not complete. > > " > > > > Similar description in ES 3.0 spec: > > " > > The minification filter requires a mipmap (is neither NEAREST nor LINEAR), > > and the texture is not mipmap complete. > > " > > > > But we can do this for ES3. We can set base level to source_level then restore > > base level after glCopyTextureCHROMIUM is done. > > Thanks for explaining. lgtm to unblock you
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483659168965290,
"parent_rev": "4a24995c28476982924693f7ea299d30e0103379", "commit_rev":
"ae0ddfaace8dd20347b3898655f0209583ad8b96"}
Message was sent while issue was closed.
Description was changed from ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to a common RunCopyTexture function. BUG=612542 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 ========== Add CopyTexture gl_tests for level == 0 CopyTextureLevel test is added for both ES2 and ES3. Move testing code to a common RunCopyTexture function. BUG=612542 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 Review-Url: https://codereview.chromium.org/2609393002 Cr-Commit-Position: refs/heads/master@{#441818} Committed: https://chromium.googlesource.com/chromium/src/+/ae0ddfaace8dd20347b3898655f0... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/ae0ddfaace8dd20347b3898655f0... |
