Readback composited webgl results for printing
Created attachment 102863 [details] Patch
Comment on attachment 102863 [details] Patch Attachment 102863 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9302242
Hmm, http://codereview.chromium.org/7566046/ is needed for this to work. But anyway, hopefully it's ok.
Created attachment 103004 [details] fix premultiply alpha
Comment on attachment 103004 [details] fix premultiply alpha Attachment 103004 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9306622
Comment on attachment 103004 [details] fix premultiply alpha The approach looks good overall. Go ahead and land the Chromium side changes. You could then either wait for the Chromium DEPS to roll in WebKit or -- better -- update chromium_rev in Source/WebKit/chromium/DEPS in this patch and ping the WebKit gardener on IRC about doing that roll along with your other changes.
Created attachment 103319 [details] Patch
Comment on attachment 103319 [details] Patch Attachment 103319 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9333215 New failing tests: fast/canvas/webgl/premultiplyalpha-test.html
Created attachment 103325 [details] Patch
Created attachment 103417 [details] Patch
Jamesr or senorblanco, could one of you look over this?
This implies having synchronous main thread access to the compositor context which is problematic.
Right, but before we actually put the compositor context in a separate thread we're very likely going to put the webgl context in the same share group as the compositor context, so we'll be able to do the readback from the webgl context. This patch is pretty much just a bridge until that happens.
I don't think we would put webgl in the same share group as the compositor, that would complicate resource and id management for no good reason.
(In reply to comment #14) > I don't think we would put webgl in the same share group as the compositor, that would complicate resource and id management for no good reason. How else would we get access to the WebGL draw buffer texture from the compositor?
(In reply to comment #14) > I don't think we would put webgl in the same share group as the compositor, that would complicate resource and id management for no good reason. On the plus side, I think this would make it possible to eliminate the readback in canvas2D->WebGL draws.
(In reply to comment #15) > (In reply to comment #14) > > I don't think we would put webgl in the same share group as the compositor, that would complicate resource and id management for no good reason. > > How else would we get access to the WebGL draw buffer texture from the compositor? Same way we do now, via magic framebuffer 0
(In reply to comment #16) > (In reply to comment #14) > > I don't think we would put webgl in the same share group as the compositor, that would complicate resource and id management for no good reason. > > On the plus side, I think this would make it possible to eliminate the readback in canvas2D->WebGL draws. That's a good point. Right now, WebGL contexts are explicitly not in the same share group partially to try to provide greater isolation (don't want to accidentally share a compositor texture w/ WebGL) and also to avoid performance issues with resource ID allocation in our current share group implementation. We can mitigate the resource ID speed issues somewhat by caching, but we do need to be careful especially since there's no way to create resources in bulk with the WebGL API.
Yeah, at least for M14 we're not going to have the compositor context in another thread, so I'd like to get this in for that. And for later versions, there are enough benefits to being able to share other textures that we'll probably find another way to ensure that textures are cleaned up properly.
Created attachment 103544 [details] remove now-unnecessary DEPS roll
Comment on attachment 103544 [details] remove now-unnecessary DEPS roll View in context: https://bugs.webkit.org/attachment.cgi?id=103544&action=review > Source/WebCore/platform/graphics/GraphicsContext3D.h:797 > +#if PLATFORM(CHROMIUM) > + void paintFramebufferToCanvas(int framebuffer, int width, int height, bool premultiplyAlpha, CanvasRenderingContext*); > +#endif This seems wrong, if it's chromium-specific shouldn't this be on Extensions3DChromium? Additionally it's a layering violation to have a dependency on CanvasRenderingContext here. CanvasRenderingContext is defined in WebCore/html/canvas/. Classes in WebCore/platform/ should not depend on anything in WebCore/ outside of the platform/ directory. It seems that this parameter is just used to get at the underlying ImageBuffer*. Could we pass in an ImageBuffer* instead, perhaps)? > Source/WebCore/platform/graphics/chromium/WebGLLayerChromium.h:42 > +class CanvasRenderingContext; Newp, you can't depend on CanvasRenderingContext here.
(In reply to comment #19) > Yeah, at least for M14 we're not going to have the compositor context in another thread, so I'd like to get this in for that. And for later versions, there are enough benefits to being able to share other textures that we'll probably find another way to ensure that textures are cleaned up properly. I agree this approach is probably good for m14. Very shortly after that it'll be a pain in the butt, though. Maybe we can deal with it the same way we deal with other readback cases (which is TBD).
(In reply to comment #21) > (From update of attachment 103544 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103544&action=review > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:797 > > +#if PLATFORM(CHROMIUM) > > + void paintFramebufferToCanvas(int framebuffer, int width, int height, bool premultiplyAlpha, CanvasRenderingContext*); > > +#endif > > This seems wrong, if it's chromium-specific shouldn't this be on Extensions3DChromium? > > Additionally it's a layering violation to have a dependency on CanvasRenderingContext here. CanvasRenderingContext is defined in WebCore/html/canvas/. Classes in WebCore/platform/ should not depend on anything in WebCore/ outside of the platform/ directory. > > It seems that this parameter is just used to get at the underlying ImageBuffer*. Could we pass in an ImageBuffer* instead, perhaps)? Oh, the code for paintRenderingResultsToCanvas already has that dependency, so I figured it was ok. I could try to replace it with ImageBuffer, but currently the CG implementation also uses that to get the correct GraphicsContext3D to paint to the canvas. I suppose I could try to make paintToCanvas static, so that's unnecessary.
(In reply to comment #23) > (In reply to comment #21) > > Additionally it's a layering violation to have a dependency on CanvasRenderingContext here. CanvasRenderingContext is defined in WebCore/html/canvas/. Classes in WebCore/platform/ should not depend on anything in WebCore/ outside of the platform/ directory. > > > > It seems that this parameter is just used to get at the underlying ImageBuffer*. Could we pass in an ImageBuffer* instead, perhaps)? > > Oh, the code for paintRenderingResultsToCanvas already has that dependency, so I figured it was ok. I could try to replace it with ImageBuffer, but currently the CG implementation also uses that to get the correct GraphicsContext3D to paint to the canvas. I suppose I could try to make paintToCanvas static, so that's unnecessary. Yeah it's unfortunate that this already exists, but we shouldn't add even more. You could pass and ImageBuffer* and a GraphicsContext3D* if you definitely need both.
Created attachment 103715 [details] Fix layering issues
Comment on attachment 103715 [details] Fix layering issues View in context: https://bugs.webkit.org/attachment.cgi?id=103715&action=review Would it be the epitome of foolishness to ask for a test for this? I think the patch looks pretty reasonable. Ken should probably take a look at it, though, as he's a lot more familiar with the WebGL preserves drawing buffer stuff than I am. > Source/WebCore/platform/graphics/GraphicsContext3D.h:797 > + void paintRenderingResultsToCanvas(CanvasRenderingContext*); > PassRefPtr<ImageData> paintRenderingResultsToImageData(); > + bool paintCompositedResultsToCanvas(CanvasRenderingContext*); these are still wrong, even though you didn't write them. Can you file a separate bug about getting these fixed? CC myself, kbr@chromium.org, and cmarrin@apple.com > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:212 > + unsigned int bufferSize = 4 * width * height; size_t please > Source/WebKit/chromium/src/GraphicsContext3DInternal.h:316 > unsigned char* m_renderOutput; > + unsigned int m_renderOutputSize; Hm, we're doing manual new[]/delete[] on renderOutput. Not very pretty - we have an OwnArrayPtr<> type in WTF that'll take care of that. Also, I think the size should be a size_t since it's a byte count.
(In reply to comment #26) > (From update of attachment 103715 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103715&action=review > > Would it be the epitome of foolishness to ask for a test for this? If you can tell me how to do a pixel test on the printed output of a page after it's been composited, I can try. > > I think the patch looks pretty reasonable. Ken should probably take a look at it, though, as he's a lot more familiar with the WebGL preserves drawing buffer stuff than I am. Ken has been at SIGGRAPH for the past few days, and I'm not sure > > > Source/WebCore/platform/graphics/GraphicsContext3D.h:797 > > + void paintRenderingResultsToCanvas(CanvasRenderingContext*); > > PassRefPtr<ImageData> paintRenderingResultsToImageData(); > > + bool paintCompositedResultsToCanvas(CanvasRenderingContext*); > > these are still wrong, even though you didn't write them. Can you file a separate bug about getting these fixed? CC myself, kbr@chromium.org, and cmarrin@apple.com > Done: https://bugs.webkit.org/show_bug.cgi?id=66121 > > Source/WebKit/chromium/src/GraphicsContext3DChromium.cpp:212 > > + unsigned int bufferSize = 4 * width * height; > > size_t please > > > Source/WebKit/chromium/src/GraphicsContext3DInternal.h:316 > > unsigned char* m_renderOutput; > > + unsigned int m_renderOutputSize; > > Hm, we're doing manual new[]/delete[] on renderOutput. Not very pretty - we have an OwnArrayPtr<> type in WTF that'll take care of that. Also, I think the size should be a size_t since it's a byte count. Yeah, makes sense.
Comment on attachment 103715 [details] Fix layering issues Given the other cleanup bug, this seems fine to me to land in its current form. John, could you file a bug about switching to use OwnArrayPtr for m_renderOutput in GraphicsContext3DInternal? This is previously existing cruft which I don't think needs to be fixed in this patch.
Created attachment 103722 [details] use OwnArrayPtr
Comment on attachment 103722 [details] use OwnArrayPtr restoring kbr's r+ cq+
Comment on attachment 103722 [details] use OwnArrayPtr Clearing flags on attachment: 103722 Committed r92908: <http://trac.webkit.org/changeset/92908>
All reviewed patches have been landed. Closing bug.