New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 845213 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Task



Sign in to add a comment

MacViews: Test using GPU rasterization and threaded rasterization in browser compositor

Project Member Reported by ccameron@chromium.org, May 21 2018

Issue description

Rasterization (writing the pixel values that end up going into textures/IOSurfaces for compositing) is a pain point for MacViews, particularly on retina displays.

GPU rasterization is the path where we draw pixels on the GPU instead of the CPU. Some potential benefits:
- allows async IOSurface allocation by default, avoids Lock/Unlock of IOSurfaces
- has much lower CPU overhead in browser process
- can be much more efficient than the CPU
- this uses larger tiles which can be more power efficient to send to CoreAnimation
- this path isn't taken ATM because HasGrContextSupport returns false at [1]

Threaded rasterization is a path where the actual drawing of pixels is done on a separate thread (or threads). This path doesn't apply to GPU rasterization (because the on-thread work for GPU rasterization is minimal). Some potential benefits:
- less blocking of the main thread in the browser process

We should look into these optimizations for MacViews browser to see if they will help with jank (especially main thread jank) issues.

[1] https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?rcl=2b51c5879b979b5a486b1761b15a798538313c61&l=2178

 

Comment 1 by enne@chromium.org, May 21 2018

Cc: piman@chromium.org danakj@chromium.org
Under viz, enabling GPU raster requires:
- In VizProcessTransportFactory::TryCreateContextsForGpuCompositing
 - kSharedWorkerContextSupportsGLES2 = true
 - kSharedWorkerContextSupportsGrContext = true 
- In Compositor::Compositor
  - settings.gpu_rasterization_forced=true

Strangely this also happens to make resize more smooth. I would guess that calling IOSurfaceLock on the browser main thread was a cause of some of the jank there.

I'll look up how to enable GPU raster in non-viz (prolly just a couple of lines here & there).
Under non-viz, we need to update GpuProcessTransportFactory::EstablishedGpuChannel
    if (!shared_worker_context_provider_) {
      bool need_alpha_channel = false;
      bool support_locking = true;
      bool support_gles2_interface = true; // was false
      bool support_raster_interface = true;
      bool support_grcontext = true; // was false

Comment 4 by piman@chromium.org, May 21 2018

This sounds great!
FYI, and this is not meant to slow you down, more of a heads up, many painting patterns at least on Aura (but I believe this is more due to Views, and so might affect Mac) do client-side caching of processed resources (e.g. take an icon, resize it, apply filters etc.) by reading back CPU pixmaps (see gfx::Canvas::GetBitmap) and sending them back when painting, i.e. GPU rasterization ends up being a bunch of texture uploads.

Comment 5 by samans@chromium.org, May 22 2018

Labels: -Type-Bug Type-Task
Status: Available (was: Untriaged)
Project Member

Comment 7 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b38a451d05aed6939cb30d5957092e4841379c2

commit 7b38a451d05aed6939cb30d5957092e4841379c2
Author: Christopher Cameron <ccameron@chromium.org>
Date: Wed May 30 02:17:11 2018

MacViews: Enable GPU rasterization (where available)

This should relieve pressure on the browser's UI thread.

Disallow anonymous GLImages when GpuMemoryBuffers are disabled
(several tests would attempt to use this path while it was
disabled, and crash).

Add a NOTREACHED to cc::TestImageFactory, so that such failures
are easier to diagnose in the future.

Bug:  845213 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ibaf017e9ee3c22b3391fe2d2b47b82c8fbff8e93
Reviewed-on: https://chromium-review.googlesource.com/1076531
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562696}
[modify] https://crrev.com/7b38a451d05aed6939cb30d5957092e4841379c2/cc/test/test_image_factory.cc
[modify] https://crrev.com/7b38a451d05aed6939cb30d5957092e4841379c2/cc/test/test_image_factory.h
[modify] https://crrev.com/7b38a451d05aed6939cb30d5957092e4841379c2/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/7b38a451d05aed6939cb30d5957092e4841379c2/ui/base/ui_base_features.cc

Cc: gov...@chromium.org abdulsyed@chromium.org
Labels: Merge-Request-68
Owner: ccameron@chromium.org
Status: Assigned (was: Available)
Merge request to 68 for the Mac part.
How is the change listed at #7 looking canary?
It has been enabled for almost a week and I haven't seen any bug reports (except for some performance and memory shifts, but that was expected).
Thank you ccameron@. 
abdulsyed@, could you ptal comment #10 and do the merge review? 
Labels: -Merge-Request-68 Merge-Approved-68
Approved - branch:3440
Ooops, this actually did have some problems with interactions with viz. So it's not ready to merge.
Project Member

Comment 14 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a494fc167ef21cb67ae68d59dd9e84eff507e06a

commit a494fc167ef21cb67ae68d59dd9e84eff507e06a
Author: Christopher Cameron <ccameron@chromium.org>
Date: Thu Jun 07 01:52:21 2018

Fix CHROMIUM_texture_storage_image detection

Whether or not CHROMIUM_texture_storage_image is supported depends
entirely on the underlying ImageFactory.

Add a function to query if the ImageFactory has the capability to create
anonymous images, and use this to enable relevant parts of FeatureInfo.
Remove the ifdef that was being used to determine whether or not to
enable CHROMIUM_texture_storage_image, because that was just attempting
to match the capabilities of the ImageFactory (and sometimes failing).

I screwed this up in the previous patch because I was checking for
gpu_memory_manager, thinking that it was gpu_memory_buffer_manager.
As it happens CHROMIUM_gpu_memory_manager is unused, so just remove
it.

Bug:  845213 ,  849478 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I9d97749863c61954151e9f06dc23949613c2181f
Reviewed-on: https://chromium-review.googlesource.com/1088313
Reviewed-by: ccameron <ccameron@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565140}
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/cc/test/test_image_factory.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/cc/test/test_image_factory.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/gles2_cmd_decoder.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/image_factory.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/service/image_factory.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/tests/texture_image_factory.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/command_buffer/tests/texture_image_factory.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.h
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc
[modify] https://crrev.com/a494fc167ef21cb67ae68d59dd9e84eff507e06a/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb932c42c8e9e5d9c4f7177bab975938b820789d

commit fb932c42c8e9e5d9c4f7177bab975938b820789d
Author: Hiroki Nakagawa <nhiroki@chromium.org>
Date: Thu Jun 07 04:23:07 2018

Revert "Fix CHROMIUM_texture_storage_image detection"

This reverts commit a494fc167ef21cb67ae68d59dd9e84eff507e06a.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified this CL at revision 565140 as the culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtL2E0OTRmYzE2N2VmMjFjYjY3YWU2OGQ1OWRkOWU4NGVmZjUwN2UwNmEM

Original change's description:
> Fix CHROMIUM_texture_storage_image detection
> 
> Whether or not CHROMIUM_texture_storage_image is supported depends
> entirely on the underlying ImageFactory.
> 
> Add a function to query if the ImageFactory has the capability to create
> anonymous images, and use this to enable relevant parts of FeatureInfo.
> Remove the ifdef that was being used to determine whether or not to
> enable CHROMIUM_texture_storage_image, because that was just attempting
> to match the capabilities of the ImageFactory (and sometimes failing).
> 
> I screwed this up in the previous patch because I was checking for
> gpu_memory_manager, thinking that it was gpu_memory_buffer_manager.
> As it happens CHROMIUM_gpu_memory_manager is unused, so just remove
> it.
> 
> Bug:  845213 ,  849478 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I9d97749863c61954151e9f06dc23949613c2181f
> Reviewed-on: https://chromium-review.googlesource.com/1088313
> Reviewed-by: ccameron <ccameron@chromium.org>
> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
> Commit-Queue: ccameron <ccameron@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#565140}

TBR=ccameron@chromium.org,sunnyps@chromium.org

Change-Id: I09717f0aa60f911ee339c85c99d8fe9c4d325dd0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  845213 ,  849478 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/1090450
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565177}
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/cc/test/test_image_factory.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/cc/test/test_image_factory.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/gles2_cmd_decoder.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/image_factory.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/service/image_factory.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/tests/texture_image_factory.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/command_buffer/tests/texture_image_factory.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.h
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc
[modify] https://crrev.com/fb932c42c8e9e5d9c4f7177bab975938b820789d/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.h

The failing test is OOPBrowserTest/Basic
[ERROR:raster_decoder.cc(2531)] [.RenderWorker]GL ERROR :GL_INVALID_VALUE : glTexStorage2DImage: use_buffer

Now, why wasn't this caught in the CQ?
Project Member

Comment 17 by bugdroid1@chromium.org, Jun 7 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f9656207a24252cb5486d98a700dbc72e5430fee

commit f9656207a24252cb5486d98a700dbc72e5430fee
Author: Christopher Cameron <ccameron@chromium.org>
Date: Thu Jun 07 07:04:57 2018

Fix CHROMIUM_texture_storage_image detection

Whether or not CHROMIUM_texture_storage_image is supported depends
entirely on the underlying ImageFactory.

Add a function to query if the ImageFactory has the capability to create
anonymous images, and use this to enable relevant parts of FeatureInfo.
Remove the ifdef that was being used to determine whether or not to
enable CHROMIUM_texture_storage_image, because that was just attempting
to match the capabilities of the ImageFactory (and sometimes failing).

I screwed this up in the previous patch because I was checking for
gpu_memory_manager, thinking that it was gpu_memory_buffer_manager. As
it happens CHROMIUM_gpu_memory_manager is unused, so just remove it.

Re-landed due to mid-air collision.

TBR=sunnyps

Bug:  845213 ,  849478 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I35ca390cccd05416799c0afa549694f2ac45f14d
Reviewed-on: https://chromium-review.googlesource.com/1090324
Reviewed-by: ccameron <ccameron@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#565204}
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/cc/test/test_image_factory.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/cc/test/test_image_factory.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/feature_info.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/feature_info.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/gles2_cmd_decoder.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/gles2_cmd_decoder.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/image_factory.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/image_factory.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/service/raster_decoder.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/tests/texture_image_factory.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/command_buffer/tests/texture_image_factory.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/in_process_command_buffer.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_android_hardware_buffer.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_dxgi.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_io_surface.h
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.cc
[modify] https://crrev.com/f9656207a24252cb5486d98a700dbc72e5430fee/gpu/ipc/service/gpu_memory_buffer_factory_native_pixmap.h

Labels: -Merge-Approved-68
Status: Fixed (was: Assigned)
Not targeting this merge for 68 -- too much motion.

Sign in to add a comment