Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1674223002:
Only call gles2::Terminate when all Display objects are deleted (Closed)
|
Created:
4 years, 10 months ago by Sami Väisänen Modified:
4 years, 10 months ago CC:
chromium-reviews, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@egl_test_branch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly call gles2::Terminate when all Display objects are deleted
Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted.
BUG=skia:2992
Committed: https://crrev.com/c22b484a0aaabbfbce221b419da48bca080e7826
Cr-Commit-Position: refs/heads/master@{#376439}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Review fixes #Patch Set 3 : Fix whitespace #
Depends on Patchset: Messages
Total messages: 26 (11 generated)
Description was changed from ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 ========== to ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 ==========
svaisanen@nvidia.com changed reviewers: + kkinnunen@nvidia.com, piman@chromium.org
https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... File gpu/command_buffer/tests/egl_test.cc (right): https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:39: TestContext() { nit: indent. 'public:' should be at +1, the constructor should be at +2. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:79: { nit: { goes to end of previous line. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:86: { nit: ditto https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:94: nit: remove redundant blank line https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:123: // Overall this functionality is not aligned with the EGL spefication. Can we fix that? https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:128: nit: no blank line https://codereview.chromium.org/1674223002/diff/1/gpu/gles2_conform_support/e... File gpu/gles2_conform_support/egl/display.cc (right): https://codereview.chromium.org/1674223002/diff/1/gpu/gles2_conform_support/e... gpu/gles2_conform_support/egl/display.cc:54: if (g_command_buffer_gles_has_atexit_manager) { nit: indent (should be at +2, not +4)
PTAL https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... File gpu/command_buffer/tests/egl_test.cc (right): https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:39: TestContext() { On 2016/02/08 17:36:08, piman wrote: > nit: indent. 'public:' should be at +1, the constructor should be at +2. Acknowledged. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:79: { On 2016/02/08 17:36:08, piman wrote: > nit: { goes to end of previous line. Acknowledged. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:86: { On 2016/02/08 17:36:08, piman wrote: > nit: ditto Acknowledged. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:94: On 2016/02/08 17:36:08, piman wrote: > nit: remove redundant blank line Acknowledged. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:123: // Overall this functionality is not aligned with the EGL spefication. On 2016/02/08 17:36:08, piman wrote: > Can we fix that? This would require some refactoring in the implementation. Return a handle to same the same display object for each eglInitialize, and refactor the per context state from display to some context class (that doesn't seem yet to exist). And then refactor SkCommandBufferContext appropriately. https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:128: On 2016/02/08 17:36:08, piman wrote: > nit: no blank line Acknowledged. https://codereview.chromium.org/1674223002/diff/1/gpu/gles2_conform_support/e... File gpu/gles2_conform_support/egl/display.cc (right): https://codereview.chromium.org/1674223002/diff/1/gpu/gles2_conform_support/e... gpu/gles2_conform_support/egl/display.cc:54: if (g_command_buffer_gles_has_atexit_manager) { On 2016/02/08 17:36:08, piman wrote: > nit: indent (should be at +2, not +4) Acknowledged.
https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... File gpu/command_buffer/tests/egl_test.cc (right): https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... gpu/command_buffer/tests/egl_test.cc:123: // Overall this functionality is not aligned with the EGL spefication. On 2016/02/09 10:05:27, Sami Väisänen wrote: > On 2016/02/08 17:36:08, piman wrote: > > Can we fix that? > > This would require some refactoring in the implementation. Return a handle to > same the same display object for each eglInitialize, and refactor the per > context state from display to some context class (that doesn't seem yet to > exist). And then refactor SkCommandBufferContext appropriately. Right, can we do that? Otherwise it seems we're doing a workaround for a workaround for a bug. Can we fix the bug instead and remove workarounds?
On 2016/02/09 19:23:51, piman wrote: > https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... > File gpu/command_buffer/tests/egl_test.cc (right): > > https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... > gpu/command_buffer/tests/egl_test.cc:123: // Overall this functionality is not > aligned with the EGL spefication. > On 2016/02/09 10:05:27, Sami Väisänen wrote: > > On 2016/02/08 17:36:08, piman wrote: > > > Can we fix that? > > > > This would require some refactoring in the implementation. Return a handle to > > same the same display object for each eglInitialize, and refactor the per > > context state from display to some context class (that doesn't seem yet to > > exist). And then refactor SkCommandBufferContext appropriately. > > > Right, can we do that? > Otherwise it seems we're doing a workaround for a workaround for a bug. Can we > fix the bug instead and remove workarounds? I completely agree. It'd be good to address the bigger issue as well, I'm just not sure who's the person for that. However this fix for allocating/deallocating the TLS key only once is still needed in any case.
On 2016/02/10 07:38:23, Sami Väisänen wrote: > On 2016/02/09 19:23:51, piman wrote: > > > https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... > > File gpu/command_buffer/tests/egl_test.cc (right): > > > > > https://codereview.chromium.org/1674223002/diff/1/gpu/command_buffer/tests/eg... > > gpu/command_buffer/tests/egl_test.cc:123: // Overall this functionality is not > > aligned with the EGL spefication. > > On 2016/02/09 10:05:27, Sami Väisänen wrote: > > > On 2016/02/08 17:36:08, piman wrote: > > > > Can we fix that? > > > > > > This would require some refactoring in the implementation. Return a handle > to > > > same the same display object for each eglInitialize, and refactor the per > > > context state from display to some context class (that doesn't seem yet to > > > exist). And then refactor SkCommandBufferContext appropriately. > > > > > > Right, can we do that? > > Otherwise it seems we're doing a workaround for a workaround for a bug. Can we > > fix the bug instead and remove workarounds? > > I completely agree. It'd be good to address the bigger issue as well, I'm just > not sure who's the person for that. There's no good person - no-one on the chromium side is working on this. It sounds like y'all (NVIDIA) are, though? > However this fix for allocating/deallocating the TLS key only once is still > needed in any case. ok, lgtm for this.
On 2016/02/10 23:48:59, piman wrote: > On 2016/02/10 07:38:23, Sami Väisänen wrote: > > I completely agree. It'd be good to address the bigger issue as well, I'm just > > not sure who's the person for that. > > There's no good person - no-one on the chromium side is working on this. It > sounds like y'all (NVIDIA) are, though? It's a bit of unfortunate wormhole though -- command buffer code structure does not match that well with egl. We'll try to do what we can at some point. > > However this fix for allocating/deallocating the TLS key only once is still > > needed in any case. > > ok, lgtm for this. Great
The CQ bit was checked by svaisanen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1674223002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1674223002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by svaisanen@nvidia.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1674223002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1674223002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by svaisanen@nvidia.com
The CQ bit was checked by svaisanen@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1674223002/#ps40001 (title: "Fix whitespace")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1674223002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1674223002/40001
Message was sent while issue was closed.
Description was changed from ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 ========== to ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 ========== to ========== Only call gles2::Terminate when all Display objects are deleted Implementation in command buffer EGL calls gles2::Terminate and gles2::Initialize incorrectly. If there are multiple Display objects each will call Initialize and Terminate and creates and frees the TLS allocated context key twice. It should only be created once and freed once all the Displays are deleted. BUG=skia:2992 Committed: https://crrev.com/c22b484a0aaabbfbce221b419da48bca080e7826 Cr-Commit-Position: refs/heads/master@{#376439} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/c22b484a0aaabbfbce221b419da48bca080e7826 Cr-Commit-Position: refs/heads/master@{#376439}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1714023002/ by markusheintz@chromium.org. The reason for reverting is: Does not compile on win c:\b\build\slave\win\build\src\gpu\command_buffer\tests\egl_test.cc(87) : error C2059: syntax error : '(' c:\b\build\slave\win\build\src\gpu\command_buffer\tests\egl_test.cc(90) : error C2061: syntax error : identifier 'glEnableProc' c:\b\build\slave\win\build\src\gpu\command_buffer\tests\egl_test.cc(91) : error C3536: 'GLES2Enable': cannot be used before it is initialized c:\b\build\slave\win\build\src\gpu\command_buffer\tests\egl_test.cc(91) : error C2064: term does not evaluate to a function taking 1 arguments https://build.chromium.org/p/chromium/builders/Win/builds/40478/steps/compile.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
