Bug 86054

Summary: Sync with impl thread when removing references to external textures
Product: WebKit Reporter: Antoine Labour <piman>
Component: New BugsAssignee: Antoine Labour <piman>
Status: RESOLVED FIXED    
Severity: Normal CC: cc-bugs, danakj, jamesr, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Antoine Labour 2012-05-09 20:24:35 PDT
Sync with impl thread when removing references to external textures
Comment 1 Antoine Labour 2012-05-09 20:26:34 PDT
Created attachment 141075 [details]
Patch
Comment 2 Antoine Labour 2012-05-09 20:28:40 PDT
Do you have any good idea for testing this?
Comment 3 WebKit Review Bot 2012-05-09 20:30:39 PDT
Attachment 141075 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 6 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dana Jansens 2012-05-10 09:49:44 PDT
Comment on attachment 141075 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +				- the layer it removed from the tree (and we had a texture)
> 
> Line contains tab character.  [whitespace/tab] [5]

s/it/is/
Comment 5 Dana Jansens 2012-05-10 09:52:58 PDT
Does acquireTextures block all drawing until a commit completes?
Comment 6 Dana Jansens 2012-05-10 09:55:22 PDT
Comment on attachment 141075 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:98
> +    if (m_textureId && m_textureId != id && layerTreeHost())

Should this just early out if m_textureId == id?
Comment 7 Antoine Labour 2012-05-10 12:41:20 PDT
Created attachment 141228 [details]
Patch
Comment 8 Antoine Labour 2012-05-10 12:42:25 PDT
Comment on attachment 141075 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +				- the layer it removed from the tree (and we had a texture)
>> 
>> Line contains tab character.  [whitespace/tab] [5]
> 
> s/it/is/

Done.

>> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.cpp:98
>> +    if (m_textureId && m_textureId != id && layerTreeHost())
> 
> Should this just early out if m_textureId == id?

Done.
Comment 9 Antoine Labour 2012-05-10 12:43:05 PDT
(In reply to comment #5)
> Does acquireTextures block all drawing until a commit completes?

Yes, that is my understanding (the approach was suggested by James).
Comment 10 WebKit Review Bot 2012-05-10 12:45:39 PDT
Attachment 141228 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:14:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Dana Jansens 2012-05-10 12:46:40 PDT
(In reply to comment #9)
> (In reply to comment #5)
> > Does acquireTextures block all drawing until a commit completes?
> 
> Yes, that is my understanding (the approach was suggested by James).

Ok, I never noticed this function before, and was curious :)
Comment 12 James Robinson 2012-05-10 14:15:18 PDT
Comment on attachment 141228 [details]
Patch

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

For tests, you could make CCLayerTreeHost::acquireLayerTextures() virtual and then make a test that construct a TextureLayerChromium with a MockCCLayerTreeHost and mock out that call.

Change looks good other than that - although I think I might prefer that the acquireLayerTexture on shutdown comes from an explicit setTextureId(0) call rather than the d'tor since TextureLayerChromium is RefCounted so we might not always know exactly when it's going down.

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:82
> +    virtual void setLayerTreeHost(CCLayerTreeHost*);

OVERRIDE please
Comment 13 James Robinson 2012-05-10 14:16:13 PDT
(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #5)
> > > Does acquireTextures block all drawing until a commit completes?
> > 
> > Yes, that is my understanding (the approach was suggested by James).
> 
> Ok, I never noticed this function before, and was curious :)

FYI it's relatively new - from http://trac.webkit.org/changeset/115355.  The idea is it's what you use before manipulating any resource being used as a front texture by the compositor (single-buffered canvas, for example).
Comment 14 Antoine Labour 2012-05-10 15:56:51 PDT
Created attachment 141285 [details]
Patch
Comment 15 Antoine Labour 2012-05-10 15:57:55 PDT
Added test. I don't like it that much because it tests the current implementation instead of the desired behavior, but it's better than nothing.
Comment 16 James Robinson 2012-05-10 16:03:54 PDT
Comment on attachment 141285 [details]
Patch

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

I feel somewhat the same way about the test. Maybe the behavior we're going for here is really calls on the context - making sure we have sufficient synchronization between the compositor's bind calls and destruction on the main thread.  Not sure how to do that reliably, though, and this seems like a reasonable proxy

> Source/WebCore/platform/graphics/chromium/TextureLayerChromium.h:82
> +    virtual void setLayerTreeHost(CCLayerTreeHost*);

OVERRIDE please
Comment 17 Antoine Labour 2012-05-10 16:08:19 PDT
Created attachment 141286 [details]
Patch for landing
Comment 18 WebKit Review Bot 2012-05-10 20:06:25 PDT
Comment on attachment 141286 [details]
Patch for landing

Clearing flags on attachment: 141286

Committed r116722: <http://trac.webkit.org/changeset/116722>
Comment 19 WebKit Review Bot 2012-05-10 20:06:31 PDT
All reviewed patches have been landed.  Closing bug.