|
|
Created:
4 years, 8 months ago by Kimmo Kinnunen Modified:
4 years, 7 months ago CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncommand_buffer: Avoid glGetError after lost context
Do not call glGetError for debug info after context has been lost.
BUG=581634
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/08a66a51c06a74f5ad20d16f8364254c5606b310
Cr-Commit-Position: refs/heads/master@{#390332}
Patch Set 1 #
Total comments: 2
Messages
Total messages: 14 (5 generated)
Description was changed from ========== command_buffer: Avoid glGetError after lost context Do not call glGetError for debug info after context has been lost. BUG=581634 ========== to ========== command_buffer: Avoid glGetError after lost context Do not call glGetError for debug info after context has been lost. BUG=581634 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ==========
kkinnunen@nvidia.com changed reviewers: + zmo@google.com
zmo@chromium.org changed reviewers: + zmo@chromium.org
https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4476: if (DebugImpl && debug() && !WasContextLost()) { I try to understand 1) what difference does this make? 2) Don't you still want to clear the error from the driver, just not expose them through log and local_set_gl_error?
https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... gpu/command_buffer/service/gles2_cmd_decoder.cc:4476: if (DebugImpl && debug() && !WasContextLost()) { On 2016/04/26 17:19:39, Zhenyao Mo wrote: > I try to understand > > 1) what difference does this make? > 2) Don't you still want to clear the error from the driver, just not expose them > through log and local_set_gl_error? I was thinking more in terms of: After the context is lost, you can not make the context current anymore. If you do not have the context current, you are not allowed to call a gl function. glGetError is a gl function. If you have debugging on and have context lost, it will call glGetError without the correct context.
On 2016/04/26 18:09:23, Kimmo Kinnunen wrote: > https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/1922703004/diff/1/gpu/command_buffer/service/... > gpu/command_buffer/service/gles2_cmd_decoder.cc:4476: if (DebugImpl && debug() > && !WasContextLost()) { > On 2016/04/26 17:19:39, Zhenyao Mo wrote: > > I try to understand > > > > 1) what difference does this make? > > 2) Don't you still want to clear the error from the driver, just not expose > them > > through log and local_set_gl_error? > > I was thinking more in terms of: > After the context is lost, you can not make the context current anymore. > If you do not have the context current, you are not allowed to call a gl > function. > glGetError is a gl function. > > If you have debugging on and have context lost, it will call glGetError without > the correct context. There is GL_CONTEXT_LOST error, so I assume glGetError can be called even the context is lost? My first question was, does this CL make any tests passing? or make debugging easier? I try to understand the impact and motivation of doing this.
On 2016/04/26 18:31:18, Zhenyao Mo wrote: > > If you have debugging on and have context lost, it will call glGetError > without > > the correct context. > > There is GL_CONTEXT_LOST error, so I assume glGetError can be called even the > context is lost? Yeah, but the decoder does not let the context be switched. > My first question was, does this CL make any tests passing? or make debugging > easier? I try to understand the impact and motivation of doing this. Yeah, debugging easier. E.g. I sometimes have to run the command buffer with logging on. After fixing the EGL interface of command buffer, the system needs to mark the service gl context lost when destroying the client context. This happens for example when running the gles2 conformance tests. In these cases, if the logging is on, this piece of code is run, and as such, it cause spurious failure.
lgtm Thanks for the explanation.
The CQ bit was checked by kkinnunen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1922703004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1922703004/1
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/08a66a51c06a74f5ad20d16f8364254c5606b310 Cr-Commit-Position: refs/heads/master@{#390332}
Message was sent while issue was closed.
Description was changed from ========== command_buffer: Avoid glGetError after lost context Do not call glGetError for debug info after context has been lost. BUG=581634 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== command_buffer: Avoid glGetError after lost context Do not call glGetError for debug info after context has been lost. BUG=581634 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/08a66a51c06a74f5ad20d16f8364254c5606b310 Cr-Commit-Position: refs/heads/master@{#390332} ========== |