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

Issue 516643002: Fix assorted issues with remote CoreAnimation (Closed)

Created:
6 years, 3 months ago by ccameron
Modified:
6 years, 3 months ago
Reviewers:
piman
CC:
chromium-reviews, Ian Vollick, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, cc-bugs_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@clean_up_accel_layers
Project:
chromium
Visibility:
Public.

Description

Fix assorted issues with remote CoreAnimation These issues came up while running for a few days with the flag --enable-remote-core-animation. Fix flashes of old frames by hooking up the DiscardBackbuffer (which happens when being made non-visible) to re-set the CAContext and CALayer (so the browser gets a new one with new content for the next frame). Add support for disabling vsync by using setNeedsDisplay to draw. Change the backpressure mechanism to rely on the browser to apply backpressure in its compositor via the cc:: output surface. Add a ScopedSetGLToRealGLApi structure to ensure that we are talking to the real GL API while in the CoreAnimation callback. BUG=312462 Committed: https://crrev.com/388cc928a95dc1a921d3b1671985046c3c626c22 Cr-Commit-Position: refs/heads/master@{#292293}

Patch Set 1 #

Total comments: 5

Patch Set 2 : Incorporate review feedback #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+208 lines, -101 lines) Patch
M content/browser/compositor/browser_compositor_view_private_mac.mm View 3 chunks +3 lines, -0 lines 0 comments Download
M content/common/gpu/image_transport_surface_calayer_mac.h View 2 chunks +14 lines, -3 lines 0 comments Download
M content/common/gpu/image_transport_surface_calayer_mac.mm View 1 8 chunks +110 lines, -35 lines 0 comments Download
M content/common/gpu/image_transport_surface_fbo_mac.h View 3 chunks +19 lines, -21 lines 0 comments Download
M content/common/gpu/image_transport_surface_fbo_mac.mm View 5 chunks +20 lines, -32 lines 2 comments Download
M content/common/gpu/image_transport_surface_iosurface_mac.h View 1 chunk +4 lines, -3 lines 0 comments Download
M content/common/gpu/image_transport_surface_iosurface_mac.cc View 1 chunk +12 lines, -7 lines 0 comments Download
M ui/compositor/compositor.h View 1 chunk +3 lines, -0 lines 0 comments Download
M ui/compositor/compositor.cc View 1 chunk +4 lines, -0 lines 0 comments Download
M ui/gl/gl_gl_api_implementation.h View 2 chunks +10 lines, -0 lines 0 comments Download
M ui/gl/gl_gl_api_implementation.cc View 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
ccameron
ccameron@chromium.org changed reviewers: + piman@chromium.org
6 years, 3 months ago (2014-08-27 20:55:20 UTC) #1
ccameron
This is the new incarnation of the fix. If content is offscreen or occluded, we ...
6 years, 3 months ago (2014-08-27 20:57:23 UTC) #2
piman
https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm File content/common/gpu/image_transport_surface_calayer_mac.mm (right): https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm#newcode199 content/common/gpu/image_transport_surface_calayer_mac.mm:199: CHECK(has_pending_draw_); nit: DCHECK. https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm#newcode218 content/common/gpu/image_transport_surface_calayer_mac.mm:218: CHECK(!has_pending_draw_); nit: DCHECK. Can ...
6 years, 3 months ago (2014-08-27 21:21:42 UTC) #3
ccameron
https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm File content/common/gpu/image_transport_surface_calayer_mac.mm (right): https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm#newcode199 content/common/gpu/image_transport_surface_calayer_mac.mm:199: CHECK(has_pending_draw_); On 2014/08/27 21:21:42, piman (OOO) wrote: > nit: ...
6 years, 3 months ago (2014-08-27 21:41:56 UTC) #4
piman
https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm File content/common/gpu/image_transport_surface_calayer_mac.mm (right): https://codereview.chromium.org/516643002/diff/1/content/common/gpu/image_transport_surface_calayer_mac.mm#newcode218 content/common/gpu/image_transport_surface_calayer_mac.mm:218: CHECK(!has_pending_draw_); On 2014/08/27 21:41:56, ccameron1 wrote: > On 2014/08/27 ...
6 years, 3 months ago (2014-08-27 22:13:05 UTC) #5
ccameron
On 2014/08/27 22:13:05, piman (OOO) wrote: > > OK, so we rely on the fact ...
6 years, 3 months ago (2014-08-27 22:46:08 UTC) #6
piman
OK, LGTM
6 years, 3 months ago (2014-08-27 23:43:01 UTC) #7
ccameron
Thanks!
6 years, 3 months ago (2014-08-27 23:48:11 UTC) #8
ccameron
The CQ bit was checked by ccameron@chromium.org
6 years, 3 months ago (2014-08-27 23:48:18 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ccameron@chromium.org/516643002/20001
6 years, 3 months ago (2014-08-27 23:49:40 UTC) #10
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 3b6aee8ed0393d852ed21fae78f539ffffe3e8f8
6 years, 3 months ago (2014-08-28 02:09:29 UTC) #11
epenner
A revert of this CL (patchset #2) has been created in https://codereview.chromium.org/517733002/ by epenner@chromium.org. The ...
6 years, 3 months ago (2014-08-28 18:22:32 UTC) #12
commit-bot: I haz the power
6 years, 3 months ago (2014-09-10 02:56:29 UTC) #13
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/388cc928a95dc1a921d3b1671985046c3c626c22
Cr-Commit-Position: refs/heads/master@{#292293}

Powered by Google App Engine
This is Rietveld 408576698