Summary: | [chromium] Accelerated canvas breaks when moving canvases or resources between Pages | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Robinson <jamesr> | ||||||||||||||||
Component: | New Bugs | Assignee: | James Robinson <jamesr> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apatrick, dglazkov, gustavo, kbr, senorblanco, tomhudson, vangelis, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
James Robinson
2011-07-29 17:10:55 PDT
Created attachment 102411 [details]
Patch
This depends on http://codereview.chromium.org/7488069/, which hasn't landed yet, so the EWS bots will be grumpy. Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9269559 Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9268565 Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9266639 Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9267608 Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9269566 Comment on attachment 102411 [details] Patch Attachment 102411 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9268585 Created attachment 102587 [details]
updated to build on other platforms
Comment on attachment 102587 [details] updated to build on other platforms Attachment 102587 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9283717 Comment on attachment 102587 [details] updated to build on other platforms Attachment 102587 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9288618 (In reply to comment #11) > (From update of attachment 102587 [details]) > Attachment 102587 [details] did not pass cr-mac-ews (chromium): > Output: http://queues.webkit.org/results/9288618 I'm guessing this is a two-sided patch? Created attachment 102708 [details]
Patch
Comment on attachment 102708 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102708&action=review > Source/WebCore/ChangeLog:10 > + pages and directly draw into contexts in different pages. Also switches DrawingBufferChromium over to use a > + different extension for resource sharing with the compositor. I think this description might be stale now: we don't use an extension at all, AFAICT, just rely on shared resources. > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.cpp:74 > + if (m_textureChanged) > + m_textureId = m_drawingBuffer->platformColorBuffer(); Looks like we never set m_textureChanged to false now. Probably OK, since presumably the texture ID never changes. But do we still need these member variables at all, actually? Can we just return m_drawingBuffer->platformColorBuffer() at the call site? Or at least just use m_textureId 0 to indicate not-set-yet? Whoops, I didn't mean to remove m_textureChanged = false. But you're right, we can just always get the current DrawingBuffer's color attachment. I think we still need the textureChanged stuff for WebGL but I can limit it to there. Created attachment 102957 [details]
Patch
Comment on attachment 102957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102957&action=review > Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.cpp:45 > , m_textureId(0) Could this also be pushed down into WebGLLayerChromium, now that Canvas2DLayerChromium doesn't use it? > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.h:27 > +#define SharedGraphicsContext3D_h Ahh, the triumphant return of SharedGraphicsContext3D. (My, you've lost a lot of weight.) Comment on attachment 102957 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102957&action=review > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:54 > + s_sharedContextCount++; It'd be nice if you didn't have to roll your own refcount. I think it could be done as follows: - derived SharedGraphicsContext3D from RefCounted - replace s_sharedContext with s_instance, a SharedGraphicsContext3D bare ptr - make (static) ShareGraphicsContext3D::instance() create s_instance if it's NULL, and return a RefPtr wrapped around the s_instance bare ptr - make the SharedGraphicsContext3D constructor create a GraphicsContext3D (m_context) - make the SharedGraphicsContext3D destructor set s_instance to NULL Then when the last reference to SGC3D drops, the instance will be set to NULL. Would that work? Created attachment 102986 [details]
Patch
Thanks, I think that does work out much better. Not marking review? as this triggers a bug in the command buffer, so right now this will make a bunch of canvas tests explode magnificently. I'll flip review? as soon as the chromium side is fixed+rolled. Comment on attachment 102986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102986&action=review > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:117 > if (m_grContext) > m_grContext->flush(0); Even though we're no longer doing the copy, I'm wondering if still need to make the context current here. GrContext::flush() probably expects its context to be current. (Sorry I didn't notice this in earlier passes). > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:39 > + s_instance->ref(); // Manually give this caller a reference to the shared instance, adoptRef() does not increase the refcount. > + return adoptRef(s_instance); Maybe I'm being dense, but couldn't this use the same code as below? As in: if (s_instance) { RefPtr<SharedGraphicsContext3D> sharedContext = adoptRef(s_instance); return sharedContext; // turning RefPtr into PassRefPtr. } It would be a little more verbose, but you'd avoid manually ref'ing (letting the refptr ref for you), and you might be able to refactor it a bit with the else case. If it doesn't work, feel free to ignore me. (In reply to comment #21) > (From update of attachment 102986 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102986&action=review > > > Source/WebCore/platform/graphics/chromium/DrawingBufferChromium.cpp:117 > > if (m_grContext) > > m_grContext->flush(0); > > Even though we're no longer doing the copy, I'm wondering if still need to make the context current here. GrContext::flush() probably expects its context to be current. (Sorry I didn't notice this in earlier passes). Ooh good catch, it probably does require the context to be current. I'll fix that. > > > Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:39 > > + s_instance->ref(); // Manually give this caller a reference to the shared instance, adoptRef() does not increase the refcount. > > + return adoptRef(s_instance); > > Maybe I'm being dense, but couldn't this use the same code as below? As in: > > if (s_instance) { > RefPtr<SharedGraphicsContext3D> sharedContext = adoptRef(s_instance); > return sharedContext; // turning RefPtr into PassRefPtr. > } > > It would be a little more verbose, but you'd avoid manually ref'ing (letting the refptr ref for you), and you might be able to refactor it a bit with the else case. If it doesn't work, feel free to ignore me. Yeah, I wrote it this way as well and it also works. I think it's a little less clear, though, that the purpose of the little dance is to increase the refcount on s_instance by one before returning which is why I wrote it out (with a comment, gasp). I'm happy to change it if you think the local RefPtr<> way is easier to understand. Comment on attachment 102986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102986&action=review >>> Source/WebCore/platform/graphics/gpu/SharedGraphicsContext3D.cpp:39 >>> + return adoptRef(s_instance); >> >> Maybe I'm being dense, but couldn't this use the same code as below? As in: >> >> if (s_instance) { >> RefPtr<SharedGraphicsContext3D> sharedContext = adoptRef(s_instance); >> return sharedContext; // turning RefPtr into PassRefPtr. >> } >> >> It would be a little more verbose, but you'd avoid manually ref'ing (letting the refptr ref for you), and you might be able to refactor it a bit with the else case. If it doesn't work, feel free to ignore me. > > Yeah, I wrote it this way as well and it also works. I think it's a little less clear, though, that the purpose of the little dance is to increase the refcount on s_instance by one before returning which is why I wrote it out (with a comment, gasp). I'm happy to change it if you think the local RefPtr<> way is easier to understand. I was thinking you could refactor it like this: if (!s_instance) { ... creation stuff here .. s_instance = new SharedGraphicsContext3D(context.release()); } RefPtr<SharedGraphicsContext3D> sharedContext = adoptRef(s_instance); return sharedContext; Created attachment 103043 [details]
Patch
Switched it around, and it looks slightly cleaner but is still pretty subtle. Ah well, I guess that's what tests are for? Also removed the WillPublishCallback stuff since that's totally unused now, and made sure the set the GrContext's GraphicsContext3D current before flushing it. All dependent patches have finally landed, so marking review? With this on top of ToT WebKit + ToT chromium the wilderness downtown demo seems to work OK. Comment on attachment 103043 [details]
Patch
Whoops, this doesn't accelerate any canvases :{. Trying to play directly with the RefPtr<> stuff is very subtle, I think I'll go back to handling the refs explicitly and clearly. Will update...
Created attachment 103050 [details]
Patch
Comment on attachment 103050 [details]
Patch
Sorry my suggestion led you astray there, and thanks for all the cleanups. r=me
Comment on attachment 103050 [details] Patch Clearing flags on attachment: 103050 Committed r92520: <http://trac.webkit.org/changeset/92520> All reviewed patches have been landed. Closing bug. *** Bug 65202 has been marked as a duplicate of this bug. *** |