Summary: | Sync with impl thread when removing references to external textures | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Labour <piman> | ||||||||||
Component: | New Bugs | Assignee: | 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
Antoine Labour
2012-05-09 20:24:35 PDT
Created attachment 141075 [details]
Patch
Do you have any good idea for testing this? 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 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/ Does acquireTextures block all drawing until a commit completes? 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? Created attachment 141228 [details]
Patch
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. (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). 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.
(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 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 (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). Created attachment 141285 [details]
Patch
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 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 Created attachment 141286 [details]
Patch for landing
Comment on attachment 141286 [details] Patch for landing Clearing flags on attachment: 141286 Committed r116722: <http://trac.webkit.org/changeset/116722> All reviewed patches have been landed. Closing bug. |