Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2070313002:
Parse GL_VERSION incorrectly when version string is without release_number (Closed)
|
Created:
4 years, 6 months ago by xinghua.cao Modified:
4 years, 6 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParse GL_VERSION incorrectly when version string is without release_number
BUG=620677
Committed: https://crrev.com/499933129cdb8dbc7c35c146f86ce75504252dd9
Cr-Commit-Position: refs/heads/master@{#401221}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add unit test case #
Total comments: 10
Patch Set 3 : Address zhenyao's comments #
Total comments: 1
Patch Set 4 : code rebase #Patch Set 5 : Four indent instead of two #
Messages
Total messages: 36 (12 generated)
Description was changed from ========== Parse GL_VERSION incorrectly when version string is without release_number BUG= ========== to ========== Parse GL_VERSION incorrectly when version string is without release_number BUG=620677 ==========
xinghua.cao@intel.com changed reviewers: + qiankun.miao@intel.com, yunchao.he@intel.com
https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc File ui/gl/gl_version_info.cc (right): https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc#ne... ui/gl/gl_version_info.cc:75: lstr = lstr.substr(10, 3); If OpenGL version string is without release number, take mesa for example, its verstion string is: 4.3 (Core Profile) Mesa 11.2.0 The code will parse the major and minor version is: major: 4, minor: 0 It is not correct, so take " " as a separator. Version string is legal without release number. The version number uses one of these forms: major_number.minor_number major_number.minor_number.release_number the reference link is https://www.opengl.org/sdk/docs/man2/xhtml/glGetString.xml
L-G-T-M
qiankun.miao@intel.com changed reviewers: + kbr@chromium.org, zmo@chromium.org
PTAL.
On 2016/06/16 14:39:22, qiankun wrote: > PTAL. lgtm
but I am not owner. kbr should review this.
https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc File ui/gl/gl_version_info.cc (right): https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc#ne... ui/gl/gl_version_info.cc:75: lstr = lstr.substr(10, 3); On 2016/06/16 10:29:05, xinghua.cao wrote: > If OpenGL version string is without release number, take mesa for example, its > verstion string is: > 4.3 (Core Profile) Mesa 11.2.0 > The code will parse the major and minor version is: > major: 4, minor: 0 > It is not correct, so take " " as a separator. > Version string is legal without release number. The version number uses one of > these forms: > major_number.minor_number major_number.minor_number.release_number > the reference link is https://www.opengl.org/sdk/docs/man2/xhtml/glGetString.xml Thanks for tracking this down. Could you please create a new file in this directory, gl_version_info_unittest.cc, and add a couple of tests for this code? Please try parsing a few representative GL_VERSION strings like: OpenGL ES 2.0 (ANGLE 2.1.0.cd1b12260360) 2.1 INTEL-10.6.33 4.5.0 NVIDIA 364.19 4.3 (Core Profile) Mesa 11.2.0 (these were taken from a few tests' logs on https://build.chromium.org/p/chromium.gpu.fyi/console ) The new test can be added to the gl_unittests target in src/ui/gl/BUILD.gn and src/ui/gl/gl_tests.gyp . Thanks.
On 2016/06/16 20:12:22, Ken Russell OOO till 6-21-2016 wrote: > https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc > File ui/gl/gl_version_info.cc (right): > > https://codereview.chromium.org/2070313002/diff/1/ui/gl/gl_version_info.cc#ne... > ui/gl/gl_version_info.cc:75: lstr = lstr.substr(10, 3); > On 2016/06/16 10:29:05, xinghua.cao wrote: > > If OpenGL version string is without release number, take mesa for example, its > > verstion string is: > > 4.3 (Core Profile) Mesa 11.2.0 > > The code will parse the major and minor version is: > > major: 4, minor: 0 > > It is not correct, so take " " as a separator. > > Version string is legal without release number. The version number uses one of > > these forms: > > major_number.minor_number major_number.minor_number.release_number > > the reference link is > https://www.opengl.org/sdk/docs/man2/xhtml/glGetString.xml > > Thanks for tracking this down. > > Could you please create a new file in this directory, > gl_version_info_unittest.cc, and add a couple of tests for this code? Please try > parsing a few representative GL_VERSION strings like: > > OpenGL ES 2.0 (ANGLE 2.1.0.cd1b12260360) > 2.1 INTEL-10.6.33 > 4.5.0 NVIDIA 364.19 > 4.3 (Core Profile) Mesa 11.2.0 > > (these were taken from a few tests' logs on > https://build.chromium.org/p/chromium.gpu.fyi/console ) > > The new test can be added to the gl_unittests target in src/ui/gl/BUILD.gn and > src/ui/gl/gl_tests.gyp . > > Thanks. Ken, Unit test case had been added. Please help to review it again, thank you.
https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... File ui/gl/gl_version_info_unittest.cc (right): https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. nit: 2016 https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:10: class GLVersionInfoTest : public testing::Test { You don't have to declare this class if it's doing nothing. Below you can just use TEST instead of TEST_F. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:16: "4.5.0 NVIDIA 364.19", nit: 4 space only indent. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:22: unsigned expected_major_minor[] = {4, 3, 4, 5, 2, 0, 2, 1}; Maybe just split into expected_major and expected_minor? https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:24: struct GLVersionInfo* version_info = use scoped_ptr
https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... File ui/gl/gl_version_info_unittest.cc (right): https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/06/20 16:20:15, Zhenyao Mo wrote: > nit: 2016 Done. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:10: class GLVersionInfoTest : public testing::Test { On 2016/06/20 16:20:15, Zhenyao Mo wrote: > You don't have to declare this class if it's doing nothing. Below you can just > use TEST instead of TEST_F. Done. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:16: "4.5.0 NVIDIA 364.19", On 2016/06/20 16:20:15, Zhenyao Mo wrote: > nit: 4 space only indent. Done. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:22: unsigned expected_major_minor[] = {4, 3, 4, 5, 2, 0, 2, 1}; On 2016/06/20 16:20:15, Zhenyao Mo wrote: > Maybe just split into expected_major and expected_minor? Done. https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:24: struct GLVersionInfo* version_info = On 2016/06/20 16:20:15, Zhenyao Mo wrote: > use scoped_ptr I am not sure where "scoped_ptr" is declared and defined, where is the header file to be referred. Zhenyao, is it OK to use std::unique_ptr like feaure_info.h file?
On 2016/06/21 at 10:06:31, xinghua.cao wrote: > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > File ui/gl/gl_version_info_unittest.cc (right): > > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > ui/gl/gl_version_info_unittest.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. > On 2016/06/20 16:20:15, Zhenyao Mo wrote: > > nit: 2016 > > Done. > > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > ui/gl/gl_version_info_unittest.cc:10: class GLVersionInfoTest : public testing::Test { > On 2016/06/20 16:20:15, Zhenyao Mo wrote: > > You don't have to declare this class if it's doing nothing. Below you can just > > use TEST instead of TEST_F. > > Done. > > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > ui/gl/gl_version_info_unittest.cc:16: "4.5.0 NVIDIA 364.19", > On 2016/06/20 16:20:15, Zhenyao Mo wrote: > > nit: 4 space only indent. > > Done. > > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > ui/gl/gl_version_info_unittest.cc:22: unsigned expected_major_minor[] = {4, 3, 4, 5, 2, 0, 2, 1}; > On 2016/06/20 16:20:15, Zhenyao Mo wrote: > > Maybe just split into expected_major and expected_minor? > > Done. > > https://codereview.chromium.org/2070313002/diff/20001/ui/gl/gl_version_info_u... > ui/gl/gl_version_info_unittest.cc:24: struct GLVersionInfo* version_info = > On 2016/06/20 16:20:15, Zhenyao Mo wrote: > > use scoped_ptr > > I am not sure where "scoped_ptr" is declared and defined, where is the header file to be referred. Zhenyao, is it OK to use std::unique_ptr like feaure_info.h file? AFAIK scoped_ptr was removed in favor of unique_ptr this weekend so you should use that.
non owner lgtm https://codereview.chromium.org/2070313002/diff/40001/ui/gl/gl_version_info_u... File ui/gl/gl_version_info_unittest.cc (right): https://codereview.chromium.org/2070313002/diff/40001/ui/gl/gl_version_info_u... ui/gl/gl_version_info_unittest.cc:25: renderer_str[i], extensions_str[i])); nit: four indent instead of two.
Thank you for adding the test. LGTM
+cwallez as FYI since this will probably affect Core Profile handling that he's looking into.
Xinghua, I'm going to take the liberty of checking the CQ box for you since we'd like to get this fix in sooner rather than later.
The CQ bit was checked by kbr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070313002/40001
On 2016/06/21 at 21:47:11, commit-bot wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/2070313002/40001 Thanks Ken
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/06/22 01:13:04, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) Lack of GPU bot capacity. http://crbug.com/614526 . Trying again.
The CQ bit was checked by kbr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070313002/40001
The CQ bit was checked by xinghua.cao@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, zmo@chromium.org Link to the patchset: https://codereview.chromium.org/2070313002/#ps80001 (title: "Four indent instead of two")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070313002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by qiankun.miao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2070313002/80001
Message was sent while issue was closed.
Description was changed from ========== Parse GL_VERSION incorrectly when version string is without release_number BUG=620677 ========== to ========== Parse GL_VERSION incorrectly when version string is without release_number BUG=620677 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Parse GL_VERSION incorrectly when version string is without release_number BUG=620677 ========== to ========== Parse GL_VERSION incorrectly when version string is without release_number BUG=620677 Committed: https://crrev.com/499933129cdb8dbc7c35c146f86ce75504252dd9 Cr-Commit-Position: refs/heads/master@{#401221} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/499933129cdb8dbc7c35c146f86ce75504252dd9 Cr-Commit-Position: refs/heads/master@{#401221} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
