Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2543123003:
gpu, cmaa: improve |do_copy| condition logic (Closed)
Descriptiongpu, cmaa: improve |do_copy| condition logic
ImageTexture binding is supported for only immutable texture [1]
[1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml
CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8.
However, in GLES2 context, immutable texture can have RGBA internal format, because of
https://codereview.chromium.org/2458103002/
This CL makes it check immutable texture directly.
BUG=535198
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/2543123003
Cr-Commit-Position: refs/heads/master@{#443326}
Committed: https://chromium.googlesource.com/chromium/src/+/5b85620a5e701039630f96339a6d1b4ceb5b14f0
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add GetTexture() instead of IsTextureAttachmentImmutable() #Patch Set 3 : use existing attachment->object_name() #
Total comments: 4
Messages
Total messages: 36 (23 generated)
Description was changed from ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 ========== to ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 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 checked by dongseong.hwang@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...
dongseong.hwang@intel.com changed reviewers: + piman@chromium.org
piman@, could you review? It's follow-up CL of https://codereview.chromium.org/2458103002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:246: attachment->IsTextureAttachmentImmutable() && Well, you already know you have a texture attachment, so, rather than adding an extra API, why don't you grab the TextureRef/Texture from the TextureManager, and have access to all fields that you may need (e.g. you probably also want to check that if internal_format == GL_RGBA, that type == GL_UNSIGNED_BYTE, because it could be e.g. GL_FLOAT or GL_HALF_FLOAT instead).
The CQ bit was checked by dongseong.hwang@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...
Thanks for reviewing. Could you review again? https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:246: attachment->IsTextureAttachmentImmutable() && On 2016/12/02 20:36:46, piman wrote: > Well, you already know you have a texture attachment, so, rather than adding an > extra API, why don't you grab the TextureRef/Texture from the TextureManager, We cannot get TextureRef/Texture from Framebuffer::Attachemnt. New patch set added GetTexture(), instead of ad-hoc IsTextureAttachmentImmutable(). > and have access to all fields that you may need (e.g. you probably also want to > check that if internal_format == GL_RGBA, that type == GL_UNSIGNED_BYTE, because > it could be e.g. GL_FLOAT or GL_HALF_FLOAT instead). As immutable texture is only allowed, this code needs to know only internal_format. void glTexStorage2D(GLenum target, GLsizei levels, GLenum internalformat, GLsizei width, GLsizei height);
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:246: attachment->IsTextureAttachmentImmutable() && On 2016/12/02 23:40:24, dshwang wrote: > On 2016/12/02 20:36:46, piman wrote: > > Well, you already know you have a texture attachment, so, rather than adding > an > > extra API, why don't you grab the TextureRef/Texture from the TextureManager, > > We cannot get TextureRef/Texture from Framebuffer::Attachemnt. New patch set > added GetTexture(), instead of ad-hoc IsTextureAttachmentImmutable(). attachment->object_name() gives you the client texture id. TextureManager::GetTexture(client_id) will give you the TextureRef. > > > and have access to all fields that you may need (e.g. you probably also want > to > > check that if internal_format == GL_RGBA, that type == GL_UNSIGNED_BYTE, > because > > it could be e.g. GL_FLOAT or GL_HALF_FLOAT instead). > > As immutable texture is only allowed, this code needs to know only > internal_format. > > void glTexStorage2D(GLenum target, GLsizei levels, GLenum internalformat, > GLsizei width, GLsizei height);
The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run
Description was changed from ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 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 ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 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 ==========
dongseong.hwang@intel.com changed reviewers: + zmo@google.com
dongseong.hwang@intel.com changed reviewers: + zmo@chromium.org - zmo@google.com
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Zhenyao, could you review because piman is on leave? We almost made a consensus. https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:246: attachment->IsTextureAttachmentImmutable() && On 2016/12/05 20:07:08, piman - On leave - No reviews wrote: > On 2016/12/02 23:40:24, dshwang wrote: > > On 2016/12/02 20:36:46, piman wrote: > > > Well, you already know you have a texture attachment, so, rather than adding > > an > > > extra API, why don't you grab the TextureRef/Texture from the > TextureManager, > > > > We cannot get TextureRef/Texture from Framebuffer::Attachemnt. New patch set > > added GetTexture(), instead of ad-hoc IsTextureAttachmentImmutable(). > > attachment->object_name() gives you the client texture id. > TextureManager::GetTexture(client_id) will give you the TextureRef. > > > > > > > and have access to all fields that you may need (e.g. you probably also want > > to > > > check that if internal_format == GL_RGBA, that type == GL_UNSIGNED_BYTE, > > because > > > it could be e.g. GL_FLOAT or GL_HALF_FLOAT instead). > > > > As immutable texture is only allowed, this code needs to know only > > internal_format. > > > > void glTexStorage2D(GLenum target, GLsizei levels, GLenum internalformat, > > GLsizei width, GLsizei height); > Done. Thank you for educating :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.h (right): https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.h:39: TextureManager* texture_manager); I prefer exposing texture_manager() to GLES2Decoder so we don't have to pass it as an arg, but it's not a big deal.
thanks for review and sorry for late response. It still works without conflict, so let me land it. https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.h (right): https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.h:39: TextureManager* texture_manager); On 2016/12/09 18:21:05, Zhenyao Mo wrote: > I prefer exposing texture_manager() to GLES2Decoder so we don't have to pass it > as an arg, but it's not a big deal. That's good idea, but currently this kind of method is exposed at GLES2DecoderImpl, which is defined in cc file. Few managers are exposed at GLES2Decoder, but all are for unittests. Let me keep consistency with existing code.
The CQ bit was checked by dongseong.hwang@intel.com
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
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dongseong.hwang@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": 40001, "attempt_start_ts": 1484247416041510,
"parent_rev": "5889b84078b874e7682a4634b1fce6eedcac67fb", "commit_rev":
"5b85620a5e701039630f96339a6d1b4ceb5b14f0"}
Message was sent while issue was closed.
Description was changed from ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 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 ========== gpu, cmaa: improve |do_copy| condition logic ImageTexture binding is supported for only immutable texture [1] [1] https://www.khronos.org/opengles/sdk/docs/man31/html/glBindImageTexture.xhtml CMAA needs to use ImageTexture binding, so it checkes internal format is RGBA8. However, in GLES2 context, immutable texture can have RGBA internal format, because of https://codereview.chromium.org/2458103002/ This CL makes it check immutable texture directly. BUG=535198 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/2543123003 Cr-Commit-Position: refs/heads/master@{#443326} Committed: https://chromium.googlesource.com/chromium/src/+/5b85620a5e701039630f96339a6d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5b85620a5e701039630f96339a6d...
Message was sent while issue was closed.
qiankun.miao@intel.com changed reviewers: + qiankun.miao@intel.com
Message was sent while issue was closed.
https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:259: copier->DoCopySubTexture( Hi DS, DoCopySubTexture only supported GLES2 formats at this point. I extended it to support part of ES3 formats: https://codereview.chromium.org/2479513002/. A few ES3 formats is still not supported yet, e.g. RGBA16I, RGBA16UI, RGBA32I, RGBA32UI, because that it is not clear how to translate 8-bit format to 16-bit or 32-bit format, see the Rationale under texImage2D section in WebGL 2.0 spec: https://www.khronos.org/registry/webgl/specs/latest/2.0/index.html#3.7.6. So it may have potential issues using DoCopySubTexture here. What do you think? I proposed a CL to fix part issue:https://codereview.chromium.org/2680703003/. But I think we still need to support other ES3 formats if we use DoCopySubTexture here or find other methods to do copy.
Message was sent while issue was closed.
https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2543123003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:259: copier->DoCopySubTexture( On 2017/02/08 03:19:56, qiankun wrote: > Hi DS, > > DoCopySubTexture only supported GLES2 formats at this point. I extended it to > support part of ES3 formats: https://codereview.chromium.org/2479513002/. A few > ES3 formats is still not supported yet, e.g. RGBA16I, RGBA16UI, RGBA32I, > RGBA32UI, because that it is not clear how to translate 8-bit format to 16-bit > or 32-bit format, see the Rationale under texImage2D section in WebGL 2.0 spec: > https://www.khronos.org/registry/webgl/specs/latest/2.0/index.html#3.7.6. So it > may have potential issues using DoCopySubTexture here. What do you think? I > proposed a CL to fix part issue:https://codereview.chromium.org/2680703003/. But > I think we still need to support other ES3 formats if we use DoCopySubTexture > here or find other methods to do copy. Hi Qiankun, CMAA requires only RGB, RGBA, and probably SRGBA. Only default framebuffer object for DrawingBuffer uses CMAA. So your https://codereview.chromium.org/2479513002/ is enough for CMAA. Thank you! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
