|
|
Created:
3 years, 9 months ago by Saman Sami Modified:
3 years, 9 months ago CC:
chromium-reviews, jam, jbauman+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, xjz+watch_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnce we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after
https://codereview.chromium.org/2736053004/ we should be good.
I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.
TBR=jam@chromium.org
BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2731793002
Cr-Commit-Position: refs/heads/master@{#456180}
Committed: https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec873723a009c31
Patch Set 1 #Patch Set 2 : Fixed #Patch Set 3 : c #Patch Set 4 : c #Patch Set 5 : const #Patch Set 6 : Fixed resize #Patch Set 7 : c #Patch Set 8 : c #Patch Set 9 : c #
Messages
Total messages: 86 (67 generated)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== DelegatedFrameHost should not create new local surface id on eviction BUG=697864 ========== to ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame comes a new surface must be allocated. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost into RendererCompositorFrameSink and therefore DelegatedFrameHost must not interfere in local surface id allocation. BUG=697864 ==========
Description was changed from ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame comes a new surface must be allocated. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost into RendererCompositorFrameSink and therefore DelegatedFrameHost must not interfere in local surface id allocation. BUG=697864 ========== to ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. BUG=697864 ==========
Description was changed from ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. BUG=697864 ========== to ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab. I did not notice any change in the behaviour compared to when we destroyed the surface. BUG=697864 ==========
Description was changed from ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab. I did not notice any change in the behaviour compared to when we destroyed the surface. BUG=697864 ========== to ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. BUG=697864 ==========
samans@chromium.org changed reviewers: + fsamuel@chromium.org, jbauman@chromium.org
PTAL
On 2017/03/07 01:22:14, Saman Sami wrote: > PTAL I'm surprised this doesn't cause a black flicker on tab switching if the previous frame's contents are destroyed before the browser stops using them.
On 2017/03/07 01:25:40, jbauman wrote: > On 2017/03/07 01:22:14, Saman Sami wrote: > > PTAL > > I'm surprised this doesn't cause a black flicker on tab switching if the > previous frame's contents are destroyed before the browser stops using them. The page goes white for a few frames, but that happens even without this change. Sorry I just realized resize seems to be broken even though tab eviction looks good to me. I'll try to fix it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/07 01:29:28, Saman Sami wrote: > On 2017/03/07 01:25:40, jbauman wrote: > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > PTAL > > > > I'm surprised this doesn't cause a black flicker on tab switching if the > > previous frame's contents are destroyed before the browser stops using them. > > The page goes white for a few frames, but that happens even without this change. > Sorry I just realized resize seems to be broken even though tab eviction looks > good to me. I'll try to fix it. I fixed resize. I think the behaviour is not different because it seems like we first switch to the new tab and then evict the old tab so the surface that we evict is not being shown and is immediately deleted.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by samans@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
samans@chromium.org changed reviewers: - samans@google.com
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
On 2017/03/07 02:04:31, Saman Sami wrote: > On 2017/03/07 01:29:28, Saman Sami wrote: > > On 2017/03/07 01:25:40, jbauman wrote: > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > PTAL > > > > > > I'm surprised this doesn't cause a black flicker on tab switching if the > > > previous frame's contents are destroyed before the browser stops using them. > > > > The page goes white for a few frames, but that happens even without this > change. > > Sorry I just realized resize seems to be broken even though tab eviction looks > > good to me. I'll try to fix it. > > I fixed resize. Did any tests catch what you broke? A quick look at bot logs makes me think not. If not can you add one? > I think the behaviour is not different because it seems like we first switch to > the new tab and then evict the old tab so the surface that we evict is not being > shown and is immediately deleted.
On 2017/03/07 16:18:55, danakj wrote: > On 2017/03/07 02:04:31, Saman Sami wrote: > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > PTAL > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching if the > > > > previous frame's contents are destroyed before the browser stops using > them. > > > > > > The page goes white for a few frames, but that happens even without this > > change. > > > Sorry I just realized resize seems to be broken even though tab eviction > looks > > > good to me. I'll try to fix it. > > > > I fixed resize. > > Did any tests catch what you broke? A quick look at bot logs makes me think not. > If not can you add one? You can see in patch set 5 that a lot of tests failed actually.
On 2017/03/07 17:14:16, Saman Sami wrote: > On 2017/03/07 16:18:55, danakj wrote: > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > PTAL > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching if the > > > > > previous frame's contents are destroyed before the browser stops using > > them. > > > > > > > > The page goes white for a few frames, but that happens even without this > > > change. > > > > Sorry I just realized resize seems to be broken even though tab eviction > > looks > > > > good to me. I'll try to fix it. > > > > > > I fixed resize. > > > > Did any tests catch what you broke? A quick look at bot logs makes me think > not. > > If not can you add one? > > You can see in patch set 5 that a lot of tests failed actually. Yeh, they didn't look resize related but wasn't sure. Sounds like they were, thanks!
Patchset #7 (id:120001) has been deleted
On 2017/03/07 17:15:25, danakj wrote: > On 2017/03/07 17:14:16, Saman Sami wrote: > > On 2017/03/07 16:18:55, danakj wrote: > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > PTAL > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching if > the > > > > > > previous frame's contents are destroyed before the browser stops using > > > them. > > > > > > > > > > The page goes white for a few frames, but that happens even without this > > > > change. > > > > > Sorry I just realized resize seems to be broken even though tab eviction > > > looks > > > > > good to me. I'll try to fix it. > > > > > > > > I fixed resize. > > > > > > Did any tests catch what you broke? A quick look at bot logs makes me think > > not. > > > If not can you add one? > > > > You can see in patch set 5 that a lot of tests failed actually. > > Yeh, they didn't look resize related but wasn't sure. Sounds like they were, > thanks! danakj@: Yes, the tests don't seem to be directly related to resize, but when resize breaks so many things go wrong and some tests are gonna fail. jbauman@: I was talking to Fady offline and he had a very good point. We call SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go blank no matter what we do afterwards (force reclaim resources or evict surface). The surface will also get destroyed immediately because there is no one referencing it.
On 2017/03/08 01:08:25, Saman Sami wrote: > On 2017/03/07 17:15:25, danakj wrote: > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > On 2017/03/07 16:18:55, danakj wrote: > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > PTAL > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching if > > the > > > > > > > previous frame's contents are destroyed before the browser stops > using > > > > them. > > > > > > > > > > > > The page goes white for a few frames, but that happens even without > this > > > > > change. > > > > > > Sorry I just realized resize seems to be broken even though tab > eviction > > > > looks > > > > > > good to me. I'll try to fix it. > > > > > > > > > > I fixed resize. > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes me > think > > > not. > > > > If not can you add one? > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they were, > > thanks! > > danakj@: Yes, the tests don't seem to be directly related to resize, but when > resize breaks so many things go wrong and some tests are gonna fail. > > jbauman@: I was talking to Fady offline and he had a very good point. We call > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go > blank no matter what we do afterwards (force reclaim resources or evict > surface). The surface will also get destroyed immediately because there is no > one referencing it. That will only happen after the browser compositor does its commit and draw, whereas ForceReclaimResources will cause the contents to disappear before the browser compositor has a chance to draw a frame that doesn't use the surface.
On 2017/03/08 01:18:05, jbauman wrote: > On 2017/03/08 01:08:25, Saman Sami wrote: > > On 2017/03/07 17:15:25, danakj wrote: > > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > > On 2017/03/07 16:18:55, danakj wrote: > > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > > PTAL > > > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching > if > > > the > > > > > > > > previous frame's contents are destroyed before the browser stops > > using > > > > > them. > > > > > > > > > > > > > > The page goes white for a few frames, but that happens even without > > this > > > > > > change. > > > > > > > Sorry I just realized resize seems to be broken even though tab > > eviction > > > > > looks > > > > > > > good to me. I'll try to fix it. > > > > > > > > > > > > I fixed resize. > > > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes me > > think > > > > not. > > > > > If not can you add one? > > > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they were, > > > thanks! > > > > danakj@: Yes, the tests don't seem to be directly related to resize, but when > > resize breaks so many things go wrong and some tests are gonna fail. > > > > jbauman@: I was talking to Fady offline and he had a very good point. We call > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go > > blank no matter what we do afterwards (force reclaim resources or evict > > surface). The surface will also get destroyed immediately because there is no > > one referencing it. > > That will only happen after the browser compositor does its commit and draw, > whereas ForceReclaimResources will cause the contents to disappear before the > browser compositor has a chance to draw a frame that doesn't use the surface. So what if we request that the frame be evicted, and then later refer to the same surface ID again? The surface has been marked as destroyed...can we UNMARK it again? Or does that add too much complexity? I guess I don't necessarily want children to be allocating new surface IDs on frame eviction. I guess the control flow is really the parent allocates the surface ID and passes it to the child.
On 2017/03/08 01:18:05, jbauman wrote: > On 2017/03/08 01:08:25, Saman Sami wrote: > > On 2017/03/07 17:15:25, danakj wrote: > > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > > On 2017/03/07 16:18:55, danakj wrote: > > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > > PTAL > > > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab switching > if > > > the > > > > > > > > previous frame's contents are destroyed before the browser stops > > using > > > > > them. > > > > > > > > > > > > > > The page goes white for a few frames, but that happens even without > > this > > > > > > change. > > > > > > > Sorry I just realized resize seems to be broken even though tab > > eviction > > > > > looks > > > > > > > good to me. I'll try to fix it. > > > > > > > > > > > > I fixed resize. > > > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes me > > think > > > > not. > > > > > If not can you add one? > > > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they were, > > > thanks! > > > > danakj@: Yes, the tests don't seem to be directly related to resize, but when > > resize breaks so many things go wrong and some tests are gonna fail. > > > > jbauman@: I was talking to Fady offline and he had a very good point. We call > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go > > blank no matter what we do afterwards (force reclaim resources or evict > > surface). The surface will also get destroyed immediately because there is no > > one referencing it. > > That will only happen after the browser compositor does its commit and draw, > whereas ForceReclaimResources will cause the contents to disappear before the > browser compositor has a chance to draw a frame that doesn't use the surface. I've put an example of the black flicker this patch causes at https://drive.google.com/a/google.com/file/d/0B-5b6Bd64rGhNHhOYmRzWXBqZk0/vie... . The flicker at the top left is caused by reducing max saved frames to 1, causing a 1 second delay in RenderWidget::WasShown (though that's probably not necessary), using --disable-gpu-vsync, having 2 windows open, causing the one window's renderer to redraw, then switching tabs in the other window. I suspect that not all those steps are necessary, but they make the timing work out more often.
On 2017/03/08 01:46:03, Fady Samuel wrote: > On 2017/03/08 01:18:05, jbauman wrote: > > On 2017/03/08 01:08:25, Saman Sami wrote: > > > On 2017/03/07 17:15:25, danakj wrote: > > > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > > > On 2017/03/07 16:18:55, danakj wrote: > > > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > > > PTAL > > > > > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab > switching > > if > > > > the > > > > > > > > > previous frame's contents are destroyed before the browser stops > > > using > > > > > > them. > > > > > > > > > > > > > > > > The page goes white for a few frames, but that happens even > without > > > this > > > > > > > change. > > > > > > > > Sorry I just realized resize seems to be broken even though tab > > > eviction > > > > > > looks > > > > > > > > good to me. I'll try to fix it. > > > > > > > > > > > > > > I fixed resize. > > > > > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes me > > > think > > > > > not. > > > > > > If not can you add one? > > > > > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they > were, > > > > thanks! > > > > > > danakj@: Yes, the tests don't seem to be directly related to resize, but > when > > > resize breaks so many things go wrong and some tests are gonna fail. > > > > > > jbauman@: I was talking to Fady offline and he had a very good point. We > call > > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will > go > > > blank no matter what we do afterwards (force reclaim resources or evict > > > surface). The surface will also get destroyed immediately because there is > no > > > one referencing it. > > > > That will only happen after the browser compositor does its commit and draw, > > whereas ForceReclaimResources will cause the contents to disappear before the > > browser compositor has a chance to draw a frame that doesn't use the surface. > > So what if we request that the frame be evicted, and then later refer to the > same surface ID again? This seems like it would be a good solution. > > The surface has been marked as destroyed...can we UNMARK it again? Or does that > add too much complexity? I think the SurfaceManager would probably break if you tried to do this now, but it probably wouldn't be too hard to fix. > > I guess I don't necessarily want children to be allocating new surface IDs on > frame eviction. I guess the control flow is really the parent allocates the > surface ID and passes it to the child. I guess it's more that the child would have to allocate a new surface ID on WasShown or the equivalent.
Description was changed from ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. BUG=697864 ========== to ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Patchset #7 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/08 01:55:37, jbauman wrote: > On 2017/03/08 01:46:03, Fady Samuel wrote: > > On 2017/03/08 01:18:05, jbauman wrote: > > > On 2017/03/08 01:08:25, Saman Sami wrote: > > > > On 2017/03/07 17:15:25, danakj wrote: > > > > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > > > > On 2017/03/07 16:18:55, danakj wrote: > > > > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > > > > PTAL > > > > > > > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab > > switching > > > if > > > > > the > > > > > > > > > > previous frame's contents are destroyed before the browser > stops > > > > using > > > > > > > them. > > > > > > > > > > > > > > > > > > The page goes white for a few frames, but that happens even > > without > > > > this > > > > > > > > change. > > > > > > > > > Sorry I just realized resize seems to be broken even though tab > > > > eviction > > > > > > > looks > > > > > > > > > good to me. I'll try to fix it. > > > > > > > > > > > > > > > > I fixed resize. > > > > > > > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes > me > > > > think > > > > > > not. > > > > > > > If not can you add one? > > > > > > > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > > > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they > > were, > > > > > thanks! > > > > > > > > danakj@: Yes, the tests don't seem to be directly related to resize, but > > when > > > > resize breaks so many things go wrong and some tests are gonna fail. > > > > > > > > jbauman@: I was talking to Fady offline and he had a very good point. We > > call > > > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will > > go > > > > blank no matter what we do afterwards (force reclaim resources or evict > > > > surface). The surface will also get destroyed immediately because there is > > no > > > > one referencing it. > > > > > > That will only happen after the browser compositor does its commit and draw, > > > whereas ForceReclaimResources will cause the contents to disappear before > the > > > browser compositor has a chance to draw a frame that doesn't use the > surface. > > > > So what if we request that the frame be evicted, and then later refer to the > > same surface ID again? > > This seems like it would be a good solution. > > > > The surface has been marked as destroyed...can we UNMARK it again? Or does > that > > add too much complexity? > > I think the SurfaceManager would probably break if you tried to do this now, but > it probably wouldn't be too hard to fix. > > > > I guess I don't necessarily want children to be allocating new surface IDs on > > frame eviction. I guess the control flow is really the parent allocates the > > surface ID and passes it to the child. > > I guess it's more that the child would have to allocate a new surface ID on > WasShown or the equivalent. Thank you John for the video. I'm going to keep EvictFrame and try to fix SurfaceManager so the old surface id can be used again.
On 2017/03/08 16:38:43, Saman Sami wrote: > On 2017/03/08 01:55:37, jbauman wrote: > > On 2017/03/08 01:46:03, Fady Samuel wrote: > > > On 2017/03/08 01:18:05, jbauman wrote: > > > > On 2017/03/08 01:08:25, Saman Sami wrote: > > > > > On 2017/03/07 17:15:25, danakj wrote: > > > > > > On 2017/03/07 17:14:16, Saman Sami wrote: > > > > > > > On 2017/03/07 16:18:55, danakj wrote: > > > > > > > > On 2017/03/07 02:04:31, Saman Sami wrote: > > > > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote: > > > > > > > > > > On 2017/03/07 01:25:40, jbauman wrote: > > > > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote: > > > > > > > > > > > > PTAL > > > > > > > > > > > > > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab > > > switching > > > > if > > > > > > the > > > > > > > > > > > previous frame's contents are destroyed before the browser > > stops > > > > > using > > > > > > > > them. > > > > > > > > > > > > > > > > > > > > The page goes white for a few frames, but that happens even > > > without > > > > > this > > > > > > > > > change. > > > > > > > > > > Sorry I just realized resize seems to be broken even though > tab > > > > > eviction > > > > > > > > looks > > > > > > > > > > good to me. I'll try to fix it. > > > > > > > > > > > > > > > > > > I fixed resize. > > > > > > > > > > > > > > > > Did any tests catch what you broke? A quick look at bot logs makes > > me > > > > > think > > > > > > > not. > > > > > > > > If not can you add one? > > > > > > > > > > > > > > You can see in patch set 5 that a lot of tests failed actually. > > > > > > > > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they > > > were, > > > > > > thanks! > > > > > > > > > > danakj@: Yes, the tests don't seem to be directly related to resize, but > > > when > > > > > resize breaks so many things go wrong and some tests are gonna fail. > > > > > > > > > > jbauman@: I was talking to Fady offline and he had a very good point. We > > > call > > > > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page > will > > > go > > > > > blank no matter what we do afterwards (force reclaim resources or evict > > > > > surface). The surface will also get destroyed immediately because there > is > > > no > > > > > one referencing it. > > > > > > > > That will only happen after the browser compositor does its commit and > draw, > > > > whereas ForceReclaimResources will cause the contents to disappear before > > the > > > > browser compositor has a chance to draw a frame that doesn't use the > > surface. > > > > > > So what if we request that the frame be evicted, and then later refer to the > > > same surface ID again? > > > > This seems like it would be a good solution. > > > > > > The surface has been marked as destroyed...can we UNMARK it again? Or does > > that > > > add too much complexity? > > > > I think the SurfaceManager would probably break if you tried to do this now, > but > > it probably wouldn't be too hard to fix. > > > > > > I guess I don't necessarily want children to be allocating new surface IDs > on > > > frame eviction. I guess the control flow is really the parent allocates the > > > surface ID and passes it to the child. > > > > I guess it's more that the child would have to allocate a new surface ID on > > WasShown or the equivalent. > > Thank you John for the video. I'm going to keep EvictFrame and try to fix > SurfaceManager so the old surface id can be used again. The changes in SurfaceManager will be in another CL that I will ask you to review later.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame. This means that after a new frame arrives a new surface must be created. This doesn't have to be the case. We can simply call ForceReclaimResources in order to reclaim the resources while keeping the surface intact. This needs to be done because we are moving the local surface id allocation out of DelegatedFrameHost and into RendererCompositorFrameSink so DelegatedFrameHost must not interfere in local surface id allocation. I tried testing this change by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Once we move local surface id allocation into the renderer process, DelegatedFrameHost can no longer create a new local surface id on frame eviction. This would normally break a few things but after https://codereview.chromium.org/2736053004/ we should be good. I tried testing this CL by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. Without https://codereview.chromium.org/2736053004/ if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating a surface using a surface id that is already in use, but that CL fixes this issue. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by samans@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jbauman@: After I landed my fix in SurfaceManager all the bots are green. Can you please take another look? Notice that we are sticking with EvictFrame and not ForceReclaimResources.
lgtm
Description was changed from ========== Once we move local surface id allocation into the renderer process, DelegatedFrameHost can no longer create a new local surface id on frame eviction. This would normally break a few things but after https://codereview.chromium.org/2736053004/ we should be good. I tried testing this CL by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. Without https://codereview.chromium.org/2736053004/ if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating a surface using a surface id that is already in use, but that CL fixes this issue. BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Once we move local surface id allocation into the renderer process, DelegatedFrameHost can no longer create a new local surface id on frame eviction. This would normally break a few things but after https://codereview.chromium.org/2736053004/ we should be good. I tried testing this CL by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. Without https://codereview.chromium.org/2736053004/ if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating a surface using a surface id that is already in use, but that CL fixes this issue. TBR=jam@chromium.org BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
samans@chromium.org changed reviewers: + jam@chromium.org
The CQ bit was checked by samans@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489182895626300, "parent_rev": "77414f1023d40d82ec1b636f67a99316b26210be", "commit_rev": "c43562c581991f1d9b843cb3cec873723a009c31"}
Message was sent while issue was closed.
Description was changed from ========== Once we move local surface id allocation into the renderer process, DelegatedFrameHost can no longer create a new local surface id on frame eviction. This would normally break a few things but after https://codereview.chromium.org/2736053004/ we should be good. I tried testing this CL by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. Without https://codereview.chromium.org/2736053004/ if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating a surface using a surface id that is already in use, but that CL fixes this issue. TBR=jam@chromium.org BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Once we move local surface id allocation into the renderer process, DelegatedFrameHost can no longer create a new local surface id on frame eviction. This would normally break a few things but after https://codereview.chromium.org/2736053004/ we should be good. I tried testing this CL by setting the maximum number of saved frames to 1 so that each new tab causes the eviction of the previous tab and compared the current behaviour vs behaviour after this CL and did not notice any difference. Without https://codereview.chromium.org/2736053004/ if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating a surface using a surface id that is already in use, but that CL fixes this issue. TBR=jam@chromium.org BUG=697864 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2731793002 Cr-Commit-Position: refs/heads/master@{#456180} Committed: https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:200001) as https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec8... |