New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2012
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 134747
issue 134750

Blocking:
issue 125948
issue 129266

Restricted
  • Only users with EditIssue permission may comment.



Sign in to add a comment

LayerRendererChromium is not getting visibility messages in single threaded compositing mode.

Project Member Reported by mmo...@chromium.org, Jun 13 2012

Issue description

Somewhere between CCLayerTreeHost::setVisible and LayerRendererChromium::setVisible the message stops being sent across only in single threaded compositing mode.  This may be related to some changes to do a forced commit when being backgrounded.
 

Comment 1 by mmo...@chromium.org, Jun 13 2012

Assigned to myself for some initial investigation.

Comment 2 by mmo...@chromium.org, Jun 13 2012

CCSingleThreadProxy::setNeedsForcedCommit has the comment "This proxy doesn't block commits when not visible so use a normal commit." and essentially calls CCLayerTreeHost::scheduleComposite() which calls m_client->scheduleComposite().

At this point, we expect to get a commit, but are not, so the comment is wrong.  Has something changed wrt background commits?

Since we are only committing when in foreground CCLTHImpl setVisible is only getting setVisible(true) and early exiting before forwarding to LRC.

Comment 3 by danakj@chromium.org, Jun 13 2012

The proxy doesn't block it but I guess somewhere in RenderViewImpl or WebViewImpl it is getting blocked?

Comment 4 by mmo...@chromium.org, Jun 13 2012

Vangelis made this comment is email discussion: 

My fix for the single thread proxy was to change the implementation of setNeedsForcedCommit to :

void CCSingleThreadProxy::setNeedsForcedCommit()
{
    CCTextureUpdater updater;
    doCommit(updater);
}

Comment 5 by danakj@chromium.org, Jun 13 2012

Cc: enne@chromium.org nduca@chromium.org

Comment 6 by vangelis@google.com, Jun 13 2012

Cc: jam...@chromium.org
This will essentially do the synchronization part between the two threads but without any additional painting.  An alternative would be to create a new method that simply does the necessary texture management and set the visibility on the impl thread to make it more explicit.  

It looks like currently setNeedsForcedCommit() only gets called in response to texture allocation changes. 

Comment 7 by danakj@chromium.org, Jun 13 2012

> An alternative would be to create a new method that simply does the necessary texture management and set the visibility on the impl thread to make it more explicit.  

If I'm understanding right, that sounds like what we used to have, with extra communication channels between threads, and what we wanted to remove, to make all communication go through commits.

Can we scheduleComposite(forced = true) to the client and make it actually call composite() on the renderer?

Comment 8 by jam...@chromium.org, Jun 13 2012

Doing a composite when going invisible doesn't sound right.

Comment 9 by danakj@chromium.org, Jun 13 2012

Oh true. How can RenderViewImpl or whatever ask for a commit instead of a composite?

Should we add something to the WebView interface for this case? And a CCLTHClient::scheduleCommit() that doesn't block on visibility? And then call scheduleCommit();scheduleComposite() from CCSingleThreadProxy::setNeedsForcedCommit()?

Or do we have the proxy just do the commit as in the code in #4? That seemed odd to me since the client has been controlling these things? But I'm such a single-thread noobie.
The proxy is responsible for keeping the main thread and impl thread data structures in sync with each other, so I think it's fine for it to force a commit.  The RenderViewImpl/whatever has already done its job by setting the visibility, it doesn't need to do anything more.
Labels: -Pri-2 Pri-1 Mstone-21
Owner: jam...@chromium.org
Status: Assigned
 Issue 129266  has been merged into this issue.
Blocking: chromium:129266
Labels: releaseblock-stable Merge-Requested
http://trac.webkit.org/changeset/121076 should address this.  I believe we need to merge this into the m21 branch.  There are two (known) outstanding issues with it:

*)  bug 134747  is a tab switching bug in threaded mode
*)  bug 134750  is a tab switching performance issue with linux and windows XP in compositing mode
Blockedon: chromium:134747 chromium:134750
Cc: ccameron@chromium.org
http://trac.webkit.org/changeset/121450 fixes  bug 134747 .  134750 is a nice-to-have.

Karen - requesting merge for http://trac.webkit.org/changeset/121076 and http://trac.webkit.org/changeset/121450 to m21 branch, when it's ready.

Comment 19 by kareng@google.com, Jun 29 2012

these cLs look really big for post-branch merge :(
http://trac.webkit.org/changeset/121450 is a one-liner with some extra instrumentation.  http://trac.webkit.org/browser/trunk/Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp?rev=121450#L490 is the only change in code.

'076 is big, but it's been pretty solid on canary so far.  I personally think the bug is worse than the merge risk.

Comment 21 by kareng@google.com, Jul 9 2012

ok goes to dev today so we'll watch.

Comment 22 by kareng@google.com, Jul 10 2012

Labels: -Merge-Requested Merge-Approved
okie we'll give these a shot and see what they do. dana said he'll land. ty dana! 

Comment 23 by kareng@google.com, Jul 10 2012

ahem SHE will land :)
The changes in CCThreadProxy depend on http://trac.webkit.org/changeset/120858

Comment 25 by enne@chromium.org, Jul 10 2012

r120858 seems pretty safe to me.  I think I'd be more worried about less tested code trying to work around it not being there.
That's how I'm feeling as I tried to do something without merging it first.. tyty
Labels: -Merge-Approved Merge-Merged
Merged the 3

Comment 28 by kareng@google.com, Jul 11 2012

Status: Fixed
marking fixed so we can verfiy.
Blocking: chromium:125948
Project Member

Comment 30 by bugdroid1@chromium.org, Oct 13 2012

Labels: Restrict-AddIssueComment-Commit
This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.
Project Member

Comment 31 by bugdroid1@chromium.org, Mar 10 2013

Labels: -Area-Internals -Feature-GPU-Compositing -Mstone-21 Cr-Internals-GPU-Compositing M-21 Cr-Internals
Project Member

Comment 32 by bugdroid1@chromium.org, Mar 14 2013

Labels: -Restrict-AddIssueComment-Commit Restrict-AddIssueComment-EditIssue
Project Member

Comment 33 by bugdroid1@chromium.org, Apr 5 2013

Labels: -Cr-Internals-GPU-Compositing Cr-Internals-Compositing

Sign in to add a comment