Bug 65402 – [chromium] Accelerated canvas breaks when moving canvases or resources between Pages

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

[James Robinson]

[chromium] Accelerated canvas breaks when moving canvases or resources between Pages

[James Robinson]

Created attachment 102411 [details]
Patch

[James Robinson]

This depends on http://codereview.chromium.org/7488069/, which hasn't landed yet, so the EWS bots will be grumpy.

[Gyuyoung Kim]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/9269559

[WebKit Review Bot]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9268565

[Gustavo Noronha (kov)]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/9266639

[WebKit Review Bot]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass cr-mac-ews (chromium):
Output: http://queues.webkit.org/results/9267608

[WebKit Review Bot]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9269566

[WebKit Review Bot]

Comment on attachment 102411 [details]
Patch

Attachment 102411 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/9268585

[James Robinson]

Created attachment 102587 [details]
updated to build on other platforms

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Stephen White]

(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?

[James Robinson]

Created attachment 102708 [details]
Patch

[Stephen White]

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?

[James Robinson]

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.

[James Robinson]

Created attachment 102957 [details]
Patch

[Stephen White]

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

[Stephen White]

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?

[James Robinson]

Created attachment 102986 [details]
Patch

[James Robinson]

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.

[Stephen White]

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.

[James Robinson]

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

[Stephen White]

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;

[James Robinson]

Created attachment 103043 [details]
Patch

[James Robinson]

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.

[James Robinson]

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

[James Robinson]

Created attachment 103050 [details]
Patch

[Stephen White]

Comment on attachment 103050 [details]
Patch

Sorry my suggestion led you astray there, and thanks for all the cleanups.  r=me

[WebKit Review Bot]

Comment on attachment 103050 [details]
Patch

Clearing flags on attachment: 103050

Committed r92520: <http://trac.webkit.org/changeset/92520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tom Hudson]

*** Bug 65202 has been marked as a duplicate of this bug. ***