Bug 84122 - [Chromium] Clean up texture ids on the impl side when losing the context
Summary: [Chromium] Clean up texture ids on the impl side when losing the context
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Antoine Labour
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-16 19:43 PDT by Antoine Labour
Modified: 2012-04-17 17:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.59 KB, patch)
2012-04-16 19:46 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (11.31 KB, patch)
2012-04-16 21:59 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch for landing (11.31 KB, patch)
2012-04-16 22:06 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch for landing (11.31 KB, patch)
2012-04-17 16:58 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2012-04-16 19:43:03 PDT
[Chromium] Clean up texture ids on the impl side when losing the context
Comment 1 Antoine Labour 2012-04-16 19:46:54 PDT
Created attachment 137466 [details]
Patch
Comment 2 James Robinson 2012-04-16 19:52:20 PDT
Comment on attachment 137466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review

Looks like the correct code but needs tests to prevent regressions

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

this won't fly (will fail an SVN presubmit check). Seems like it shouldn't be too hard to construct a unit test for this - just magick up a CCLayerTreeHostImpl, stick an instance of each CCLayerImpl type touched here into the tree, synthesize a lost context, then recreate with a mock context that explodes if you try to bind a non-zero texture and do a draw pass
Comment 3 Antoine Labour 2012-04-16 19:59:25 PDT
Yep, sounds good - I was starting to look into how to best add a test, your suggestion sounds excellent.

Enne: could you give a look at the CCTiledLayerImpl to make sure I'm not doing anything incredibly stupid. In particular, my understanding is that we're ensured that m_tiler is != NULL when we get a lost context, but wanted to double-check on that.
Comment 4 Adrienne Walker 2012-04-16 20:09:45 PDT
Comment on attachment 137466 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137466&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:240
> +    m_tiler->reset();

I think if you lose the context prior to the first commit, m_tiler would be null.  We should probably just always create a tiler to not need these sorts of checks.
Comment 5 James Robinson 2012-04-16 20:12:14 PDT
The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption)
Comment 6 Adrienne Walker 2012-04-16 20:14:01 PDT
(In reply to comment #5)
> The CCTiledLayerImpl wouldn't exist in the impl tree until the first commit, and after the commit we always do a pushPropertiesTo() before trying to draw it. So I think it'll always be there by the time we can notice a lost context (although perhaps this is a fragile assumption)

Hmm, I'll buy that argument.  It still seems a little sketchy in general, but should be ok here.
Comment 7 Antoine Labour 2012-04-16 21:59:41 PDT
Created attachment 137476 [details]
Patch
Comment 8 James Robinson 2012-04-16 22:02:08 PDT
Comment on attachment 137476 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137476&action=review

R=me

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1157
> +// ressource that wasn't created by it (resources created by

typo ressource -> resource
Comment 9 Antoine Labour 2012-04-16 22:06:43 PDT
Created attachment 137478 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-04-16 22:34:55 PDT
Comment on attachment 137478 [details]
Patch for landing

Attachment 137478 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12419363
Comment 11 James Robinson 2012-04-17 10:28:56 PDT
Comment on attachment 137478 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=137478&action=review

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:1265
> +class FakeVideoFrameProvider: public VideoFrameProvider {

Looks like you collided with http://trac.webkit.org/changeset/114335 - you want to implement WebKit::WebVideoFrameProvider
Comment 12 Antoine Labour 2012-04-17 16:58:16 PDT
Created attachment 137639 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-04-17 17:53:00 PDT
Comment on attachment 137639 [details]
Patch for landing

Clearing flags on attachment: 137639

Committed r114473: <http://trac.webkit.org/changeset/114473>
Comment 14 WebKit Review Bot 2012-04-17 17:53:12 PDT
All reviewed patches have been landed.  Closing bug.