Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(19)

Issue 636003002: Clean up GPU back-pressure with remote CALayers (Closed)

Created:
6 years, 2 months ago by ccameron
Modified:
6 years, 2 months ago
Reviewers:
Tom Sepez, Mike West, piman
CC:
chromium-reviews, mkwst+moarreviews-ipc_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Clean up GPU back-pressure with remote CALayers If a CAOpenGLLayer is attached to a NSView that is completely occluded or isn't in the window hierarchy, then when we ask that layer to draw, it will not draw, or will draw after a long delay. This optimization can be a problem if we need to renderer to continue to produce frames, e.g, for tab capture. Fix this issue by having the frame acknowledgement from the browser to the GPU process (which currently informs the GPU process of the CGL renderer ID) inform the GPU process of the occlusion status of the NSView being drawn to. Use this information in the GPU process to force the CAOpenGLLayer to draw as soon as a new frame arrives. Add the same timeout mechanism as is in the IOSurface drawing path to limit CoreAnimation's throttling (based on GPU back-pressure) to 6 fps, to avoid having the renderer block indefinitely. This is something of a hack, but is an unfortunate necessity. Note that this change assumes that calling setNeedsDisplay and then displayIfNeeded will result in the CALayer being draw except in the case where the CALayer is backing a NSView that is not in the window hierarchy. This exception only happens during tab capture of a backgrounded tab, and the contents not drawn are actually never seen, so it is safe to skip the draw entirely. Also, wrap some Mac-only IPCs and structures #if defined(OS_MACOSX). BUG=312462 Committed: https://crrev.com/95e3616034fcf2ef6d1a7caf8b289ad87ad8b6d0 Cr-Commit-Position: refs/heads/master@{#298828}

Patch Set 1 #

Patch Set 2 : Better variable names #

Total comments: 4

Patch Set 3 : Review feedback #

Patch Set 4 : Add missed ifdef #

Patch Set 5 : O(1) more compile issue #

Unified diffs Side-by-side diffs Delta from patch set Stats (+89 lines, -45 lines) Patch
M content/browser/compositor/browser_compositor_ca_layer_tree_mac.h View 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/compositor/browser_compositor_ca_layer_tree_mac.mm View 1 chunk +6 lines, -0 lines 0 comments Download
M content/browser/compositor/browser_compositor_view_mac.mm View 1 chunk +3 lines, -0 lines 0 comments Download
M content/common/gpu/gpu_messages.h View 1 2 3 2 chunks +11 lines, -3 lines 0 comments Download
M content/common/gpu/image_transport_surface.h View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
M content/common/gpu/image_transport_surface.cc View 3 chunks +6 lines, -0 lines 0 comments Download
M content/common/gpu/image_transport_surface_calayer_mac.h View 1 2 4 chunks +13 lines, -4 lines 0 comments Download
M content/common/gpu/image_transport_surface_calayer_mac.mm View 1 2 5 chunks +34 lines, -33 lines 0 comments Download
M content/common/gpu/image_transport_surface_fbo_mac.h View 1 chunk +3 lines, -2 lines 0 comments Download
M content/common/gpu/image_transport_surface_fbo_mac.mm View 1 chunk +1 line, -1 line 0 comments Download
M content/common/gpu/image_transport_surface_iosurface_mac.h View 1 chunk +1 line, -1 line 0 comments Download
M content/common/gpu/image_transport_surface_iosurface_mac.cc View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 19 (7 generated)
ccameron
Quite some time ago, I took a stab at some of these issues in https://codereview.chromium.org/505053002/. ...
6 years, 2 months ago (2014-10-08 01:08:22 UTC) #2
piman
lgtm https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/image_transport_surface_calayer_mac.h File content/common/gpu/image_transport_surface_calayer_mac.h (right): https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/image_transport_surface_calayer_mac.h#newcode63 content/common/gpu/image_transport_surface_calayer_mac.h:63: base::WeakPtrFactory<CALayerStorageProvider> pending_draw_weak_factory_; Why not keep as last member? ...
6 years, 2 months ago (2014-10-08 01:22:16 UTC) #3
Mike West
Drive-by messages change LGTM, but you'll need someone like tsepez@ to sign off on it. ...
6 years, 2 months ago (2014-10-08 08:09:19 UTC) #5
Tom Sepez
lgtm https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/gpu_messages.h File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/gpu_messages.h#newcode86 content/common/gpu/gpu_messages.h:86: /* If the browser needs framerate throttling based ...
6 years, 2 months ago (2014-10-08 17:18:20 UTC) #6
ccameron
Thanks all! https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/gpu_messages.h File content/common/gpu/gpu_messages.h (right): https://codereview.chromium.org/636003002/diff/20001/content/common/gpu/gpu_messages.h#newcode86 content/common/gpu/gpu_messages.h:86: /* If the browser needs framerate throttling ...
6 years, 2 months ago (2014-10-08 18:44:01 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636003002/40001
6 years, 2 months ago (2014-10-08 18:46:04 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/6120) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_rel/builds/22871) android_dbg_tests_recipe ...
6 years, 2 months ago (2014-10-08 18:58:47 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636003002/60001
6 years, 2 months ago (2014-10-09 01:09:46 UTC) #13
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_32/builds/1136)
6 years, 2 months ago (2014-10-09 03:47:20 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/636003002/80001
6 years, 2 months ago (2014-10-09 07:33:57 UTC) #17
commit-bot: I haz the power
Committed patchset #5 (id:80001) as b7a67175b36f67cdd61258c68d366ead40ceb3d8
6 years, 2 months ago (2014-10-09 09:06:51 UTC) #18
commit-bot: I haz the power
6 years, 2 months ago (2014-10-09 09:09:43 UTC) #19
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/95e3616034fcf2ef6d1a7caf8b289ad87ad8b6d0
Cr-Commit-Position: refs/heads/master@{#298828}

Powered by Google App Engine
This is Rietveld 408576698