Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2148723004:
WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader (Closed)
DescriptionWebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader.
Fix bugs in attrib-type-match.html
BUG=628459
TEST=conformance2/rendering/attrib-type-match.html
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
Committed: https://crrev.com/c487531682715938285a34298659c53c84cd8ca6
Cr-Commit-Position: refs/heads/master@{#407168}
Patch Set 1 #Patch Set 2 : add matrix type, and coding style change #Patch Set 3 : set real_stride to non-zero to pass the DCHECK #Patch Set 4 : Fix a bug: some attributes in shader may be no API to feed data #Patch Set 5 : Set correct data for 'size' in attribInfo, although it is meaningless for this CL #Patch Set 6 : Fix a bug introduced by my CL which checks the buffer binding for vertexAttrib1234f and fails #Patch Set 7 : small change to size #Patch Set 8 : A simpler implementation without unused info stored in VertexAttrib::VertexAttrib into Ver… #
Total comments: 6
Patch Set 9 : addressed zmo@'s feedback #Patch Set 10 : coding style #
Total comments: 1
Patch Set 11 : addressed zmo@'s feedback #Patch Set 12 : update vertex attrib base type for vertexAttrib{I}Pointer only if the vertex attrib array is enabled #
Total comments: 21
Patch Set 13 : code rebase + webgl2 identifier update + log #Patch Set 14 : fix the bug in Windows gpu bot #
Total comments: 14
Patch Set 15 : addressed zmo@'s and kbr@'s feedbacks #Patch Set 16 : Fixed bots fail, addressed zmo's feedback: It's not a must to enableVertexArray before vertexAttribPointer #Patch Set 17 : addressed zmo@'s feedback: uint/int type in shader w/o data feed by vertexAttrib API is invalid, fo… #Patch Set 18 : still limit the max_vertex_attribs to 16 to fix gpu_unittests failures #Patch Set 19 : apply zmo's suggestion #Patch Set 20 : More fixes #Patch Set 21 : fix bugs #Patch Set 22 : more fixes #Patch Set 23 : Fix bug: builtin variables don't need to check the type match if no vertexAttrib API to … #Patch Set 24 : code clean #
Total comments: 10
Messages
Total messages: 106 (69 generated)
Description was changed from ========== make sure attrib type match WebGL 2: make sure attrib type match BUG=295792 ========== to ========== make sure attrib type match WebGL 2: make sure attrib type match BUG=295792 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 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== make sure attrib type match WebGL 2: make sure attrib type match BUG=295792 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 ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=295792 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
yunchao.he@intel.com changed reviewers: + kbr@chromium.org, qiankun.miao@intel.com, zmo@chromium.org
Still WIP, has not checked yet. Just FYI to avoid duplicated work.
On 2016/07/14 15:44:35, yunchao wrote: > Still WIP, has not checked yet. Just FYI to avoid duplicated work. Thank you. I was about to work on this. :)
Description was changed from ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=295792 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=628489 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
Description was changed from ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=628489 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
On 2016/07/14 17:36:26, Zhenyao Mo wrote: > On 2016/07/14 15:44:35, yunchao wrote: > > Still WIP, has not checked yet. Just FYI to avoid duplicated work. > > Thank you. I was about to work on this. :) I applied this patch, all 50+ test cases failures in attrib-type-match.html can pass right now. But the bots failed. I look into these failures, the test cases doesn't obey to the updated spec. I will update the test cases, WDYT, @zmo and @kbr? Looking forward to your suggestions.
On 2016/07/15 01:30:05, yunchao wrote: > On 2016/07/14 17:36:26, Zhenyao Mo wrote: > > On 2016/07/14 15:44:35, yunchao wrote: > > > Still WIP, has not checked yet. Just FYI to avoid duplicated work. > > > > Thank you. I was about to work on this. :) > > I applied this patch, all 50+ test cases failures in attrib-type-match.html can > pass right now. But the bots failed. I look into these failures, the test cases > doesn't obey to the updated spec. I will update the test cases, WDYT, @zmo and > @kbr? Looking forward to your suggestions. Yes, please update the test cases and make sure they all pass under the new rule with this CL. Thank you.
On 2016/07/15 01:35:30, Zhenyao Mo wrote: > On 2016/07/15 01:30:05, yunchao wrote: > > On 2016/07/14 17:36:26, Zhenyao Mo wrote: > > > On 2016/07/14 15:44:35, yunchao wrote: > > > > Still WIP, has not checked yet. Just FYI to avoid duplicated work. > > > > > > Thank you. I was about to work on this. :) > > > > I applied this patch, all 50+ test cases failures in attrib-type-match.html > can > > pass right now. But the bots failed. I look into these failures, the test > cases > > doesn't obey to the updated spec. I will update the test cases, WDYT, @zmo and > > @kbr? Looking forward to your suggestions. > > Yes, please update the test cases and make sure they all pass under the new rule > with this CL. > > Thank you. OK. I will update the test cases today. Thanks a lot!
Hi Zhenyao and Ken, If the shader has a variable vec3. For instance float vec3 normal; And we use vertexAttrib4f(index, a, b,c, d) to feed a vec4 data to 'normal'. Is this valid or invalid? I read the spec, seems that nowhere says it is an invalid operation and should generates an error. So I think it is valid. WDYT?
On 2016/07/15 17:05:03, yunchao wrote: > Hi Zhenyao and Ken, If the shader has a variable vec3. For instance > float vec3 normal; > > And we use vertexAttrib4f(index, a, b,c, d) to feed a vec4 data to 'normal'. Is > this valid or invalid? > > I read the spec, seems that nowhere says it is an invalid operation and should > generates an error. So I think it is valid. WDYT? vertexAttrib function only modifies the vertex attrib value state (they have default values). So you can do vertexAttrib1234 to certain location, it simply changes whatever members the function touched, the others remain the same. When you execute a shader, they look value up, it's always the values in the state, they always have full size, regardless of the function call. That's my understanding. SO yes, it's valid.
On 2016/07/15 17:21:16, Zhenyao Mo wrote: > On 2016/07/15 17:05:03, yunchao wrote: > > Hi Zhenyao and Ken, If the shader has a variable vec3. For instance > > float vec3 normal; > > > > And we use vertexAttrib4f(index, a, b,c, d) to feed a vec4 data to 'normal'. > Is > > this valid or invalid? > > > > I read the spec, seems that nowhere says it is an invalid operation and should > > generates an error. So I think it is valid. WDYT? > > vertexAttrib function only modifies the vertex attrib value state (they have > default values). > > So you can do vertexAttrib1234 to certain location, it simply changes whatever > members the function touched, the others remain the same. > > When you execute a shader, they look value up, it's always the values in the > state, they always have full size, regardless of the function call. > > That's my understanding. SO yes, it's valid. OK. Thanks a lot!
Zhenyao and Ken, a couple of test cases in gles2_conform_test failed. But I can not run gles2_conform_test at my side in Linux/MacOS, and there is no doc to tell how to run gles2_conform_test, could you tell me how to run gles2_conform_test? And simply tell me what gles2_conform_test do, why the patch will lead to failures in gles2_conform_test, if possible. Seems that the gles2_conform_test is just a small project in gpu/gles2_conform_support.
On 2016/07/19 03:00:24, yunchao wrote: > Zhenyao and Ken, a couple of test cases in gles2_conform_test failed. But I can > not run gles2_conform_test at my side in Linux/MacOS, and there is no doc to > tell how to run gles2_conform_test, could you tell me how to run > gles2_conform_test? And simply tell me what gles2_conform_test do, why the patch > will lead to failures in gles2_conform_test, if possible. Seems that the > gles2_conform_test is just a small project in gpu/gles2_conform_support. Yunchao, unfortunately we can't release that part of the code. I can take a look at that failures tomorrow. However, I did a quick look at your CL. Although your method seems correct, you encounter the same per issue as piman@ pointed in this CL: https://codereview.chromium.org/2142353002/ Please do something similar, like encode the states, and at draw time it's simply some interger comparison.
On 2016/07/19 03:06:47, Zhenyao Mo wrote: > On 2016/07/19 03:00:24, yunchao wrote: > > Zhenyao and Ken, a couple of test cases in gles2_conform_test failed. But I > can > > not run gles2_conform_test at my side in Linux/MacOS, and there is no doc to > > tell how to run gles2_conform_test, could you tell me how to run > > gles2_conform_test? And simply tell me what gles2_conform_test do, why the > patch > > will lead to failures in gles2_conform_test, if possible. Seems that the > > gles2_conform_test is just a small project in gpu/gles2_conform_support. > > Yunchao, unfortunately we can't release that part of the code. I can take a > look at that failures tomorrow. > > However, I did a quick look at your CL. Although your method seems correct, you > encounter the same per issue as piman@ pointed in this CL: > > https://codereview.chromium.org/2142353002/ > > Please do something similar, like encode the states, and at draw time it's > simply some interger comparison. OK. Thanks a lot. I will take a look.
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
On 2016/07/19 03:06:47, Zhenyao Mo wrote: > On 2016/07/19 03:00:24, yunchao wrote: > > Zhenyao and Ken, a couple of test cases in gles2_conform_test failed. But I > can > > not run gles2_conform_test at my side in Linux/MacOS, and there is no doc to > > tell how to run gles2_conform_test, could you tell me how to run > > gles2_conform_test? And simply tell me what gles2_conform_test do, why the > patch > > will lead to failures in gles2_conform_test, if possible. Seems that the > > gles2_conform_test is just a small project in gpu/gles2_conform_support. > > Yunchao, unfortunately we can't release that part of the code. I can take a > look at that failures tomorrow. > > However, I did a quick look at your CL. Although your method seems correct, you > encounter the same per issue as piman@ pointed in this CL: > > https://codereview.chromium.org/2142353002/ > > Please do something similar, like encode the states, and at draw time it's > simply some interger comparison. @zmo, Sorry, the latest patch set (patch set 9) has not been tested yet. But it is too late at my side. If you have time, you can still help to have a look at the patch set 8 that why the gles2_conform_test failed. Thanks a lot, zhenyao. I suppose that the latest patch set will fail against gles2_conform_test too. IMHO, there are some bugs in gles2_conform_test itself.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2016/07/19 16:31:49, yunchao wrote: > On 2016/07/19 03:06:47, Zhenyao Mo wrote: > > On 2016/07/19 03:00:24, yunchao wrote: > > > Zhenyao and Ken, a couple of test cases in gles2_conform_test failed. But I > > can > > > not run gles2_conform_test at my side in Linux/MacOS, and there is no doc to > > > tell how to run gles2_conform_test, could you tell me how to run > > > gles2_conform_test? And simply tell me what gles2_conform_test do, why the > > patch > > > will lead to failures in gles2_conform_test, if possible. Seems that the > > > gles2_conform_test is just a small project in gpu/gles2_conform_support. > > > > Yunchao, unfortunately we can't release that part of the code. I can take a > > look at that failures tomorrow. > > > > However, I did a quick look at your CL. Although your method seems correct, > you > > encounter the same per issue as piman@ pointed in this CL: > > > > https://codereview.chromium.org/2142353002/ > > > > Please do something similar, like encode the states, and at draw time it's > > simply some interger comparison. > > > @zmo, Sorry, the latest patch set (patch set 9) has not been tested yet. But it > is too late at my side. > > If you have time, you can still help to have a look at the patch set 8 that why > the gles2_conform_test failed. Thanks a lot, zhenyao. > I suppose that the latest patch set will fail against gles2_conform_test too. > IMHO, there are some bugs in gles2_conform_test itself. No problem. I can test with patch set 8.
Yunchao, I took more serious look and find you have some misunderstandings in the vertex attrib and vertex array object. See my comments below. The GLES2_conform tests failure is definitely this CL's fault. This CL should not change ES2 behavior at all no matter what, because there is no way ES2 can have int or uint types. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9878: state_.vertex_attrib_manager->SetAttribBaseType(index, GL_FLOAT); This is wrong. This is setting the generic vertex attrib values, it's per context, not part of the VertexArrayObject (VAO). So instead, you should have a state next to ContextState::attrib_values for this. Only if a vertex attrib is enabled, then it is the VAO's setting of that vertex attrib should be used (set by VertexAttribPointer{I}). If disabled, you should check the generic vertex attrib values and their types. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10144: state_.vertex_attrib_manager->SetAttribBaseType(indx, GL_FLOAT); This is correct, but not the vertexattribxfv calls. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/vertex_attrib_manager.cc (right): https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/vertex_attrib_manager.cc:133: attrib_base_type_[ii] = 0; For attrib values, they do have default values, so their default type is GL_FLOAT rather than 0. i.e., you don't need to call vertexattrib2fv or so to give it values and types.
Patchset #11 (id:220001) has been deleted
Zhenyao and Ken, I have updated the code. PTAL. This CL also pending on https://github.com/KhronosGroup/WebGL/pull/1928.But gpu bots for mac succeed, I will take a look. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9878: state_.vertex_attrib_manager->SetAttribBaseType(index, GL_FLOAT); On 2016/07/19 20:23:34, Zhenyao Mo wrote: > This is wrong. This is setting the generic vertex attrib values, it's per > context, not part of the VertexArrayObject (VAO). So instead, you should have a > state next to ContextState::attrib_values for this. > > Only if a vertex attrib is enabled, then it is the VAO's setting of that vertex > attrib should be used (set by VertexAttribPointer{I}). If disabled, you should > check the generic vertex attrib values and their types. Done. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10144: state_.vertex_attrib_manager->SetAttribBaseType(indx, GL_FLOAT); On 2016/07/19 20:23:34, Zhenyao Mo wrote: > This is correct, but not the vertexattribxfv calls. Acknowledged. https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... File gpu/command_buffer/service/vertex_attrib_manager.cc (right): https://codereview.chromium.org/2148723004/diff/160001/gpu/command_buffer/ser... gpu/command_buffer/service/vertex_attrib_manager.cc:133: attrib_base_type_[ii] = 0; On 2016/07/19 20:23:34, Zhenyao Mo wrote: > For attrib values, they do have default values, so their default type is > GL_FLOAT rather than 0. i.e., you don't need to call vertexattrib2fv or so to > give it values and types. Done. https://codereview.chromium.org/2148723004/diff/200001/gpu/command_buffer/ser... File gpu/command_buffer/service/program_manager.h (right): https://codereview.chromium.org/2148723004/diff/200001/gpu/command_buffer/ser... gpu/command_buffer/service/program_manager.h:404: return vertex_input_type_mask_; A small change here. I'd like to name vertex_input_type_, instead of vertex_input_type_mask_, because this variable indeed store base_type for vertex inputs. And it is the same thing for fragment_output_type_mask_. But the variable names of fragment_output_written_mask_ and vertex_input_written_mask_ would stay the same as they are. Because they actually store masks. WDYT?
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
Description was changed from ========== WebGL 2: make sure attrib type match. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure attrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
Description was changed from ========== WebGL 2: make sure attrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_optional_gpu_tests_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_optional_gpu_...)
zmo@chromium.org changed reviewers: + piman@chromium.org
+piman for a second pair of eyes. yunchao, you are getting close, but some extra work. Thanks for pushing this forward. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:249: generic_attrib_type_written_mask_ |= ~(0x3 << shift_bits); You don't need this written mask. Every vertex attrib has a default type (FLOAT), you should just initialize it, and written mask should just be all true (therefore you don't need it). https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:340: // We have up to 16 attribs, each is encoded into 2 bits, total 32 bits You can't really limit it to 16. Instead, it should be GL_MAX_VERTEX_ATTRIBS. This mask should be uint32_t[ceil(GL_MAX_VERTEX_ATTRIBS / 16)]. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:342: uint32_t generic_attrib_base_type_; I really think we should keep the type_mask_ instead of just type_. mask_ indicates it's encoded bits. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9042: bool GLES2DecoderImpl::AttribsTypeMatch() { This function is incorrect. Basically we should compare shader attrib types with 1) if a vertex attrib array is enabled, use the vertex_attrib_manager's types (set by VertexAttribPoint) 2) if a vertex attrib array is disabled, use the generic vertex attrib type from ContextState. So what you need to do (to make this function efficient) is to keep the disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ to filter 2. Then compare. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9106: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { This should be WebGL2 and ES3. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10015: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { You should update it whether it's enabled or not, I'll explain in another comment what should happen. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10115: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { Same here. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/program_manager.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/program_manager.h:564: uint32_t vertex_input_type_mask_; Again, you will need to have arrays here. It can be more than 16. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/vertex_attrib_manager.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/vertex_attrib_manager.h:306: uint32_t attrib_base_type_mask_; Same here, you need a array.
I defer to Mo's review, but one comment. Thanks for moving this forward. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/feature_info.cc:1421: // Switch statement to cause a compile-time error if we miss a case. This comment is wrong. Please use a switch statement like above.
@zmo (and @kbr and @piman), could you have a look at the comments inline. Thanks a lot! https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9042: bool GLES2DecoderImpl::AttribsTypeMatch() { On 2016/07/20 17:54:56, Zhenyao Mo wrote: > This function is incorrect. Basically we should compare shader attrib types > with > 1) if a vertex attrib array is enabled, use the vertex_attrib_manager's types > (set by VertexAttribPoint) > 2) if a vertex attrib array is disabled, use the generic vertex attrib type from > ContextState. > > So what you need to do (to make this function efficient) is to keep the > disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ to > filter 2. > > Then compare. Zhenyao, thanks for your careful review, But I have different opinion here. My implementation is almost the same with your suggestions. But here is a tiny difference for case 2) above. If an attribute in shader with INT/UINT type, for instance: ivec2 a; uint b; If These two attributes have no vertexAttrib API to feed data. In 2), they will think unmatched, because the default type is FLOAT by default for generic attrib in ContextState. But they are actually OK, I think. My code is almost the same with your suggestions, but it will record the written_mask from call sites of vertexAttrib{1|2|3|4}{i|ui}f{v}, and only compare those written attributes by vertexAttrib API: mask = shader_mask & (vao_attrib_mask | generic_attrib_mask); WDYT?
Patchset #15 (id:320001) has been deleted
Patchset #14 (id:300001) has been deleted
The CQ bit was checked by yunchao.he@intel.com to run a CQ dry run
Patchset #14 (id:340001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/07/21 06:15:27, yunchao wrote: > @zmo (and @kbr and @piman), could you have a look at the comments inline. Thanks > a lot! > > https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:9042: bool > GLES2DecoderImpl::AttribsTypeMatch() { > On 2016/07/20 17:54:56, Zhenyao Mo wrote: > > This function is incorrect. Basically we should compare shader attrib types > > with > > 1) if a vertex attrib array is enabled, use the vertex_attrib_manager's types > > (set by VertexAttribPoint) > > 2) if a vertex attrib array is disabled, use the generic vertex attrib type > from > > ContextState. > > > > So what you need to do (to make this function efficient) is to keep the > > disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ > to > > filter 2. > > > > Then compare. > > Zhenyao, thanks for your careful review, But I have different opinion here. My > implementation is almost the same with your suggestions. But here is a tiny > difference for case 2) above. If an attribute in shader with INT/UINT type, for > instance: > ivec2 a; > uint b; > > If These two attributes have no vertexAttrib API to feed data. In 2), they will > think unmatched, because the default type is FLOAT by default for generic attrib > in ContextState. But they are actually OK, I think. I don't think so. The spec says by default a vertex attrib is (0.0, 0.0, 0.0, 1.0), (spec 3.0.4 section 2.8) which is float type. So if a shader variable is int or uint, the value it gets in shader execution is undefined. It might run OK in your local machine, but it might fail on others. That's why we want to forbid it to get rid of the undefined behavior. I am looking at your CL right now, but this needs to be addresses. > > My code is almost the same with your suggestions, but it will record the > written_mask from call sites of vertexAttrib{1|2|3|4}{i|ui}f{v}, and only > compare those written attributes by vertexAttrib API: > > mask = shader_mask & (vao_attrib_mask | generic_attrib_mask); > > WDYT?
On 2016/07/21 12:46:52, Zhenyao Mo wrote: > On 2016/07/21 06:15:27, yunchao wrote: > > @zmo (and @kbr and @piman), could you have a look at the comments inline. > Thanks > > a lot! > > > > > https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... > > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > > > > https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... > > gpu/command_buffer/service/gles2_cmd_decoder.cc:9042: bool > > GLES2DecoderImpl::AttribsTypeMatch() { > > On 2016/07/20 17:54:56, Zhenyao Mo wrote: > > > This function is incorrect. Basically we should compare shader attrib types > > > with > > > 1) if a vertex attrib array is enabled, use the vertex_attrib_manager's > types > > > (set by VertexAttribPoint) > > > 2) if a vertex attrib array is disabled, use the generic vertex attrib type > > from > > > ContextState. > > > > > > So what you need to do (to make this function efficient) is to keep the > > > disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ > > to > > > filter 2. > > > > > > Then compare. > > > > Zhenyao, thanks for your careful review, But I have different opinion here. My > > implementation is almost the same with your suggestions. But here is a tiny > > difference for case 2) above. If an attribute in shader with INT/UINT type, > for > > instance: > > ivec2 a; > > uint b; > > > > If These two attributes have no vertexAttrib API to feed data. In 2), they > will > > think unmatched, because the default type is FLOAT by default for generic > attrib > > in ContextState. But they are actually OK, I think. > > I don't think so. The spec says by default a vertex attrib is (0.0, 0.0, 0.0, > 1.0), (spec 3.0.4 section 2.8) which is float type. So if a shader variable is > int or uint, the value it gets in shader execution is undefined. It might run OK > in your local machine, but it might fail on others. That's why we want to > forbid it to get rid of the undefined behavior. > > I am looking at your CL right now, but this needs to be addresses. > > > > > My code is almost the same with your suggestions, but it will record the > > written_mask from call sites of vertexAttrib{1|2|3|4}{i|ui}f{v}, and only > > compare those written attributes by vertexAttrib API: > > > > mask = shader_mask & (vao_attrib_mask | generic_attrib_mask); > > > > WDYT? OK. I am still working on this CL. To expand the number of vertex attributes from 16 = sizeof(uint32_t)/2 to max_vertex_attribs.
https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.h (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:342: uint32_t generic_attrib_base_type_; You didn't address the issue of attribs to be more than 16. Basically all the masks here and in vertex_attrib_manager needs to be an array of uint32_t. The size of the array is ceil(MAX_VERTEX_ATTRIBS / 16) https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9086: bool GLES2DecoderImpl::AttribsTypeMatch() { This function needs to be revised 1) you have to have a disable/enable mask from vertex_attrib_manager, see my comments in another place. 2) The masks should be arrays instead of a single uint32_t, see my comments in another place. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9090: uint32_t generic_attrib_mask = state_.GetGenericVertexAttribTypeMask(); Again, I really don't think you need this generic mask. Consider all of them written (since they have default type as FLOAT), so this is always 0xFFFFFFFF. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9150: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { This should be WebGL2 or ES3. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9282: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { same here, webgl2 or es3. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10068: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { This is incorrect. You can call VertexAttribPointer (at the call time the vertex attrib array is disabled), but later enable the vertex attrib array. Then the state you are tracking will be wrong. You have to store a type mask and enable_disable mask separately. (The enable / disable mask needs to be part of the vertex_attrib_manager as it is per vertex array object, not per context) https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/program_manager.cc (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/program_manager.cc:377: for (size_t ii = 0; ii < attrib_infos_.size(); ++ii) { See, if size() > 16, your code will be wrong.
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
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...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_optional_gpu_tests_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
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...)
The CQ bit was checked by yunchao.he@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...
Your CL relies on deprecated CQ feature(s):
* Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
tryserver.chromium.linux
tryserver.chromium.mac
tryserver.chromium.win
For more details, see http://crbug.com/617627.
On 2016/07/21 15:45:00, commit-bot: I haz the power wrote: > Your CL relies on deprecated CQ feature(s): > * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without > "master." prefix is deprecated: > tryserver.chromium.linux > tryserver.chromium.mac > tryserver.chromium.win > For more details, see http://crbug.com/617627. Adding "master." to the front of tryserver.chromium* in the description will fix this warning.
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...)
Description was changed from ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
The CQ bit was checked by yunchao.he@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...
The CQ bit was checked by yunchao.he@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...
The CQ bit was checked by yunchao.he@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...
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_...)
The CQ bit was checked by yunchao.he@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_optional_...)
The CQ bit was checked by yunchao.he@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by yunchao.he@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
The CQ bit was checked by yunchao.he@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Zhenyao and Ken, could you have another look? I think the code is OK now. Thanks a lot! https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:249: generic_attrib_type_written_mask_ |= ~(0x3 << shift_bits); On 2016/07/20 17:54:56, Zhenyao Mo wrote: > You don't need this written mask. > > Every vertex attrib has a default type (FLOAT), you should just initialize it, > and written mask should just be all true (therefore you don't need it). Done. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:340: // We have up to 16 attribs, each is encoded into 2 bits, total 32 bits On 2016/07/20 17:54:56, Zhenyao Mo wrote: > You can't really limit it to 16. Instead, it should be GL_MAX_VERTEX_ATTRIBS. > This mask should be uint32_t[ceil(GL_MAX_VERTEX_ATTRIBS / 16)]. Done. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:342: uint32_t generic_attrib_base_type_; On 2016/07/20 17:54:56, Zhenyao Mo wrote: > I really think we should keep the type_mask_ instead of just type_. mask_ > indicates it's encoded bits. Acknowledged. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/feature_info.cc:1421: // Switch statement to cause a compile-time error if we miss a case. On 2016/07/21 00:16:21, Ken Russell wrote: > This comment is wrong. Please use a switch statement like above. Done. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9042: bool GLES2DecoderImpl::AttribsTypeMatch() { On 2016/07/20 17:54:56, Zhenyao Mo wrote: > This function is incorrect. Basically we should compare shader attrib types > with > 1) if a vertex attrib array is enabled, use the vertex_attrib_manager's types > (set by VertexAttribPoint) > 2) if a vertex attrib array is disabled, use the generic vertex attrib type from > ContextState. > > So what you need to do (to make this function efficient) is to keep the > disable_enable mask, and use enable_mask_ to filter 1) and use ~enable_mask_ to > filter 2. > > Then compare. Acknowledged. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9106: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { On 2016/07/20 17:54:56, Zhenyao Mo wrote: > This should be WebGL2 and ES3. Done. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10015: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { On 2016/07/20 17:54:56, Zhenyao Mo wrote: > You should update it whether it's enabled or not, I'll explain in another > comment what should happen. Acknowledged. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10115: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { On 2016/07/20 17:54:56, Zhenyao Mo wrote: > Same here. Acknowledged. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/program_manager.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/program_manager.h:564: uint32_t vertex_input_type_mask_; On 2016/07/20 17:54:56, Zhenyao Mo wrote: > Again, you will need to have arrays here. It can be more than 16. Done. https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... File gpu/command_buffer/service/vertex_attrib_manager.h (right): https://codereview.chromium.org/2148723004/diff/260001/gpu/command_buffer/ser... gpu/command_buffer/service/vertex_attrib_manager.h:306: uint32_t attrib_base_type_mask_; On 2016/07/20 17:54:56, Zhenyao Mo wrote: > Same here, you need a array. Done. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.h (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:342: uint32_t generic_attrib_base_type_; On 2016/07/21 13:10:46, Zhenyao Mo wrote: > You didn't address the issue of attribs to be more than 16. Basically all the > masks here and in vertex_attrib_manager needs to be an array of uint32_t. The > size of the array is ceil(MAX_VERTEX_ATTRIBS / 16) Done. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9086: bool GLES2DecoderImpl::AttribsTypeMatch() { On 2016/07/21 13:10:46, Zhenyao Mo wrote: > This function needs to be revised > > 1) you have to have a disable/enable mask from vertex_attrib_manager, see my > comments in another place. > 2) The masks should be arrays instead of a single uint32_t, see my comments in > another place. Acknowledged. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9090: uint32_t generic_attrib_mask = state_.GetGenericVertexAttribTypeMask(); On 2016/07/21 13:10:46, Zhenyao Mo wrote: > Again, I really don't think you need this generic mask. Consider all of them > written (since they have default type as FLOAT), so this is always 0xFFFFFFFF. Acknowledged. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9150: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { On 2016/07/21 13:10:46, Zhenyao Mo wrote: > This should be WebGL2 or ES3. Done. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9282: if (feature_info_->IsWebGL2Context() && !AttribsTypeMatch()) { On 2016/07/21 13:10:46, Zhenyao Mo wrote: > same here, webgl2 or es3. Done. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:10068: if (state_.vertex_attrib_manager->GetVertexAttrib(indx)->enabled()) { On 2016/07/21 13:10:46, Zhenyao Mo wrote: > This is incorrect. You can call VertexAttribPointer (at the call time the > vertex attrib array is disabled), but later enable the vertex attrib array. > Then the state you are tracking will be wrong. You have to store a type mask > and enable_disable mask separately. (The enable / disable mask needs to be part > of the vertex_attrib_manager as it is per vertex array object, not per context) Done. https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... File gpu/command_buffer/service/program_manager.cc (right): https://codereview.chromium.org/2148723004/diff/360001/gpu/command_buffer/ser... gpu/command_buffer/service/program_manager.cc:377: for (size_t ii = 0; ii < attrib_infos_.size(); ++ii) { On 2016/07/21 13:10:46, Zhenyao Mo wrote: > See, if size() > 16, your code will be wrong. Acknowledged.
On 2016/07/21 16:07:54, qiankun wrote: > On 2016/07/21 15:45:00, commit-bot: I haz the power wrote: > > Your CL relies on deprecated CQ feature(s): > > * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without > > "master." prefix is deprecated: > > tryserver.chromium.linux > > tryserver.chromium.mac > > tryserver.chromium.win > > For more details, see http://crbug.com/617627. > > Adding "master." to the front of tryserver.chromium* in the description will fix > this warning. You are correct, Thank you, Qiankun.
https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.cc (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:227: max_vertex_attribs_ = 0; This should go to the initialization list above. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.cc:228: generic_attrib_base_type_mask_.resize(1); No need for this. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/context_state.h (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:249: uint32_t packed_size = max_vertex_attribs_ / 16; nit: this can be ceil(), but your code is also correct. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/context_state.h:253: generic_attrib_base_type_mask_[i] = 0xFFFFFFFF; set this to be 0x55555555 * SHADER_VARIABLE_FLOAT see https://cs.chromium.org/chromium/src/gpu/command_buffer/service/framebuffer_m... Otherwise it's hard to read what's going on here. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/feature_info.cc (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/feature_info.cc:1428: // Switch statement to cause a compile-time error if we miss a case. Since you add a default, this comment is incorrect. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9087: bool GLES2DecoderImpl::AttribsTypeMatch() { This is still incorrect. See my comment in vertex_attrib_manager. Basically you are confused about VertexAttribPointer being called and EnableVertexAttribArray/DisableVertexAttribArray. You are not tracking if a vertex array object is enabled or not. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9096: state_.GetGenericVertexAttribBaseTypeMask(index)) | nit: one extra indent https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9098: state_.vertex_attrib_manager->attrib_base_type_mask(index)); nit: one extra indent. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/vertex_attrib_manager.h (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/vertex_attrib_manager.h:316: std::vector<uint32_t> attrib_type_written_mask_; You don't need this written mask. If it's not written, the type_mask bits will be 00, so that's enough information. Instead, you will need a enable_mask, indication if this vertex attrib array is enabled or not. We only use the corresponding bits if it is enabled. Otherwise, we will look into generic vertex attrib value type from context_state.
lgtm with nits. Let's land this now and I'll fix the nits. https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... gpu/command_buffer/service/gles2_cmd_decoder.cc:9087: bool GLES2DecoderImpl::AttribsTypeMatch() { On 2016/07/22 14:36:47, Zhenyao Mo wrote: > This is still incorrect. See my comment in vertex_attrib_manager. > > Basically you are confused about VertexAttribPointer being called and > EnableVertexAttribArray/DisableVertexAttribArray. > > You are not tracking if a vertex array object is enabled or not. Sorry I missed the enable/disable part in the vertex_attrib_manager. This looks correct.
The CQ bit was checked by zmo@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:560001)
Message was sent while issue was closed.
On 2016/07/22 16:05:43, Zhenyao Mo wrote: > lgtm with nits. Let's land this now and I'll fix the nits. > > https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... > File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): > > https://codereview.chromium.org/2148723004/diff/560001/gpu/command_buffer/ser... > gpu/command_buffer/service/gles2_cmd_decoder.cc:9087: bool > GLES2DecoderImpl::AttribsTypeMatch() { > On 2016/07/22 14:36:47, Zhenyao Mo wrote: > > This is still incorrect. See my comment in vertex_attrib_manager. > > > > Basically you are confused about VertexAttribPointer being called and > > EnableVertexAttribArray/DisableVertexAttribArray. > > > > You are not tracking if a vertex array object is enabled or not. > > Sorry I missed the enable/disable part in the vertex_attrib_manager. This looks > correct. Sorry to be late... but I just have corrected the nits...
Message was sent while issue was closed.
Description was changed from ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 ========== WebGL 2: make sure VertexAttrib type match the corresponding attrib type in shader. Fix bugs in attrib-type-match.html BUG=628459 TEST=conformance2/rendering/attrib-type-match.html 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 Committed: https://crrev.com/c487531682715938285a34298659c53c84cd8ca6 Cr-Commit-Position: refs/heads/master@{#407168} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/c487531682715938285a34298659c53c84cd8ca6 Cr-Commit-Position: refs/heads/master@{#407168} |
