Bug 86275 – [Chromium] Support for direct GPU texture copy between Canvas2D and WebGL Textures

• Component: WebGL
• Status: RESOLVED FIXED

[Jeff Timanus]

Calls to WebGL's texImage2D interface with Canvas2D elements incur a costly software read-back for hardware accelerated instances of Canvas2D.

A fast path should be implemented, allowing for a direct copy between the GPU textures.  The GL_CHROMIUM_copy_texture extension was designed for this purpose. (crrev.com/133213)

Issue tracked also at crbug.com/101051

[Jeff Timanus]

Created attachment 141535 [details]
Patch

[Jeff Timanus]

Comment on attachment 141535 [details]
Patch

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

I've had this patch percolating for a while.  It uses the GL_CHROMIUM_copy_texture extension to enable the copy of a texture from a BGRA-backed Ganesh instance.

I think that some of the details will still need some work.  See in-line comments.  Please comment on the general structure of the approach.

There is still a problem with this change.  Essentially the code path invokes the following sequence of operations, when copying to a texture, followed by generation of mip-maps.

1. Construct a texture, A.  ->  Done from WebGL
2. Bind A as TEXTURE_2D.
3. gl.texImage2D(..., canvas2D)
   1. glTexImage2D(canvas.size, format,.... NULL).  ->  Done from GL_CHROMIUM_copy_texture
   2. Bind texture, A, as backing store for a fbo.
   3. glDrawArrays on the fbo.
4. gl.generateMipmaps(TEXTURE_2D) -> Called from WebGL

In both ANGLE, and desktop-gl, the mip-map generation is clearing the texture contents that were rendered via the framebuffer.  I am presently writing a webgl sample that demonstrates this problem.  My haunch is that the mip-map generation code is not behaving properly for textures that have never been initialized with pixel data from a texImage2D call.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2205
> +{

The name of this routine, and the use of an int for internalFormat may be worth changing.  Also, is there a preference for reference vs pointer arguments?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3599
> +        WebGLTexture* texture = validateTextureBinding("texImage2D", target, true);

The extension only works for TEXTURE_2D targets.  This can be extended, but this is a first pass, to get part of the canvas2d-webgl path accelerated.

This entire chunk of code could use some re-working, to unify this path and the existing path in texImage2DBase.

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:671
> +    SkGpuRenderTarget* renderTarget = m_canvas->getDevice()->accessRenderTarget();

At present, this is the only way to get to the backing texture.  A cleaner accessor needs to be added to SkDevice.

[Kenneth Russell]

+gman who might have ideas or experience about the glGenerateMipmaps problem

[Gregg Tavares]

Is the texture NPOT? You can't generate mips on an NPOT texture from WebGL

[Gregg Tavares]

Also you can't render an NPOT texture from WebGL unless you set wrapping to CLAMP and choose filtering modes that don't use MIPS.

If you don't follow those restrictions it will render in black.

[Jeff Timanus]

(In reply to comment #4)
> Is the texture NPOT? You can't generate mips on an NPOT texture from WebGL

The sample that generates the failure is a the following site:  http://www.spoonofdeath.com/delph/webgltext.html

The texture is 512x512, so it is NPOT.

(In reply to comment #5)
> Also you can't render an NPOT texture from WebGL unless you set wrapping to CLAMP and choose filtering modes that don't use MIPS.
> 
> If you don't follow those restrictions it will render in black.

The demo works fine without my change, and GL_CHROMIUM_copy_texture_2d should be restoring the clamp/filtering settings.  I'll extend the existing test cases, just to triple check.

[Jeff Timanus]

(In reply to comment #6)
> (In reply to comment #4)
> > Is the texture NPOT? You can't generate mips on an NPOT texture from WebGL
> 
> The sample that generates the failure is a the following site:  http://www.spoonofdeath.com/delph/webgltext.html
> 
> The texture is 512x512, so it is NPOT.
Correction:  It IS POT.
> 
> (In reply to comment #5)
> > Also you can't render an NPOT texture from WebGL unless you set wrapping to CLAMP and choose filtering modes that don't use MIPS.
> > 
> > If you don't follow those restrictions it will render in black.
> 
> The demo works fine without my change, and GL_CHROMIUM_copy_texture_2d should be restoring the clamp/filtering settings.  I'll extend the existing test cases, just to triple check.

[Jeff Timanus]

Created attachment 141767 [details]
Patch

[Jeff Timanus]

(In reply to comment #8)
> Created an attachment (id=141767) [details]
> Patch

This patch rebaselines the change to wk rev 116969.

[Stephen White]

Comment on attachment 141767 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.h:60
> +    virtual bool copyToWebGLTexture(GraphicsContext3D& context, WebGLTexture& texture, int internalFormat) { return false; }

Out of curiosity, does this optimization make sense to do on WebGL as well?  i.e., can you draw from one WebGL canvas to another via texImage2D?

> Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3598
> +#if 0

This should go away, obviously.

> Source/WebCore/platform/graphics/GraphicsContext.h:35
> +#include "GraphicsTypes3D.h"

I'm not sure that adding dependencies on WebGL objects to GraphicsContext is a great idea.  At a minimum, perhaps we should keep the dependencies to just Platform3DObject, rather than WebGLTexture if possible (it looks like they are already, perhaps just the forward declaration is wrong).  And perhaps we should name the function a little more generically, perhaps something like copyBackingStoreToTexture() or something.

In fact, perhaps this should be on the ImageBuffer, rather than the GraphicsContext.  Another approach might be to provide a way to get the backing store texture ID out of ImageBuffer, and do all the copying up in CanvasRenderingContext[2D], rather than down in PlatformContextSkia.  I'm not certain which approach is best to be honest.

> Source/WebCore/platform/graphics/GraphicsContext.h:132
> +    class WebGLTexture;

Spurious forward declaration?

> Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:677
> +    if (!context.makeContextCurrent())
> +      return false;

Do we need to restore the current context?  (Don't think so, since I think all GC3D stubs now set the context, but I could be wrong).

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 141767 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:47:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 17 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 141767 [details]
Patch

Attachment 141767 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12682873

[Build Bot]

Comment on attachment 141767 [details]
Patch

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

[Early Warning System Bot]

Comment on attachment 141767 [details]
Patch

Attachment 141767 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12705479

[Build Bot]

Comment on attachment 141767 [details]
Patch

Attachment 141767 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12694682

[Gyuyoung Kim]

Comment on attachment 141767 [details]
Patch

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

[Jeff Timanus]

(In reply to comment #10)
> (From update of attachment 141767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141767&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext.h:60
> > +    virtual bool copyToWebGLTexture(GraphicsContext3D& context, WebGLTexture& texture, int internalFormat) { return false; }
> 
> Out of curiosity, does this optimization make sense to do on WebGL as well?  i.e., can you draw from one WebGL canvas to another via texImage2D?

Yes, the WebGL-WebGL path is currently slow as it performs a read-back.  To allow the fast-path between WebGL contexts, WebGLRenderingContext would need to override the copyToWebGLTexture(...) method.  This path only provides the implementation for Canvas2D.

There is also the issue that WebGL contexts share resources with the compositor, but not necessarily with each-other.  This extension assumes resource sharing.  Al Patrick is working on a similar extension that could be used for the WebGL case:  http://codereview.chromium.org/10106015/

> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3598
> > +#if 0
> 
> This should go away, obviously.

Leftover debugging code.  Fixed.

> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:35
> > +#include "GraphicsTypes3D.h"
> 
> I'm not sure that adding dependencies on WebGL objects to GraphicsContext is a great idea.  At a minimum, perhaps we should keep the dependencies to just Platform3DObject, rather than WebGLTexture if possible (it looks like they are already, perhaps just the forward declaration is wrong).  And perhaps we should name the function a little more generically, perhaps something like copyBackingStoreToTexture() or something.

I'll make the function name more generic, as I agree that the name is currently not appropriate.  Yes, there is no need to pass WebGLTexture instances, when Platform3DObject types are suitable.

> 
> In fact, perhaps this should be on the ImageBuffer, rather than the GraphicsContext.  Another approach might be to provide a way to get the backing store texture ID out of ImageBuffer, and do all the copying up in CanvasRenderingContext[2D], rather than down in PlatformContextSkia.  I'm not certain which approach is best to be honest.

Now that I have the patch fully working, I'll implement this alternative and upload a patch so we can pick & choose the best impl.

> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:132
> > +    class WebGLTexture;
> 
> Spurious forward declaration?

Spurious.  Removed. 

> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:677
> > +    if (!context.makeContextCurrent())
> > +      return false;
> 
> Do we need to restore the current context?  (Don't think so, since I think all GC3D stubs now set the context, but I could be wrong).

Ganesh resets the current context on all GL calls, but I'm not certain if the GC3D calls will enforce the correct context.  WebGraphicsContext3DInProcessCommandBufferImpl::ClearContext() is called by most routines, but it is presently a no-op.

(In reply to comment #10)
> (From update of attachment 141767 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141767&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext.h:60
> > +    virtual bool copyToWebGLTexture(GraphicsContext3D& context, WebGLTexture& texture, int internalFormat) { return false; }
> 
> Out of curiosity, does this optimization make sense to do on WebGL as well?  i.e., can you draw from one WebGL canvas to another via texImage2D?
> 
> > Source/WebCore/html/canvas/WebGLRenderingContext.cpp:3598
> > +#if 0
> 
> This should go away, obviously.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:35
> > +#include "GraphicsTypes3D.h"
> 
> I'm not sure that adding dependencies on WebGL objects to GraphicsContext is a great idea.  At a minimum, perhaps we should keep the dependencies to just Platform3DObject, rather than WebGLTexture if possible (it looks like they are already, perhaps just the forward declaration is wrong).  And perhaps we should name the function a little more generically, perhaps something like copyBackingStoreToTexture() or something.
> 
> In fact, perhaps this should be on the ImageBuffer, rather than the GraphicsContext.  Another approach might be to provide a way to get the backing store texture ID out of ImageBuffer, and do all the copying up in CanvasRenderingContext[2D], rather than down in PlatformContextSkia.  I'm not certain which approach is best to be honest.
> 
> > Source/WebCore/platform/graphics/GraphicsContext.h:132
> > +    class WebGLTexture;
> 
> Spurious forward declaration?
> 
> > Source/WebCore/platform/graphics/skia/PlatformContextSkia.cpp:677
> > +    if (!context.makeContextCurrent())
> > +      return false;
> 
> Do we need to restore the current context?  (Don't think so, since I think all GC3D stubs now set the context, but I could be wrong).

[Jeff Timanus]

Created attachment 142544 [details]
Patch

[Jeff Timanus]

Comment on attachment 142544 [details]
Patch

I believe that this patch is close to how the final version of this path will operate.  

Note that the gen-mips issue is still present, so this is still not 100% ready for prime-time.  However, I'm marking for review, because the gen-mips issue could be related to ANGLE or the Chromium GPU-command buffer, and the fix will likely not be related to these changes.

[James Robinson]

Comment on attachment 142544 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:73
> +#if PLATFORM(CHROMIUM)
> +#include "LayerChromium.h"
> +#endif

is this #include being used? it's a bit odd for the cross-platform CanvasRenderingContext2D to know about the chromium compositor implementation details

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:67
> +    virtual GraphicsContext3D* graphicsContext3D() OVERRIDE;

I've got another patch pending on Canvas2DLayerChromium that I was planning to land today.  I don't want to slow you down and I'm happy to take care of any merge headaches - do you think that generally this patch is ready to land now-ish, or it needs some more iterations / rolls?

[Build Bot]

Comment on attachment 142544 [details]
Patch

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

[James Robinson]

Comment on attachment 142544 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:207
> +    m_data.m_canvas->flush();
> +    m_data.m_platformLayer->graphicsContext3D()->flush();

Does the canvas context have to be made current before flush()ing the SkCanvas?  I don't think it is safe to reach in and touch the back texture like this in the non-double-buffered multi-threaded case since the compositor thread might be using this texture.  Look at the layerWillDraw() logic inside Canvas2DLayerChromium.  Rather than reaching in by hand and grabbing the layer's texture, I think it'd be better to have the layer (or some other class) be responsible for making sure that this texture can be provided.

As I mentioned I have another refactor in this area planned.  I can make sure to provide an API that'll make a texture representing the current state of this canvas is available, if that's what this wants.

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:209
> +    if (!context.makeContextCurrent())

Is there a strong reason for doing this on the webgl context vs doing it on the canvas context and flushing?  Seems like a bit of a tossup since we have to make both current and flush 'em both, but hmmmm..

> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:212
> +    Extensions3D* extensions = context.getExtensions();

do we expose this extension on the webgl context?

[Jeff Timanus]

(In reply to comment #21)
> (From update of attachment 142544 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142544&action=review
> 
> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:73
> > +#if PLATFORM(CHROMIUM)
> > +#include "LayerChromium.h"
> > +#endif
> 
> is this #include being used? it's a bit odd for the cross-platform CanvasRenderingContext2D to know about the chromium compositor implementation details

Yes, that's a leftover include.  I'll drop it.

> 
> > Source/WebCore/platform/graphics/chromium/Canvas2DLayerChromium.h:67
> > +    virtual GraphicsContext3D* graphicsContext3D() OVERRIDE;
> 
> I've got another patch pending on Canvas2DLayerChromium that I was planning to land today.  I don't want to slow you down and I'm happy to take care of any merge headaches - do you think that generally this patch is ready to land now-ish, or it needs some more iterations / rolls?

I think it needs some more iterations.  The mip-maps problem essentially blocks this change, because the common usage pattern is to render to the canvas, copy it to WebGL, and then immediately gen a mip-chain.

This patch is smallish, so I can easily cope with merge issues - you can move forward with your changes without blocking me too much.

[Stephen White]

Comment on attachment 142544 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:2278
> +bool CanvasRenderingContext2D::copyToPlatformTexture(GraphicsContext3D& destinationContext, Platform3DObject texture, int internalFormat)
> +{
> +#if PLATFORM(CHROMIUM)
> +    return canvas()->buffer()->copyToPlatformTexture(destinationContext, texture, internalFormat);
> +#else

Since we can't do WebGL->WebGL copies yet, could we move this code into WebGLRenderingContext instead and remove the virtual override for now?  It feels a little weird to have WebGL types in CanvasRenderingContext2D, while I think WebGL already knows about the Canvas2D types you need (ImageBuffer).

[Early Warning System Bot]

Comment on attachment 142544 [details]
Patch

Attachment 142544 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12715699

[Early Warning System Bot]

Comment on attachment 142544 [details]
Patch

Attachment 142544 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12722567

[Jeff Timanus]

Comment on attachment 142544 [details]
Patch

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

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:207
>> +    m_data.m_platformLayer->graphicsContext3D()->flush();
> 
> Does the canvas context have to be made current before flush()ing the SkCanvas?  I don't think it is safe to reach in and touch the back texture like this in the non-double-buffered multi-threaded case since the compositor thread might be using this texture.  Look at the layerWillDraw() logic inside Canvas2DLayerChromium.  Rather than reaching in by hand and grabbing the layer's texture, I think it'd be better to have the layer (or some other class) be responsible for making sure that this texture can be provided.
> 
> As I mentioned I have another refactor in this area planned.  I can make sure to provide an API that'll make a texture representing the current state of this canvas is available, if that's what this wants.

The canvas context does not need to be made current.  All of the Ganesh calls to GL make the context current.  We could add extra make-current calls here for clarity, though.

You are right that there is the potential to race with the compositor when invoking the flush.  I'll make the change to use layerWillDraw, or the new equivalent to prevent this problem.

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:209
>> +    if (!context.makeContextCurrent())
> 
> Is there a strong reason for doing this on the webgl context vs doing it on the canvas context and flushing?  Seems like a bit of a tossup since we have to make both current and flush 'em both, but hmmmm..

The issue is that the copy has to respect the pixel-storage settings that are current in the WebGL context.  (Flip-y, pre-multiply alpha).  

It could be done in the canvas context, but then we would have to manage this state.

>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:212
>> +    Extensions3D* extensions = context.getExtensions();
> 
> do we expose this extension on the webgl context?

That's a good question.  I will have to double check if this is exposed to WebGL.

[Kenneth Russell]

Comment on attachment 142544 [details]
Patch

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

>>> Source/WebCore/platform/graphics/skia/ImageBufferSkia.cpp:212
>>> +    Extensions3D* extensions = context.getExtensions();
>> 
>> do we expose this extension on the webgl context?
> 
> That's a good question.  I will have to double check if this is exposed to WebGL.

No, this extension isn't exposed to WebGL.

[WebKit Review Bot]

Comment on attachment 142544 [details]
Patch

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

New failing tests:
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
fast/canvas/webgl/tex-image-and-uniform-binding-bugs.html
fast/canvas/webgl/tex-sub-image-2d-bad-args.html
platform/chromium/virtual/gpu/fast/canvas/webgl/tex-sub-image-2d-bad-args.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
platform/chromium/virtual/gpu/fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/texture-active-bind.html
platform/chromium/virtual/gpu/fast/canvas/webgl/tex-image-and-uniform-binding-bugs.html
fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/canvas-2d-webgl-texture.html
platform/chromium/virtual/gpu/fast/canvas/webgl/texture-active-bind.html
fast/canvas/webgl/texture-complete.html

[WebKit Review Bot]

Created attachment 142557 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

Comment on attachment 142544 [details]
Patch

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

[Jeff Timanus]

Created attachment 142562 [details]
Patch

[Jeff Timanus]

Comment on attachment 142562 [details]
Patch

This patch addresses previous comments, including calling layerWillDraw(...) before flushing the canvas.  

James, please don't hold up your work for this change.  I need to fix the mip issue, and associated layout failures.  As long as you provide a means to access the texture backing the canvas in a safe way, then I'm good to go.  I presume you will need this feature anyway for canvas-canvas draws.

[Build Bot]

Comment on attachment 142562 [details]
Patch

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

[WebKit Review Bot]

Comment on attachment 142562 [details]
Patch

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

New failing tests:
fast/canvas/webgl/premultiplyalpha-test.html
fast/canvas/webgl/tex-image-and-uniform-binding-bugs.html
fast/canvas/webgl/tex-sub-image-2d-bad-args.html
platform/chromium/virtual/gpu/fast/canvas/webgl/premultiplyalpha-test.html
platform/chromium/virtual/gpu/fast/canvas/webgl/tex-sub-image-2d-bad-args.html
http/tests/security/webgl-remote-read-remote-image-allowed.html
platform/chromium/virtual/gpu/fast/canvas/webgl/gl-teximage.html
fast/canvas/webgl/texture-active-bind.html
fast/canvas/webgl/gl-teximage.html
http/tests/security/webgl-remote-read-remote-image-allowed-with-credentials.html
platform/chromium/virtual/gpu/fast/canvas/webgl/texture-active-bind.html
fast/canvas/webgl/texture-complete.html
fast/canvas/webgl/canvas-2d-webgl-texture.html

[WebKit Review Bot]

Created attachment 142580 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Early Warning System Bot]

Comment on attachment 142562 [details]
Patch

Attachment 142562 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12718693

[Early Warning System Bot]

Comment on attachment 142562 [details]
Patch

Attachment 142562 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12724299

[Jeff Timanus]

Created attachment 148626 [details]
Patch

[Jeff Timanus]

Created attachment 148637 [details]
Patch

[Jeff Timanus]

Comment on attachment 148637 [details]
Patch

This should be close to the last revision of this change.  The copy is now performed by the ImageBuffer class, which accesses the backing store texture of the canvas via the Canvas2DLayerBridge.

Points of note:
 - I added the plumbing for the flipy and pre-multiplied-alpha GL command buffer extensions.  Note that the WebGLRenderingContext does NOT forward these settings to the graphics context.  The flipy and premultiply alpha settings cached on the WebGLRenderingContext instance are used just as they were previously:  When performing texture uploads via the software read-back path.  The settings are used on the GraphicsContext side only when invoking ImageBuffer::copyToPlatformTexture.
 - The mips issue is still present in this version of the patch.  The problem is not related to this change, and can reproduced in WebGL.  See tracking issue:  crbug.com/132641
 - The fast gpu path causes a text difference in fast/canvas/webgl/gltex-image/.  See tracking bug for new rebaseline:  wkbug.com/89494

[James Robinson]

Comment on attachment 148637 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:162
> +    if (m_canvas)
> +        m_canvas->flush();

does this end up calling SkDeferredCanvas::DeviceContext::prepareForDraw() under the scenes? If we're in deferred mode single-buffered mode we need to make sure to let the compositor know that we're messing with its texture.

[WebKit Review Bot]

Comment on attachment 148637 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/tex-sub-image-2d-bad-args.html

[WebKit Review Bot]

Created attachment 148689 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Kenneth Russell]

Comment on attachment 148637 [details]
Patch

This looks good to me. Thanks for pushing it through. It's great to see the first highly optimized texture upload path to WebGL from other DOM elements.

I'll defer the r+ to James, given apparent interactions with the threaded compositor. Also it looks like there is still at least one layout test failure to iron out.

[Jeff Timanus]

Comment on attachment 148637 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/Canvas2DLayerBridge.cpp:162
>> +        m_canvas->flush();
> 
> does this end up calling SkDeferredCanvas::DeviceContext::prepareForDraw() under the scenes? If we're in deferred mode single-buffered mode we need to make sure to let the compositor know that we're messing with its texture.

Yes, is invoked.  

I just confirmed this behaviour with Justin.

A SkDeferredCanvas will always have an associated SkDeferredCanvas::DeferredDevice, and flush on this class will invoke prepareForDraw() if a device context has been installed on the device.  Justin also confirms that in webkit, a device context is always bound on deferred canvasses.

[Jeff Timanus]

(In reply to comment #46)
> (From update of attachment 148637 [details])
> This looks good to me. Thanks for pushing it through. It's great to see the first highly optimized texture upload path to WebGL from other DOM elements.
> 
> I'll defer the r+ to James, given apparent interactions with the threaded compositor. Also it looks like there is still at least one layout test failure to iron out.

Thanks for the review, Ken.  This change should make it very straight-forward to hardware accelerate other paths to WebGL textures.

The remaining layout test failure does not repro on my Linux or Windows boxes.  I'll investigate why it's failing on the bot.

[James Robinson]

OK cool!  The compositor-ish changes seem fine, then.

[Jeff Timanus]

Created attachment 148880 [details]
Patch

[WebKit Review Bot]

Comment on attachment 148880 [details]
Patch

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

New failing tests:
platform/chromium/virtual/gpu/fast/canvas/webgl/tex-sub-image-2d-bad-args.html

[WebKit Review Bot]

Created attachment 148949 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Jeff Timanus]

(In reply to comment #48)
> (In reply to comment #46)
> > (From update of attachment 148637 [details] [details])
> > This looks good to me. Thanks for pushing it through. It's great to see the first highly optimized texture upload path to WebGL from other DOM elements.
> > 
> > I'll defer the r+ to James, given apparent interactions with the threaded compositor. Also it looks like there is still at least one layout test failure to iron out.
> 
> Thanks for the review, Ken.  This change should make it very straight-forward to hardware accelerate other paths to WebGL textures.
> 
> The remaining layout test failure does not repro on my Linux or Windows boxes.  I'll investigate why it's failing on the bot.

The layout test failure is not related to my change.  Running DRT at chromium rev 143420 I do not observe any failures.

The webkit chromium deps is at 142842.  At this revision I can reproduce the failure.

Should I also roll chromium deps with my change?

[Kenneth Russell]

(In reply to comment #53)
> The layout test failure is not related to my change.  Running DRT at chromium rev 143420 I do not observe any failures.
> 
> The webkit chromium deps is at 142842.  At this revision I can reproduce the failure.

It's great news that your patch is OK!

> Should I also roll chromium deps with my change?

Sure, that sounds fine. Can you regenerate the patch?

[Jeff Timanus]

Created attachment 149064 [details]
Patch

[Jeff Timanus]

Comment on attachment 149064 [details]
Patch

This time is the charm.  Rolling chromium deps resolved the failing unit test.

[Kenneth Russell]

Comment on attachment 149064 [details]
Patch

Fantastic! Great work!

[WebKit Review Bot]

Comment on attachment 149064 [details]
Patch

Clearing flags on attachment: 149064

Committed r121055: <http://trac.webkit.org/changeset/121055>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.