Bug 86054 – Sync with impl thread when removing references to external textures

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Labour]

Sync with impl thread when removing references to external textures

[Antoine Labour]

Created attachment 141075 [details]
Patch

[Antoine Labour]

Do you have any good idea for testing this?

[WebKit Review Bot]

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.

[Dana Jansens]

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/

[Dana Jansens]

Does acquireTextures block all drawing until a commit completes?

[Dana Jansens]

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?

[Antoine Labour]

Created attachment 141228 [details]
Patch

[Antoine Labour]

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.

[Antoine Labour]

(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).

[WebKit Review Bot]

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.

[Dana Jansens]

(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 :)

[James Robinson]

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

[James Robinson]

(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).

[Antoine Labour]

Created attachment 141285 [details]
Patch

[Antoine Labour]

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.

[James Robinson]

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

[Antoine Labour]

Created attachment 141286 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 141286 [details]
Patch for landing

Clearing flags on attachment: 141286

Committed r116722: <http://trac.webkit.org/changeset/116722>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.