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

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

Created:
3 years, 9 months ago by Saman Sami
Modified:
3 years, 9 months 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

Messages

Total messages: 46 (31 generated)
Saman Sami
PTAL
3 years, 9 months 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. ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months ago (2017-03-09 03:04:18 UTC) #12
Saman Sami
PTAL
3 years, 9 months 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 ...
3 years, 9 months 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: ...
3 years, 9 months 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! ...
3 years, 9 months 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 ...
3 years, 9 months 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 ...
3 years, 9 months 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, ...
3 years, 9 months ago (2017-03-09 22:32:04 UTC) #37
Fady Samuel
lgtm
3 years, 9 months 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
3 years, 9 months ago (2017-03-09 23:54:28 UTC) #43
commit-bot: I haz the power
3 years, 9 months 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...

Powered by Google App Engine
This is Rietveld 408576698