|
|
DescriptionThis is necessary for tests that wish to disable WebGL
A few different tests are blacklisting the WebGL GPU feature. This will
no longer work when SwiftShader is fully integrated in chromium, as it
will become a fallback for GPU accelerated WebGL, and the WebGL feature
will stay enabled.
In order to fix this issue, the WebGL feature will now only be enabled
when accelerated. When SwiftShader is used, these features will now be
disabled. A new utility function, IsWebGLEnabled(), which checks for
both swiftshader and the WebGL feature, was added in order to verify if
WebGL can actually be used by chromium.
BUG=630728
R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org
TBR=laforge@chromium.org
TBR_REASON=Trivial change to swiftshader_component_installer.cc, which will soon be deleted entirely
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2737983002
Cr-Commit-Position: refs/heads/master@{#456405}
Committed: https://chromium.googlesource.com/chromium/src/+/9e3f185360c78d129af6a5fbc4e0515c3b2f6793
Patch Set 1 #Patch Set 2 : Added SwiftShader as a GPU feature #
Total comments: 7
Patch Set 3 : Removed kDisableSoftwareRasterizer from disabling SwiftShader #Patch Set 4 : Restored kDisableSoftwareRasterizer for disabling SwiftShader #
Total comments: 2
Patch Set 5 : WebGL and WebGL2 features will only enabled when accelerated #
Total comments: 4
Patch Set 6 : Reverted WebGL2 change and renamed wegl -> accelerated_webgl #
Total comments: 1
Patch Set 7 : Removed change to requirements_info.cc #
Total comments: 2
Patch Set 8 : Fixed SwiftShader tests #Patch Set 9 : Formatting fix #Messages
Total messages: 75 (29 generated)
Hi Devlin, I looked into the solution we talked about yesterday, but it looked a bit hacky, and since disabling WebGL was sort of a hack, it started feeling like a hack on top of a hack, so I moved away from it. I took a step back and looked at what we were trying to accomplish, which is to disable WebGL for tests. I found 3 different spots where extensions tests did something similar, so I removed all that code and instead introduced a testing only function that explicitly disables WebGL. Tell me what you think. Thanks.
Description was changed from ========== Change how tests disable WebGL A few different tests were using GpuDataManager::InitializeForTesting in order to set GPU info of a GPU with the WebGL feature blacklisted. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled, even if a fake bad GPU is set. In order to fix this issue, I added a function that explicitly disables WebGL in the GPUDataManagerImpl object, without relying on a fake bad GPU to do it. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ========== to ========== Change how tests disable WebGL A few different tests were using GpuDataManager::InitializeForTesting in order to set GPU info of a GPU with the WebGL feature blacklisted. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled, even if a fake bad GPU is set. In order to fix this issue, I added a function that explicitly disables WebGL in the GPUDataManagerImpl object, without relying on a fake bad GPU to do it. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ==========
sugoi@chromium.org changed reviewers: + zmo@chromium.org
Adding zmo@ to get his opinion on the fix.
On 2017/03/08 19:34:51, sugoi1 wrote: > Adding zmo@ to get his opinion on the fix. I feel it's better to add a new blacklist feature "SwiftShader" and we just disable it for tests. So it not only covers the WebGL case, but other cases in the future (say, WebGL2).
On 2017/03/08 21:09:07, Zhenyao Mo wrote: > On 2017/03/08 19:34:51, sugoi1 wrote: > > Adding zmo@ to get his opinion on the fix. > > I feel it's better to add a new blacklist feature "SwiftShader" and we just > disable it for tests. So it not only covers the WebGL case, but other cases in > the future (say, WebGL2). Or if you want to stay on the simple side, just add a commandline switch to disable SwiftShader, and pass it in for most tests, except for tests that require SwiftShader.
On 2017/03/08 21:10:36, Zhenyao Mo wrote: > On 2017/03/08 21:09:07, Zhenyao Mo wrote: > > On 2017/03/08 19:34:51, sugoi1 wrote: > > > Adding zmo@ to get his opinion on the fix. > > > > I feel it's better to add a new blacklist feature "SwiftShader" and we just > > disable it for tests. So it not only covers the WebGL case, but other cases in > > the future (say, WebGL2). > > Or if you want to stay on the simple side, just add a commandline switch to > disable SwiftShader, and pass it in for most tests, except for tests that > require SwiftShader. I'll try the SwiftShader as a GPU feature first and see how that goes.
This is a little different than I had originally envisioned, but I'm fine with this approach. I'll wait to stamp 'til zmo@ signs off in case there are any significant changes, but otherwise lg.
Description was changed from ========== Change how tests disable WebGL A few different tests were using GpuDataManager::InitializeForTesting in order to set GPU info of a GPU with the WebGL feature blacklisted. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled, even if a fake bad GPU is set. In order to fix this issue, I added a function that explicitly disables WebGL in the GPUDataManagerImpl object, without relying on a fake bad GPU to do it. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial ========== to ========== Change how tests disable WebGL A few different tests were using GpuDataManager::InitializeForTesting in order to set GPU info of a GPU with the WebGL feature blacklisted. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled, even if a fake bad GPU is set. In order to fix this issue, I added a function that explicitly disables WebGL in the GPUDataManagerImpl object, without relying on a fake bad GPU to do it. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Change how tests disable WebGL A few different tests were using GpuDataManager::InitializeForTesting in order to set GPU info of a GPU with the WebGL feature blacklisted. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled, even if a fake bad GPU is set. In order to fix this issue, I added a function that explicitly disables WebGL in the GPUDataManagerImpl object, without relying on a fake bad GPU to do it. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
On 2017/03/08 21:09:07, Zhenyao Mo wrote: > On 2017/03/08 19:34:51, sugoi1 wrote: > > Adding zmo@ to get his opinion on the fix. > > I feel it's better to add a new blacklist feature "SwiftShader" and we just > disable it for tests. So it not only covers the WebGL case, but other cases in > the future (say, WebGL2). PTAL. SwiftShader was added as a GPU feature and can now be disabled just like any other GPU feature. I also added the related unit test.
https://codereview.chromium.org/2737983002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/2737983002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:643: "{\n" Note: Those whitespace changes are from running 'git cl format'
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Should we rename "webgl" as "accelerated_webgl" (similar to accelerated_2d_canvas, etc.)? It'd be super confusing if disabling webgl doesn't disable webgl.
On 2017/03/09 18:31:34, piman - On leave - No reviews wrote: > Should we rename "webgl" as "accelerated_webgl" (similar to > accelerated_2d_canvas, etc.)? It'd be super confusing if disabling webgl doesn't > disable webgl. Well, we still want WebGL to be enabled when SwiftShader is on and we still want to enable SwiftShader when the hardware doesn't support WebGL. When SwiftShader is on, we want to return WebGL as enabled, but wouldn't want Accelerated_WebGL. I'm not certain we want to make that distinction. Note that only a few tests use this, AFAIK.
On 2017/03/09 18:49:32, sugoi1 wrote: > On 2017/03/09 18:31:34, piman - On leave - No reviews wrote: > > Should we rename "webgl" as "accelerated_webgl" (similar to > > accelerated_2d_canvas, etc.)? It'd be super confusing if disabling webgl > doesn't > > disable webgl. > > Well, we still want WebGL to be enabled when SwiftShader is on and we still want > to enable SwiftShader when the hardware doesn't support WebGL. > When SwiftShader is on, we want to return WebGL as enabled, but wouldn't want > Accelerated_WebGL. I'm not certain we want to make that distinction. > Note that only a few tests use this, AFAIK. WebGL can be in three states: enabled_accelerated, enabled_software_rendered, disabled. So now in GPU blacklist, when we add "webgl", we actually mean to disable accelerated_webgl. WebGL can be either enabled_software_rendered or disabled, depending on if SwiftShader is enabled. So I think piman has a valid point. (I'll review this CL and provide comments shortly after)
https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { Consider this situation: on a system where kDisableSoftwareRasterizer is passed, and accelerated WebGL is blacklisted. You will still want the SwiftShader for WebGL, but not for Rasterizer. What you did here is not to allow SwiftShader for WebGL, which I think it's an overkill. https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:1241: blacklisted_features_.insert(gpu::GPU_FEATURE_TYPE_SWIFTSHADER); Same here.
https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { On 2017/03/09 19:18:03, Zhenyao Mo wrote: > Consider this situation: on a system where kDisableSoftwareRasterizer is passed, > and accelerated WebGL is blacklisted. > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > What you did here is not to allow SwiftShader for WebGL, which I think it's an > overkill. Ok, but this isn't new, I simply moved it from GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can remove it if it's wrong.
I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds some complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, SwiftShader is fairly close to supporting WebGL 2, so we'd also have to add the gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED flag, I can come up with a solution, but I personally think you original idea of adding the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { On 2017/03/09 19:24:33, sugoi1 wrote: > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > Consider this situation: on a system where kDisableSoftwareRasterizer is > passed, > > and accelerated WebGL is blacklisted. > > > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > > > What you did here is not to allow SwiftShader for WebGL, which I think it's an > > overkill. > > Ok, but this isn't new, I simply moved it from > GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can remove it > if it's wrong. Removed kDisableSoftwareRasterizer check. https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:1241: blacklisted_features_.insert(gpu::GPU_FEATURE_TYPE_SWIFTSHADER); On 2017/03/09 19:18:03, Zhenyao Mo wrote: > Same here. Removed this change.
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/2737983002/diff/20001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:939: switches::kDisableSoftwareRasterizer)) { On 2017/03/09 20:07:12, sugoi1 wrote: > On 2017/03/09 19:24:33, sugoi1 wrote: > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > Consider this situation: on a system where kDisableSoftwareRasterizer is > > passed, > > > and accelerated WebGL is blacklisted. > > > > > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > > > > > What you did here is not to allow SwiftShader for WebGL, which I think it's > an > > > overkill. > > > > Ok, but this isn't new, I simply moved it from > > GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can remove > it > > if it's wrong. > > Removed kDisableSoftwareRasterizer check. Wait a second, I just looked into it and the kDisableSoftwareRasterizer flag is only used to disable SwiftShader and the description mentions a 3D software rasterizer, so I think this was correct before.
On 2017/03/09 20:07:12, sugoi1 wrote: > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds some > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, SwiftShader is > fairly close to supporting WebGL 2, so we'd also have to add the > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED flag, I > can come up with a solution, but I personally think you original idea of adding > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). Sorry for not being clear, but the suggestion is simply a name change, i.e., change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) not asking you to change the method. > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > content/browser/gpu/gpu_data_manager_impl_private.cc:939: > switches::kDisableSoftwareRasterizer)) { > On 2017/03/09 19:24:33, sugoi1 wrote: > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > Consider this situation: on a system where kDisableSoftwareRasterizer is > > passed, > > > and accelerated WebGL is blacklisted. > > > > > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > > > > > What you did here is not to allow SwiftShader for WebGL, which I think it's > an > > > overkill. > > > > Ok, but this isn't new, I simply moved it from > > GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can remove > it > > if it's wrong. > > Removed kDisableSoftwareRasterizer check. > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > content/browser/gpu/gpu_data_manager_impl_private.cc:1241: > blacklisted_features_.insert(gpu::GPU_FEATURE_TYPE_SWIFTSHADER); > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > Same here. > > Removed this change.
On 2017/03/09 21:06:56, Zhenyao Mo wrote: > On 2017/03/09 20:07:12, sugoi1 wrote: > > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds > some > > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, SwiftShader is > > fairly close to supporting WebGL 2, so we'd also have to add the > > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED flag, I > > can come up with a solution, but I personally think you original idea of > adding > > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > Sorry for not being clear, but the suggestion is simply a name change, i.e., > change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > not asking you to change the method. > > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > content/browser/gpu/gpu_data_manager_impl_private.cc:939: > > switches::kDisableSoftwareRasterizer)) { > > On 2017/03/09 19:24:33, sugoi1 wrote: > > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > > Consider this situation: on a system where kDisableSoftwareRasterizer is > > > passed, > > > > and accelerated WebGL is blacklisted. > > > > > > > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > > > > > > > What you did here is not to allow SwiftShader for WebGL, which I think > it's > > an > > > > overkill. > > > > > > Ok, but this isn't new, I simply moved it from > > > GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can > remove > > it > > > if it's wrong. > > > > Removed kDisableSoftwareRasterizer check. > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > content/browser/gpu/gpu_data_manager_impl_private.cc:1241: > > blacklisted_features_.insert(gpu::GPU_FEATURE_TYPE_SWIFTSHADER); > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > Same here. > > > > Removed this change. No, removing it feels wrong because you are no longer honoring kDisableSoftwareRasterizer. What I suggest is with kDisableSoftwareRasterizer, we don't insert GPU_FEATURE_TYPE_SWIFTSHADER into the blacklisted feature list, because WebGL or WebGL2 might need it. Instead, we look at the rasterizer side not to go down the SwiftShader software rasterizing path.
On 2017/03/09 21:06:56, Zhenyao Mo wrote: > On 2017/03/09 20:07:12, sugoi1 wrote: > > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds > some > > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, SwiftShader is > > fairly close to supporting WebGL 2, so we'd also have to add the > > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED flag, I > > can come up with a solution, but I personally think you original idea of > adding > > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > Sorry for not being clear, but the suggestion is simply a name change, i.e., > change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > not asking you to change the method. But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when SwiftShader is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL would be enabled when SwiftShader is enabled.
On 2017/03/09 21:10:45, Zhenyao Mo wrote: > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > On 2017/03/09 20:07:12, sugoi1 wrote: > > > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that adds > > some > > > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, SwiftShader > is > > > fairly close to supporting WebGL 2, so we'd also have to add the > > > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > > > > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED flag, > I > > > can come up with a solution, but I personally think you original idea of > > adding > > > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > > > Sorry for not being clear, but the suggestion is simply a name change, i.e., > > change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > > > not asking you to change the method. > > > > > > > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > > File content/browser/gpu/gpu_data_manager_impl_private.cc (right): > > > > > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > > content/browser/gpu/gpu_data_manager_impl_private.cc:939: > > > switches::kDisableSoftwareRasterizer)) { > > > On 2017/03/09 19:24:33, sugoi1 wrote: > > > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > > > Consider this situation: on a system where kDisableSoftwareRasterizer is > > > > passed, > > > > > and accelerated WebGL is blacklisted. > > > > > > > > > > You will still want the SwiftShader for WebGL, but not for Rasterizer. > > > > > > > > > > What you did here is not to allow SwiftShader for WebGL, which I think > > it's > > > an > > > > > overkill. > > > > > > > > Ok, but this isn't new, I simply moved it from > > > > GpuDataManagerImplPrivate::EnableSwiftShaderIfNecessary() below. I can > > remove > > > it > > > > if it's wrong. > > > > > > Removed kDisableSoftwareRasterizer check. > > > > > > > > > https://codereview.chromium.org/2737983002/diff/20001/content/browser/gpu/gpu... > > > content/browser/gpu/gpu_data_manager_impl_private.cc:1241: > > > blacklisted_features_.insert(gpu::GPU_FEATURE_TYPE_SWIFTSHADER); > > > On 2017/03/09 19:18:03, Zhenyao Mo wrote: > > > > Same here. > > > > > > Removed this change. > > No, removing it feels wrong because you are no longer honoring > kDisableSoftwareRasterizer. > > What I suggest is with kDisableSoftwareRasterizer, we don't insert > GPU_FEATURE_TYPE_SWIFTSHADER into the blacklisted feature list, because WebGL or > WebGL2 might need it. > > Instead, we look at the rasterizer side not to go down the SwiftShader software > rasterizing path. Look more at the original code, you are right, it is an issue from the original code, and this CL is not the source. I am OK not addressing this issue in this CL, but could you at least add two TODOs in the code so we will clean it up later? So the only request is to change the enums from WEBGL/WEBGL2 to ACCELERATED_WEBGL/WEBGL2.
On Thu, Mar 9, 2017 at 1:12 PM, <sugoi@chromium.org> wrote: > On 2017/03/09 21:06:56, Zhenyao Mo wrote: >> On 2017/03/09 20:07:12, sugoi1 wrote: >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that >> > adds >> some >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, >> > SwiftShader > is >> > fairly close to supporting WebGL 2, so we'd also have to add the >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. >> > >> > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED >> > flag, > I >> > can come up with a solution, but I personally think you original idea of >> adding >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). >> >> Sorry for not being clear, but the suggestion is simply a name change, >> i.e., >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) >> >> not asking you to change the method. > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > SwiftShader > is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL > would > be enabled when SwiftShader is enabled. When we blacklist WEBGL, we really mean blacklisting it from running on top of the GPU. So when SwiftShader is enabled, ACCELERATED_WEBGL is still blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated software rendering path. > > https://codereview.chromium.org/2737983002/ -- 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/09 21:15:16, Zhenyao Mo wrote: > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > >> On 2017/03/09 20:07:12, sugoi1 wrote: > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that > >> > adds > >> some > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > >> > SwiftShader > > is > >> > fairly close to supporting WebGL 2, so we'd also have to add the > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > >> > > >> > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED > >> > flag, > > I > >> > can come up with a solution, but I personally think you original idea of > >> adding > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > >> > >> Sorry for not being clear, but the suggestion is simply a name change, > >> i.e., > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > >> > >> not asking you to change the method. > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > SwiftShader > > is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL > > would > > be enabled when SwiftShader is enabled. > > When we blacklist WEBGL, we really mean blacklisting it from running > on top of the GPU. > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > software rendering path. Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has a SwiftShader specific path, so when SwiftShader is enabled, GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) would still return false, despite it being set in blacklisted_features_.
On 2017/03/09 21:18:05, sugoi1 wrote: > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > >> On 2017/03/09 20:07:12, sugoi1 wrote: > > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that > > >> > adds > > >> some > > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > > >> > SwiftShader > > > is > > >> > fairly close to supporting WebGL 2, so we'd also have to add the > > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > >> > > > >> > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED > > >> > flag, > > > I > > >> > can come up with a solution, but I personally think you original idea of > > >> adding > > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > >> > > >> Sorry for not being clear, but the suggestion is simply a name change, > > >> i.e., > > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > >> > > >> not asking you to change the method. > > > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > > SwiftShader > > > is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL > > > would > > > be enabled when SwiftShader is enabled. > > > > When we blacklist WEBGL, we really mean blacklisting it from running > > on top of the GPU. > > > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > > software rendering path. > > Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has a > SwiftShader specific path, so when SwiftShader is enabled, > GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) would > still return false, despite it being set in blacklisted_features_. I see. You have a good point here. IsFeatureBlacklisted(ACCELERATED_WEBGL) should indeed return false, however, the caller probably means to ask IsWebGLEnabled, which could return true if SwiftShader is in place. It's something beyond the scope of this CL. LGTM if piman has no objections.
On 2017/03/09 21:31:05, Zhenyao Mo wrote: > On 2017/03/09 21:18:05, sugoi1 wrote: > > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > > >> On 2017/03/09 20:07:12, sugoi1 wrote: > > > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that > > > >> > adds > > > >> some > > > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > > > >> > SwiftShader > > > > is > > > >> > fairly close to supporting WebGL 2, so we'd also have to add the > > > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > >> > > > > >> > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED > > > >> > flag, > > > > I > > > >> > can come up with a solution, but I personally think you original idea > of > > > >> adding > > > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > > >> > > > >> Sorry for not being clear, but the suggestion is simply a name change, > > > >> i.e., > > > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > > >> > > > >> not asking you to change the method. > > > > > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > > > SwiftShader > > > > is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL > > > > would > > > > be enabled when SwiftShader is enabled. > > > > > > When we blacklist WEBGL, we really mean blacklisting it from running > > > on top of the GPU. > > > > > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > > > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > > > software rendering path. > > > > Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has a > > SwiftShader specific path, so when SwiftShader is enabled, > > GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) would > > still return false, despite it being set in blacklisted_features_. > > I see. You have a good point here. IsFeatureBlacklisted(ACCELERATED_WEBGL) > should indeed return false, however, the caller probably means to ask > IsWebGLEnabled, which could return true if SwiftShader is in place. > > It's something beyond the scope of this CL. > > LGTM if piman has no objections. Thanks. We talked offline and we clarified the confusion around kDisableSoftwareRasterizer. Basically, it should probably be called kDisableSoftwareGL or kDisableSwiftShader since it has nothing to do with the Chromium rasterizer.
extensions lgtm
https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklis... File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklis... gpu/config/gpu_blacklist.cc:45: list->AddSupportedFeature("swiftshader", GPU_FEATURE_TYPE_SWIFTSHADER); Won't this cause swiftshader to be blacklisted if "all" is specified in the GPU blacklist?
On 2017/03/09 21:31:05, Zhenyao Mo wrote: > On 2017/03/09 21:18:05, sugoi1 wrote: > > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > > >> On 2017/03/09 20:07:12, sugoi1 wrote: > > > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but that > > > >> > adds > > > >> some > > > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > > > >> > SwiftShader > > > > is > > > >> > fairly close to supporting WebGL 2, so we'd also have to add the > > > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > >> > > > > >> > If you feel strongly about adding a GPU_FEATURE_TYPE_WEBGL_ACCELERATED > > > >> > flag, > > > > I > > > >> > can come up with a solution, but I personally think you original idea > of > > > >> adding > > > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > > >> > > > >> Sorry for not being clear, but the suggestion is simply a name change, > > > >> i.e., > > > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > > >> > > > >> not asking you to change the method. > > > > > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > > > SwiftShader > > > > is enabled, currently, so if we'd just rename it, _TYPE_ACCELERATED_WEBGL > > > > would > > > > be enabled when SwiftShader is enabled. > > > > > > When we blacklist WEBGL, we really mean blacklisting it from running > > > on top of the GPU. > > > > > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > > > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > > > software rendering path. > > > > Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has a > > SwiftShader specific path, so when SwiftShader is enabled, > > GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) would > > still return false, despite it being set in blacklisted_features_. > > I see. You have a good point here. IsFeatureBlacklisted(ACCELERATED_WEBGL) > should indeed return false, however, the caller probably means to ask > IsWebGLEnabled, which could return true if SwiftShader is in place. > > It's something beyond the scope of this CL. > > LGTM if piman has no objections. I'd prefer we rename webgl->accelerated_webgl, fix IsFeatureBlacklisted to do the right thing, and if necessary provide a separate function to reason about the webgl feature (i.e. !IsFeatureBlacklisted(WEBGL_ACCELERATED) || ShouldUseSwiftShader).
zmo@chromium.org changed reviewers: + ericrk@chromium.org
Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not actually related to rasterizer and should just be changed to kDisableSwiftShader.
On 2017/03/09 22:33:15, Zhenyao Mo wrote: > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not > actually related to rasterizer and should just be changed to > kDisableSwiftShader. This command-line flag was intended to be somewhat user-accessible, so I'd prefer not to change it.
On 2017/03/09 22:36:59, jbauman wrote: > On 2017/03/09 22:33:15, Zhenyao Mo wrote: > > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not > > actually related to rasterizer and should just be changed to > > kDisableSwiftShader. > > This command-line flag was intended to be somewhat user-accessible, so I'd > prefer not to change it. Is the intended use of that command-line for rasterizer not to run on the GPU accelerated path on top of SwiftShader? If yes, then my original comment was still valid: currently it just forbids SwiftShader, not only for Rasterizer, but also for WebGL. So if we don't rename it, we definitely should fix the semantic so semantic matches the name.
On 2017/03/09 22:57:32, Zhenyao Mo wrote: > On 2017/03/09 22:36:59, jbauman wrote: > > On 2017/03/09 22:33:15, Zhenyao Mo wrote: > > > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not > > > actually related to rasterizer and should just be changed to > > > kDisableSwiftShader. > > > > This command-line flag was intended to be somewhat user-accessible, so I'd > > prefer not to change it. > > Is the intended use of that command-line for rasterizer not to run on the GPU > accelerated path on top of SwiftShader? If yes, then my original comment was > still valid: currently it just forbids SwiftShader, not only for Rasterizer, but > also for WebGL. So if we don't rename it, we definitely should fix the semantic > so semantic matches the name. I think the confusion might be that SwiftShader is also a "software rasterizer". The term is a bit overloaded in Chrome at the moment - could refer to skia SW rasterization or SwiftShader. jbauman@, is your concern in changing the name that users might already be depending on this flag? Or that the new name (kDisableSwiftshader) is less obvious to users regarding its purpose?
On 2017/03/09 23:06:58, ericrk wrote: > On 2017/03/09 22:57:32, Zhenyao Mo wrote: > > On 2017/03/09 22:36:59, jbauman wrote: > > > On 2017/03/09 22:33:15, Zhenyao Mo wrote: > > > > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is not > > > > actually related to rasterizer and should just be changed to > > > > kDisableSwiftShader. > > > > > > This command-line flag was intended to be somewhat user-accessible, so I'd > > > prefer not to change it. > > > > Is the intended use of that command-line for rasterizer not to run on the GPU > > accelerated path on top of SwiftShader? If yes, then my original comment was > > still valid: currently it just forbids SwiftShader, not only for Rasterizer, > but > > also for WebGL. So if we don't rename it, we definitely should fix the > semantic > > so semantic matches the name. > > I think the confusion might be that SwiftShader is also a "software rasterizer". > The term is a bit overloaded in Chrome at the moment - could refer to skia SW > rasterization or SwiftShader. > > jbauman@, is your concern in changing the name that users might already be > depending on this flag? Or that the new name (kDisableSwiftshader) is less > obvious to users regarding its purpose? Yeah, I'm worried that people might be already using the flag or the entry in about:flags which we would want to have the same name for. I doubt it's too common, but there are some google hits for it.
On 2017/03/09 23:11:48, jbauman wrote: > On 2017/03/09 23:06:58, ericrk wrote: > > On 2017/03/09 22:57:32, Zhenyao Mo wrote: > > > On 2017/03/09 22:36:59, jbauman wrote: > > > > On 2017/03/09 22:33:15, Zhenyao Mo wrote: > > > > > Per discussion with ericrk, he agrees kDisableSoftwareRasterization is > not > > > > > actually related to rasterizer and should just be changed to > > > > > kDisableSwiftShader. > > > > > > > > This command-line flag was intended to be somewhat user-accessible, so I'd > > > > prefer not to change it. > > > > > > Is the intended use of that command-line for rasterizer not to run on the > GPU > > > accelerated path on top of SwiftShader? If yes, then my original comment > was > > > still valid: currently it just forbids SwiftShader, not only for Rasterizer, > > but > > > also for WebGL. So if we don't rename it, we definitely should fix the > > semantic > > > so semantic matches the name. > > > > I think the confusion might be that SwiftShader is also a "software > rasterizer". > > The term is a bit overloaded in Chrome at the moment - could refer to skia SW > > rasterization or SwiftShader. > > > > jbauman@, is your concern in changing the name that users might already be > > depending on this flag? Or that the new name (kDisableSwiftshader) is less > > obvious to users regarding its purpose? > > Yeah, I'm worried that people might be already using the flag or the entry in > about:flags which we would want to have the same name for. I doubt it's too > common, but there are some google hits for it. Ok, well, I wouldn't have changed the name in this cl anyway, so we can just leave the name as is, I personally have no problem with that.
On 2017/03/09 22:21:00, piman - On leave - No reviews wrote: > On 2017/03/09 21:31:05, Zhenyao Mo wrote: > > On 2017/03/09 21:18:05, sugoi1 wrote: > > > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > > > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > > > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > > > >> On 2017/03/09 20:07:12, sugoi1 wrote: > > > > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but > that > > > > >> > adds > > > > >> some > > > > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > > > > >> > SwiftShader > > > > > is > > > > >> > fairly close to supporting WebGL 2, so we'd also have to add the > > > > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > > >> > > > > > >> > If you feel strongly about adding a > GPU_FEATURE_TYPE_WEBGL_ACCELERATED > > > > >> > flag, > > > > > I > > > > >> > can come up with a solution, but I personally think you original idea > > of > > > > >> adding > > > > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to me). > > > > >> > > > > >> Sorry for not being clear, but the suggestion is simply a name change, > > > > >> i.e., > > > > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > > > >> > > > > >> not asking you to change the method. > > > > > > > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > > > > SwiftShader > > > > > is enabled, currently, so if we'd just rename it, > _TYPE_ACCELERATED_WEBGL > > > > > would > > > > > be enabled when SwiftShader is enabled. > > > > > > > > When we blacklist WEBGL, we really mean blacklisting it from running > > > > on top of the GPU. > > > > > > > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > > > > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > > > > software rendering path. > > > > > > Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has a > > > SwiftShader specific path, so when SwiftShader is enabled, > > > GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) would > > > still return false, despite it being set in blacklisted_features_. > > > > I see. You have a good point here. IsFeatureBlacklisted(ACCELERATED_WEBGL) > > should indeed return false, however, the caller probably means to ask > > IsWebGLEnabled, which could return true if SwiftShader is in place. > > > > It's something beyond the scope of this CL. > > > > LGTM if piman has no objections. > > I'd prefer we rename webgl->accelerated_webgl, fix IsFeatureBlacklisted to do > the right thing, and if necessary provide a separate function to reason about > the webgl feature (i.e. !IsFeatureBlacklisted(WEBGL_ACCELERATED) || > ShouldUseSwiftShader). Noted. Is it a big deal if I do this in a follow up rather than within this cl? I had already started looking at the relationship between blacklisted features and swiftshader here: https://codereview.chromium.org/2724203006/ I could include what you propose in that other cl, if that's ok with you, unless what you meant is that I shouldn't add GPU_FEATURE_TYPE_SWIFTSHADER at all, in which case I'd have to do it here.
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/2737983002/diff/60001/gpu/config/gpu_blacklis... File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/60001/gpu/config/gpu_blacklis... gpu/config/gpu_blacklist.cc:45: list->AddSupportedFeature("swiftshader", GPU_FEATURE_TYPE_SWIFTSHADER); On 2017/03/09 22:08:39, jbauman wrote: > Won't this cause swiftshader to be blacklisted if "all" is specified in the GPU blacklist? Yes. Does this happen when a card gets blacklisted or does this only happen in testing?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/10 01:52:53, sugoi1 wrote: > On 2017/03/09 22:21:00, piman - On leave - No reviews wrote: > > On 2017/03/09 21:31:05, Zhenyao Mo wrote: > > > On 2017/03/09 21:18:05, sugoi1 wrote: > > > > On 2017/03/09 21:15:16, Zhenyao Mo wrote: > > > > > On Thu, Mar 9, 2017 at 1:12 PM, <mailto:sugoi@chromium.org> wrote: > > > > > > On 2017/03/09 21:06:56, Zhenyao Mo wrote: > > > > > >> On 2017/03/09 20:07:12, sugoi1 wrote: > > > > > >> > I looked into adding gpu::GPU_FEATURE_TYPE_WEBGL_ACCELERATED, but > > that > > > > > >> > adds > > > > > >> some > > > > > >> > complexity to regular gpu::GPU_FEATURE_TYPE_WEBGL checks. Also, > > > > > >> > SwiftShader > > > > > > is > > > > > >> > fairly close to supporting WebGL 2, so we'd also have to add the > > > > > >> > gpu::GPU_FEATURE_TYPE_WEBGL2_ACCELERATED flag at some point. > > > > > >> > > > > > > >> > If you feel strongly about adding a > > GPU_FEATURE_TYPE_WEBGL_ACCELERATED > > > > > >> > flag, > > > > > > I > > > > > >> > can come up with a solution, but I personally think you original > idea > > > of > > > > > >> adding > > > > > >> > the GPU_FEATURE_TYPE_SWIFTSHADER flag makes testing clearer (to > me). > > > > > >> > > > > > >> Sorry for not being clear, but the suggestion is simply a name > change, > > > > > >> i.e., > > > > > >> change _TYPE_WEBGL to _TYPE_ACCELERATED_WEBGL (same for WEBGL2) > > > > > >> > > > > > >> not asking you to change the method. > > > > > > > > > > > > But then wouldn't be incorrect? _TYPE_WEBGL is not blacklisted when > > > > > > SwiftShader > > > > > > is enabled, currently, so if we'd just rename it, > > _TYPE_ACCELERATED_WEBGL > > > > > > would > > > > > > be enabled when SwiftShader is enabled. > > > > > > > > > > When we blacklist WEBGL, we really mean blacklisting it from running > > > > > on top of the GPU. > > > > > > > > > > So when SwiftShader is enabled, ACCELERATED_WEBGL is still > > > > > blacklisted. But WEBGL is not, WEBGL is running on an un-accelerated > > > > > software rendering path. > > > > > > > > Oh, my issue is that GpuDataManagerImplPrivate::IsFeatureBlacklisted() has > a > > > > SwiftShader specific path, so when SwiftShader is enabled, > > > > GpuDataManagerImplPrivate::IsFeatureBlacklisted(...ACCELERATED_WEBGL) > would > > > > still return false, despite it being set in blacklisted_features_. > > > > > > I see. You have a good point here. IsFeatureBlacklisted(ACCELERATED_WEBGL) > > > should indeed return false, however, the caller probably means to ask > > > IsWebGLEnabled, which could return true if SwiftShader is in place. > > > > > > It's something beyond the scope of this CL. > > > > > > LGTM if piman has no objections. > > > > I'd prefer we rename webgl->accelerated_webgl, fix IsFeatureBlacklisted to do > > the right thing, and if necessary provide a separate function to reason about > > the webgl feature (i.e. !IsFeatureBlacklisted(WEBGL_ACCELERATED) || > > ShouldUseSwiftShader). > > Noted. Is it a big deal if I do this in a follow up rather than within this cl? > I had already started looking at the relationship between blacklisted features > and swiftshader here: > https://codereview.chromium.org/2724203006/ > I could include what you propose in that other cl, if that's ok with you, unless > what you meant is that I shouldn't add GPU_FEATURE_TYPE_SWIFTSHADER at all, in > which case I'd have to do it here. Just FYI, I'm now working on this, I'll have something up for review later today.
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, I added SwiftShader as a GPU feature, which can also be blacklisted. This will allow SwiftShader to work properly while still being able to disable WebGL for the purpose of testing. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org TBR=piman@chromium.org TBR_REASON=This is only for changes in gpu_data_manager.h, which are trivial CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, WebGL and WebGL2 features will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Implemented Antoine's suggestion. I added a new function, IsWebGLEnabled(), which needs to be checked, instead of checking if the WebGL feature is blacklisted, when chromium needs to know if WebGL is available. Hopefully I used the new function in the correct spots. Please tell me if I missed anything.
https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:836: gpu_preferences->enable_es3_apis = (enabled || !blacklisted) && !disabled; Mmh, I think we still want to be precise about webgl2. Is is the case that using SwiftShader implies webgl2 is disabled altogether, or is it that it is just unaccelerated? If the former, then we should keep the blacklist feature named "webgl2"/"GPU_FEATURE_TYPE_WEBGL2", but this code is still right. If the latter, then we need to change the code here, because blacklisting "accelerated webgl2" shouldn't result in disabling webgl2 altogether. https://codereview.chromium.org/2737983002/diff/80001/gpu/config/gpu_blacklis... File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/80001/gpu/config/gpu_blacklis... gpu/config/gpu_blacklist.cc:25: list->AddSupportedFeature("webgl", GPU_FEATURE_TYPE_ACCELERATED_WEBGL); We should rename the feature name (as well as in the blacklist entries) to "accelerated_webgl".
https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_data_manager_impl_private.cc (right): https://codereview.chromium.org/2737983002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_data_manager_impl_private.cc:836: gpu_preferences->enable_es3_apis = (enabled || !blacklisted) && !disabled; On 2017/03/10 20:31:40, piman - On leave - No reviews wrote: > Mmh, I think we still want to be precise about webgl2. Is is the case that using > SwiftShader implies webgl2 is disabled altogether, or is it that it is just > unaccelerated? > > If the former, then we should keep the blacklist feature named > "webgl2"/"GPU_FEATURE_TYPE_WEBGL2", but this code is still right. If the latter, > then we need to change the code here, because blacklisting "accelerated webgl2" > shouldn't result in disabling webgl2 altogether. Ok, I can revert the WebGL2 change. For now, SwiftShader is not yet used for WebGL2. https://codereview.chromium.org/2737983002/diff/80001/gpu/config/gpu_blacklis... File gpu/config/gpu_blacklist.cc (right): https://codereview.chromium.org/2737983002/diff/80001/gpu/config/gpu_blacklis... gpu/config/gpu_blacklist.cc:25: list->AddSupportedFeature("webgl", GPU_FEATURE_TYPE_ACCELERATED_WEBGL); On 2017/03/10 20:31:40, piman - On leave - No reviews wrote: > We should rename the feature name (as well as in the blacklist entries) to > "accelerated_webgl". Ok, I had started doing that, but this is used as a hardcoded string in many places, so I thought changing the name is a bit error-prone, but I'll do the change.
Done. PTAL
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, WebGL and WebGL2 features will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2737983002/diff/100001/extensions/common/mani... File extensions/common/manifest_handlers/requirements_info.cc (right): https://codereview.chromium.org/2737983002/diff/100001/extensions/common/mani... extensions/common/manifest_handlers/requirements_info.cc:121: if (feature == "accelerated_webgl") { I think this one is wrong, will revert.
LGTM with the revert of that change in extensions code.
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...
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org TBR=laforge@chromium.org TBR_REASON=Trivial change to swiftshader_component_installer.cc, which will soon be deleted entirely CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sugoi@chromium.org to run a CQ dry run
lgtm with nits https://codereview.chromium.org/2737983002/diff/120001/content/browser/gpu/gp... File content/browser/gpu/gpu_data_manager_impl_private_unittest.cc (right): https://codereview.chromium.org/2737983002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private_unittest.cc:157: {"id" : 1, "features" : ["accelerated_webgl"]}, { Can we not take the clang-format change here? It looks wrong to me and much harder to read. https://codereview.chromium.org/2737983002/diff/120001/content/browser/gpu/gp... content/browser/gpu/gpu_data_manager_impl_private_unittest.cc:203: {"id" : 1, "features" : ["accelerated_2d_canvas"]}, { Same here. The formatting doesn't make sense here.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, though I agree with zmo on using "clang-format off" on those strings in gpu_data_manager_impl_private_unittest.cc
On 2017/03/10 23:31:11, jbauman wrote: > lgtm, though I agree with zmo on using "clang-format off" on those strings in > gpu_data_manager_impl_private_unittest.cc Ok, will do the formatting modification before landing.
The CQ bit was checked by sugoi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, piman@chromium.org, zmo@chromium.org, jbauman@chromium.org Link to the patchset: https://codereview.chromium.org/2737983002/#ps160001 (title: "Formatting fix")
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": 160001, "attempt_start_ts": 1489419129108370, "parent_rev": "496a61f5ab35b8227f0313bc8c176a936db2b705", "commit_rev": "9e3f185360c78d129af6a5fbc4e0515c3b2f6793"}
Message was sent while issue was closed.
Description was changed from ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org TBR=laforge@chromium.org TBR_REASON=Trivial change to swiftshader_component_installer.cc, which will soon be deleted entirely CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== This is necessary for tests that wish to disable WebGL A few different tests are blacklisting the WebGL GPU feature. This will no longer work when SwiftShader is fully integrated in chromium, as it will become a fallback for GPU accelerated WebGL, and the WebGL feature will stay enabled. In order to fix this issue, the WebGL feature will now only be enabled when accelerated. When SwiftShader is used, these features will now be disabled. A new utility function, IsWebGLEnabled(), which checks for both swiftshader and the WebGL feature, was added in order to verify if WebGL can actually be used by chromium. BUG=630728 R=rdevlin.cronin@chromium.org, jbauman@chromium.org, zmo@chromium.org, piman@chromium.org TBR=laforge@chromium.org TBR_REASON=Trivial change to swiftshader_component_installer.cc, which will soon be deleted entirely CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2737983002 Cr-Commit-Position: refs/heads/master@{#456405} Committed: https://chromium.googlesource.com/chromium/src/+/9e3f185360c78d129af6a5fbc4e0... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/9e3f185360c78d129af6a5fbc4e0... |