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
Created attachment 141535 [details] Patch
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.
+gman who might have ideas or experience about the glGenerateMipmaps problem
Is the texture NPOT? You can't generate mips on an NPOT texture from WebGL
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.
(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.
(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.
Created attachment 141767 [details] Patch
(In reply to comment #8) > Created an attachment (id=141767) [details] > Patch This patch rebaselines the change to wk rev 116969.
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).
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
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.
Comment on attachment 141767 [details] Patch Attachment 141767 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12682873
Comment on attachment 141767 [details] Patch Attachment 141767 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12706344
Comment on attachment 141767 [details] Patch Attachment 141767 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12705479
Comment on attachment 141767 [details] Patch Attachment 141767 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12694682
Comment on attachment 141767 [details] Patch Attachment 141767 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12685836
(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).
Created attachment 142544 [details] Patch
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.
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?
Comment on attachment 142544 [details] Patch Attachment 142544 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12717636
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?
(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.
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).
Comment on attachment 142544 [details] Patch Attachment 142544 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12715699
Comment on attachment 142544 [details] Patch Attachment 142544 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12722567
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.
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.
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
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
Comment on attachment 142544 [details] Patch Attachment 142544 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12709874
Created attachment 142562 [details] Patch
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.
Comment on attachment 142562 [details] Patch Attachment 142562 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12725242
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
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
Comment on attachment 142562 [details] Patch Attachment 142562 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12718693
Comment on attachment 142562 [details] Patch Attachment 142562 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12724299
Created attachment 148626 [details] Patch
Created attachment 148637 [details] Patch
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
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.
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
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
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.
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.
(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.
OK cool! The compositor-ish changes seem fine, then.
Created attachment 148880 [details] Patch
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
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
(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?
(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?
Created attachment 149064 [details] Patch
Comment on attachment 149064 [details] Patch This time is the charm. Rolling chromium deps resolved the failing unit test.
Comment on attachment 149064 [details] Patch Fantastic! Great work!
Comment on attachment 149064 [details] Patch Clearing flags on attachment: 149064 Committed r121055: <http://trac.webkit.org/changeset/121055>
All reviewed patches have been landed. Closing bug.