Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2770713005:
Implement most of the remaining entry points in the passthrough cmd decoder. (Closed)
|
Created:
3 years, 9 months ago by Geoff Lang Modified:
3 years, 9 months ago Reviewers:
Zhenyao Mo CC:
chromium-reviews, piman+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement most of the remaining entry points in the passthrough cmd decoder.
BUG=602688
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2770713005
Cr-Commit-Position: refs/heads/master@{#459216}
Committed: https://chromium.googlesource.com/chromium/src/+/e5677919d8e367737058eefab33419a575938695
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address zmo's comments #
Total comments: 3
Patch Set 3 : Always check the length parameter. #
Messages
Total messages: 26 (15 generated)
Description was changed from ========== Implement most of the remaining entry points in the passthrough cmd decoder. BUG=602688 ========== to ========== Implement most of the remaining entry points in the passthrough cmd decoder. BUG=602688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 geofflang@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: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
geofflang@chromium.org changed reviewers: + zmo@chromium.org
PTAL
lgtm with a few minor suggestions https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right): https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1020: GLint active_uniform_block_max_length = 0; nit: should be *max_name_length. https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1031: glGetActiveUniformBlockName(GetProgramServiceID(program, resources_), index, Can we cache the service_id above so we don't have to look for it again here? https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1034: *name = length > 0 ? std::string(buffer.data()) : std::string(); nit: can we DCHECK(name) before using it? https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1034: *name = length > 0 ? std::string(buffer.data()) : std::string(); To be on the safe side, can we DCHECK length < active_uniform_block_max_name_length) and std::string(buffer.data(), length)? https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1338: shader_source_length > 0 ? std::string(buffer.data()) : std::string(); Same suggestions here.
https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right): https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1020: GLint active_uniform_block_max_length = 0; On 2017/03/23 17:02:46, Zhenyao Mo wrote: > nit: should be *max_name_length. Done. https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1031: glGetActiveUniformBlockName(GetProgramServiceID(program, resources_), index, On 2017/03/23 17:02:46, Zhenyao Mo wrote: > Can we cache the service_id above so we don't have to look for it again here? Done. https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1034: *name = length > 0 ? std::string(buffer.data()) : std::string(); On 2017/03/23 17:02:46, Zhenyao Mo wrote: > To be on the safe side, can we DCHECK length < > active_uniform_block_max_name_length) and std::string(buffer.data(), length)? Done. https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1034: *name = length > 0 ? std::string(buffer.data()) : std::string(); On 2017/03/23 17:02:46, Zhenyao Mo wrote: > nit: can we DCHECK(name) before using it? name is safe to use, it is a local variable from the handler. https://codereview.chromium.org/2770713005/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1338: shader_source_length > 0 ? std::string(buffer.data()) : std::string(); On 2017/03/23 17:02:46, Zhenyao Mo wrote: > Same suggestions here. Done.
The CQ bit was checked by geofflang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2770713005/#ps20001 (title: "Address zmo's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2770713005/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right): https://codereview.chromium.org/2770713005/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1246: *infolog = info_log_len > 0 ? std::string(buffer.data(), info_log_len) I mean the same with std::string(buffer.data(), len). https://codereview.chromium.org/2770713005/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1341: ? std::string(buffer.data(), shader_source_length) The same with std::string(buffer.data(), len).
The CQ bit was unchecked by geofflang@chromium.org
https://codereview.chromium.org/2770713005/diff/20001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc (right): https://codereview.chromium.org/2770713005/diff/20001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc:1341: ? std::string(buffer.data(), shader_source_length) On 2017/03/23 17:45:56, Zhenyao Mo wrote: > The same with std::string(buffer.data(), len). For this one, the length should be exact but I can check it. For glGetActiveUniformBlockName, there is no name length query (just max length).
The CQ bit was checked by geofflang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2770713005/#ps40001 (title: "Always check the length parameter.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm again
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by geofflang@chromium.org
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": 1490299686630910,
"parent_rev": "3d3dfcd34e38fcecaa1ff14bcd12cbc98259f36c", "commit_rev":
"e5677919d8e367737058eefab33419a575938695"}
Message was sent while issue was closed.
Description was changed from ========== Implement most of the remaining entry points in the passthrough cmd decoder. BUG=602688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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 most of the remaining entry points in the passthrough cmd decoder. BUG=602688 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;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/2770713005 Cr-Commit-Position: refs/heads/master@{#459216} Committed: https://chromium.googlesource.com/chromium/src/+/e5677919d8e367737058eefab334... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/e5677919d8e367737058eefab334... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
