cc: Redraw incomplete frames when new texture uploads finish

cc: Redraw incomplete frames when new texture uploads finish

This patch makes sure that, when the displayed frame has
checkerboarding or uses low resolution tiles, we redraw on
a vsync when a new visible high-res tile has been uploaded.

Checking for completed texture uploads is given it's own
scheduler state to centralize control and also to prevent
texture uploads from triggering duplicate frames.

BUG=169603

[nduca, 2013-01-12 09:14:02]

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc#newcode243
cc/tile_manager.cc:243: visible_highres_tile_was_completed = true;
why not just trigger a setNeedsRedraw here? In this case, I think its okay for
the TileManager to just call m_client->didCompleteTileOnActiveTree() and then
have lthi convert it into setNeedsRedraw.

The distinction between this and the scheduledAction patch you just did may seem
a bit obscure to you: in this case, we aren't needing special behavior on the
redraw, as in it doesn't need to be combiend with other states. So it doesn't
need to push down to the rest of the machine.

[brianderson, 2013-01-12 18:48:06]

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc#newcode243
cc/tile_manager.cc:243: visible_highres_tile_was_completed = true;
On 2013/01/12 09:14:02, nduca wrote:
> why not just trigger a setNeedsRedraw here? In this case, I think its okay for
> the TileManager to just call m_client->didCompleteTileOnActiveTree() and then
> have lthi convert it into setNeedsRedraw.
> 
> The distinction between this and the scheduledAction patch you just did may
seem
> a bit obscure to you: in this case, we aren't needing special behavior on the
> redraw, as in it doesn't need to be combiend with other states. So it doesn't
> need to push down to the rest of the machine.

I did it this way for two reasons:

1) After the decoupling patch goes through, triggering setNeedsRedraw directly
from here will cause spurious draws.  This function will be called within
scheduledActionDrawAndSwap*, after the setNeedsRedraw has been cleared but
before we actually draw.  So it's possible the upcoming draw is complete
(because all the uploads just finished) and we needlessly setNeedsRedraw for the
next vsync here.

2) If we are in the ACTION_CHECK_FOR_NEW_VISIBLE_TEXTURES state and this
function returns false, the caller can decide to stay in that state without
doing another tree traversal.

[nduca, 2013-01-12 22:04:40]

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc#newcode243
cc/tile_manager.cc:243: visible_highres_tile_was_completed = true;
But look at this function. At this specific line, inside the active-tree,
distance to visible = 0, resolution=high, we KNOW its onscreen. As in, ITS ON
SCREEN. **It would be drawn**

How on earth can the draw be complete yet us get this condition?

[brianderson, 2013-01-14 18:51:12]

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (right):

https://codereview.chromium.org/11879012/diff/1/cc/tile_manager.cc#newcode243
cc/tile_manager.cc:243: visible_highres_tile_was_completed = true;
On 2013/01/12 22:04:40, nduca wrote:
> But look at this function. At this specific line, inside the active-tree,
> distance to visible = 0, resolution=high, we KNOW its onscreen. As in, ITS ON
> SCREEN. **It would be drawn**
> 
> How on earth can the draw be complete yet us get this condition?


The problem is that there can be a missmatch between the state machine's state
and the actual state when we are in this function during an
ACTION_DRAW_IF_POSSIBLE.

SchedulerStateMachine::updateState is called before an action is taken, which
for ACTION_DRAW_IF_POSSIBLE means that m_needsRedraw is set to false before
we've actually drawn the current frame.

Currently, CheckForCompletedSetPixels is always called right before drawing to
make sure we have the most up-to-date information. If CheckForCompletedSetPixels
triggers a setNeedsRedraw directly, it's possible that the frame we are about to
draw for the current vsync is complete and we are just telling the state machine
to redraw an identical frame on the next vsync.

I will admit this is not very clear and that part of the confusion comes from
the way I have the decoupling patch always call CheckForCompletedSetPixels
before drawing. There are a few options we can take to make things clearer:

1) Keep the patch as-is and add a fat comment.
2) Add an explicit state/action that checks for completed pixels before drawing
and remove the check for completed pixels in the draw action.
3) Call SchedulerStateMachine::updateState after taking an action (instead of
before) so m_needsRedraw isn't cleared until after we've actually drawn.  Will
probably have side-effects that affect a lot of the state machine code.

Although I think 2 will be the most verbose, I think it will also be the
clearest and most explicit.  I plan to implement 2, unless anyone has a better
solution.

[nduca, 2013-01-14 20:52:43]

2 sounds good. 3 seems too painful. THanks for the long explanation, I think
maybe I get it. :)

[brianderson, 2013-01-16 01:49:05]

ptal

[nduca, 2013-01-16 04:09:47]

Now i'm more confused. I think we may want to discuss this tomorrow?

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:717: m_client->setNeedsRedrawOnImplThread();
see now i dont get why this one does setNeedsRedraw but the other one does
setNeedsRedrawOnBlahBlahBlah

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:845: if (!activeTree()->AreVisibleResourcesReady()) {
I'm confused, why are we checking this again here?

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:50: virtual void
setNeedsRedrawOnVisibleTextureUploadOnImplThread() = 0;
onVisibleHighResolutionTileSomethingOrOther? That this triggers redraw/etc is
really a proxy-side decision.

[brianderson, 2013-01-16 20:12:22]

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:717: m_client->setNeedsRedrawOnImplThread();
On 2013/01/16 04:09:47, nduca wrote:
> see now i dont get why this one does setNeedsRedraw but the other one does
> setNeedsRedrawOnBlahBlahBlah

In this case, we know we have a better frame to draw.  In the other case, we
don't know if we have a better frame to draw until the next vsync (so it
indicates we will need an eventual redraw instead of an immediate one).  The
scheduler uses this info to tick without necessarily drawing.

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.cc...
cc/layer_tree_host_impl.cc:845: if (!activeTree()->AreVisibleResourcesReady()) {
On 2013/01/16 04:09:47, nduca wrote:
> I'm confused, why are we checking this again here?

This checks if the active tree just drawn was incomplete (checkered or low-res).
 If it was incomplete it lets the proxy know that it needs to redraw when it can
draw a more complete frame.

Maybe a better name for setNeedsRedrawOnVisibleTextureUploadOnImplThread would
be setSwapUsedIncompleteTexturesOnImplThread.

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.h
File cc/layer_tree_host_impl.h (right):

https://codereview.chromium.org/11879012/diff/7012/cc/layer_tree_host_impl.h#...
cc/layer_tree_host_impl.h:50: virtual void
setNeedsRedrawOnVisibleTextureUploadOnImplThread() = 0;
On 2013/01/16 04:09:47, nduca wrote:
> onVisibleHighResolutionTileSomethingOrOther? That this triggers redraw/etc is
> really a proxy-side decision.

I'll move the decision to draw to the proxy.

[nduca, 2013-01-16 23:04:32]

I like the name you proposed for the client interface. That actually really
helps me grok this. didSwapUse maybe?

[nduca, 2013-01-17 22:05:18]

lgtm with enevitable nits

https://codereview.chromium.org/11879012/diff/16002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11879012/diff/16002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:938: {
Please add a member bool inside_draw_ and then do dcheck on it, then change this
comment to be a comment above the decheck or even better DCHECK(!inside_draw_)
<< "explanation"

https://codereview.chromium.org/11879012/diff/16002/cc/scheduler.h
File cc/scheduler.h (right):

https://codereview.chromium.org/11879012/diff/16002/cc/scheduler.h#newcode42
cc/scheduler.h:42: virtual void scheduledActionCheckForNewTextures() = 0;
We're using different words throughout.

CheckForCompletedTextures and use it throughout the stack?

[brianderson, 2013-01-18 00:08:17]

Comments addressed. Going ahead with commit.

https://codereview.chromium.org/11879012/diff/16002/cc/layer_tree_host_impl.cc
File cc/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/11879012/diff/16002/cc/layer_tree_host_impl.c...
cc/layer_tree_host_impl.cc:938: {
On 2013/01/17 22:05:18, nduca wrote:
> Please add a member bool inside_draw_ and then do dcheck on it, then change
this
> comment to be a comment above the decheck or even better DCHECK(!inside_draw_)
> << "explanation"

Decided to add and use LayerTreeHostImplClient::isInsideDraw because the proxies
knows best if we are inside a draw.

[commit-bot: I haz the power, 2013-01-18 00:10:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11879012/21001

[commit-bot: I haz the power, 2013-01-18 00:30:57]

Failed to trigger a try job on android_clang_dbg
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-01-18 00:31:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11879012/26002

[commit-bot: I haz the power, 2013-01-18 02:50:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/brianderson@chromium.org/11879012/26002

[commit-bot: I haz the power, 2013-01-18 03:15:22]

Couldn't make sense out of svn commit message:
Sending        cc/layer_tree_host_impl.cc
Sending        cc/layer_tree_host_impl.h
Sending        cc/layer_tree_host_impl_unittest.cc
Sending        cc/scheduler.cc
Sending        cc/scheduler.h
Sending        cc/scheduler_state_machine.cc
Sending        cc/scheduler_state_machine.h
Sending        cc/scheduler_unittest.cc
Sending        cc/single_thread_proxy.cc
Sending        cc/single_thread_proxy.h
Sending        cc/test/fake_layer_tree_host_impl_client.cc
Sending        cc/test/fake_layer_tree_host_impl_client.h
Sending        cc/test/fake_tile_manager_client.h
Sending        cc/thread_proxy.cc
Sending        cc/thread_proxy.h
Sending        cc/tile_manager.cc
Sending        cc/tile_manager.h
Transmitting file data .................
Committed revision 177580.

Warning: post-commit hook failed (exit code 2) with output:
/opt/shared_hooks/post-commit.d/10_mailer.py:137: DeprecationWarning: The popen2
module is deprecated.  Use the subprocess module.
  import popen2
Post-commit mail timed out for r177580. Your commit succeeded,
but no mail was sent.