Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2826043002:
Do not share FBOs/VAOs/Transform feedback objects across contexts (Closed)
DescriptionDo not share FBOs/VAOs/Transform feedback objects across contexts
Framebuffer, vertex array and transform feedback objects are not shared
across opengl contexts. Therefore, instead of using ShareGroup's IdHandler,
we can manage IDs for these objects locally in GLES2Implementation itself.
BUG=722717
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/2826043002
Cr-Commit-Position: refs/heads/master@{#472883}
Committed: https://chromium.googlesource.com/chromium/src/+/2dfc7029286f40d73b131760e3b7467c68b8bb17
Patch Set 1 #Patch Set 2 : FBOs/VAOs should not be shared between contexts #
Total comments: 10
Patch Set 3 : @review comments #
Total comments: 10
Patch Set 4 : @review comments #
Total comments: 6
Patch Set 5 : Added service side changes #
Total comments: 4
Patch Set 6 : fixed unittests failures; zmo@ review comments #
Total comments: 7
Patch Set 7 : added DeleteFramebuffers() expectation #
Total comments: 3
Messages
Total messages: 74 (44 generated)
Description was changed from ========== [WIP] FBOs/VAOs should not be shared between contexts Framebuffer and vertex array objects are not shared between contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None ========== to ========== [WIP] FBOs/VAOs should not be shared between contexts Framebuffer and vertex array objects are not shared between contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ==========
Description was changed from ========== [WIP] FBOs/VAOs should not be shared between contexts Framebuffer and vertex array objects are not shared between contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ========== FBOs/VAOs should not be shared between contexts Framebuffer and vertex array objects are not shared between contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ==========
c.padhi@samsung.com changed reviewers: + jbauman@chromium.org, piman@chromium.org, zmo@chromium.org
I have made an attempt to manage IDs for FBOs/VAOs locally. Please review and let me know your opinion on this. Thank you.
The CQ bit was checked by zmo@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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:249: id_allocators_[i].reset(new IdAllocator()); Some of these allocators are never used. Why? https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_format.h (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:69: kTransformFeedbacks, Transform feedback objects are also not shared. https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:74: enum IdNamespaces { kFramebuffers, kQueries, kVertexArrays, kNumIdNamespaces }; Although in general I agree with the spirit of this CL, but I think splitting into SharedIdNamespaces and IdNamespaces is bad idea. Now kFramebuffers == kBuffers, what mechanism prevents/detects the mis-use of GetIdHandler(kFramebuffers) and GetIdHandler(kBuffers) get confused?
Below unittests are failing. GLES2ImplementationStrictSharedTest.CrossContextGenerationAutoFlushTestFramebuffers GLES2ImplementationStrictSharedTest.CrossContextGenerationTestFramebuffers GLES2ImplementationStrictSharedTest.FlushGenerationTestFramebuffers We now use IdAllocator which frees the glGenFramebuffer Ids when FreeID() is called. Due to this, these Ids get reused even though glDeleteFramebuffer command has not been flushed by glFlush causing these failures. Are these unittests still relevant, now that FBOs are not shared between contexts? Can these be removed or some modification is required? Sometimes, I get the below error on Linux. ERROR:gles2_cmd_decoder.cc(5313)] Error: 4 for Command kGenFramebuffersImmediate. This failure is caused because GetFramebuffer(client_ids[ii]) is valid in GLES2DecoderImpl::GenFramebuffersHelper(). Previous Id was reused after FreeID() was called in GLES2Implementation::DeleteFramebuffersHelper.However its corresponding Framebuffer was not yet removed on service side.Adding CommandBufferHelper::Finish() in GLES2Implementation ::DeleteFramebuffersHelper helped, but it doesn't seem to be the right solution. Please provide your input on this. https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:249: id_allocators_[i].reset(new IdAllocator()); On 2017/04/19 20:24:28, Zhenyao Mo wrote: > Some of these allocators are never used. Why? As of now, 3 allocators are created, one each for kFramebuffers, kQueries, kVertexArrays and all 3 are being are used. Please refer gles2_implementation_impl_autogen.h and gles2_implementation.cc in this CL. Which of these you think are never used? https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_format.h (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:69: kTransformFeedbacks, On 2017/04/19 20:24:28, Zhenyao Mo wrote: > Transform feedback objects are also not shared. Sure. I will add necessary changes for this too. https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:74: enum IdNamespaces { kFramebuffers, kQueries, kVertexArrays, kNumIdNamespaces }; On 2017/04/19 20:24:28, Zhenyao Mo wrote: > Although in general I agree with the spirit of this CL, but I think splitting > into SharedIdNamespaces and IdNamespaces is bad idea. Now kFramebuffers == > kBuffers, what mechanism prevents/detects the mis-use of > GetIdHandler(kFramebuffers) and GetIdHandler(kBuffers) get confused? I had to split this to create IdAllocators only for three(FBO/VAO/Query object) locally in GLES2Implementation. Is it not necessary to remove kFramebuffers, kQueries, kVertexArrays from IdNamespaces? Please suggest.
ping
On 2017/05/08 15:41:24, Chandan wrote: > ping I was on vacation. Sorry about the delay. Will get back to you soon.
On 2017/05/09 00:04:54, Zhenyao Mo wrote: > On 2017/05/08 15:41:24, Chandan wrote: > > ping > > I was on vacation. Sorry about the delay. Will get back to you soon. Sure. Thank you.
The CQ bit was checked by zmo@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...
https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_format.h (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:74: enum IdNamespaces { kFramebuffers, kQueries, kVertexArrays, kNumIdNamespaces }; On 2017/04/20 10:56:07, Chandan wrote: > On 2017/04/19 20:24:28, Zhenyao Mo wrote: > > Although in general I agree with the spirit of this CL, but I think splitting > > into SharedIdNamespaces and IdNamespaces is bad idea. Now kFramebuffers == > > kBuffers, what mechanism prevents/detects the mis-use of > > GetIdHandler(kFramebuffers) and GetIdHandler(kBuffers) get confused? > > I had to split this to create IdAllocators only for three(FBO/VAO/Query object) > locally in GLES2Implementation. Is it not necessary to remove kFramebuffers, > kQueries, kVertexArrays from IdNamespaces? Please suggest. Per offline discussion with piman, we can use enum class in c++ 11. This will prevent from misuse, say both kFramebuffers and kBuffers are 0. Of course then you will need to manually define the mapping from enums to indices, but I think it's worth it. https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:82: static_assert(kTextures == 3, "kTextures should equal 4"); These need to be updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Added Transform feedback objects to the list. Uploaded patch set 2 based on my assumption of review comments. @Zhenyao Mo, PTAL. Thank you. https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... File gpu/command_buffer/common/gles2_cmd_format.h (right): https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:74: enum IdNamespaces { kFramebuffers, kQueries, kVertexArrays, kNumIdNamespaces }; On 2017/05/09 22:21:54, Zhenyao Mo wrote: > On 2017/04/20 10:56:07, Chandan wrote: > > On 2017/04/19 20:24:28, Zhenyao Mo wrote: > > > Although in general I agree with the spirit of this CL, but I think > splitting > > > into SharedIdNamespaces and IdNamespaces is bad idea. Now kFramebuffers == > > > kBuffers, what mechanism prevents/detects the mis-use of > > > GetIdHandler(kFramebuffers) and GetIdHandler(kBuffers) get confused? > > > > I had to split this to create IdAllocators only for three(FBO/VAO/Query > object) > > locally in GLES2Implementation. Is it not necessary to remove kFramebuffers, > > kQueries, kVertexArrays from IdNamespaces? Please suggest. > > Per offline discussion with piman, we can use enum class in c++ 11. This will > prevent from misuse, say both kFramebuffers and kBuffers are 0. Of course then > you will need to manually define the mapping from enums to indices, but I think > it's worth it. By mapping, you mean converting enums to int using static_cast wherever applicable? Something like: static_cast<int>( id_namespaces::SharedIdNamespaces::kNumSharedIdNamespaces); https://codereview.chromium.org/2826043002/diff/20001/gpu/command_buffer/comm... gpu/command_buffer/common/gles2_cmd_format.h:82: static_assert(kTextures == 3, "kTextures should equal 4"); On 2017/05/09 22:21:54, Zhenyao Mo wrote: > These need to be updated. Need to add static_assert for other enum values as well? Here too, a static_cast<int> is required? static_assert(static_cast<int>(SharedIdNamespaces::kBuffers) == 0, "kBuffers should equal 0"); https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6050: alloc_code = (""" GetIdHandler(ToInt(SharedIdNamespaces::k%(resource_types)s))-> How do we format python files in chromium?
Description was changed from ========== FBOs/VAOs should not be shared between contexts Framebuffer and vertex array objects are not shared between contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ==========
Description was changed from ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 zmo@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: Your CL can not be processed by CQ because of: * Failed to parse additional trybots: missing master (or bucket) and trybuilders in `master.tryserver.chromium.android:android_optional_gpu_tests_rel;`. Correct syntax is "trymaster:bot1,bot2;trybucket:bot3,bot4".
Description was changed from ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 c.padhi@samsung.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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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/...)
You will need to make all bots green, otherwise I can't really do an effective review, knowing some things are definitely incorrect. I just asked for try job privilege for you, so you can click the CQ (Dry run) button by yourself. For now, you can look at the stdio of each failing tests and see what the issues are. You can also build and run them locally to reproduce and fix.
On 2017/05/10 23:17:11, Zhenyao Mo wrote: > You will need to make all bots green, otherwise I can't really do an effective > review, knowing some things are definitely incorrect. > > I just asked for try job privilege for you, so you can click the CQ (Dry run) > button by yourself. > > For now, you can look at the stdio of each failing tests and see what the issues > are. You can also build and run them locally to reproduce and fix. Thanks. I already have access to try bot. Below unittests are failing. GLES2ImplementationStrictSharedTest.CrossContextGenerationAutoFlushTestFramebuffers GLES2ImplementationStrictSharedTest.CrossContextGenerationTestFramebuffers GLES2ImplementationStrictSharedTest.FlushGenerationTestFramebuffers I am aware of these failures and had mentioned the same in one of my previous replies. We now use IdAllocator which frees the glGenFramebuffer Ids when FreeID() is called. Due to this, these Ids get reused even though glDeleteFramebuffer command has not been flushed by glFlush causing these failures. Are these unittests still relevant, now that FBOs are not shared between contexts? Can these be removed or some modification is required?
Yes, the three failing tests can be deleted. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6045: """ IdAllocator* id_allocator = GetIdAllocator(ToInt(IdNamespaces::k%s)); This defeats the purpose of using enum classes. The purpose is we don't accidentally pass a wrong resource_type into this function. If you map to int, then it can still happen. The ToInt needs to be moved into GetIdAllocator, and the function arg still needs to be enum class type. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6050: alloc_code = (""" GetIdHandler(ToInt(SharedIdNamespaces::k%(resource_types)s))-> On 2017/05/10 14:47:37, Chandan wrote: > How do we format python files in chromium? You can move """... to the next line https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6050: alloc_code = (""" GetIdHandler(ToInt(SharedIdNamespaces::k%(resource_types)s))-> Same here. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:4410: if (share_group_->bind_generates_resource()) This is unnecessary since we no longer share it across contexts. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... File gpu/command_buffer/client/share_group.h (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... gpu/command_buffer/client/share_group.h:36: inline int ToInt(Type t) { You don't need this. See my comments in other places. We should still pass enum class to GetIdAllocator() and other functions, and only do static_cast inside these functions.
The CQ bit was checked by c.padhi@samsung.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...
zmo@, Thanks for the review. PTAL @PS4. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... File gpu/command_buffer/build_gles2_cmd_buffer.py (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6045: """ IdAllocator* id_allocator = GetIdAllocator(ToInt(IdNamespaces::k%s)); On 2017/05/11 22:10:57, Zhenyao Mo wrote: > This defeats the purpose of using enum classes. The purpose is we don't > accidentally pass a wrong resource_type into this function. If you map to int, > then it can still happen. > > The ToInt needs to be moved into GetIdAllocator, and the function arg still > needs to be enum class type. Acknowledged. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/buil... gpu/command_buffer/build_gles2_cmd_buffer.py:6050: alloc_code = (""" GetIdHandler(ToInt(SharedIdNamespaces::k%(resource_types)s))-> On 2017/05/11 22:10:57, Zhenyao Mo wrote: > Same here. Done. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation.cc (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation.cc:4410: if (share_group_->bind_generates_resource()) On 2017/05/11 22:10:57, Zhenyao Mo wrote: > This is unnecessary since we no longer share it across contexts. Removed. https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... File gpu/command_buffer/client/share_group.h (right): https://codereview.chromium.org/2826043002/diff/40001/gpu/command_buffer/clie... gpu/command_buffer/client/share_group.h:36: inline int ToInt(Type t) { On 2017/05/11 22:10:57, Zhenyao Mo wrote: > You don't need this. See my comments in other places. We should still pass > enum class to GetIdAllocator() and other functions, and only do static_cast > inside these functions. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
A bunch of new tests begin to fail. piman took a look and pointed out that you also need to fix it on the service side. At the moment, framebuffer_manager_ is part of the ContextGroup (see gpu/command_buffer/service/context_group.h). Instead, it needs to be part of GLES2DecoderImpl like transform_feedback_manager_ and vertex_array_manager_ (see gpu/command_buffer/service/gles2_cmd_decoder.cc) So there are a bit more work to do to get this right. https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie... File gpu/command_buffer/client/gles2_implementation_impl_autogen.h (right): https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation_impl_autogen.h:787: helper_->CommandBufferHelper::Flush(); We also need to modify the code generator to get rid of this. It's no longer necessary because framebuffers are no longer in a share group. https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation_impl_autogen.h:874: helper_->CommandBufferHelper::Flush(); Same here. https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie... gpu/command_buffer/client/gles2_implementation_impl_autogen.h:3002: helper_->CommandBufferHelper::Flush(); Same here.
@zmo, PTAL @PS5.
Service side unittests are failing with below errors:
[ RUN ] Service/GLES3DecoderTest.Basic/0
unknown file: Failure
Unexpected mock function call - returning directly.
Function call: DeleteFramebuffersEXT(1, 0x7ffd002103b4)
Google Mock tried the following 1 expectation, but it didn't match:
../../gpu/command_buffer/service/test_helper.cc:736: EXPECT_CALL(*gl,
DeleteFramebuffersEXT(1, _))...
Expected: the expectation is active
Actual: it is retired
Expected: to be called once
Actual: called once - saturated and retired
[ FAILED ] Service/GLES3DecoderTest.Basic/0, where GetParam() = false (94 ms)
I am not that familiar with google mock test.
Your inputs, please.
https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie...
File gpu/command_buffer/client/gles2_implementation_impl_autogen.h (right):
https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_impl_autogen.h:787:
helper_->CommandBufferHelper::Flush();
On 2017/05/12 17:37:46, Zhenyao Mo wrote:
> We also need to modify the code generator to get rid of this. It's no longer
> necessary because framebuffers are no longer in a share group.
Done.
https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_impl_autogen.h:874:
helper_->CommandBufferHelper::Flush();
On 2017/05/12 17:37:46, Zhenyao Mo wrote:
> Same here.
Done.
https://codereview.chromium.org/2826043002/diff/60001/gpu/command_buffer/clie...
gpu/command_buffer/client/gles2_implementation_impl_autogen.h:3002:
helper_->CommandBufferHelper::Flush();
On 2017/05/12 17:37:46, Zhenyao Mo wrote:
> Same here.
Done.
The CQ bit was checked by c.padhi@samsung.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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
kbr@chromium.org changed reviewers: + kbr@chromium.org
Please create a bug for this and refer to it in the BUG= line. It looks like this is a fairly large and deep change and there should be a single place where the history of the changes is recorded. If possible please also provide some background for how you found this issue; thanks.
Description was changed from ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=None 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 ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=722717 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 ==========
On 2017/05/16 01:14:27, Ken Russell wrote: > Please create a bug for this and refer to it in the BUG= line. It looks like > this is a fairly large and deep change and there should be a single place where > the history of the changes is recorded. If possible please also provide some > background for how you found this issue; thanks. Done.
On 2017/05/16 13:08:42, Chandan wrote: > On 2017/05/16 01:14:27, Ken Russell wrote: > > Please create a bug for this and refer to it in the BUG= line. It looks like > > this is a fairly large and deep change and there should be a single place > where > > the history of the changes is recorded. If possible please also provide some > > background for how you found this issue; thanks. > > Done. Thanks. I think you'll need to study the unit tests more closely to get them to pass.
After addressing my latest comments, please take a look at the failing unit tests. It's not hard to understand. Usually the failing cause is quite obvious. If you can't figure out, we can take a look together at a certain one, and then you can tackle the rest. https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5732: if (!group_->bind_generates_resource()) { This is still needed. https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3262: framebuffer_manager_.get()); This is problematic. Now creating a new context can change this pointer cached in texture_manager, which is per group. It looks like texture_manager() needs to cache a list of framebuffer_managers instead of just one.
On 2017/05/16 18:53:40, Zhenyao Mo wrote: > After addressing my latest comments, please take a look at the failing unit > tests. It's not hard to understand. Usually the failing cause is quite > obvious. If you can't figure out, we can take a look together at a certain one, > and then you can tackle the rest. > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder.cc:5732: if > (!group_->bind_generates_resource()) { > This is still needed. > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > gpu/command_buffer/service/gles2_cmd_decoder.cc:3262: > framebuffer_manager_.get()); > This is problematic. Now creating a new context can change this pointer cached > in texture_manager, which is per group. It looks like texture_manager() needs > to cache a list of framebuffer_managers instead of just one. Thanks for the review. I will address your review comments tomorrow morning. It seems the reason for failure of these gpu_unittests is the same and is as follows: unknown file: Failure Unexpected mock function call - returning directly. Function call: DeleteFramebuffersEXT(1, 0x7ffd002103b4) Google Mock tried the following 1 expectation, but it didn't match: ../../gpu/command_buffer/service/test_helper.cc:736: EXPECT_CALL(*gl, DeleteFramebuffersEXT(1, _))... Expected: the expectation is active Actual: it is retired Expected: to be called once Actual: called once - saturated and retired I am not that familiar with Google Mock. Therefore, I am not able to figure out the above. Am I missing something here? Your inputs, please?
On 2017/05/16 19:14:43, Chandan wrote: > On 2017/05/16 18:53:40, Zhenyao Mo wrote: > > After addressing my latest comments, please take a look at the failing unit > > tests. It's not hard to understand. Usually the failing cause is quite > > obvious. If you can't figure out, we can take a look together at a certain > one, > > and then you can tackle the rest. > > > > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): > > > > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:5732: if > > (!group_->bind_generates_resource()) { > > This is still needed. > > > > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:3262: > > framebuffer_manager_.get()); > > This is problematic. Now creating a new context can change this pointer > cached > > in texture_manager, which is per group. It looks like texture_manager() needs > > to cache a list of framebuffer_managers instead of just one. > > Thanks for the review. I will address your review comments tomorrow morning. > > It seems the reason for failure of these gpu_unittests is the same and is as > follows: > > unknown file: Failure > > Unexpected mock function call - returning directly. > Function call: DeleteFramebuffersEXT(1, 0x7ffd002103b4) > Google Mock tried the following 1 expectation, but it didn't match: > > ../../gpu/command_buffer/service/test_helper.cc:736: EXPECT_CALL(*gl, > DeleteFramebuffersEXT(1, _))... > Expected: the expectation is active > Actual: it is retired > Expected: to be called once > Actual: called once - saturated and retired > > I am not that familiar with Google Mock. Therefore, I am not able to figure out > the above. > Am I missing something here? Your inputs, please? It basically says you are expecting one DeleteFramebuffers() call, but now at least it's called twice in the test. You will need to build gpu_unittests target, and run it with gpu_unittests --gtest_filter=*BlendColorValidArgs* (an example of one failing test), and set a break point at ui/gl/gl_mock.h DeleteFramebuffers() function, and see with/without your CL, what has changed (i.e., where is the extra dele call coming from). Hope this helps I think you can add a break point in the gl mock (
The CQ bit was checked by c.padhi@samsung.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...
https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (left): https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:5732: if (!group_->bind_generates_resource()) { On 2017/05/16 18:53:40, Zhenyao Mo wrote: > This is still needed. Done. https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2826043002/diff/80001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3262: framebuffer_manager_.get()); On 2017/05/16 18:53:40, Zhenyao Mo wrote: > This is problematic. Now creating a new context can change this pointer cached > in texture_manager, which is per group. It looks like texture_manager() needs > to cache a list of framebuffer_managers instead of just one. Done. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4870: framebuffer_manager_->Destroy(false); I think this needs a revisit. Passing false instead of have_context(=true in gpu_unittests) to avoid second DeleteFramebuffers call to fix tests failures.
Thank you Zhenyao Mo. Your input helped me figure out the root cause of these unittests failures. The extra DeleteFramebuffers() call was made from FramebufferManager::Destroy(have_context) in GLES2DecoderImpl::Destroy(). Prior to this CL, FramebufferManager::Destroy(have_context) was called from ContextGroup::Destroy() with have_context=false which did not invoke glDeleteFramebuffersEXT in Framebuffer destructor. Now that we moved framebuffer_manager_ to GLES2DecoderImpl, FramebufferManager::Destroy(have_context) wascalled with have_context=true which invoked the extra call resulting in the above failure.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Mostly looks good. Thanks for keeping pushing on this CL. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4870: framebuffer_manager_->Destroy(false); On 2017/05/17 13:08:08, Chandan wrote: > I think this needs a revisit. Passing false instead of have_context(=true in > gpu_unittests) to avoid second DeleteFramebuffers call to fix tests failures. I don't think this is the right fix. Instead, you should modify the test expectations by adding the DeleteFramebuffers() expectation to adapt to the new behavior. Take a look at GLES2DecoderTestBase::ResetDecoder(). https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1933: for (unsigned int i = 0; i < framebuffer_managers_.size(); i++) { nit: ++i instead. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2351: for (unsigned int i = 0; i < framebuffer_managers_.size(); i++) { nit: ++i instead.
The CQ bit was checked by c.padhi@samsung.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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please take another look. Thank you. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:4870: framebuffer_manager_->Destroy(false); On 2017/05/17 18:06:51, Zhenyao Mo wrote: > On 2017/05/17 13:08:08, Chandan wrote: > > I think this needs a revisit. Passing false instead of have_context(=true in > > gpu_unittests) to avoid second DeleteFramebuffers call to fix tests failures. > > I don't think this is the right fix. Instead, you should modify the test > expectations by adding the DeleteFramebuffers() expectation to adapt to the new > behavior. > > Take a look at GLES2DecoderTestBase::ResetDecoder(). Done. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... File gpu/command_buffer/service/texture_manager.cc (right): https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:1933: for (unsigned int i = 0; i < framebuffer_managers_.size(); i++) { On 2017/05/17 18:06:51, Zhenyao Mo wrote: > nit: ++i instead. Done. https://codereview.chromium.org/2826043002/diff/100001/gpu/command_buffer/ser... gpu/command_buffer/service/texture_manager.cc:2351: for (unsigned int i = 0; i < framebuffer_managers_.size(); i++) { On 2017/05/17 18:06:51, Zhenyao Mo wrote: > nit: ++i instead. Done. https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:545: EXPECT_CALL(*gl_, DeleteFramebuffersEXT(1, _)).Times(AnyNumber()); The no. of DeleteFramebuffers calls possible here is equal to the size of the framebuffers_ map in FramebufferManager at the time of its destruction i.e. when the map is cleared, which can be >=0. Therefore, I have added AnyNumber() as the cardinality here.
One concern. Otherwise it looks good. https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:545: EXPECT_CALL(*gl_, DeleteFramebuffersEXT(1, _)).Times(AnyNumber()); On 2017/05/18 13:33:36, Chandan wrote: > The no. of DeleteFramebuffers calls possible here is equal to the size of the > framebuffers_ map in FramebufferManager at the time of its destruction i.e. when > the map is cleared, which can be >=0. Therefore, I have added AnyNumber() as the > cardinality here. If you set it to Times(1).RetiresOnSaturation(), are any tests failing? If not, I'd prefer that. AnyNumber() is too allowing, so usually we avoid doing that if we can.
https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://codereview.chromium.org/2826043002/diff/120001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:545: EXPECT_CALL(*gl_, DeleteFramebuffersEXT(1, _)).Times(AnyNumber()); On 2017/05/18 17:39:53, Zhenyao Mo wrote: > On 2017/05/18 13:33:36, Chandan wrote: > > The no. of DeleteFramebuffers calls possible here is equal to the size of the > > framebuffers_ map in FramebufferManager at the time of its destruction i.e. > when > > the map is cleared, which can be >=0. Therefore, I have added AnyNumber() as > the > > cardinality here. > > If you set it to Times(1).RetiresOnSaturation(), are any tests failing? If not, > I'd prefer that. AnyNumber() is too allowing, so usually we avoid doing that if > we can. Yes, around 24 tests were failing for the reason that I have mentioned above. For some test cases, no. of DeleteFramebuffers calls from FramebufferManager::Destroy() was 0 since |framebuffers_| map size was 0. For some it was 1 and for some it was 2. Therefore, Times(1).RetiresOnSaturation() didn't help.
lgtm then Thank you
The CQ bit was checked by c.padhi@samsung.com
On 2017/05/18 18:13:25, Zhenyao Mo wrote: > lgtm then > > Thank you Thank you for the review.
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": 120001, "attempt_start_ts": 1495132334095400,
"parent_rev": "0619e89fda91cf9c1a540fb42c0193ff579216aa", "commit_rev":
"bdb9e93d3bfdd07b66472316e75588881c7d2d59"}
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1495132334095400,
"parent_rev": "5ec5b67cbab33cea51b0ee11a286c885c2de4d5d", "commit_rev":
"2dfc7029286f40d73b131760e3b7467c68b8bb17"}
Message was sent while issue was closed.
Description was changed from ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=722717 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 ========== Do not share FBOs/VAOs/Transform feedback objects across contexts Framebuffer, vertex array and transform feedback objects are not shared across opengl contexts. Therefore, instead of using ShareGroup's IdHandler, we can manage IDs for these objects locally in GLES2Implementation itself. BUG=722717 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/2826043002 Cr-Commit-Position: refs/heads/master@{#472883} Committed: https://chromium.googlesource.com/chromium/src/+/2dfc7029286f40d73b131760e3b7... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2dfc7029286f40d73b131760e3b7... |
