Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2087283003:
Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. (Closed)
DescriptionFixed running gpu process with kOverrideUseGLWithOSMesaForTests.
On windows with no graphics card gpu process was not started because
GpuDataManager was considered that there is no gpu and all gpu features
are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL
when check gpu info.
BUG=621316
Committed: https://crrev.com/98e5974f32d724c56b27eea7c3ea97599f6ab7e7
Cr-Commit-Position: refs/heads/master@{#402412}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Set kUseGL higher priority than kOverrideUseGLForTests. Fixed AutofillRiskFingerprintTest.GetFinger… #Patch Set 3 : Set kOverrideUseGLForTests higher priority than kUseGL. #
Total comments: 2
Messages
Total messages: 38 (13 generated)
Description was changed from ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 ========== to ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 ==========
kirr@yandex-team.ru changed reviewers: + danakj@chromium.org, piman@chromium.org
PTAL Is kOverrideUseGLWithOSMesaForTests switch is really needed? At first sight seems that kUseGL is enough, but I can't see crbug.com/364729 from https://codereview.chromium.org/261013004/
lgtm
On 2016/06/23 11:31:38, kirr wrote: > PTAL > > Is kOverrideUseGLWithOSMesaForTests switch is really needed? I think it's nice to separate user command line flags vs internal flags. Otherwise you have to do things like check if the flag was specified by a user, and remove it, then append your own with the value you want. Looks like this https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.cc... is totally wrong tho since it's trying to use kUseGL in browser tests, but the decision is made completely differently above in that file. > At first sight seems that kUseGL is enough, but I can't see > crbug.com/364729 from > https://codereview.chromium.org/261013004/
https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:533: (!command_line->HasSwitch(switches::kUseGL) && Ideally we never have both kUseGL and kOverrideUseGL, perhaps you can DCHECK that instead? It may be some tests are setting it though since I last looked at this and did the KOverrideUseGL thing, and we'd need to fix them. I thought I was DCHECKing that kUseGL wasn't present in BrowserTestBase but I don't see it right now. LGTM otherwise.
On Thu, Jun 23, 2016 at 1:01 PM, <danakj@chromium.org> wrote: > > > https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > > https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... > content/browser/gpu/gpu_data_manager_impl_private.cc:533: > (!command_line->HasSwitch(switches::kUseGL) && > Ideally we never have both kUseGL and kOverrideUseGL, perhaps you can > DCHECK that instead? It may be some tests are setting it though since I > last looked at this and did the KOverrideUseGL thing, and we'd need to > fix them. I thought I was DCHECKing that kUseGL wasn't present in > BrowserTestBase but I don't see it right now. > I believe --use-gl is set by the test runner on Chrome OS on EGL platforms, that happens on the Chrome OS side. > > LGTM otherwise. > > https://codereview.chromium.org/2087283003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 23, 2016 at 1:05 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Thu, Jun 23, 2016 at 1:01 PM, <danakj@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... >> File content/browser/gpu/gpu_data_manager_impl_private.cc (right): >> >> >> https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... >> content/browser/gpu/gpu_data_manager_impl_private.cc:533: >> (!command_line->HasSwitch(switches::kUseGL) && >> Ideally we never have both kUseGL and kOverrideUseGL, perhaps you can >> DCHECK that instead? It may be some tests are setting it though since I >> last looked at this and did the KOverrideUseGL thing, and we'd need to >> fix them. I thought I was DCHECKing that kUseGL wasn't present in >> BrowserTestBase but I don't see it right now. >> > > I believe --use-gl is set by the test runner on Chrome OS on EGL > platforms, that happens on the Chrome OS side. > Oh, which one is supposed to take precedence then? Over here the override wins: https://cs.chromium.org/chromium/src/ui/gl/init/gl_factory.cc?rcl=0&l=34 But in this CL the kUseGL wins. > > >> >> LGTM otherwise. >> >> https://codereview.chromium.org/2087283003/ >> > > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Jun 23, 2016 at 1:15 PM, Dana Jansens <danakj@chromium.org> wrote: > On Thu, Jun 23, 2016 at 1:05 PM, Antoine Labour <piman@chromium.org> > wrote: > >> >> >> On Thu, Jun 23, 2016 at 1:01 PM, <danakj@chromium.org> wrote: >> >>> >>> >>> https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... >>> File content/browser/gpu/gpu_data_manager_impl_private.cc (right): >>> >>> >>> https://codereview.chromium.org/2087283003/diff/1/content/browser/gpu/gpu_dat... >>> content/browser/gpu/gpu_data_manager_impl_private.cc:533: >>> (!command_line->HasSwitch(switches::kUseGL) && >>> Ideally we never have both kUseGL and kOverrideUseGL, perhaps you can >>> DCHECK that instead? It may be some tests are setting it though since I >>> last looked at this and did the KOverrideUseGL thing, and we'd need to >>> fix them. I thought I was DCHECKing that kUseGL wasn't present in >>> BrowserTestBase but I don't see it right now. >>> >> >> I believe --use-gl is set by the test runner on Chrome OS on EGL >> platforms, that happens on the Chrome OS side. >> > > Oh, which one is supposed to take precedence then? Over here the override > wins: > > https://cs.chromium.org/chromium/src/ui/gl/init/gl_factory.cc?rcl=0&l=34 > > But in this CL the kUseGL wins. > Good point. I think the override should win. > > >> >> >>> >>> LGTM otherwise. >>> >>> https://codereview.chromium.org/2087283003/ >>> >> >> > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by kirr@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2087283003/1
> >> I believe --use-gl is set by the test runner on Chrome OS on EGL > >> platforms, that happens on the Chrome OS side. > >> > > > > Oh, which one is supposed to take precedence then? Over here the override > > wins: > > > > https://cs.chromium.org/chromium/src/ui/gl/init/gl_factory.cc?rcl=0&l=34 > > > > But in this CL the kUseGL wins. > > > > Good point. I think the override should win. Thanks for pointing this. If kUseGL is set externally I think kUseGL should wins. Otherwise we will not able to set gl backend externally in browser_tests. Should I change also gl_factory.cc?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please see one more time. kUseGL now has a priority. Is it ok? Should I remove word "override" from kOverrideUseGLWithOSMesaForTests?
The CQ bit was checked by kirr@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2087283003/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On Fri, Jun 24, 2016 at 6:26 AM, <kirr@yandex-team.ru> wrote: > Please see one more time. > kUseGL now has a priority. Is it ok? > Should I remove word "override" from kOverrideUseGLWithOSMesaForTests? > I don't think we want to allow overriding the test-override. Our pixel results would be completely different without OSMesa. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Jun 24, 2016 at 10:40 AM, <danakj@chromium.org> wrote: > On Fri, Jun 24, 2016 at 6:26 AM, <kirr@yandex-team.ru> wrote: > >> Please see one more time. >> kUseGL now has a priority. Is it ok? >> Should I remove word "override" from kOverrideUseGLWithOSMesaForTests? >> > > I don't think we want to allow overriding the test-override. Our pixel > results would be completely different without OSMesa. > Right, as explained above, Chrome OS sets --use-gl=egl, and we still need tests to use OSMesa. If you want to run tests without OSMesa, you can use --use-gpu-in-tests Antoine -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/06/24 19:18:53, piman wrote: > On Fri, Jun 24, 2016 at 10:40 AM, <mailto:danakj@chromium.org> wrote: > > > On Fri, Jun 24, 2016 at 6:26 AM, <mailto:kirr@yandex-team.ru> wrote: > > > >> Please see one more time. > >> kUseGL now has a priority. Is it ok? > >> Should I remove word "override" from kOverrideUseGLWithOSMesaForTests? > >> > > > > I don't think we want to allow overriding the test-override. Our pixel > > results would be completely different without OSMesa. > > > > Right, as explained above, Chrome OS sets --use-gl=egl, and we still need > tests to use OSMesa. > If you want to run tests without OSMesa, you can use --use-gpu-in-tests > > Antoine > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok. Done.
lgtm https://codereview.chromium.org/2087283003/diff/40001/components/autofill/con... File components/autofill/content/browser/risk/fingerprint_browsertest.cc (left): https://codereview.chromium.org/2087283003/diff/40001/components/autofill/con... components/autofill/content/browser/risk/fingerprint_browsertest.cc:192: #endif Is this change related to the issue at hand?
https://codereview.chromium.org/2087283003/diff/40001/components/autofill/con... File components/autofill/content/browser/risk/fingerprint_browsertest.cc (left): https://codereview.chromium.org/2087283003/diff/40001/components/autofill/con... components/autofill/content/browser/risk/fingerprint_browsertest.cc:192: #endif On 2016/06/27 15:06:44, piman wrote: > Is this change related to the issue at hand? I think this test should also be fixed by commit. Locally test is passed now.
The CQ bit was checked by kirr@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2087283003/#ps40001 (title: "Set kOverrideUseGLForTests higher priority than kUseGL.")
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
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...)
kirr@yandex-team.ru changed reviewers: + isherman@chromium.org
+isherman@
LGTM
The CQ bit was checked by kirr@yandex-team.ru
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 ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 ========== to ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 ========== to ========== Fixed running gpu process with kOverrideUseGLWithOSMesaForTests. On windows with no graphics card gpu process was not started because GpuDataManager was considered that there is no gpu and all gpu features are blacklisted. Make kOverrideUseGLWithOSMesaForTests equal to kUseGL when check gpu info. BUG=621316 Committed: https://crrev.com/98e5974f32d724c56b27eea7c3ea97599f6ab7e7 Cr-Commit-Position: refs/heads/master@{#402412} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/98e5974f32d724c56b27eea7c3ea97599f6ab7e7 Cr-Commit-Position: refs/heads/master@{#402412} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
