Bug 66251 - Canvas resizing can be slow
Summary: Canvas resizing can be slow
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Stephen White
URL:
Keywords:
Depends on: 66305
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-15 13:55 PDT by Stephen White
Modified: 2011-08-16 14:34 PDT (History)
3 users (show)

See Also:


Attachments
Patch (27.18 KB, patch)
2011-08-15 15:21 PDT, Stephen White
no flags Details | Formatted Diff | Diff
Patch (24.79 KB, patch)
2011-08-16 08:19 PDT, Stephen White
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Stephen White 2011-08-15 13:55:18 PDT
Currently, resizing canvas elements can be slow.  This could be improved by:

1)  Deferring all ImageBuffer creation until the first actual draw call.
2)  [chromium] Moving ownership of the DrawingBuffer into ImageBuffer, so it can benefit from Ganesh's texure cache reuse for its FBOs.
3)  [chromium] Avoiding use of texImage2DResourceSafe() when allocating FBOs for canvas.
Comment 1 Stephen White 2011-08-15 15:21:47 PDT
Created attachment 103962 [details]
Patch
Comment 2 James Robinson 2011-08-15 15:50:36 PDT
Comment on attachment 103962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103962&action=review

I like the way this is going, although I'm not sure I understand the complete picture yet.  Left some comments inline.

> Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:273
> +        m_context->texImage2D(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE, 0);

this seems bad - in the command buffer, this'll cause a client-side buffer to get allocated, transferred, and uploaded.

> Source/WebKit/chromium/ChangeLog:9
> +        Allow the hostWindow param (and m_webViewImpl) to be NULL.  This
> +        makes it much easier to enable GPU acceleration at a lower level
> +        in WebKit's platform/graphics layer, without needing access to the
> +        Page* or HostWindow*.

this is a really good change but probably should be a separate patch
Comment 3 Stephen White 2011-08-15 18:11:14 PDT
(In reply to comment #2)
> (From update of attachment 103962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103962&action=review
> 
> I like the way this is going, although I'm not sure I understand the complete picture yet.  Left some comments inline.
> 
> > Source/WebCore/platform/graphics/gpu/DrawingBuffer.cpp:273
> > +        m_context->texImage2D(GraphicsContext3D::TEXTURE_2D, 0, internalColorFormat, m_size.width(), m_size.height(), 0, colorFormat, GraphicsContext3D::UNSIGNED_BYTE, 0);
> 
> this seems bad - in the command buffer, this'll cause a client-side buffer to get allocated, transferred, and uploaded.

Really?  Hmm. I thought that it didn't.  Still, considering that GraphicsContext3D::texImage2DResourceSafe() was doing the same thing, I don't think this is any net change for the command buffer impl.  At least this'll prevent it in the in-process version from doing it.

We really need some kind of "resourceSafety" creation attribute, or a flavour of glTexImage2D() that doesn't do that braindead memset() and upload.  Only WebGL needs it, AFAIK.

> > Source/WebKit/chromium/ChangeLog:9
> > +        Allow the hostWindow param (and m_webViewImpl) to be NULL.  This
> > +        makes it much easier to enable GPU acceleration at a lower level
> > +        in WebKit's platform/graphics layer, without needing access to the
> > +        Page* or HostWindow*.
> 
> this is a really good change but probably should be a separate patch

OK, if you insist.  :)
Comment 4 Stephen White 2011-08-16 08:19:46 PDT
Created attachment 104047 [details]
Patch
Comment 5 Kenneth Russell 2011-08-16 13:19:23 PDT
Comment on attachment 104047 [details]
Patch

Looks good to me.
Comment 6 Stephen White 2011-08-16 14:34:26 PDT
Committed r93157: <http://trac.webkit.org/changeset/93157>