Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680703003:
Reland of Select correct copy method for DoCopySubTexture (Closed)
DescriptionSelect correct copy method for DoCopySubTexture
BUG=687180, 691741
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/2680703003
Cr-Commit-Position: refs/heads/master@{#450286}
Committed: https://chromium.googlesource.com/chromium/src/+/292b01d4c6d8f83a8973dc11ad76cdd63b1fb33c
Patch Set 1 #
Total comments: 4
Patch Set 2 : rebase only #Patch Set 3 : fix perf regression on chromeos #
Total comments: 1
Messages
Total messages: 31 (16 generated)
Description was changed from ========== Select correct copy method for DoCopySubTexture BUG=687180 ========== to ========== Select correct copy method for DoCopySubTexture BUG=687180 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
https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:292: source_texture, 0, internal_format, 0, 0, 0, 0, width_, height_, internal_format may be non-color-renderable. It caused CopySubTexture hit NOTREACHED() at https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_cop.... See bug: https://bugs.chromium.org/p/chromium/issues/detail?id=687180.
lgtm https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:266: if (GLES2Util::IsSizedColorFormat(internal_format)) { I have a question: if the format is float, even if the channel bits do not match, can we still do the copying? As far as I understand, the bits matching rule only applies to fixed point to fixed point copying, not fixed point to float point copying.
qiankun.miao@intel.com changed reviewers: + dongseong.hwang@intel.com
I am landing this CL as-is to see if the bug would be fixed. I think we still need to solve the format issue below. https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:266: if (GLES2Util::IsSizedColorFormat(internal_format)) { On 2017/02/07 18:58:07, Zhenyao Mo wrote: > I have a question: if the format is float, even if the channel bits do not > match, can we still do the copying? As far as I understand, the bits matching > rule only applies to fixed point to fixed point copying, not fixed point to > float point copying. I didn't see the bits matching rule only applies to fixed-point-to-fixed-point copying, see validation for CopyTexImage2D: https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec.... But there is another problem, see my commetns: https://codereview.chromium.org/2543123003/#msg35. Some formats (e.g. RGBA16I, RGBA16UI, RGBA32I, RGBA32UI) are not supported by DoCopySubTexture yet. This is still a big potential issue to be fixed.
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": 1, "attempt_start_ts": 1486525482346980, "parent_rev":
"3abea62a6cf6a2e0ca7e1026923f0cd0f5e45b3c", "commit_rev":
"a8450b810e89819add240a319586c899683dff9f"}
Message was sent while issue was closed.
Description was changed from ========== Select correct copy method for DoCopySubTexture BUG=687180 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 ========== Select correct copy method for DoCopySubTexture BUG=687180 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/2680703003 Cr-Commit-Position: refs/heads/master@{#448905} Committed: https://chromium.googlesource.com/chromium/src/+/a8450b810e89819add240a319586... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/a8450b810e89819add240a319586...
Message was sent while issue was closed.
Thanks Qiankun for your excellent work on this code path and continuing to follow up on these issues. Apologies for not keeping up with your code reviews.
Message was sent while issue was closed.
On 2017/02/08 03:44:23, qiankun wrote: > I am landing this CL as-is to see if the bug would be fixed. I think we still > need to solve the format issue below. > > https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... > File > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > (right): > > https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:266: > if (GLES2Util::IsSizedColorFormat(internal_format)) { > On 2017/02/07 18:58:07, Zhenyao Mo wrote: > > I have a question: if the format is float, even if the channel bits do not > > match, can we still do the copying? As far as I understand, the bits matching > > rule only applies to fixed point to fixed point copying, not fixed point to > > float point copying. > > I didn't see the bits matching rule only applies to fixed-point-to-fixed-point > copying, see validation for CopyTexImage2D: > https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec.... > > But there is another problem, see my commetns: > https://codereview.chromium.org/2543123003/#msg35. Some formats (e.g. RGBA16I, > RGBA16UI, RGBA32I, RGBA32UI) are not supported by DoCopySubTexture yet. This is > still a big potential issue to be fixed. I don't think we ever want to implement Gpu-to-Gpu copying from RGBA8 to higher bit int formats because the semantics is undefined / ambiguous.
Message was sent while issue was closed.
https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2680703003/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:266: if (GLES2Util::IsSizedColorFormat(internal_format)) { On 2017/02/08 03:44:23, qiankun wrote: > On 2017/02/07 18:58:07, Zhenyao Mo wrote: > > I have a question: if the format is float, even if the channel bits do not > > match, can we still do the copying? As far as I understand, the bits matching > > rule only applies to fixed point to fixed point copying, not fixed point to > > float point copying. > > I didn't see the bits matching rule only applies to fixed-point-to-fixed-point > copying, see validation for CopyTexImage2D: > https://cs.chromium.org/chromium/src/gpu/command_buffer/service/gles2_cmd_dec.... > > But there is another problem, see my commetns: > https://codereview.chromium.org/2543123003/#msg35. Some formats (e.g. RGBA16I, > RGBA16UI, RGBA32I, RGBA32UI) are not supported by DoCopySubTexture yet. This is > still a big potential issue to be fixed. We don't have plan to use CMAA on float texture. Currently, RGB, RGBA, BGRA, SRGB support are enough, which DrawingBuffer requires. In the future, skia might use it for default framebuffer, and I think those format are still enough. Thanks and lgtm after fact. :)
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in https://codereview.chromium.org/2695833003/ by marcheu@chromium.org. The reason for reverting is: The new DoCopySubTexture causes a performance regression on WebGL on Chrome OS. BUG=chromium:691741 TEST=on celes, webgl performance is back to normal .
Message was sent while issue was closed.
Description was changed from ========== Select correct copy method for DoCopySubTexture BUG=687180 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/2680703003 Cr-Commit-Position: refs/heads/master@{#448905} Committed: https://chromium.googlesource.com/chromium/src/+/a8450b810e89819add240a319586... ========== to ========== Select correct copy method for DoCopySubTexture BUG=687180 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 ========== Select correct copy method for DoCopySubTexture BUG=687180 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 ========== Select correct copy method for DoCopySubTexture BUG=687180, 691741 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 ==========
Please review it again. https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc (right): https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:277: bool color_renderable = Immutable RGB8 texture was treated as non-color-renderable before. So, DoCopySubTexture went DIRECT_COPY path other than DIRECT_DRAW path. DIRECT_DRAW path is faster than DIRECT_COPY path on chromeos.
The CQ bit was checked by qiankun.miao@intel.com 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...
On 2017/02/14 03:21:33, qiankun wrote: > Please review it again. > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > File > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > (right): > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:277: > bool color_renderable = > Immutable RGB8 texture was treated as non-color-renderable before. So, > DoCopySubTexture went DIRECT_COPY path other than DIRECT_DRAW path. DIRECT_DRAW > path is faster than DIRECT_COPY path on chromeos. LGTM if it's tested not to regress perf
On 2017/02/14 03:53:24, Zhenyao Mo wrote: > On 2017/02/14 03:21:33, qiankun wrote: > > Please review it again. > > > > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > > File > > > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > > (right): > > > > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:277: > > bool color_renderable = > > Immutable RGB8 texture was treated as non-color-renderable before. So, > > DoCopySubTexture went DIRECT_COPY path other than DIRECT_DRAW path. > DIRECT_DRAW > > path is faster than DIRECT_COPY path on chromeos. > > LGTM if it's tested not to regress perf Agreed. I defer to Mo's review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/14 03:53:24, Zhenyao Mo wrote: > On 2017/02/14 03:21:33, qiankun wrote: > > Please review it again. > > > > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > > File > > > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc > > (right): > > > > > https://codereview.chromium.org/2680703003/diff/40001/gpu/command_buffer/serv... > > > gpu/command_buffer/service/gles2_cmd_apply_framebuffer_attachment_cmaa_intel.cc:277: > > bool color_renderable = > > Immutable RGB8 texture was treated as non-color-renderable before. So, > > DoCopySubTexture went DIRECT_COPY path other than DIRECT_DRAW path. > DIRECT_DRAW > > path is faster than DIRECT_COPY path on chromeos. > > LGTM if it's tested not to regress perf FPS of http://webglsamples.org/aquarium/aquarium.html with 4000 fishes changed from ~30 to ~50 when applying patchset 3 locally.
The CQ bit was checked by qiankun.miao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from dongseong.hwang@intel.com Link to the patchset: https://codereview.chromium.org/2680703003/#ps40001 (title: "fix perf regression on chromeos")
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": 1487055724931410,
"parent_rev": "e45268fb9d6c1c9c5a57617f6d16e4795c349d69", "commit_rev":
"292b01d4c6d8f83a8973dc11ad76cdd63b1fb33c"}
Message was sent while issue was closed.
Description was changed from ========== Select correct copy method for DoCopySubTexture BUG=687180, 691741 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 ========== Select correct copy method for DoCopySubTexture BUG=687180, 691741 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/2680703003 Cr-Commit-Position: refs/heads/master@{#450286} Committed: https://chromium.googlesource.com/chromium/src/+/292b01d4c6d8f83a8973dc11ad76... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/292b01d4c6d8f83a8973dc11ad76... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
