Browser LTHI should not always swap |
||||||
Issue descriptionCurrently, the LTHI for the browser will never take the "frame->has_no_damage" early out in DrawLayers, due to CompositorFrameSink::Capabilities::must_always_swap being true. This is inefficient, and should be avoided. Unfortunately, removing this swap currently causes TabCapture to break in the offscreen tab case. From what I understand: 1) Extensions can create an "offscreen tab". This tab produces content and is swapped via DelegatedFrameHost. 2) Once swapped, the tab's surface will become part of the next frame's "referenced_surfaces". 3) While not drawn directly, the surface can have copy requests added. These will be processed by SurfaceAggregator when it handles copy requests on undrawn surfaces. Unfortunately, removing the "always swap" behavior breaks step #2, as the new surface does not contribute to the visual output of the page, and therefore its addition doesn't cause LTHI to swap a new frame that includes it. (note that any visible damage will cause LTHI to put up a new frame, and this new frame will contain the offscreen surface). To fix this, we likely need something in LTHI to say "If a new surface is present that wasn't present in the previous frame, swap even if there's no damage" - this will ensure that new surfaces are available for the aggregator to process copy requests from. That may be too broad of a solution for the renderer LTHI, so we may want to restrict this behavior to the browser LTHI.
,
Jan 11 2017
That's what I said as well. I think the complication comes that the tab capture adds the copy request in "will draw" for the surface. The complication with moving that is that it rate limits (with some complicated oracle) about when it wants to capture. Were you able to add a copy request in DFH the first time a surface is seen?
,
Jan 11 2017
Currently, the SurfaceAggregator only knows about surfaces which are in the current Frame (as created by LTHI) - these are either drawn surfaces, or undrawn surfaces that are referenced in Frame::metadata::referenced_surfaces. Without an LTHI frame update, the aggregator never enumerates the surface (drawn or undrawn) and sees the copy request. An alternative which might work better would be to have SurfaceAggregator consider all surfaces (whether or not they're in the frame) when looking for copy requests? It seems like this should be possible, the SurfaceManager has references to all of them, and the aggregator has a reference to the SurfaceManager.
,
Jan 11 2017
Yah that sounds reasonable. The difference is that only surfaces referenced by the latest commit do copyrequest right now, whereas what you propose could find more surfaces. But it seems okay?
,
Feb 13 2018
,
Feb 13 2018
,
Jun 13 2018
ericrk@, have you begun on this ? I can have a look, if it helps.
,
Jul 13
ericrk: I changed LTHI::HasDamage() so that if |referenced_surfaces| changed then we'll have damage and swap in https://crrev.com/c/1114066. That fixes what you describe being the problem at step 2. I think that means the browser LTHI shouldn't need to set |must_always_swap| as true anymore?
,
Jul 13
,
Jul 20
I believe this is now resolved after crrev.com/c/1136804?
,
Jul 24
I forgot to also include this crbug in https://crrev.com/c/1136804 but yes it's fixed. |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by danakj@chromium.org
, Jan 11 2017