Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846653002:
cc : Stop pushing layers from hidden subtrees at commit
Descriptioncc : Stop pushing layers from hidden subtrees at commit
A cc::Layer is hidden if it has an ancestor with
Layer::hide_layer_and_subtree = true and has no descendant with copy
request. With this CL, we also stop creating property tree nodes for
these layers and stop sending them at commit to the compositor thread.
BUG=595843
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : . #
Total comments: 1
Patch Set 5 : rebase #Patch Set 6 : don't add hidden layers to push propreties list #
Total comments: 4
Patch Set 7 : rebase #Patch Set 8 : fix ash_unittests #Patch Set 9 : rebase #Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : rebase #Patch Set 13 : send hidden surface ids also #Patch Set 14 : fix cc_unittests after rebase #
Total comments: 8
Patch Set 15 : hide mask layer also #
Total comments: 8
Patch Set 16 : rebase #Patch Set 17 : comments #Patch Set 18 : . #
Total comments: 2
Patch Set 19 : only add valid surface ids #Patch Set 20 : rebase #Patch Set 21 : add TODO #
Total comments: 2
Patch Set 22 : rebase #Messages
Total messages: 79 (51 generated)
Description was changed from ========== cc : Stop pushing layers from hidden subtrees at commit A cc::Layer is hidden if it has an ancestor with Layer::hide_layer_and_subtree = true and has no descendant with copy request. With this CL, we also stop creating property tree nodes for these layers and stop sending them at commit to the compositor thread. BUG= ========== to ========== cc : Stop pushing layers from hidden subtrees at commit A cc::Layer is hidden if it has an ancestor with Layer::hide_layer_and_subtree = true and has no descendant with copy request. With this CL, we also stop creating property tree nodes for these layers and stop sending them at commit to the compositor thread. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== cc : Stop pushing layers from hidden subtrees at commit A cc::Layer is hidden if it has an ancestor with Layer::hide_layer_and_subtree = true and has no descendant with copy request. With this CL, we also stop creating property tree nodes for these layers and stop sending them at commit to the compositor thread. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc : Stop pushing layers from hidden subtrees at commit A cc::Layer is hidden if it has an ancestor with Layer::hide_layer_and_subtree = true and has no descendant with copy request. With this CL, we also stop creating property tree nodes for these layers and stop sending them at commit to the compositor thread. BUG=595843 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
jaydasika@chromium.org changed reviewers: + enne@chromium.org, weiliangc@chromium.org
PTAL
The CQ bit was checked by jaydasika@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...
https://codereview.chromium.org/2846653002/diff/60001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_common_unittest.cc (left): https://codereview.chromium.org/2846653002/diff/60001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_common_unittest.cc:5371: TEST_F(LayerTreeHostCommonTest, SubtreeHidden_SingleLayerImpl) { Deleted these tests as hidden subtrees now don't exists on compositor thread and there are similar tests on main thread already.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
I don't remember what the goal of this feature was when added. Is it something where it's desirable to raster this (like offscreen content that wants to be shown quickly without a lot of sudden raster work) or is this just a way to hide content without extra work of detaching layers from trees, etc? Did you dig up what the original goal of this feature was? (Sorry that my memory is failing.)
On 2017/04/27 21:46:10, enne wrote: > I don't remember what the goal of this feature was when added. Is it something > where it's desirable to raster this (like offscreen content that wants to be > shown quickly without a lot of sudden raster work) or is this just a way to hide > content without extra work of detaching layers from trees, etc? Did you dig up > what the original goal of this feature was? (Sorry that my memory is failing.) Going be the description in https://codereview.chromium.org/16896017 which added it, I think its a way to hide layers and still have a way to allow readbacks on them.
The CQ bit was checked by jaydasika@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: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by jaydasika@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: linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
The CQ bit was checked by jaydasika@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/04/27 21:53:51, jaydasika wrote: > On 2017/04/27 21:46:10, enne wrote: > > I don't remember what the goal of this feature was when added. Is it > something > > where it's desirable to raster this (like offscreen content that wants to be > > shown quickly without a lot of sudden raster work) or is this just a way to > hide > > content without extra work of detaching layers from trees, etc? Did you dig > up > > what the original goal of this feature was? (Sorry that my memory is > failing.) > > Going be the description in https://codereview.chromium.org/16896017 which added > it, I think its a way to hide layers and still have a way to allow readbacks on > them. Looks like this is mostly recursively setting visibility from animator. So two thoughts in possibilities: 1. actually call is_hidden recursively and skip these layers from property tree builder. 2. if the visibility is always correct with animator's transform and opacity values, maybe it would just work out without setting hide for subtrees.
https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1073: if (layer->is_hidden()) This is a little awkward, here. I think what you want to say is that if a layer is hidden, it shouldn't push properties. This says if a layer becomes hidden, then ignore additional push properties requests. I think those are not quite the same? And then the tree sychronizer redundantly fixes this up, so maybe this call isn't needed here?
The CQ bit was checked by jaydasika@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by jaydasika@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...
https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1073: if (layer->is_hidden()) On 2017/05/01 20:15:09, enne wrote: > This is a little awkward, here. I think what you want to say is that if a layer > is hidden, it shouldn't push properties. This says if a layer becomes hidden, > then ignore additional push properties requests. I think those are not quite > the same? > > And then the tree sychronizer redundantly fixes this up, so maybe this call > isn't needed here? Yes, what we want to say is if a layer is hidden, it shouldn't push properties. The reason I added this here is for this sequence : hide layer-> commit -> change a property on hidden layer -> commit. Even during second commit, we don't want to push properties of the layer. But if the layer tree topology hasn't changed, we won't do full tree sync and never hit the code in tree synchronizer that deletes hidden layers from push properties layer list. I agree this is unnecessarily complex, maybe we should just check while pushing properties if the layer is hidden and skip it there.
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_...)
https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1073: if (layer->is_hidden()) On 2017/05/01 at 23:41:39, jaydasika wrote: > On 2017/05/01 20:15:09, enne wrote: > > This is a little awkward, here. I think what you want to say is that if a layer > > is hidden, it shouldn't push properties. This says if a layer becomes hidden, > > then ignore additional push properties requests. I think those are not quite > > the same? > > > > And then the tree sychronizer redundantly fixes this up, so maybe this call > > isn't needed here? > > Yes, what we want to say is if a layer is hidden, it shouldn't push properties. > > The reason I added this here is for this sequence : hide layer-> commit -> change a property on hidden layer -> commit. > Even during second commit, we don't want to push properties of the layer. But if the layer tree topology hasn't changed, we won't do full tree sync and never hit the code in tree synchronizer that deletes hidden layers from push properties layer list. > > I agree this is unnecessarily complex, maybe we should just check while pushing properties if the layer is hidden and skip it there. That sounds good!
The CQ bit was checked by jaydasika@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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Tab capture browser tests are failing with this patch. It happens because they expect hidden layers to be on the compositor thread even when they don't have a copy request on them. The copy request is on the surface and the layer doesn't know about it.
On Thu, May 11, 2017 at 2:05 PM, <jaydasika@chromium.org> wrote: > Tab capture browser tests are failing with this patch. It happens because > they > expect hidden layers to be on the compositor thread even when they don't > have a > copy request on them. The copy request is on the surface and the layer > doesn't > know about it. > A copy request on the surface implies its copying stuff from a different compositor. How do missing layers in the embedding compositor get involved? > > > > https://codereview.chromium.org/2846653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/11 20:15:40, danakj wrote: > On Thu, May 11, 2017 at 2:05 PM, <mailto:jaydasika@chromium.org> wrote: > > > Tab capture browser tests are failing with this patch. It happens because > > they > > expect hidden layers to be on the compositor thread even when they don't > > have a > > copy request on them. The copy request is on the surface and the layer > > doesn't > > know about it. > > > > A copy request on the surface implies its copying stuff from a different > compositor. How do missing layers in the embedding compositor get involved? I don't know how tab capture works. miu@ or jbauman@ can answer your question better I think ? I will just paste here what they told me : Firstly, the crashes are in offscreen tab capture browser tests. miu@ told me that offscreen tab capture attaches a layer to the browser window outside its visible region as this was the only way on Win/Linux to drive the display compositing needed to capture tabs that we really want to have offscreen. I then talked to jbauman@ who told me that we probably need to plumb all surface layers or atleast their ids to the compositor thread to make this work. > > > > > > > > > > https://codereview.chromium.org/2846653002/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
On 2017/05/11 20:50:57, jaydasika wrote: > On 2017/05/11 20:15:40, danakj wrote: > > On Thu, May 11, 2017 at 2:05 PM, <mailto:jaydasika@chromium.org> wrote: > > > > > Tab capture browser tests are failing with this patch. It happens because Specifically, only the "offscreen tab" ones. But, I would expect that ONscreen tabs that become hidden (i.e., user switches to another tab) would suffer the same fate. > I don't know how tab capture works. miu@ or jbauman@ can answer your question > better I think ? So, this hack is required on Win/CrOS/Linux, but not on Mac: https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tab_captur... Note: This hack originates before Surfaces existed (and maybe it is not necessary for capture of Surfaces?). We re-attach the aura::Window for hidden tabs (or offscreen tabs) to a browser window (*any* browser window, in fact, the hack does not care which). The purpose of the hack was to make sure the tab contents' copy requests are executed. Whenever the aura::Window's layer is not in a layer tree, nothing was causing the layer to composite nor execute the copy requests. IIUC, the reason is that browser compositor was the only thing driving drawing of the layer, but when the layer is not in the layer tree, it is unreachable. (Also, for some reason unknown to me, this has never been an issue on Mac. I'm not sure how Mac is different w.r.t. what drives compositing of a layer.) I suspect jbauman@ could give a better explanation, from the perspective of compositing; and whether there's a similar issue with "orphaned Surfaces" just like this "orphaned Layer" issue. BTW--I'm moving much of screen capture into VIZ (for Mus), and one thing I haven't exactly worked out yet is how I will handle Surfaces for tabs that become hidden. Are such Surfaces removed from the hierarchy, or do they remain in the hierarchy but are made invisible? From jbauman@'s recent brownbag talk, I suspect he has a lot of interesting ideas here, especially around Headless Displays. :) If the explanations aren't straightforward, perhaps we should VC?
On 2017/05/11 22:25:20, miu wrote: > On 2017/05/11 20:50:57, jaydasika wrote: > > On 2017/05/11 20:15:40, danakj wrote: > > > On Thu, May 11, 2017 at 2:05 PM, <mailto:jaydasika@chromium.org> wrote: > > > > > > > Tab capture browser tests are failing with this patch. It happens because > > Specifically, only the "offscreen tab" ones. But, I would expect that ONscreen > tabs that become hidden (i.e., user switches to another tab) would suffer the > same fate. > > > I don't know how tab capture works. miu@ or jbauman@ can answer your question > > better I think ? > > So, this hack is required on Win/CrOS/Linux, but not on Mac: > https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tab_captur... > > Note: This hack originates before Surfaces existed (and maybe it is not > necessary for capture of Surfaces?). > > We re-attach the aura::Window for hidden tabs (or offscreen tabs) to a browser > window (*any* browser window, in fact, the hack does not care which). The > purpose of the hack was to make sure the tab contents' copy requests are > executed. Whenever the aura::Window's layer is not in a layer tree, nothing was > causing the layer to composite nor execute the copy requests. IIUC, the reason > is that browser compositor was the only thing driving drawing of the layer, but > when the layer is not in the layer tree, it is unreachable. > > (Also, for some reason unknown to me, this has never been an issue on Mac. I'm > not sure how Mac is different w.r.t. what drives compositing of a layer.) > > I suspect jbauman@ could give a better explanation, from the perspective of > compositing; and whether there's a similar issue with "orphaned Surfaces" just > like this "orphaned Layer" issue. It's essentially the same issue with surfaces as with layers. Display compositors are what schedule and execute rendering, so the Surface needs to be somehow attached to a Display to ensure that someone is going to draw it. I thought about just attaching the Surface at random to some display, but we'd need to make sure they're compatible (GL vs. software rendering) and the display is actually going to render at some time (e.g. not invisible). The headless display is another way to handle that, but the current solution mostly works, so I never got around to implementing it. > > BTW--I'm moving much of screen capture into VIZ (for Mus), and one thing I > haven't exactly worked out yet is how I will handle Surfaces for tabs that > become hidden. Are such Surfaces removed from the hierarchy, or do they remain > in the hierarchy but are made invisible? From jbauman@'s recent brownbag talk, I > suspect he has a lot of interesting ideas here, especially around Headless > Displays. :) If the explanations aren't straightforward, perhaps we should VC?
On Thu, May 11, 2017 at 6:25 PM, <miu@chromium.org> wrote: > On 2017/05/11 20:50:57, jaydasika wrote: > > On 2017/05/11 20:15:40, danakj wrote: > > > On Thu, May 11, 2017 at 2:05 PM, <mailto:jaydasika@chromium.org> > wrote: > > > > > > > Tab capture browser tests are failing with this patch. It happens > because > > Specifically, only the "offscreen tab" ones. But, I would expect that > ONscreen > tabs that become hidden (i.e., user switches to another tab) would suffer > the > same fate. > > > I don't know how tab capture works. miu@ or jbauman@ can answer your > question > > better I think ? > > So, this hack is required on Win/CrOS/Linux, but not on Mac: > https://cs.chromium.org/chromium/src/chrome/browser/ > extensions/api/tab_capture/tab_capture_registry.cc?rcl= > da176b49eafe0f94f32919620f7ea6fb3e0d808f&l=40 > > Note: This hack originates before Surfaces existed (and maybe it is not > necessary for capture of Surfaces?). > > We re-attach the aura::Window for hidden tabs (or offscreen tabs) to a > browser > window (*any* browser window, in fact, the hack does not care which). The > purpose of the hack was to make sure the tab contents' copy requests are > executed. Whenever the aura::Window's layer is not in a layer tree, > nothing was > causing the layer to composite nor execute the copy requests. IIUC, the > reason > is that browser compositor was the only thing driving drawing of the > layer, but > when the layer is not in the layer tree, it is unreachable. > Right, attaching any layer to the UI shouldn't be needed for a copy request on a surface, but would for a request on a layer, in order for the request to get plumbed through to the Display. > > (Also, for some reason unknown to me, this has never been an issue on Mac. > I'm > not sure how Mac is different w.r.t. what drives compositing of a layer.) > > I suspect jbauman@ could give a better explanation, from the perspective > of > compositing; and whether there's a similar issue with "orphaned Surfaces" > just > like this "orphaned Layer" issue. > > BTW--I'm moving much of screen capture into VIZ (for Mus), and one thing I > haven't exactly worked out yet is how I will handle Surfaces for tabs that > become hidden. Are such Surfaces removed from the hierarchy, or do they > remain > in the hierarchy but are made invisible? From jbauman@'s recent brownbag > talk, I > suspect he has a lot of interesting ideas here, especially around Headless > Displays. :) If the explanations aren't straightforward, perhaps we should > VC? > > https://codereview.chromium.org/2846653002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
So, jaydasika, I'm not sure if our discussion helped you figure out what's causing the breakage in this change. But, it would seem that there are descendant layers (or Surfaces) that *do* have copy requests, but are not visible on-screen (possibly fully occluded?), and somehow this is not being detected after your change.
On 2017/05/12 23:10:37, miu wrote: > So, jaydasika, I'm not sure if our discussion helped you figure out what's > causing the breakage in this change. But, it would seem that there are > descendant layers (or Surfaces) that *do* have copy requests, but are not > visible on-screen (possibly fully occluded?), and somehow this is not being > detected after your change. This change makes sure that if the descendant *layers* had copy requests, we *do* send them to the compositor thread. But it has no knowledge on copy requests on surfaces. So, the problem wrt this patch is hidden surfaces with copy requests. I have verified that the offscreen tab capture tests *pass* when I send (non-hidden layers + all surface layers) to the compositor thread.
On 2017/05/13 at 00:56:51, jaydasika wrote: > On 2017/05/12 23:10:37, miu wrote: > > So, jaydasika, I'm not sure if our discussion helped you figure out what's > > causing the breakage in this change. But, it would seem that there are > > descendant layers (or Surfaces) that *do* have copy requests, but are not > > visible on-screen (possibly fully occluded?), and somehow this is not being > > detected after your change. > > This change makes sure that if the descendant *layers* had copy requests, we *do* send them to the compositor thread. But it has no knowledge on copy requests on surfaces. So, the problem wrt this patch is hidden surfaces with copy requests. I have verified that the offscreen tab capture tests *pass* when I send (non-hidden layers + all surface layers) to the compositor thread. Hmm, that's a little confusing to me. Why would a surface layer need to be sent in order for a tab capture to succeed? The surface layer itself is just a surface id that generates a SurfaceDrawQuad. It smells a bit like the bug is somewhere in the vicinity of SurfaceAggregator in that it can't make a copy request of a surface if that surface is disconnected from the tree of compositor frames, and that it needs the SurfaceDrawQuad to connect everything together.
On 2017/05/15 at 17:24:04, enne wrote: > On 2017/05/13 at 00:56:51, jaydasika wrote: > > On 2017/05/12 23:10:37, miu wrote: > > > So, jaydasika, I'm not sure if our discussion helped you figure out what's > > > causing the breakage in this change. But, it would seem that there are > > > descendant layers (or Surfaces) that *do* have copy requests, but are not > > > visible on-screen (possibly fully occluded?), and somehow this is not being > > > detected after your change. > > > > This change makes sure that if the descendant *layers* had copy requests, we *do* send them to the compositor thread. But it has no knowledge on copy requests on surfaces. So, the problem wrt this patch is hidden surfaces with copy requests. I have verified that the offscreen tab capture tests *pass* when I send (non-hidden layers + all surface layers) to the compositor thread. > > Hmm, that's a little confusing to me. Why would a surface layer need to be sent in order for a tab capture to succeed? The surface layer itself is just a surface id that generates a SurfaceDrawQuad. It smells a bit like the bug is somewhere in the vicinity of SurfaceAggregator in that it can't make a copy request of a surface if that surface is disconnected from the tree of compositor frames, and that it needs the SurfaceDrawQuad to connect everything together. Jayadev, are you still blocked on a solution to the problem discussed here? Did Enne's suggestion help? I can help debug if needed.
On 2017/05/16 20:25:43, chrishtr wrote: > On 2017/05/15 at 17:24:04, enne wrote: > > On 2017/05/13 at 00:56:51, jaydasika wrote: > > > On 2017/05/12 23:10:37, miu wrote: > > > > So, jaydasika, I'm not sure if our discussion helped you figure out what's > > > > causing the breakage in this change. But, it would seem that there are > > > > descendant layers (or Surfaces) that *do* have copy requests, but are not > > > > visible on-screen (possibly fully occluded?), and somehow this is not > being > > > > detected after your change. > > > > > > This change makes sure that if the descendant *layers* had copy requests, we > *do* send them to the compositor thread. But it has no knowledge on copy > requests on surfaces. So, the problem wrt this patch is hidden surfaces with > copy requests. I have verified that the offscreen tab capture tests *pass* when > I send (non-hidden layers + all surface layers) to the compositor thread. > > > > Hmm, that's a little confusing to me. Why would a surface layer need to be > sent in order for a tab capture to succeed? The surface layer itself is just a > surface id that generates a SurfaceDrawQuad. It smells a bit like the bug is > somewhere in the vicinity of SurfaceAggregator in that it can't make a copy > request of a surface if that surface is disconnected from the tree of compositor > frames, and that it needs the SurfaceDrawQuad to connect everything together. > > Jayadev, are you still blocked on a solution to the problem discussed here? Did > Enne's suggestion help? I can help debug if needed. Enne's suggestion helped me understand what/where the problem is, but I don't have a solution. If I am following this discussion correctly, the real solution here is make surface copy requests work even when the surface is disconnected. I don't know how much work is needed to do that. A hacky solution to land this without actually solving that is to push all non-hidden layers + all surface layers. Is the hacky solution ok for now ?
On 2017/05/17 at 00:57:30, jaydasika wrote: > On 2017/05/16 20:25:43, chrishtr wrote: > > Jayadev, are you still blocked on a solution to the problem discussed here? Did > > Enne's suggestion help? I can help debug if needed. > > Enne's suggestion helped me understand what/where the problem is, but I don't have a solution. If I am following this discussion correctly, the real solution here is make surface copy requests work even when the surface is disconnected. I don't know how much work is needed to do that. A hacky solution to land this without actually solving that is to push all non-hidden layers + all surface layers. Is the hacky solution ok for now ? I would rather fix this correctly, sorry. I don't think we should have to submit surface quads to get copy requests to work properly. As I said before, I think the problem is in SurfaceAggregator. It looks like the only place copy output requests are take off is when a surface quad is encountered and in CopyUndrawnPasses which looks at "referenced" surfaces. It looks like from comments that some CompositorFrame has to say it references this surface. Maybe the less hacky solution that uses existing infrastructure is to make sure that if surfaces are skipped we remember them and still stick them on the CompositorFrame metadata's referenced_surfaces list. (Although honestly, it seems like SurfaceManager should just figure this out and provide a list of surfaces with copy output requests to the aggregator itself.) +fsamuel who might have thoughts here, although this is maybe getting a bit far afield from this patch.
The CQ bit was checked by jaydasika@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...
Sending hidden surface layer ids also at commit/activation. Verified locally that the offscreen tab capture tests pass with that change. https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2846653002/diff/100001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1073: if (layer->is_hidden()) On 2017/05/02 01:46:01, enne wrote: > On 2017/05/01 at 23:41:39, jaydasika wrote: > > On 2017/05/01 20:15:09, enne wrote: > > > This is a little awkward, here. I think what you want to say is that if a > layer > > > is hidden, it shouldn't push properties. This says if a layer becomes > hidden, > > > then ignore additional push properties requests. I think those are not > quite > > > the same? > > > > > > And then the tree sychronizer redundantly fixes this up, so maybe this call > > > isn't needed here? > > > > Yes, what we want to say is if a layer is hidden, it shouldn't push > properties. > > > > The reason I added this here is for this sequence : hide layer-> commit -> > change a property on hidden layer -> commit. > > Even during second commit, we don't want to push properties of the layer. But > if the layer tree topology hasn't changed, we won't do full tree sync and never > hit the code in tree synchronizer that deletes hidden layers from push > properties layer list. > > > > I agree this is unnecessarily complex, maybe we should just check while > pushing properties if the layer is hidden and skip it there. > > That sounds good! Done.
On 2017/05/17 01:26:49, enne wrote: > On 2017/05/17 at 00:57:30, jaydasika wrote: > > On 2017/05/16 20:25:43, chrishtr wrote: > > > Jayadev, are you still blocked on a solution to the problem discussed here? > Did > > > Enne's suggestion help? I can help debug if needed. > > > > Enne's suggestion helped me understand what/where the problem is, but I don't > have a solution. If I am following this discussion correctly, the real solution > here is make surface copy requests work even when the surface is disconnected. I > don't know how much work is needed to do that. A hacky solution to land this > without actually solving that is to push all non-hidden layers + all surface > layers. Is the hacky solution ok for now ? > > I would rather fix this correctly, sorry. I don't think we should have to > submit surface quads to get copy requests to work properly. > > As I said before, I think the problem is in SurfaceAggregator. It looks like > the only place copy output requests are take off is when a surface quad is > encountered and in CopyUndrawnPasses which looks at "referenced" surfaces. It > looks like from comments that some CompositorFrame has to say it references this > surface. Maybe the less hacky solution that uses existing infrastructure is to > make sure that if surfaces are skipped we remember them and still stick them on > the CompositorFrame metadata's referenced_surfaces list. (Although honestly, it > seems like SurfaceManager should just figure this out and provide a list of > surfaces with copy output requests to the aggregator itself.) > > +fsamuel who might have thoughts here, although this is maybe getting a bit far > afield from this patch. We do have a long term plan to make sense of all of this and to build a nice coherent system but that's multiple months of work, I think. I think it makes sense to make this happen from a code health perspective but it is not necessarily a product-blocking thing. At the high level, rjkroege@ and I both dislike the notion of |referenced_surfaces| which seems disconnected from actual visuals: "here is what is on screen right now". |referenced_surfaces| basically says, "Hey display compositor, here are things that may or may not be visually present in this frame but I might need later, please keep them around for me. Thanks". |referenced_surfaces| are used for copy requests AND minimized/occluded windows (on Chrome OS). I'd like to get rid of |referenced_surfaces| in the long term, and make the compositor smart enough to deal with both of those cases correctly. 1. Capture display: If we would like to capture a particular window or tab then we simply put it on the capture cc::Display, and do the usual aggregation and surface draw occlusion and so on. The difficulty with this approach is we would like to make sure we don't do work twice across displays and so we need something that avoids doing composites twice. This is a problem for software mirroring across displays on Chrome OS too, I think, so doing these optimizations does have some benefits for Chrome OS. 2. Generalized Frame Eviction: If a surface isn't visually on display anywhere then we are under no obligation to keep its resources hanging out indefinitely. It should be up to the display compositing system (viz) when we can return resources to clients if they're not visually present, perhaps with hints from embedders. The idea here is the set of surfaces that don't contribute to a display frame (whether capture display or physical display) are "candidates for eviction". Some priority queue (with hints from the embedder along with a component of time and size and so on) decides when its OK to evict surfaces and return resources to clients. The complexity of this feature stems from the need to occasionally wait for a new frame to be generated from a child when a window moves or something. Once these two features are implemented then *I think* referenced_surfaces can go away. It'll be a while though, unfortunately. I don't really have a good shortcut to this. As a side note, currently, we ship |activation_dependencies| in addition to DrawQuads over IPC. As activation_dependencies are simply the set of all primary SurfaceDrawQuads, then we don't actually need to ship those things over IPC, we can construct that list on the other side of the IPC boundary. Thus, in principle, I think, long term, we won't need any vector of SurfaceIds in CompositorFrameMetadata.
Agreed in general that referenced_surfaces should probably go away. It just seemed like something that was in use already that this code could lean on in the medium term. This patch does add a dependency on referenced_surfaces, but I think if referenced_surfaces went away and SurfaceManager knew how to handle orphaned copy requests, then this code from this patch could all get deleted without any other complication. So, it adds a dependency, but I don't think introduces extra complication for its removal in the future.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_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 jaydasika@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2846653002/diff/260001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2846653002/diff/260001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:1229: if (!data_for_children.is_hidden && MaskLayer(layer)) { Maybe this should early out after the loop through the children? https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... File cc/trees/tree_synchronizer.cc (right): https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:40: tree_impl->ClearHiddenSurfaceLayerIds(); This seems like something that should happen as a part of layer tree push properties to and not inside of tree synchronizer? https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:117: if (!IsHidden(layer)) { I'm not sure if this needs to be done in this patch, but I think that https://cs.chromium.org/chromium/src/cc/trees/draw_property_utils.cc?l=808 and this likely need to converge at some point. It seems like anything that we're skipping for property tree updates probably also needs to not be pushed. (Similarly, do we need to push non-drawing layers at this point outside of tests?) I think this is fine for this patch, but could you leave a TODO for me to skip more layers here? https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:141: if (!IsHidden(layer)) { Maybe setting IsHidden should just SetNeedsCommit instead of SetNeedsPushProperties? It doesn't seem like the layer should have anything to push here. https://codereview.chromium.org/2846653002/diff/280001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/2846653002/diff/280001/cc/layers/layer.h#newc... cc/layers/layer.h:317: virtual bool IsSurfaceLayer() const; I'm not really that keen on virtuals that say what kind of layer a layer is, because then other code starts depending on it. I think I'd prefer a pattern that was either: (1) Have each surface layer register itself with LTH (like picture layers register with LayerTreeImpl) and then just iterate through all surface layers on commit to append surface ids. Don't worry about hidden vs not and just put all surface ids in the referenced list. (2) Have each surface layer register its surface id with LTH whenever it changes / gets a new LTH. Similarly don't worry about hidden vs not. https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1219: using SurfaceIdsList = std::vector<const SurfaceId*>; I think this should be SurfaceId and not SurfaceId*. Lifetimes seem a little bit scary here. Also in general, I think std::vector<const SurfaceId> isn't really much longer than SurfaceIdsList and is a lot clearer. If you don't mind avoiding the typedef, I'd appreciate it. https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1221: const SurfaceIdsList* hidden_surface_layer_ids) { Maybe pass by const ref here? https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1226: const SurfaceIdsList* LayerTreeImpl::HiddenSurfaceLayerIds() const { Maybe a const ref and not a pointer, if it's never going to be null?
The CQ bit was checked by jaydasika@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...
https://codereview.chromium.org/2846653002/diff/260001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2846653002/diff/260001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:1229: if (!data_for_children.is_hidden && MaskLayer(layer)) { On 2017/05/19 22:27:05, enne wrote: > Maybe this should early out after the loop through the children? Done. https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... File cc/trees/tree_synchronizer.cc (right): https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:40: tree_impl->ClearHiddenSurfaceLayerIds(); On 2017/05/19 22:27:05, enne wrote: > This seems like something that should happen as a part of layer tree push > properties to and not inside of tree synchronizer? Done. https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:117: if (!IsHidden(layer)) { On 2017/05/19 22:27:05, enne wrote: > I'm not sure if this needs to be done in this patch, but I think that > https://cs.chromium.org/chromium/src/cc/trees/draw_property_utils.cc?l=808 and > this likely need to converge at some point. It seems like anything that we're > skipping for property tree updates probably also needs to not be pushed. > > (Similarly, do we need to push non-drawing layers at this point outside of > tests?) > > I think this is fine for this patch, but could you leave a TODO for me to skip > more layers here? Acknowledged. https://codereview.chromium.org/2846653002/diff/260001/cc/trees/tree_synchron... cc/trees/tree_synchronizer.cc:141: if (!IsHidden(layer)) { On 2017/05/19 22:27:05, enne wrote: > Maybe setting IsHidden should just SetNeedsCommit instead of > SetNeedsPushProperties? It doesn't seem like the layer should have anything to > push here. I will remove SetNeedsPushProperties from Layer::SetIsHidden. But, I think this is still needed. If some layer property changes on a hidden layer, it will end up in the push_properties layer list https://codereview.chromium.org/2846653002/diff/280001/cc/layers/layer.h File cc/layers/layer.h (right): https://codereview.chromium.org/2846653002/diff/280001/cc/layers/layer.h#newc... cc/layers/layer.h:317: virtual bool IsSurfaceLayer() const; On 2017/05/19 22:27:05, enne wrote: > I'm not really that keen on virtuals that say what kind of layer a layer is, > because then other code starts depending on it. I think I'd prefer a pattern > that was either: > > (1) Have each surface layer register itself with LTH (like picture layers > register with LayerTreeImpl) and then just iterate through all surface layers on > commit to append surface ids. Don't worry about hidden vs not and just put all > surface ids in the referenced list. > > (2) Have each surface layer register its surface id with LTH whenever it changes > / gets a new LTH. Similarly don't worry about hidden vs not. (2) done https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1219: using SurfaceIdsList = std::vector<const SurfaceId*>; On 2017/05/19 22:27:05, enne wrote: > I think this should be SurfaceId and not SurfaceId*. Lifetimes seem a little > bit scary here. > > Also in general, I think std::vector<const SurfaceId> isn't really much longer > than SurfaceIdsList and is a lot clearer. If you don't mind avoiding the > typedef, I'd appreciate it. Done. https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1221: const SurfaceIdsList* hidden_surface_layer_ids) { On 2017/05/19 22:27:05, enne wrote: > Maybe pass by const ref here? Done. https://codereview.chromium.org/2846653002/diff/280001/cc/trees/layer_tree_im... cc/trees/layer_tree_impl.cc:1226: const SurfaceIdsList* LayerTreeImpl::HiddenSurfaceLayerIds() const { On 2017/05/19 22:27:05, enne wrote: > Maybe a const ref and not a pointer, if it's never going to be null? Done. https://codereview.chromium.org/2846653002/diff/340001/cc/layers/surface_laye... File cc/layers/surface_layer.cc (right): https://codereview.chromium.org/2846653002/diff/340001/cc/layers/surface_laye... cc/layers/surface_layer.cc:124: return nullptr; Not sure if this is correct / there is a simpler way to do this. I just copied this block from LTHI. https://codereview.chromium.org/2846653002/diff/340001/cc/trees/layer_tree_ho... File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/2846653002/diff/340001/cc/trees/layer_tree_ho... cc/trees/layer_tree_host.cc:1096: surface_layer_ids_.push_back(surface_id); Will add a DCHECK for checking surface id is not already in the list.
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 jaydasika@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_...)
There are too many changes here, splitting this CL, will first try to land https://codereview.chromium.org/2905533002/
The CQ bit was checked by jaydasika@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.
Addressed all the comments. With https://codereview.chromium.org/2905533002/ landed, this now passes all the tests.
https://codereview.chromium.org/2846653002/diff/400001/cc/trees/property_tree... File cc/trees/property_tree_builder.cc (right): https://codereview.chromium.org/2846653002/diff/400001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:1148: if (MaskLayer(layer)) Is this required? I worry that this is one more avenue for inconstency (like bounds), and would rather just have mask layers never have their hidden state queried or set anywhere. https://codereview.chromium.org/2846653002/diff/400001/cc/trees/property_tree... cc/trees/property_tree_builder.cc:1185: SetIsHidden(layer, data_for_children.is_hidden); I am not that keen on setting (more) data on layers during property tree building. I think I can understand building property trees setting property tree indexes, but this is like extra calculated data that gets set. It just makes the data flow (and understanding which data is read and which is write on layers) harder to understand. For instance, with this patch, somebody could call Layer::SetIsHidden(true) and then this function could come through and clobber it to be SetIsHidden(false), which would be quite confusing. I think maybe having an internal SetIsInHiddenSubtree function (named differently than IsHidden) that was protected/friended could help this. Alternatively, maybe the callers in ui could recursively call SetIsHidden and we could drop the SubtreeIsHidden API, although that's a larger change. |
