Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 2736053004: SurfaceIds must be reusable as soon as their surfaces are marked destroyed (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 months, 2 weeks ago by Saman Sami
Modified:
7 months, 2 weeks ago
Reviewers:
saman, Fady Samuel, jbauman
CC:
chromium-reviews, cc-bugs_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Once we move SurfaceId allocation from DelegatedFrameHost to RendererCompositorFrameSink, DelegatedFrameHost will evict the renderer's surfaces without the renderer knowing when we are under memory pressure. This means that the renderer might continue sending frames using the same local surface id that is destroyed, and we need to make sure we can handle this situation properly. 1) If the renderer's surface is in the garbage collector's queue, we just take it out of there and use it again. 2) If the renderer's surface is actually garbage collected, we create the surface again. I have added unit tests and also tested this CL together with https://codereview.chromium.org/2731793002/. Without this CL, if the max number of saved frames is 1 and we switch between two tabs very fast, a DCHECK fails in SurfaceManager saying that the surface we are trying to create already exists, but using this CL this issue doesn't happen. CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2736053004 Cr-Commit-Position: refs/heads/master@{#455928} Committed: https://chromium.googlesource.com/chromium/src/+/347e05c9fcda35ab2fd31cd7e7b9d89bc10aed26

Patch Set 1 #

Patch Set 2 : Move surface creation to SurfaceManager #

Patch Set 3 : Undo changes in surface #

Total comments: 10

Patch Set 4 : Addressed comments #

Patch Set 5 : Split tests #

Total comments: 6

Patch Set 6 : Changed signature #

Total comments: 4

Patch Set 7 : Make deregister private #

Unified diffs Side-by-side diffs Delta from patch set Stats (+126 lines, -22 lines) Patch
M cc/surfaces/compositor_frame_sink_support_unittest.cc View 1 2 3 4 1 chunk +68 lines, -0 lines 0 comments Download
M cc/surfaces/surface_factory.cc View 1 2 3 4 5 1 chunk +3 lines, -4 lines 0 comments Download
M cc/surfaces/surface_manager.h View 1 2 3 4 5 6 3 chunks +9 lines, -3 lines 0 comments Download
M cc/surfaces/surface_manager.cc View 1 2 3 4 5 6 4 chunks +46 lines, -15 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 46 (31 generated)
Saman Sami
PTAL
7 months, 2 weeks ago (2017-03-08 17:24:33 UTC) #6
Saman Sami
On 2017/03/08 17:24:33, Saman Sami wrote: > PTAL I will also add unit tests later. ...
7 months, 2 weeks ago (2017-03-08 17:34:31 UTC) #9
Fady Samuel
not lgtm I'm not a fan of this approach. I think we should reuse the ...
7 months, 2 weeks ago (2017-03-08 18:20:22 UTC) #10
Fady Samuel
not lgtm I'm not a fan of this approach. I think we should reuse the ...
7 months, 2 weeks ago (2017-03-08 18:20:23 UTC) #11
Fady Samuel
On 2017/03/08 18:20:23, Fady Samuel wrote: > not lgtm > > I'm not a fan ...
7 months, 2 weeks ago (2017-03-09 03:04:18 UTC) #12
Saman Sami
PTAL
7 months, 2 weeks ago (2017-03-09 16:19:54 UTC) #17
Fady Samuel
Looking pretty good. I added some comments. A couple of unit tests would be useful ...
7 months, 2 weeks ago (2017-03-09 16:30:08 UTC) #18
Saman Sami
PTAL https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_manager.cc File cc/surfaces/surface_manager.cc (right): https://codereview.chromium.org/2736053004/diff/40001/cc/surfaces/surface_manager.cc#newcode100 cc/surfaces/surface_manager.cc:100: SurfaceId surface_id, On 2017/03/09 16:30:08, Fady Samuel wrote: ...
7 months, 2 weeks ago (2017-03-09 20:45:50 UTC) #28
Fady Samuel
Mostly looks good. I've proposed a safer signature for SurfaceManager::CreateSurface. Thanks! The tests are awesome! ...
7 months, 2 weeks ago (2017-03-09 21:32:17 UTC) #30
Saman Sami
PTAL jbauman@: This CL lets the renderer keep submitting frames even if the browser process ...
7 months, 2 weeks ago (2017-03-09 21:45:17 UTC) #34
jbauman
lgtm https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_manager.h#newcode38 cc/surfaces/surface_manager.h:38: class Surface; add "class SurfaceFactory;" here (or add ...
7 months, 2 weeks ago (2017-03-09 22:21:37 UTC) #35
Saman Sami
Thanks for the lgtm! https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_manager.h File cc/surfaces/surface_manager.h (right): https://codereview.chromium.org/2736053004/diff/120001/cc/surfaces/surface_manager.h#newcode38 cc/surfaces/surface_manager.h:38: class Surface; On 2017/03/09 22:21:37, ...
7 months, 2 weeks ago (2017-03-09 22:32:04 UTC) #37
Fady Samuel
lgtm
7 months, 2 weeks ago (2017-03-09 22:35:12 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2736053004/140001
7 months, 2 weeks ago (2017-03-09 23:54:28 UTC) #43
commit-bot: I haz the power
7 months, 2 weeks ago (2017-03-10 00:46:35 UTC) #46
Message was sent while issue was closed.
Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/347e05c9fcda35ab2fd31cd7e7b9...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa