Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2248213004:
Fix gl_unittests on Mac core profile. (Closed)
|
Created:
4 years, 4 months ago by Zhenyao Mo Modified:
4 years, 4 months ago CC:
reveman, ccameron, chromium-reviews, piman, vmiura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix gl_unittests on Mac core profile.
Also, switch ScopedVertexAttribArray to use vertex array object when possible.
This is an optimization anyway, and fix one of the test.
BUG=635081
TEST=bots
NOTRY=true
R=kbr@chromium.org,erikchen@chromium.org
Committed: https://crrev.com/8bcf1e521df79bab4060b682e0c840f92771ca22
Cr-Commit-Position: refs/heads/master@{#412889}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 10
Patch Set 3 : revision #
Total comments: 2
Patch Set 4 : format #Messages
Total messages: 38 (21 generated)
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...
Erik, Ken: Please review. Others: FYI (feel free to review)
On 2016/08/18 00:27:32, Zhenyao Mo wrote: > Erik, Ken: Please review. > Others: FYI (feel free to review) https://codereview.chromium.org/2251263002/ is on top of this CL and verifies this CL works fine with core profile (if bots there are all green)
piman@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc File ui/gl/scoped_binders.cc (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc... ui/gl/scoped_binders.cc:113: glBindVertexArrayOES(vao_); The 2 paths are not really equivalent... the old path only changes the binding for attribute |index| and leaves the other ones alone, whereas the new path disables all other attributes (by setting up a new vao). In particular, it means we can't nest ScopedVertexAttribArray in the new path. How about only creating a vao if none is bound, and otherwise modify the currently bound vao, and then resetting it in the destructor? Then we keep existing semantics.
https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (left): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:90: is_gles ? kShaderFloatPrecision : "", is_gles will always evaluate to false in this case? Can you add DCHECKs here and in GL_TEXTURE_RECTANGLE_ARB that is_gles must be false? https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:37: std::string GenerateShaderVersionString() { This function is never used. https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:69: "%s\n" to match LoadVertexShader, can you move the "\n" into the parameter?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Exceeded global retry quota
https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc File ui/gl/scoped_binders.cc (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc... ui/gl/scoped_binders.cc:113: glBindVertexArrayOES(vao_); On 2016/08/18 00:44:50, piman wrote: > The 2 paths are not really equivalent... the old path only changes the binding > for attribute |index| and leaves the other ones alone, whereas the new path > disables all other attributes (by setting up a new vao). > > In particular, it means we can't nest ScopedVertexAttribArray in the new path. > > How about only creating a vao if none is bound, and otherwise modify the > currently bound vao, and then resetting it in the destructor? Then we keep > existing semantics. On ES3, there is a default vao, on desktop GL, there is not, but we always create one on command buffer initialization time. So the only situation that the Get* calls here cause problem is the test. So what about I leave this function totally untouched, but create a vao and bind it in the test?
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/2248213004/diff/20001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (left): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:90: is_gles ? kShaderFloatPrecision : "", On 2016/08/18 00:45:54, erikchen wrote: > is_gles will always evaluate to false in this case? Can you add DCHECKs here and > in GL_TEXTURE_RECTANGLE_ARB that is_gles must be false? As far as I know there is no ES drivers on Mac. I'll DCHECK.
https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc File ui/gl/scoped_binders.cc (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc... ui/gl/scoped_binders.cc:113: glBindVertexArrayOES(vao_); On 2016/08/18 00:54:19, Zhenyao Mo wrote: > On 2016/08/18 00:44:50, piman wrote: > > The 2 paths are not really equivalent... the old path only changes the binding > > for attribute |index| and leaves the other ones alone, whereas the new path > > disables all other attributes (by setting up a new vao). > > > > In particular, it means we can't nest ScopedVertexAttribArray in the new path. > > > > How about only creating a vao if none is bound, and otherwise modify the > > currently bound vao, and then resetting it in the destructor? Then we keep > > existing semantics. > > On ES3, there is a default vao, on desktop GL, there is not, but we always > create one on command buffer initialization time. So the only situation that > the Get* calls here cause problem is the test. > > So what about I leave this function totally untouched, but create a vao and bind > it in the test? On desktop only then? VAOs are not available on ES2 I believe. Is so, sgtm
https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc File ui/gl/scoped_binders.cc (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/scoped_binders.cc... ui/gl/scoped_binders.cc:113: glBindVertexArrayOES(vao_); On 2016/08/18 00:57:46, piman wrote: > On 2016/08/18 00:54:19, Zhenyao Mo wrote: > > On 2016/08/18 00:44:50, piman wrote: > > > The 2 paths are not really equivalent... the old path only changes the > binding > > > for attribute |index| and leaves the other ones alone, whereas the new path > > > disables all other attributes (by setting up a new vao). > > > > > > In particular, it means we can't nest ScopedVertexAttribArray in the new > path. > > > > > > How about only creating a vao if none is bound, and otherwise modify the > > > currently bound vao, and then resetting it in the destructor? Then we keep > > > existing semantics. > > > > On ES3, there is a default vao, on desktop GL, there is not, but we always > > create one on command buffer initialization time. So the only situation that > > the Get* calls here cause problem is the test. > > > > So what about I leave this function totally untouched, but create a vao and > bind > > it in the test? > > On desktop only then? VAOs are not available on ES2 I believe. Is so, sgtm It sounds good to me to modify the test rather than this class, if this scoped binder would otherwise work in all cases where it's used in Chromium.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by zmo@chromium.org to run a CQ dry run
Revised. Please review again. https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:37: std::string GenerateShaderVersionString() { On 2016/08/18 00:45:54, erikchen wrote: > This function is never used. Done. https://codereview.chromium.org/2248213004/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:69: "%s\n" On 2016/08/18 00:45:54, erikchen wrote: > to match LoadVertexShader, can you move the "\n" into the parameter? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
lgtm
reveman@chromium.org changed reviewers: + reveman@chromium.org
lgtm with nit https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:63: "uniform SamplerType a_texture;\n" nit: did you cl format this? please do if you didn't.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:63: "uniform SamplerType a_texture;\n" On 2016/08/18 17:27:56, reveman wrote: > nit: did you cl format this? please do if you didn't. Done.
Description was changed from ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots R=kbr@chromium.org,erikchen@chromium.org ========== to ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots NOTRY=true R=kbr@chromium.org,erikchen@chromium.org ==========
On 2016/08/18 17:57:23, Zhenyao Mo wrote: > https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... > File ui/gl/test/gl_image_test_template.h (right): > > https://codereview.chromium.org/2248213004/diff/80001/ui/gl/test/gl_image_tes... > ui/gl/test/gl_image_test_template.h:63: "uniform SamplerType a_texture;\n" > On 2016/08/18 17:27:56, reveman wrote: > > nit: did you cl format this? please do if you didn't. > > Done. patchset 4 is the same as patchset 3 except formatting, so landing with NOTRY due to patchset3 cq dry run all green.
The CQ bit was checked by zmo@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from erikchen@chromium.org, reveman@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/2248213004/#ps100001 (title: "format")
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 ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots NOTRY=true R=kbr@chromium.org,erikchen@chromium.org ========== to ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots NOTRY=true R=kbr@chromium.org,erikchen@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots NOTRY=true R=kbr@chromium.org,erikchen@chromium.org ========== to ========== Fix gl_unittests on Mac core profile. Also, switch ScopedVertexAttribArray to use vertex array object when possible. This is an optimization anyway, and fix one of the test. BUG=635081 TEST=bots NOTRY=true R=kbr@chromium.org,erikchen@chromium.org Committed: https://crrev.com/8bcf1e521df79bab4060b682e0c840f92771ca22 Cr-Commit-Position: refs/heads/master@{#412889} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8bcf1e521df79bab4060b682e0c840f92771ca22 Cr-Commit-Position: refs/heads/master@{#412889} |
