Bug 76668 - [chromium] Connect damage tracker properly to top-level events such as tab switch, readback
Summary: [chromium] Connect damage tracker properly to top-level events such as tab sw...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
URL:
Keywords:
Depends on: 76805 80199
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-19 15:22 PST by Shawn Singh
Modified: 2012-09-17 10:12 PDT (History)
4 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Shawn Singh 2012-01-19 15:22:43 PST
There are a number of top-level events that should force the screen to redraw, even if nothing has changed in the layer tree.  In these cases, we have to tell the damage tracker that the entire screen should be considered as damaged.  As per discussion with Nat and James, each case should be handled as closely as possible to the impl thread, and ideally they should be addressed without forcing everyone to be aware of the damage tracker in their future changes.

(1) composite and readback: this occurs for example when capturing thumbnails.  It should be caught in the proxy, and an extra boolean, forceRedrawEverything, should be added to CCLayerTreeHostImpl::setNeedsRedraw()

(2) zoom:  (And, if there are any other transforms that occur outside of all layers.)  This case can be handled inside of the CCLayerTreeHostImpl.

(3) tab switches / buffer lost:  Tab switches should only need to force damage if the tab's front buffer is gone.  This case can be handled inside of the CCLayerTreeHostImpl / LayerRendererChromium.

(4) context lost: This case can also be handled inside of the CCLayerTreeHostImpl

(5) resize: technically resize will affect layout and layers, so this damage should already be tracked properly.

(6) damage from an overlapping external window, on a non-composited window manager:  this case needs to be handled by render_widget and CCLayerTreeHost main thread; this will be addressed by a separate patch at a later time.


This bug will attempt to cover (1) - (4), hopefully it will be straightforward...
Comment 1 Nat Duca 2012-01-19 15:29:39 PST
(In reply to comment #0)
> (1) composite and readback: this occurs for example when capturing thumbnails.  It should be caught in the proxy, and an extra boolean, forceRedrawEverything, should be added to CCLayerTreeHostImpl::setNeedsRedraw()

Why? It seems to me that the compositeAndReadback implementation should just take care to tell the cclayertreehostimpl that the framebuffer is damaged. Single thread and thread proxy will handle this differently, but setNeedsDRaw isn't the right way for this specific case.


> (2) zoom:  (And, if there are any other transforms that occur outside of all layers.)  This case can be handled inside of the CCLayerTreeHostImpl.

Ideally, the damage tracker is simply away of the page transform when it last captured things and considers that changing, no?


> (3) tab switches / buffer lost:  Tab switches should only need to force damage if the tab's front buffer is gone.  This case can be handled inside of the CCLayerTreeHostImpl / LayerRendererChromium.
Yes.

> (4) context lost: This case can also be handled inside of the CCLayerTreeHostImpl
The LayerRendererChromium is deleted on context lost and a new one created. When we discussed this ages ago, the damage tracker was attached to the rendersurface [the persisted resource]... ideally, things should just work when that disappears.

> (5) resize: technically resize will affect layout and layers, so this damage should already be tracked properly.
Lets make sure thats the case.

> (6) damage from an overlapping external window, on a non-composited window manager:  this case needs to be handled by render_widget and CCLayerTreeHost main thread; this will be addressed by a separate patch at a later time.
This is the case where CCLayerTreeHost::setNeedsRedraw needs to imply a m_layerTreeHostImpl->layerRenderer()->...->assumeFrameBuffferIsGone(). Doing that requires modification of the thread proxy because you have to distinguish between redraws requested by the main thread and redraws requested by animation.


These feel like different patches to me.
Comment 2 Shawn Singh 2012-01-19 15:44:47 PST
(In reply to comment #1)

Thanks for the reply =)  No arguments with anything you said... I'll try all of that and see what happens.
Comment 3 Jonathan Backer 2012-01-20 09:06:42 PST
(In reply to comment #1)
> (In reply to comment #0)
> > (1) composite and readback: this occurs for example when capturing thumbnails.  It should be caught in the proxy, and an extra boolean, forceRedrawEverything, should be added to CCLayerTreeHostImpl::setNeedsRedraw()

So I'm having trouble reproducing the problem with the thumbnails. I've tried to reproduce as shawnsingh@ described offline (I'm using --force-compositing-mode --enable-partial-swap --disable-plugins --show-fps-counter on OSX and linux). Moreover, I just can't see why this would be a problem. On OSX the pixels persist in a texture that we should always be able to read from. I think I found a fix for a small bug here (codereview.chromium.org/9226018). shawnsingh@: could you try that patch?
Comment 4 Shawn Singh 2012-01-22 20:50:23 PST
Proposed fix for tab switch portion: https://bugs.webkit.org/show_bug.cgi?id=76805
Comment 5 Shawn Singh 2012-04-13 23:26:36 PDT
I think this bug is no longer applicable, thanks to efforts of many people.  To my knowledge, all the necessary events are now propagating their information as needed.  Is there any reason to keep this bug open?
Comment 6 Nat Duca 2012-04-16 23:18:53 PDT
Agreed. Good catch shawn. Kill it.

With fire.
Comment 7 Shawn Singh 2012-04-17 09:33:21 PDT
Fixed thanks to many other bugs contributed by many people.