|
|
Created:
3 years, 10 months ago by sugoi1 Modified:
3 years, 9 months ago Reviewers:
Ken Russell (switch to Gerrit), sugoi1, laforge, piman, Sébastien Marchand, Nico, vmiura, jbauman, emaxx, grt (UTC plus 2) CC:
chromium-reviews, grt+watch_chromium.org, jam, pennymac+watch_chromium.org, darin-cc_chromium.org, wfh+watch_chromium.org, piman+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMoving SwiftShader from component to bundled library
Multiple things are modified here:
- The SwiftShader component was entirely removed
- The kSwiftShaderPath switch was removed, since SwiftShader
will now be in a constant location
- SwiftShader files were added to FILES.cfg and chrome.release
BUG=630728
TBR=piman@chromium.org
TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial
Review-Url: https://codereview.chromium.org/2715563002
Cr-Commit-Position: refs/heads/master@{#456764}
Committed: https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f04318ebf396c184
Patch Set 1 #
Total comments: 13
Patch Set 2 : Fixed some comments #
Total comments: 2
Patch Set 3 : Trying to fix analysis pass #Patch Set 4 : 2nd fix for analysis pass #Patch Set 5 : Fix for content_unittests #Patch Set 6 : Specify x11 only support on Linux #
Total comments: 11
Patch Set 7 : Fixed nits and some unit tests #
Total comments: 2
Patch Set 8 : Prevented !GpuAccessAllowed() from enabling SwiftShader #
Total comments: 12
Patch Set 9 : Restored GpuAccessAllowed in EnableSwiftShaderIfNecessary #Patch Set 10 : Made the BUILD.gn modification Windows only for now #Patch Set 11 : Added comment to policy browser test #Patch Set 12 : Update past extension test fixes #Messages
Total messages: 100 (49 generated)
sugoi@chromium.org changed reviewers: + grt@chromium.org, jbauman@chromium.org, kbr@chromium.org, laforge@chromium.org, sebmarchand@chromium.org
I can split this in multiple cls if required, I just need help making sure I'm doing this right.
https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... chrome/tools/build/win/FILES.cfg:395: }, There are conditions like BUILDFLAG(ENABLE_SWIFTSHADER) throughout the code but I don't see similar conditions in the bundling files. What's the situation here?
Does the browser build target have a dep on swiftshader yet? Also have we made sure we're willing to take the download size hit for having this always installed?
On 2017/02/23 02:06:42, jbauman wrote: > Does the browser build target have a dep on swiftshader yet? I don't think so, but I can easily add one. > Also have we made sure we're willing to take the download size hit for having > this always installed? The bundled library should be ~1MB, now that it's using Subzero as the JIT compiler, instead of LLVM, and that size is supposed to be acceptable (It was the main reason we switched from LLVM to Subzero).
sugoi@google.com changed reviewers: + sugoi@google.com
https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... chrome/tools/build/win/FILES.cfg:395: }, On 2017/02/23 01:58:05, Ken Russell wrote: > There are conditions like BUILDFLAG(ENABLE_SWIFTSHADER) throughout the code but > I don't see similar conditions in the bundling files. What's the situation here? Not sure yet, I'm not super knowledgeable about how to configure this file. That being said, this file is for Windows and ENABLE_SWIFTSHADER is defined as "is_chrome_branded && is_win", which is what this file is used for, if I understand correctly.
OK. LGTM from my standpoint. The PMs will have to give a thumbs-up on adding this to the installer.
On 2017/02/23 03:52:39, Ken Russell wrote: > OK. LGTM from my standpoint. The PMs will have to give a thumbs-up on adding > this to the installer. LGTM Part of the calculus for shifting Flash player to be distributed purely as a component (i.e. completely decoupling it from the install payload - in Chrome 54) was that it gave us the opportunity to pull in components that were core to the web platform (CDM and Swiftshader) since the size increase would be more than offset by the size decrease of pulling Flash. The caveat, at the time, was that SwiftShader (compressed) was too big to pull into the product. With the transition to using Subzero, bringing the size down to 1MB, that brought it under the threshold (i.e. size wise now it's a wash). So that it to say, we've been planning this size swap for a while, we're just now able to realize it.
On 2017/02/23 03:40:52, sugoi wrote: > On 2017/02/23 02:06:42, jbauman wrote: > > Does the browser build target have a dep on swiftshader yet? > I don't think so, but I can easily add one. Yeah, I think we need this. > > > Also have we made sure we're willing to take the download size hit for having > > this always installed? > The bundled library should be ~1MB, now that it's using Subzero as the JIT > compiler, instead of LLVM, and that size is supposed to be acceptable (It was > the main reason we switched from LLVM to Subzero). Ok, sounds good.
https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( Check BUILDFLAG(ENABLE_SWIFTSHADER) here. Otherwise this will automatically switch to using swiftshader even when it's disabled. https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w... File ui/gl/init/gl_initializer_win.cc (right): https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w... ui/gl/init/gl_initializer_win.cc:105: (use_gl == kGLImplementationSwiftShaderName) || This should return false if !BUILDFLAG(ENABLE_SWIFTSHADER) and using_swift_shader is true. Otherwise the browser process could think it's using swiftshader and the gpu process would ignore that and use D3D.
https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ are these only for is_chrome_branded=true builds? do you need the inclusion of these files to be conditional on the BUILDFLAG? https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll', are there pdb files for these as well?
A few changes: 1) Moved swiftshader deps from "all" to "browser". 2) Removed "is_chrome_branded" for the ENABLE_SWIFTSHADER build flag. SwiftShader is open source and should be available in chromium also. 3) Fixed ENABLE_SWIFTSHADER build flag related comments. https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ On 2017/02/23 15:00:50, grt (UTC plus 1) wrote: > are these only for is_chrome_branded=true builds? They could be used with any version, but is_chrome_branded would be good enough for now. Just out of curiosity, does this file also create an installer for non chrome branded versions? > do you need the inclusion of these files to be conditional on the BUILDFLAG? I think these should always be bundled on Windows. Note that these libraries are only built for x86 or x64, but I think that covers what chrome supports on Windows, right? (We have that restriction because of MIPS on Linux and I guess ARM would also not work for SwiftShader at the moment). https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll', On 2017/02/23 15:00:50, grt (UTC plus 1) wrote: > are there pdb files for these as well? Yes, I think so (I'm not in front of my Windows desktop at the moment, but I think that there are pdb files generated in the out\Default\swiftshader directory).
The CQ bit was checked by sugoi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ On 2017/02/23 15:22:55, sugoi wrote: > On 2017/02/23 15:00:50, grt (UTC plus 1) wrote: > > are these only for is_chrome_branded=true builds? > They could be used with any version, but is_chrome_branded would be good enough > for now. Just out of curiosity, does this file also create an installer for non > chrome branded versions? Yes. Since the BUILDFLAG is no longer conditional on is_chrome_branded=true, it's set for all Windows builds, correct? If so, please move this new block so that it immediately precedes [HIDPI]. > > do you need the inclusion of these files to be conditional on the BUILDFLAG? > I think these should always be bundled on Windows. Note that these libraries are > only built for x86 or x64, but I think that covers what chrome supports on > Windows, right? (We have that restriction because of MIPS on Linux and I guess > ARM would also not work for SwiftShader at the moment). Yup, Windows is x86 and x64 only. https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/1/chrome/tools/build/win/FILE... chrome/tools/build/win/FILES.cfg:387: 'filename': 'swiftshader/libEGL.dll', On 2017/02/23 15:22:55, sugoi wrote: > On 2017/02/23 15:00:50, grt (UTC plus 1) wrote: > > are there pdb files for these as well? > > Yes, I think so (I'm not in front of my Windows desktop at the moment, but I > think that there are pdb files generated in the out\Default\swiftshader > directory). In that case, please add stanzas for the .pdbs as well (see chrome.dll.pdb for an example). https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:3609: deps += [ "//third_party/swiftshader" ] is it //chrome/browser that depends on SS, or something down in //content?
https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2715563002/diff/20001/chrome/browser/BUILD.gn... chrome/browser/BUILD.gn:3609: deps += [ "//third_party/swiftshader" ] On 2017/02/23 15:33:38, grt (UTC plus 1) wrote: > is it //chrome/browser that depends on SS, or something down in //content? Oh yeah, it should be content/browser, not chrome/browser, will fix.
LGTM, please try this on the win-pgo trybot before submitting.
The CQ bit was checked by sugoi@google.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_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) 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 sugoi@google.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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sugoi@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...
More changes, jbauman@ PTAL. - Modified EnableSwiftShaderIfNecessary so that use_swiftshader_ can also be turned off when updating the GPU info - Many fixes to GpuDataManager unittests to separate expected results between platforms which support SwiftShader and those that do not. - Added GPU_FEATURE_TYPE_WEBGL2 as a blacklisted feature for SwiftShader - Added the .pdb files to FILES.cfg
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by sugoi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (left): https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:674: if (use_swiftshader_) I think we still need this so we don't try to update the GPU info using swiftshader's info, as our real blacklisting information should happen based on the real GPU. Though I think we should eventually change this to be more explicit about rejecting an update from swiftshader.
lgtm with some nits from me. please wait for seb to confirm that the duplicate symbol name isn't an issue. thanks. https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb', +sebmarchand: this has the same filename as the EGL dll above. do you know if the archive process preserves the 'swiftshader' directory name? https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:300: feature == gpu::GPU_FEATURE_TYPE_WEBGL2) nit: add braces since the condition now spans two lines. alternatively, simplify to: return (feature == gpu::GPU_FEATURE_TYPE_ACCELERATED_2D_CANVAS || feature == gpu::GPU_FEATURE_TYPE_WEBGL2); https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:1234: (!GpuAccessAllowed(NULL) || nit: NULL -> nullptr https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali... File ui/gl/init/gl_initializer_win.cc (right): https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali... ui/gl/init/gl_initializer_win.cc:110: gles_path = module_path.Append(L"swiftshader/"); nit: put these next three lines within #else/#endif so that you don't get a warning/error from the compiler about unreachable code when !SS
https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb', On 2017/02/27 09:31:55, grt (UTC plus 1) wrote: > +sebmarchand: this has the same filename as the EGL dll above. do you know if > the archive process preserves the 'swiftshader' directory name? We don't publish the symbols for the stuff not directly in |build_dir| (see https://chrome-internal.googlesource.com/chrome/tools/build/+/master/scripts/... , we don't process the sub directories), you'll need to do this explicitly (like we do for the Syzygy stuff).
I still have to land a modification to the symbols related script, as sebmarchand@ pointed out. https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... File chrome/installer/mini_installer/chrome.release (right): https://codereview.chromium.org/2715563002/diff/1/chrome/installer/mini_insta... chrome/installer/mini_installer/chrome.release:79: swiftshader\libEGL.dll: %(VersionDir)s\swiftshader\ On 2017/02/23 15:33:38, grt (UTC plus 1) wrote: > On 2017/02/23 15:22:55, sugoi wrote: > > On 2017/02/23 15:00:50, grt (UTC plus 1) wrote: > > > are these only for is_chrome_branded=true builds? > > They could be used with any version, but is_chrome_branded would be good > enough > > for now. Just out of curiosity, does this file also create an installer for > non > > chrome branded versions? > > Yes. Since the BUILDFLAG is no longer conditional on is_chrome_branded=true, > it's set for all Windows builds, correct? If so, please move this new block so > that it immediately precedes [HIDPI]. > > > > do you need the inclusion of these files to be conditional on the BUILDFLAG? > > I think these should always be bundled on Windows. Note that these libraries > are > > only built for x86 or x64, but I think that covers what chrome supports on > > Windows, right? (We have that restriction because of MIPS on Linux and I guess > > ARM would also not work for SwiftShader at the moment). > > Yup, Windows is x86 and x64 only. Done. https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/1/content/browser/gpu/gpu_dat... content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if (!base::CommandLine::ForCurrentProcess()->HasSwitch( On 2017/02/23 04:30:45, jbauman wrote: > Check BUILDFLAG(ENABLE_SWIFTSHADER) here. Otherwise this will automatically > switch to using swiftshader even when it's disabled. Done. https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w... File ui/gl/init/gl_initializer_win.cc (right): https://codereview.chromium.org/2715563002/diff/1/ui/gl/init/gl_initializer_w... ui/gl/init/gl_initializer_win.cc:105: (use_gl == kGLImplementationSwiftShaderName) || On 2017/02/23 04:30:45, jbauman wrote: > This should return false if !BUILDFLAG(ENABLE_SWIFTSHADER) and > using_swift_shader is true. Otherwise the browser process could think it's using > swiftshader and the gpu process would ignore that and use D3D. Done. https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... File chrome/tools/build/win/FILES.cfg (right): https://codereview.chromium.org/2715563002/diff/100001/chrome/tools/build/win... chrome/tools/build/win/FILES.cfg:621: 'filename': 'swiftshader/libEGL.dll.pdb', On 2017/02/27 16:00:22, Sébastien Marchand wrote: > On 2017/02/27 09:31:55, grt (UTC plus 1) wrote: > > +sebmarchand: this has the same filename as the EGL dll above. do you know if > > the archive process preserves the 'swiftshader' directory name? > > We don't publish the symbols for the stuff not directly in |build_dir| (see > https://chrome-internal.googlesource.com/chrome/tools/build/+/master/scripts/... > , we don't process the sub directories), you'll need to do this explicitly (like > we do for the Syzygy stuff). Will add a special case for SwiftShader in that script https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (left): https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:674: if (use_swiftshader_) On 2017/02/25 00:30:15, jbauman wrote: > I think we still need this so we don't try to update the GPU info using > swiftshader's info, as our real blacklisting information should happen based on > the real GPU. Though I think we should eventually change this to be more > explicit about rejecting an update from swiftshader. Done. https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:300: feature == gpu::GPU_FEATURE_TYPE_WEBGL2) On 2017/02/27 09:31:55, grt (UTC plus 1) wrote: > nit: add braces since the condition now spans two lines. alternatively, simplify > to: > return (feature == gpu::GPU_FEATURE_TYPE_ACCELERATED_2D_CANVAS || > feature == gpu::GPU_FEATURE_TYPE_WEBGL2); Done. https://codereview.chromium.org/2715563002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:1234: (!GpuAccessAllowed(NULL) || On 2017/02/27 09:31:55, grt (UTC plus 1) wrote: > nit: NULL -> nullptr Done. https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali... File ui/gl/init/gl_initializer_win.cc (right): https://codereview.chromium.org/2715563002/diff/100001/ui/gl/init/gl_initiali... ui/gl/init/gl_initializer_win.cc:110: gles_path = module_path.Append(L"swiftshader/"); On 2017/02/27 09:31:56, grt (UTC plus 1) wrote: > nit: put these next three lines within #else/#endif so that you don't get a > warning/error from the compiler about unreachable code when !SS Done. Also did a similar fix in the x11 path.
The CQ bit was checked by sugoi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
still lgtm w/ one more nit https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl.h (right): https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl.h:15: #include "base/files/file_path.h" unused
lgtm. Once you commit this we should check GPU.EGLDisplayType and ensure it doesn't change too much.
https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if (!GpuAccessAllowed(nullptr) || This call to GpuAccessAllowed() causes SwiftShader to be enabled too often. This is due to: https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... Because EnableSwiftShaderIfNecessary() is called within UpdateGpuInfo(), GpuAccessAllowed() can return false simply because UpdatePreliminaryBlacklistedFeatures() hasn't been called yet. I don't think GpuAccessAllowed() is necessary here, correct me if I'm wrong. I'll remove it so that SwiftShader is only enabled when WebGL is explictly blacklisted, which should be enough for now.
The CQ bit was checked by sugoi@google.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...
sugoi@google.com changed reviewers: + piman@chromium.org, sugoi@chromium.org, thakis@chromium.org
Adding thakis@ for the changes in chrome/browser.
Description was changed from ========== Moving SwiftShader from component to bundled library Multiple things are modified here and I can split this in multiple cls if required by the reviewers: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 ========== to ========== Moving SwiftShader from component to bundled library Multiple things are modified here and I can split this in multiple cls if required by the reviewers: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/03/01 16:50:22, sugoi1 wrote: > https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > https://codereview.chromium.org/2715563002/diff/120001/content/browser/gpu/gp... > content/browser/gpu/gpu_data_manager_impl_private.cc:1235: if > (!GpuAccessAllowed(nullptr) || > This call to GpuAccessAllowed() causes SwiftShader to be enabled too often. > This is due to: > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > Because EnableSwiftShaderIfNecessary() is called within UpdateGpuInfo(), > GpuAccessAllowed() can return false simply because > UpdatePreliminaryBlacklistedFeatures() hasn't been called yet. I don't think > GpuAccessAllowed() is necessary here, correct me if I'm wrong. > > I'll remove it so that SwiftShader is only enabled when WebGL is explictly > blacklisted, which should be enough for now. This is necessary if the GPU isn't available but isn't blacklisted. For example if it failed to launch, crashed three times, or is blocked because the set of preliminarily blacklisted features is different from the full set of blacklisted features.
Please get a reviewer from c/b/extensions/OWNERS and c/b/policy/OWNERS for the test changes. Especially the policy change looks like it needs eyes from someone with domain expertise.
sugoi@chromium.org changed reviewers: + asargent@chromium.org, emaxx@chromium.org, rdevlin.cronin@chromium.org - sugoi@google.com
Adding emaxx@ for c/b/policy Adding asargent@ for c/b/extensions/api/webstore_private Adding rdevlin.cronin@ for c/b/extensions
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:435: if (content::GpuDataManager::GetInstance()->ShouldUseSwiftShader()) { It seems strange that this change means the GpuDataManager no longer respects the blacklist - why is that?
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:435: if (content::GpuDataManager::GetInstance()->ShouldUseSwiftShader()) { On 2017/03/01 23:47:24, Devlin wrote: > It seems strange that this change means the GpuDataManager no longer respects > the blacklist - why is that? When WebGL is blacklisted on a given GPU, SwiftShader is automatically enabled and is used to render WebGL. WebGL will now be available on 100% of Windows machines, so it can't be disabled.
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && I'm not entirely following this. Is this changed because GpuAccessAllowed() returns true even when SwiftShader is going to be used instead of the real GPU? If that's the case, could it be possible to change the test to verify that GPU is _actually_ not allowed? Without relying on this implicit interplay between GpuAccessAllowed() and ShouldUseSwiftShader().
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && On 2017/03/02 16:06:29, emaxx wrote: > I'm not entirely following this. Is this changed because GpuAccessAllowed() > returns true even when SwiftShader is going to be used instead of the real GPU? That's indeed the first line of GpuAccessAllowed(): https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > If that's the case, could it be possible to change the test to verify that GPU > is _actually_ not allowed? Without relying on this implicit interplay between > GpuAccessAllowed() and ShouldUseSwiftShader(). I'm honestly not sure how. Disabling the hardware GPU automatically enables SwiftShader. I'll check if GpuAccessAllowed() can return false even if SwiftShader is enabled without breaking other things.
sugoi@chromium.org changed reviewers: - asargent@chromium.org
Removing asargent@ from the reviewers list since he no longer works on Chrome.
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:436: EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted( nit: How about content::GpuDataManager* gpu_manager = content::GpuDataManager::GetInstance(); EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(), gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL); https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:4610: EXPECT_TRUE(service()->IsExtensionEnabled(id)); Unfortunately, this defeats the purpose of the test, which is checking what happens when requirements aren't satisfied. If they are, then we didn't test anything. :) https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:4696: EXPECT_EQ(0u, GetErrors().size()); ditto here
I guess the question here is if there are other good reasons for disabling webgl from a policy or extension, other than that it needs GPU access. My gut says "yes" and making "webgl disabled" mean "use swiftshader instead of gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of this change in behavior? On Thu, Mar 2, 2017 at 4:59 PM, <rdevlin.cronin@chromium.org> wrote: > > https://codereview.chromium.org/2715563002/diff/140001/ > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc > File > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc > (right): > > https://codereview.chromium.org/2715563002/diff/140001/ > chrome/browser/extensions/api/webstore_private/webstore_ > private_apitest.cc#newcode436 > chrome/browser/extensions/api/webstore_private/webstore_ > private_apitest.cc:436: > EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted( > nit: How about > content::GpuDataManager* gpu_manager = > content::GpuDataManager::GetInstance(); > EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(), > > gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL); > > https://codereview.chromium.org/2715563002/diff/140001/ > chrome/browser/extensions/extension_service_unittest.cc > File chrome/browser/extensions/extension_service_unittest.cc (right): > > https://codereview.chromium.org/2715563002/diff/140001/ > chrome/browser/extensions/extension_service_unittest.cc#newcode4610 > chrome/browser/extensions/extension_service_unittest.cc:4610: > EXPECT_TRUE(service()->IsExtensionEnabled(id)); > Unfortunately, this defeats the purpose of the test, which is checking > what happens when requirements aren't satisfied. If they are, then we > didn't test anything. :) > > https://codereview.chromium.org/2715563002/diff/140001/ > chrome/browser/extensions/extension_service_unittest.cc#newcode4696 > chrome/browser/extensions/extension_service_unittest.cc:4696: > EXPECT_EQ(0u, GetErrors().size()); > ditto here > > https://codereview.chromium.org/2715563002/ > -- 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 2017/03/03 15:54:13, Nico wrote: > I guess the question here is if there are other good reasons for disabling > webgl from a policy or extension, other than that it needs GPU access. My > gut says "yes" and making "webgl disabled" mean "use swiftshader instead of > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of > this change in behavior? Well, the main goal of the SwiftShader integration is to be able to tell Web Developers that they can use WebGL and that it will always be available. If it's important that WebGL be sometimes disabled (I honestly can't see why that would be the case, but if anyone has an example of a case where it would be important, I'll be glad to look into it), then I'll need a way to distinguish blacklisted features between "unsupported by the hardware" and "requested through some API". > On Thu, Mar 2, 2017 at 4:59 PM, <mailto:rdevlin.cronin@chromium.org> wrote: > > > > > https://codereview.chromium.org/2715563002/diff/140001/ > > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc > > File > > chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc > > (right): > > > > https://codereview.chromium.org/2715563002/diff/140001/ > > chrome/browser/extensions/api/webstore_private/webstore_ > > private_apitest.cc#newcode436 > > chrome/browser/extensions/api/webstore_private/webstore_ > > private_apitest.cc:436: > > EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted( > > nit: How about > > content::GpuDataManager* gpu_manager = > > content::GpuDataManager::GetInstance(); > > EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(), > > > > gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL); > > > > https://codereview.chromium.org/2715563002/diff/140001/ > > chrome/browser/extensions/extension_service_unittest.cc > > File chrome/browser/extensions/extension_service_unittest.cc (right): > > > > https://codereview.chromium.org/2715563002/diff/140001/ > > chrome/browser/extensions/extension_service_unittest.cc#newcode4610 > > chrome/browser/extensions/extension_service_unittest.cc:4610: > > EXPECT_TRUE(service()->IsExtensionEnabled(id)); > > Unfortunately, this defeats the purpose of the test, which is checking > > what happens when requirements aren't satisfied. If they are, then we > > didn't test anything. :) > > > > https://codereview.chromium.org/2715563002/diff/140001/ > > chrome/browser/extensions/extension_service_unittest.cc#newcode4696 > > chrome/browser/extensions/extension_service_unittest.cc:4696: > > EXPECT_EQ(0u, GetErrors().size()); > > ditto here > > > > https://codereview.chromium.org/2715563002/ > > > > -- > 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.
On 2017/03/03 16:11:45, sugoi1 wrote: > On 2017/03/03 15:54:13, Nico wrote: > > I guess the question here is if there are other good reasons for disabling > > webgl from a policy or extension, other than that it needs GPU access. My > > gut says "yes" and making "webgl disabled" mean "use swiftshader instead of > > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of > > this change in behavior? > > Well, the main goal of the SwiftShader integration is to be able to tell Web > Developers that they can use WebGL and that it will always be available. > If it's important that WebGL be sometimes disabled (I honestly can't see why > that would be the case, but if anyone has an example of a case where it would be > important, I'll be glad to look into it), then I'll need a way to distinguish > blacklisted features between "unsupported by the hardware" and "requested > through some API". > Looking at http://crbug.com/337713 that tracked adding of the HardwareAccelerationModeEnabled admin policy, it seems that the motivation behind it was: 1) dealing with buggy hardware (causing black screen etc.); 2) dealing with very high CPU usage in virtualized environment. So I guess #2 is important for deciding on whether SwiftShader should be subject to the policy. Is it possible for the software renderer to cause excessively high CPU load in typical usage patterns when working under VM? Another thought: looking at that bug, the policy was considered by many as a way for enterprises to trigger the effect of the "--disable-gpu" command line flag. Does this command line flag disable SwiftShader too? On 2017/03/02 16:23:17, sugoi1 wrote: > That's indeed the first line of GpuAccessAllowed(): > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... I saw this, but I was trying to say that this behavior seems rather unexpected from interface perspective - the method name seems misleading to me. > I'm honestly not sure how. Disabling the hardware GPU automatically enables > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > SwiftShader is enabled without breaking other things. I'm not familiar with that code, but what about splitting this method into two - one is _truely_ about hardware access, and other about GPU in total (either hardware accelerated or software emulated)? Then the test (and the other callsites) could be made to verify exactly what it wants to.
On 2017/03/03 16:42:52, emaxx wrote: > On 2017/03/03 16:11:45, sugoi1 wrote: > > On 2017/03/03 15:54:13, Nico wrote: > > > I guess the question here is if there are other good reasons for disabling > > > webgl from a policy or extension, other than that it needs GPU access. My > > > gut says "yes" and making "webgl disabled" mean "use swiftshader instead of > > > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware of > > > this change in behavior? > > > > Well, the main goal of the SwiftShader integration is to be able to tell Web > > Developers that they can use WebGL and that it will always be available. > > If it's important that WebGL be sometimes disabled (I honestly can't see why > > that would be the case, but if anyone has an example of a case where it would > be > > important, I'll be glad to look into it), then I'll need a way to distinguish > > blacklisted features between "unsupported by the hardware" and "requested > > through some API". > > > > Looking at http://crbug.com/337713 that tracked adding of the > HardwareAccelerationModeEnabled admin policy, it seems that the motivation > behind it was: > 1) dealing with buggy hardware (causing black screen etc.); > 2) dealing with very high CPU usage in virtualized environment. > > So I guess #2 is important for deciding on whether SwiftShader should be > subject to the policy. Is it possible for the software renderer to cause > excessively high CPU load in typical usage patterns when working under > VM? > > > Another thought: looking at that bug, the policy was considered by many as > a way for enterprises to trigger the effect of the "--disable-gpu" command > line flag. Does this command line flag disable SwiftShader too? --disable-gpu is how you enable SwiftShader > On 2017/03/02 16:23:17, sugoi1 wrote: > > That's indeed the first line of GpuAccessAllowed(): > > > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > I saw this, but I was trying to say that this behavior seems rather > unexpected from interface perspective - the method name seems misleading > to me. AFAIK, GpuAccessAllowed means that chrome has a GPU process, which it does when using SwiftShader. > > I'm honestly not sure how. Disabling the hardware GPU automatically enables > > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > > SwiftShader is enabled without breaking other things. > > I'm not familiar with that code, but what about splitting this method > into two - one is _truely_ about hardware access, and other about GPU in > total (either hardware accelerated or software emulated)? Then the test > (and the other callsites) could be made to verify exactly what it wants to. Yes, I think it has to be done that avoid issues from modifying the current meaning of GpuAccessAllowed().
On 2017/03/03 16:56:05, sugoi1 wrote: > On 2017/03/03 16:42:52, emaxx wrote: > > On 2017/03/03 16:11:45, sugoi1 wrote: > > > On 2017/03/03 15:54:13, Nico wrote: > > > > I guess the question here is if there are other good reasons for disabling > > > > webgl from a policy or extension, other than that it needs GPU access. My > > > > gut says "yes" and making "webgl disabled" mean "use swiftshader instead > of > > > > gpu for webgl" seems pretty surprising to me. Are all stakeholders aware > of > > > > this change in behavior? > > > > > > Well, the main goal of the SwiftShader integration is to be able to tell Web > > > Developers that they can use WebGL and that it will always be available. > > > If it's important that WebGL be sometimes disabled (I honestly can't see why > > > that would be the case, but if anyone has an example of a case where it > would > > be > > > important, I'll be glad to look into it), then I'll need a way to > distinguish > > > blacklisted features between "unsupported by the hardware" and "requested > > > through some API". > > > > > > > Looking at http://crbug.com/337713 that tracked adding of the > > HardwareAccelerationModeEnabled admin policy, it seems that the motivation > > behind it was: > > 1) dealing with buggy hardware (causing black screen etc.); > > 2) dealing with very high CPU usage in virtualized environment. > > > > So I guess #2 is important for deciding on whether SwiftShader should be > > subject to the policy. Is it possible for the software renderer to cause > > excessively high CPU load in typical usage patterns when working under > > VM? > > > > > > Another thought: looking at that bug, the policy was considered by many as > > a way for enterprises to trigger the effect of the "--disable-gpu" command > > line flag. Does this command line flag disable SwiftShader too? > > --disable-gpu is how you enable SwiftShader > > > On 2017/03/02 16:23:17, sugoi1 wrote: > > > That's indeed the first line of GpuAccessAllowed(): > > > > > > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > > > I saw this, but I was trying to say that this behavior seems rather > > unexpected from interface perspective - the method name seems misleading > > to me. > > AFAIK, GpuAccessAllowed means that chrome has a GPU process, which it does when > using SwiftShader. > > > > I'm honestly not sure how. Disabling the hardware GPU automatically enables > > > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > > > SwiftShader is enabled without breaking other things. > > > > I'm not familiar with that code, but what about splitting this method > > into two - one is _truely_ about hardware access, and other about GPU in > > total (either hardware accelerated or software emulated)? Then the test > > (and the other callsites) could be made to verify exactly what it wants to. > * Yes, I think it has to be done that way in order to avoid issues from modifying the current meaning of GpuAccessAllowed().
The CQ bit was checked by sugoi@google.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...
Hi rdevlin.cronin@ and emaxx@, I've looked into the issues you pointed out and I can't figure out better fixes than the ones I already proposed. If you feel that there would be better fixes, I'd be glad to discuss it with you, possibly over VC, but as far as I can see, the changes proposed here are working as intended. https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/webstore_private/webstore_private_apitest.cc:436: EXPECT_FALSE(content::GpuDataManager::GetInstance()->IsFeatureBlacklisted( On 2017/03/02 21:59:29, Devlin wrote: > nit: How about > content::GpuDataManager* gpu_manager = content::GpuDataManager::GetInstance(); > EXPECT_EQ(gpu_manager->ShouldUseSwiftShader(), > gpu_manager->IsFeatureBlacklisted(gpu::GPU_FEATURE_TYPE_WEBGL); > Done. https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:4610: EXPECT_TRUE(service()->IsExtensionEnabled(id)); On 2017/03/02 21:59:29, Devlin wrote: > Unfortunately, this defeats the purpose of the test, which is checking what > happens when requirements aren't satisfied. If they are, then we didn't test > anything. :) I've looked into this, and I still think this is the proper fix. The BlackListWebGL() function installs GPU info where WebGL is blacklisted, which, as of this cl, has the effect of enabling SwiftShader, which is the desired behavior. If I were to make a testing specific fix, it would not represent reality. The test is still useful IMHO, since it verifies that, when SwiftShader is enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader is only being enabled on Windows, so the test still behaves as usual on other platforms. https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && On 2017/03/02 16:23:16, sugoi1 wrote: > On 2017/03/02 16:06:29, emaxx wrote: > > I'm not entirely following this. Is this changed because GpuAccessAllowed() > > returns true even when SwiftShader is going to be used instead of the real > GPU? > > That's indeed the first line of GpuAccessAllowed(): > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > > If that's the case, could it be possible to change the test to verify that GPU > > is _actually_ not allowed? Without relying on this implicit interplay between > > GpuAccessAllowed() and ShouldUseSwiftShader(). > > I'm honestly not sure how. Disabling the hardware GPU automatically enables > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > SwiftShader is enabled without breaking other things. After looking into this, I think this code is better this way. Essentially, GpuAccessAllowed() has never meant that the Hardware GPU was being used, it meant that a GPU process is created. SwiftShader was already there before, but because SwiftShader was Chrome branded only, the test never knew about SwiftShader, which is not the experience that actual Chrome users would have gotten. If you want to know that a real hardware GPU is being used, it seems normal to check that no software renderer is running. I could add a function to GpuDataManager that combines both checks, but it would only be used here.
sugoi@chromium.org changed reviewers: + vmiura@chromium.org - sugoi@google.com
The CQ bit was checked by sugoi@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/2715563002/diff/140001/chrome/browser/policy/... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... chrome/browser/policy/policy_browsertest.cc:4081: content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && On 2017/03/06 18:31:42, sugoi wrote: > On 2017/03/02 16:23:16, sugoi1 wrote: > > On 2017/03/02 16:06:29, emaxx wrote: > > > I'm not entirely following this. Is this changed because GpuAccessAllowed() > > > returns true even when SwiftShader is going to be used instead of the real > > GPU? > > > > That's indeed the first line of GpuAccessAllowed(): > > > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > > > > If that's the case, could it be possible to change the test to verify that > GPU > > > is _actually_ not allowed? Without relying on this implicit interplay > between > > > GpuAccessAllowed() and ShouldUseSwiftShader(). > > > > I'm honestly not sure how. Disabling the hardware GPU automatically enables > > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > > SwiftShader is enabled without breaking other things. > > After looking into this, I think this code is better this way. Essentially, > GpuAccessAllowed() has never meant that the Hardware GPU was being used, it > meant that a GPU process is created. SwiftShader was already there before, but > because SwiftShader was Chrome branded only, the test never knew about > SwiftShader, which is not the experience that actual Chrome users would have > gotten. If you want to know that a real hardware GPU is being used, it seems > normal to check that no software renderer is running. I could add a function to > GpuDataManager that combines both checks, but it would only be used here. I see, thanks for the explanation. I'd still find it more clear if this knowledge would be kept inside GpuDataManager under special-purpose method (name something like like "HardwareGpuAccessAllowed"). First, because it would be more visible in case of some changes on the side of the GPU component - e.g. (just speculating) introducing a different software engine apart from SwiftShader. Second, because in the test here we want to verify that the hardware acceleration is actually disabled, meanwhile the test now will pass if ShouldUseSwiftShader() returns true, and that's regardless of the other state. This is not equivalent (considering the potential case when SwiftShader is disabled at runtime and an attempt to use real GPU takes place) and may mask bugs. But if you're opposed to this change, then, please, drop an explanatory comment here in the test - something like that: "The policy controls the state of the hardware GPU acceleration - it's not applied to the state of the software emulation SwiftShader. But GpuAccessAllowed() returns true when either the hardware acceleration or SwiftShader are allowed. ShouldUseSwiftShader() returning true indicates that SwiftShader is used instead of the hardware acceleration. Therefore the results of these two methods are combined here in order to check that the hardware GPU acceleration is not going to be used."
On 2017/03/06 20:10:51, emaxx wrote: > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... > File chrome/browser/policy/policy_browsertest.cc (right): > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/policy/... > chrome/browser/policy/policy_browsertest.cc:4081: > content::GpuDataManager::GetInstance()->GpuAccessAllowed(nullptr) && > On 2017/03/06 18:31:42, sugoi wrote: > > On 2017/03/02 16:23:16, sugoi1 wrote: > > > On 2017/03/02 16:06:29, emaxx wrote: > > > > I'm not entirely following this. Is this changed because > GpuAccessAllowed() > > > > returns true even when SwiftShader is going to be used instead of the real > > > GPU? > > > > > > That's indeed the first line of GpuAccessAllowed(): > > > > > > https://cs.chromium.org/chromium/src/content/browser/gpu/gpu_data_manager_imp... > > > > > > > If that's the case, could it be possible to change the test to verify that > > GPU > > > > is _actually_ not allowed? Without relying on this implicit interplay > > between > > > > GpuAccessAllowed() and ShouldUseSwiftShader(). > > > > > > I'm honestly not sure how. Disabling the hardware GPU automatically enables > > > SwiftShader. I'll check if GpuAccessAllowed() can return false even if > > > SwiftShader is enabled without breaking other things. > > > > After looking into this, I think this code is better this way. Essentially, > > GpuAccessAllowed() has never meant that the Hardware GPU was being used, it > > meant that a GPU process is created. SwiftShader was already there before, but > > because SwiftShader was Chrome branded only, the test never knew about > > SwiftShader, which is not the experience that actual Chrome users would have > > gotten. If you want to know that a real hardware GPU is being used, it seems > > normal to check that no software renderer is running. I could add a function > to > > GpuDataManager that combines both checks, but it would only be used here. > > I see, thanks for the explanation. > > I'd still find it more clear if this knowledge would be kept inside > GpuDataManager under special-purpose method (name something like like > "HardwareGpuAccessAllowed"). > First, because it would be more visible in case of some changes on the side of > the GPU component - e.g. (just speculating) introducing a different software > engine apart from SwiftShader. > Second, because in the test here we want to verify that the hardware > acceleration is actually disabled, meanwhile the test now will pass if > ShouldUseSwiftShader() returns true, and that's regardless of the other state. > This is not equivalent (considering the potential case when SwiftShader is > disabled at runtime and an attempt to use real GPU takes place) and may mask > bugs. > > But if you're opposed to this change, then, please, drop an explanatory comment > here in the test - something like that: > > "The policy controls the state of the hardware GPU acceleration - it's not > applied to the state of the software emulation SwiftShader. But > GpuAccessAllowed() returns true when either the hardware acceleration or > SwiftShader are allowed. ShouldUseSwiftShader() returning true indicates that > SwiftShader is used instead of the hardware acceleration. Therefore the results > of these two methods are combined here in order to check that the hardware GPU > acceleration is not going to be used." I'm not strongly opposed to making the change, but in order to avoid making too many changes in this cl, I'll add the comment you suggested. If you would prefer getting a function specifically for this test, similar to the HardwareGpuAccessAllowed function you proposed, I will gladly do it in a follow up to this cl, once this has landed and passes tests on all bots. I just want to land this ASAP in order to get as much coverage as possible before M59. Thanks.
On 2017/03/06 20:40:24, sugoi1 wrote: > I'm not strongly opposed to making the change, but in order to avoid making too > many changes in this cl, I'll add the comment you suggested. If you would prefer > getting a function specifically for this test, similar to the > HardwareGpuAccessAllowed function you proposed, I will gladly do it in a follow > up to this cl, once this has landed and passes tests on all bots. I just want to > land this ASAP in order to get as much coverage as possible before M59. Thanks. LGTM
https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/extension_service_unittest.cc:4610: EXPECT_TRUE(service()->IsExtensionEnabled(id)); On 2017/03/06 18:31:42, sugoi wrote: > On 2017/03/02 21:59:29, Devlin wrote: > > Unfortunately, this defeats the purpose of the test, which is checking what > > happens when requirements aren't satisfied. If they are, then we didn't test > > anything. :) > > I've looked into this, and I still think this is the proper fix. The > BlackListWebGL() function installs GPU info where WebGL is blacklisted, which, > as of this cl, has the effect of enabling SwiftShader, which is the desired > behavior. If I were to make a testing specific fix, it would not represent > reality. > > The test is still useful IMHO, since it verifies that, when SwiftShader is > enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader is > only being enabled on Windows, so the test still behaves as usual on other > platforms. The thing is, this isn't actually a GPU test, WebGL test, swiftshader test, etc. The behavior it's testing is that if an extension is disabled because of unsupported requirements, and then updates and no longer has unsupported requirements, it's automatically re-enabled. (See also the comment on line 4587.) Disabling WebGL was an easy way of triggering an unsupported requirement, because it's entirely cross-platform and was easy to do from a unittest, but it's not the sole purpose of the test. The way it's rewritten now, we don't actually test the flow that we're supposed to be exercising, so we lose test coverage. If we can no longer use webgl as an unavailable requirement, we'll have to substitute something else or find a test-only way of marking a requirement as unavailable.
nit: Please remove "and I can split this in multiple cls if required by the reviewers" from the CL description. While that's a useful comment for reviewers, it isn't relevant to the history of this CL once it lands.
Description was changed from ========== Moving SwiftShader from component to bundled library Multiple things are modified here and I can split this in multiple cls if required by the reviewers: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ========== to ========== Moving SwiftShader from component to bundled library Multiple things are modified here: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ==========
On 2017/03/07 09:49:27, grt (UTC plus 1) wrote: > nit: Please remove "and I can split this in > multiple cls if required by the reviewers" from the CL description. While that's > a useful comment for reviewers, it isn't relevant to the history of this CL once > it lands. Done
On 2017/03/06 22:58:10, Devlin wrote: > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > File chrome/browser/extensions/extension_service_unittest.cc (right): > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/extension_service_unittest.cc:4610: > EXPECT_TRUE(service()->IsExtensionEnabled(id)); > On 2017/03/06 18:31:42, sugoi wrote: > > On 2017/03/02 21:59:29, Devlin wrote: > > > Unfortunately, this defeats the purpose of the test, which is checking what > > > happens when requirements aren't satisfied. If they are, then we didn't > test > > > anything. :) > > > > I've looked into this, and I still think this is the proper fix. The > > BlackListWebGL() function installs GPU info where WebGL is blacklisted, which, > > as of this cl, has the effect of enabling SwiftShader, which is the desired > > behavior. If I were to make a testing specific fix, it would not represent > > reality. > > > > The test is still useful IMHO, since it verifies that, when SwiftShader is > > enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader is > > only being enabled on Windows, so the test still behaves as usual on other > > platforms. > > The thing is, this isn't actually a GPU test, WebGL test, swiftshader test, etc. > The behavior it's testing is that if an extension is disabled because of > unsupported requirements, and then updates and no longer has unsupported > requirements, it's automatically re-enabled. (See also the comment on line > 4587.) Disabling WebGL was an easy way of triggering an unsupported > requirement, because it's entirely cross-platform and was easy to do from a > unittest, but it's not the sole purpose of the test. The way it's rewritten > now, we don't actually test the flow that we're supposed to be exercising, so we > lose test coverage. > > If we can no longer use webgl as an unavailable requirement, we'll have to > substitute something else or find a test-only way of marking a requirement as > unavailable. Understood. I'm not the most knowledgeable person about chrome extensions and wouldn't exactly know where to start. Could I add a TODO for someone who knows about extensions to look into other options than disabling WebGL? Otherwise, do you have any suggestions? I understand the purpose of the test, but I would prefer if it wasn't based on a state that would never be experienced by Chrome users, if possible.
On 2017/03/07 13:15:26, sugoi1 wrote: > On 2017/03/06 22:58:10, Devlin wrote: > > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > > File chrome/browser/extensions/extension_service_unittest.cc (right): > > > > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > > chrome/browser/extensions/extension_service_unittest.cc:4610: > > EXPECT_TRUE(service()->IsExtensionEnabled(id)); > > On 2017/03/06 18:31:42, sugoi wrote: > > > On 2017/03/02 21:59:29, Devlin wrote: > > > > Unfortunately, this defeats the purpose of the test, which is checking > what > > > > happens when requirements aren't satisfied. If they are, then we didn't > > test > > > > anything. :) > > > > > > I've looked into this, and I still think this is the proper fix. The > > > BlackListWebGL() function installs GPU info where WebGL is blacklisted, > which, > > > as of this cl, has the effect of enabling SwiftShader, which is the desired > > > behavior. If I were to make a testing specific fix, it would not represent > > > reality. > > > > > > The test is still useful IMHO, since it verifies that, when SwiftShader is > > > enabled, WebGL doesn't get disabled. Also note that, currently, SwiftShader > is > > > only being enabled on Windows, so the test still behaves as usual on other > > > platforms. > > > > The thing is, this isn't actually a GPU test, WebGL test, swiftshader test, > etc. > > The behavior it's testing is that if an extension is disabled because of > > unsupported requirements, and then updates and no longer has unsupported > > requirements, it's automatically re-enabled. (See also the comment on line > > 4587.) Disabling WebGL was an easy way of triggering an unsupported > > requirement, because it's entirely cross-platform and was easy to do from a > > unittest, but it's not the sole purpose of the test. The way it's rewritten > > now, we don't actually test the flow that we're supposed to be exercising, so > we > > lose test coverage. > > > > If we can no longer use webgl as an unavailable requirement, we'll have to > > substitute something else or find a test-only way of marking a requirement as > > unavailable. > > Understood. I'm not the most knowledgeable person about chrome extensions and > wouldn't exactly know where to start. Could I add a TODO for someone who knows > about extensions to look into other options than disabling WebGL? Otherwise, do > you have any suggestions? I understand the purpose of the test, but I would > prefer if it wasn't based on a state that would never be experienced by Chrome > users, if possible. (Talked offline, but tl;dr is that I'd recommend allowing for tests to supply their own result of a requirements check so that we move away from dependency on any particular feature. Feel free to ping if you have any questions/run into any issues.)
On 2017/03/08 02:37:24, Devlin wrote: > On 2017/03/07 13:15:26, sugoi1 wrote: > > On 2017/03/06 22:58:10, Devlin wrote: > > > > > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > > > File chrome/browser/extensions/extension_service_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/2715563002/diff/140001/chrome/browser/extensi... > > > chrome/browser/extensions/extension_service_unittest.cc:4610: > > > EXPECT_TRUE(service()->IsExtensionEnabled(id)); > > > On 2017/03/06 18:31:42, sugoi wrote: > > > > On 2017/03/02 21:59:29, Devlin wrote: > > > > > Unfortunately, this defeats the purpose of the test, which is checking > > what > > > > > happens when requirements aren't satisfied. If they are, then we didn't > > > test > > > > > anything. :) > > > > > > > > I've looked into this, and I still think this is the proper fix. The > > > > BlackListWebGL() function installs GPU info where WebGL is blacklisted, > > which, > > > > as of this cl, has the effect of enabling SwiftShader, which is the > desired > > > > behavior. If I were to make a testing specific fix, it would not represent > > > > reality. > > > > > > > > The test is still useful IMHO, since it verifies that, when SwiftShader is > > > > enabled, WebGL doesn't get disabled. Also note that, currently, > SwiftShader > > is > > > > only being enabled on Windows, so the test still behaves as usual on other > > > > platforms. > > > > > > The thing is, this isn't actually a GPU test, WebGL test, swiftshader test, > > etc. > > > The behavior it's testing is that if an extension is disabled because of > > > unsupported requirements, and then updates and no longer has unsupported > > > requirements, it's automatically re-enabled. (See also the comment on line > > > 4587.) Disabling WebGL was an easy way of triggering an unsupported > > > requirement, because it's entirely cross-platform and was easy to do from a > > > unittest, but it's not the sole purpose of the test. The way it's rewritten > > > now, we don't actually test the flow that we're supposed to be exercising, > so > > we > > > lose test coverage. > > > > > > If we can no longer use webgl as an unavailable requirement, we'll have to > > > substitute something else or find a test-only way of marking a requirement > as > > > unavailable. > > > > Understood. I'm not the most knowledgeable person about chrome extensions and > > wouldn't exactly know where to start. Could I add a TODO for someone who knows > > about extensions to look into other options than disabling WebGL? Otherwise, > do > > you have any suggestions? I understand the purpose of the test, but I would > > prefer if it wasn't based on a state that would never be experienced by Chrome > > users, if possible. > > (Talked offline, but tl;dr is that I'd recommend allowing for tests to supply > their own result of a requirements check so that we move away from dependency on > any particular feature. Feel free to ping if you have any questions/run into > any issues.) Spun off another cl for fixing extension tests here: https://codereview.chromium.org/2737983002/ Will be back to this cl once extensions tests are fixed.
The CQ bit was checked by sugoi@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...
sugoi@chromium.org changed reviewers: - emaxx@chromium.org, rdevlin.cronin@chromium.org
Removing emaxx@ and rdevlin.cronin@ from reviewers list. Fixes for policy tests and extension tests were landed in separate cls.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome/ lgtm
The CQ bit was checked by sugoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, laforge@chromium.org, sebmarchand@chromium.org, jbauman@chromium.org, grt@chromium.org, emaxx@chromium.org Link to the patchset: https://codereview.chromium.org/2715563002/#ps220001 (title: "Update past extension test fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 220001, "attempt_start_ts": 1489514629912500, "parent_rev": "8e5a6ea2b78481bc734b96b2350dad28698161d2", "commit_rev": "842efc6922b0cd66febd6f36f04318ebf396c184"}
Message was sent while issue was closed.
Description was changed from ========== Moving SwiftShader from component to bundled library Multiple things are modified here: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ========== to ========== Moving SwiftShader from component to bundled library Multiple things are modified here: - The SwiftShader component was entirely removed - The kSwiftShaderPath switch was removed, since SwiftShader will now be in a constant location - SwiftShader files were added to FILES.cfg and chrome.release BUG=630728 TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial Review-Url: https://codereview.chromium.org/2715563002 Cr-Commit-Position: refs/heads/master@{#456764} Committed: https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f043... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/842efc6922b0cd66febd6f36f043... |