Bug 65402

Summary: [chromium] Accelerated canvas breaks when moving canvases or resources between Pages
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: 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 Flags
Patch
none
updated to build on other platforms
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description James Robinson 2011-07-29 17:10:55 PDT
[chromium] Accelerated canvas breaks when moving canvases or resources between Pages
Comment 1 James Robinson 2011-07-29 17:13:07 PDT
Created attachment 102411 [details]
Patch
Comment 2 James Robinson 2011-07-29 17:14:29 PDT
This depends on http://codereview.chromium.org/7488069/, which hasn't landed yet, so the EWS bots will be grumpy.
Comment 3 Gyuyoung Kim 2011-07-29 17:20:29 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9269559
Comment 4 WebKit Review Bot 2011-07-29 17:22:04 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9268565
Comment 5 Gustavo Noronha (kov) 2011-07-29 17:23:17 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9266639
Comment 6 WebKit Review Bot 2011-07-29 17:49:48 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9267608
Comment 7 WebKit Review Bot 2011-07-29 18:14:55 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9269566
Comment 8 WebKit Review Bot 2011-07-29 18:34:00 PDT
Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9268585
Comment 9 James Robinson 2011-08-01 16:11:13 PDT
Created attachment 102587 [details]
updated to build on other platforms
Comment 10 WebKit Review Bot 2011-08-01 16:46:21 PDT
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 11 WebKit Review Bot 2011-08-01 19:16:45 PDT
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
Comment 12 Stephen White 2011-08-02 08:34:00 PDT
(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?
Comment 13 James Robinson 2011-08-02 15:51:00 PDT
Created attachment 102708 [details]
Patch
Comment 14 Stephen White 2011-08-03 15:13:57 PDT
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?
Comment 15 James Robinson 2011-08-03 15:45:46 PDT
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.
Comment 16 James Robinson 2011-08-04 12:30:07 PDT
Created attachment 102957 [details]
Patch
Comment 17 Stephen White 2011-08-04 12:52:50 PDT
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 18 Stephen White 2011-08-04 13:34:30 PDT
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?
Comment 19 James Robinson 2011-08-04 14:59:06 PDT
Created attachment 102986 [details]
Patch
Comment 20 James Robinson 2011-08-04 15:00:19 PDT
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 21 Stephen White 2011-08-04 15:16:33 PDT
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.
Comment 22 James Robinson 2011-08-04 15:25:01 PDT
(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 23 Stephen White 2011-08-04 15:43:23 PDT
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;
Comment 24 James Robinson 2011-08-04 22:51:32 PDT
Created attachment 103043 [details]
Patch
Comment 25 James Robinson 2011-08-04 22:54:14 PDT
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 26 James Robinson 2011-08-04 23:29:38 PDT
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...
Comment 27 James Robinson 2011-08-05 01:21:22 PDT
Created attachment 103050 [details]
Patch
Comment 28 Stephen White 2011-08-05 08:32:45 PDT
Comment on attachment 103050 [details]
Patch

Sorry my suggestion led you astray there, and thanks for all the cleanups.  r=me
Comment 29 WebKit Review Bot 2011-08-05 15:14:51 PDT
Comment on attachment 103050 [details]
Patch

Clearing flags on attachment: 103050

Committed r92520: <http://trac.webkit.org/changeset/92520>
Comment 30 WebKit Review Bot 2011-08-05 15:14:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Tom Hudson 2011-08-24 11:35:30 PDT
*** Bug 65202 has been marked as a duplicate of this bug. ***