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

Issue 9980016: Delete background tab IOSurfaces on Mac. (Closed)

Created:
8 years, 8 months ago by jbates
Modified:
8 years, 8 months ago
CC:
chromium-reviews, Vangelis Kokkevis, Nico, darin (slow to review)
Visibility:
Public.

Description

Delete background tab IOSurfaces on Mac. The deleting part is easy: just handle the AcceleratedSurfaceSuspend call by unrefing the IOSurface. The hard part is dealing with a NSView drawRect when we don't have the IOSurface (or software BackingStore). To solve this, I reuse the GetBackingStore code to wait for a new frame from the renderer. When the BuffersSwapped message arrives on the IO thread for Mac, an UpdateRect message is synthesized with the SwapBuffers data. The UpdateRect message wakes up the UI thread and allows GetBackingStore to resume. The accelerated path can have multiple frames in the pipeline, so it is rarely enough to just wait for the next UpdateRect. Instead, the GetBackingStore method is updated to wait up to 40ms to get the correctly-sized frame (whether it's accelerated or software). The original GetBackingStore code waits for a frame that matches current_size_. However, this CL makes GetBackingStore wait for a frame that matches the view_->GetViewBounds(). current_size_ is equal to the last UpdateRect, which may or may not match GetViewBounds. (Anyone know why the original code doesn't use GetViewBounds?) This allows us to recover from missing BackingStores or IOSurfaces if the renderer/GPU can produce a new frame within 40ms. We probably want to increase 40 to something like 60 or 100ms though because of the deep GPU pipeline. In addition, thanks to the blocking GetBackingStore, this fixes some additional bugs: - no more resize gutter jank on accelerated pages (Mac only for now, but should work on Windows with a followup patch). - no more white flash while resizing flicker-test2.html. BUG=117624, 58782, 85519, 124328, 106586 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=134033

Patch Set 1 #

Patch Set 2 : checkpoint #

Patch Set 3 : . #

Patch Set 4 : checkpoint #

Patch Set 5 : working #

Patch Set 6 : review time #

Patch Set 7 : review time #

Patch Set 8 : compile #

Patch Set 9 : unittests #

Patch Set 10 : update / merge #

Patch Set 11 : fixed some perf issues on mac #

Total comments: 20

Patch Set 12 : kbr, piman feedback, cleanup, popup deadlock fix #

Patch Set 13 : separated SwapBuffers from UpdateRect msg #

Patch Set 14 : no change - update/merge #

Patch Set 15 : remove unused AboutToWaitForUpdateMsg #

Patch Set 16 : changed 40ms timeout to 50ms for GetBackingStore #

Total comments: 2

Patch Set 17 : comments and piman feedback #

Total comments: 11

Patch Set 18 : darin feedback #

Patch Set 19 : compile #

Patch Set 20 : no change - update/merge #

Total comments: 4

Patch Set 21 : darin feedback #

Patch Set 22 : darin feedback retry #

Patch Set 23 : remove check for existing render_process_host_id in helper map #

Patch Set 24 : fixed auto-resize and fullscreen toggle #

Unified diffs Side-by-side diffs Delta from patch set Stats (+644 lines, -263 lines) Patch
M content/browser/gpu/gpu_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/gpu/gpu_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +51 lines, -0 lines 0 comments Download
M content/browser/renderer_host/compositing_iosurface_mac.mm View 1 2 3 4 2 chunks +8 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_message_filter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +9 lines, -3 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +23 lines, -2 lines 0 comments Download
M content/browser/renderer_host/render_widget_helper.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 8 chunks +59 lines, -39 lines 0 comments Download
M content/browser/renderer_host/render_widget_helper.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 8 chunks +62 lines, -25 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 3 chunks +21 lines, -5 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 9 chunks +140 lines, -59 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 4 chunks +19 lines, -8 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_gtk.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +7 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.h View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +8 lines, -15 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_mac.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 chunks +104 lines, -66 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_win.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_win.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +8 lines, -0 lines 0 comments Download
M content/browser/renderer_host/test_render_view_host.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/test_render_view_host.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M content/common/gpu/image_transport_surface_mac.cc View 1 2 3 4 5 6 7 8 9 10 11 8 chunks +72 lines, -27 lines 0 comments Download
M content/common/view_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +11 lines, -1 line 0 comments Download
M content/port/browser/render_widget_host_view_port.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -0 lines 0 comments Download
M content/public/browser/render_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -3 lines 0 comments Download
M content/renderer/render_widget.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +4 lines, -1 line 0 comments Download
M content/test/mock_render_process_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -3 lines 0 comments Download
M content/test/mock_render_process_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +4 lines, -3 lines 0 comments Download

Messages

Total messages: 22 (0 generated)
jbates
There are a couple remaining issues to fix before committing, but let's get some reviews ...
8 years, 8 months ago (2012-04-17 21:37:16 UTC) #1
jbates
Updated the description to explain what was done.
8 years, 8 months ago (2012-04-17 22:00:51 UTC) #2
Ken Russell (switch to Gerrit)
LGTM overall. Assuming this has been tested. A few minor comments. Someone other than me ...
8 years, 8 months ago (2012-04-18 02:00:27 UTC) #3
piman
https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/renderer_host/render_widget_host_impl.cc File content/browser/renderer_host/render_widget_host_impl.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/renderer_host/render_widget_host_impl.cc#newcode642 content/browser/renderer_host/render_widget_host_impl.cc:642: backing_store = BackingStoreManager::GetBackingStore(this, view_size); Why doing this twice? https://chromiumcodereview.appspot.com/9980016/diff/28001/content/common/view_messages.h ...
8 years, 8 months ago (2012-04-18 20:02:00 UTC) #4
vangelis
https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/renderer_host/compositing_iosurface_mac.mm File content/browser/renderer_host/compositing_iosurface_mac.mm (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/renderer_host/compositing_iosurface_mac.mm#newcode146 content/browser/renderer_host/compositing_iosurface_mac.mm:146: CGLFlushDrawable(cglContext_); Can we send the acknowledgement back to the ...
8 years, 8 months ago (2012-04-18 20:32:26 UTC) #5
piman
https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h File content/browser/gpu/gpu_process_host.h (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h#newcode202 content/browser/gpu/gpu_process_host.h:202: std::map<int, scoped_refptr<RenderWidgetHelper> > render_widget_helpers_; Is this map really necessary? ...
8 years, 8 months ago (2012-04-18 21:07:44 UTC) #6
jbates
Will upload new CL once the discussions are resolved. https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.cc#newcode557 content/browser/gpu/gpu_process_host.cc:557: ...
8 years, 8 months ago (2012-04-18 23:01:58 UTC) #7
jbates
On 2012/04/18 23:01:58, jbates wrote: > Will upload new CL once the discussions are resolved. ...
8 years, 8 months ago (2012-04-18 23:13:27 UTC) #8
piman
https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h File content/browser/gpu/gpu_process_host.h (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h#newcode202 content/browser/gpu/gpu_process_host.h:202: std::map<int, scoped_refptr<RenderWidgetHelper> > render_widget_helpers_; On 2012/04/18 23:01:58, jbates wrote: ...
8 years, 8 months ago (2012-04-18 23:32:22 UTC) #9
jbates
https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h File content/browser/gpu/gpu_process_host.h (right): https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h#newcode202 content/browser/gpu/gpu_process_host.h:202: std::map<int, scoped_refptr<RenderWidgetHelper> > render_widget_helpers_; On 2012/04/18 23:32:22, piman wrote: ...
8 years, 8 months ago (2012-04-19 01:24:18 UTC) #10
jbates
On 2012/04/19 01:24:18, jbates wrote: > https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h > File content/browser/gpu/gpu_process_host.h (right): > > https://chromiumcodereview.appspot.com/9980016/diff/28001/content/browser/gpu/gpu_process_host.h#newcode202 > ...
8 years, 8 months ago (2012-04-19 21:31:24 UTC) #11
jbates
piman: ptal darin: ptal (mainly GetBackingStore) sky: content/browser/OWNERS joi: content/port/OWNERS
8 years, 8 months ago (2012-04-19 23:48:02 UTC) #12
piman
LGTM, but I didn't look at the mac parts. https://chromiumcodereview.appspot.com/9980016/diff/43023/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/43023/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode648 content/browser/renderer_host/render_widget_host_view_aura.cc:648: ...
8 years, 8 months ago (2012-04-19 23:58:18 UTC) #13
jbates
https://chromiumcodereview.appspot.com/9980016/diff/43023/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/43023/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode648 content/browser/renderer_host/render_widget_host_view_aura.cc:648: // only useable for software frames. On 2012/04/19 23:58:18, ...
8 years, 8 months ago (2012-04-20 01:14:04 UTC) #14
Jói
LGTM for content/port.
8 years, 8 months ago (2012-04-20 09:33:20 UTC) #15
sky
Darin is an OWNER for all this, so I'm letting him review.
8 years, 8 months ago (2012-04-20 15:36:33 UTC) #16
darin (slow to review)
This looks like a good design change. https://chromiumcodereview.appspot.com/9980016/diff/50024/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/50024/content/browser/gpu/gpu_process_host.cc#newcode560 content/browser/gpu/gpu_process_host.cc:560: scoped_completion_runner.Release(); I ...
8 years, 8 months ago (2012-04-20 17:11:26 UTC) #17
jbates
https://chromiumcodereview.appspot.com/9980016/diff/50024/content/browser/gpu/gpu_process_host.cc File content/browser/gpu/gpu_process_host.cc (right): https://chromiumcodereview.appspot.com/9980016/diff/50024/content/browser/gpu/gpu_process_host.cc#newcode560 content/browser/gpu/gpu_process_host.cc:560: scoped_completion_runner.Release(); On 2012/04/20 17:11:27, darin wrote: > I presume ...
8 years, 8 months ago (2012-04-20 23:01:04 UTC) #18
darin (slow to review)
LGTM https://chromiumcodereview.appspot.com/9980016/diff/65001/content/browser/gpu/gpu_process_host.h File content/browser/gpu/gpu_process_host.h (right): https://chromiumcodereview.appspot.com/9980016/diff/65001/content/browser/gpu/gpu_process_host.h#newcode196 content/browser/gpu/gpu_process_host.h:196: std::map<int, scoped_refptr<RenderWidgetHelper> > render_widget_helpers_; this shouldn't be needed ...
8 years, 8 months ago (2012-04-23 16:47:14 UTC) #19
jbates
https://chromiumcodereview.appspot.com/9980016/diff/65001/content/browser/gpu/gpu_process_host.h File content/browser/gpu/gpu_process_host.h (right): https://chromiumcodereview.appspot.com/9980016/diff/65001/content/browser/gpu/gpu_process_host.h#newcode196 content/browser/gpu/gpu_process_host.h:196: std::map<int, scoped_refptr<RenderWidgetHelper> > render_widget_helpers_; On 2012/04/23 16:47:14, darin wrote: ...
8 years, 8 months ago (2012-04-23 20:48:18 UTC) #20
darin (slow to review)
LGTM
8 years, 8 months ago (2012-04-24 03:07:46 UTC) #21
Hironori Bono
8 years, 8 months ago (2012-04-26 03:19:58 UTC) #22
Greetings,

Unfortunately, this change caused lots of leaks while running content_unittests
because content_unittests does not seem to have an IO thread and it cannot
delete RenderWidgetHelper. Is it an expected behavior of this change?

(*1)
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...

Regards,

Hironori Bono

Powered by Google App Engine
This is Rietveld 408576698