WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
84122
[Chromium] Clean up texture ids on the impl side when losing the context
https://bugs.webkit.org/show_bug.cgi?id=84122
Summary
[Chromium] Clean up texture ids on the impl side when losing the context
Antoine Labour
Reported
2012-04-16 19:43:03 PDT
[Chromium] Clean up texture ids on the impl side when losing the context
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
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Antoine Labour
Comment 1
2012-04-16 19:46:54 PDT
Created
attachment 137466
[details]
Patch
James Robinson
Comment 2
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
Antoine Labour
Comment 3
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.
Adrienne Walker
Comment 4
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.
James Robinson
Comment 5
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)
Adrienne Walker
Comment 6
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.
Antoine Labour
Comment 7
2012-04-16 21:59:41 PDT
Created
attachment 137476
[details]
Patch
James Robinson
Comment 8
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
Antoine Labour
Comment 9
2012-04-16 22:06:43 PDT
Created
attachment 137478
[details]
Patch for landing
WebKit Review Bot
Comment 10
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
James Robinson
Comment 11
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
Antoine Labour
Comment 12
2012-04-17 16:58:16 PDT
Created
attachment 137639
[details]
Patch for landing
WebKit Review Bot
Comment 13
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
>
WebKit Review Bot
Comment 14
2012-04-17 17:53:12 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug