Bug 65658 – Readback composited webgl results for printing

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

[John Bauman]

Readback composited webgl results for printing

[John Bauman]

Created attachment 102863 [details]
Patch

[WebKit Review Bot]

Comment on attachment 102863 [details]
Patch

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

[John Bauman]

Hmm, http://codereview.chromium.org/7566046/ is needed for this to work. But anyway, hopefully it's ok.

[John Bauman]

Created attachment 103004 [details]
fix premultiply alpha

[WebKit Review Bot]

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

[Kenneth Russell]

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.

[John Bauman]

Created attachment 103319 [details]
Patch

[WebKit Review Bot]

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

[John Bauman]

Created attachment 103325 [details]
Patch

[John Bauman]

Created attachment 103417 [details]
Patch

[John Bauman]

Jamesr or senorblanco, could one of you look over this?

[James Robinson]

This implies having synchronous main thread access to the compositor context which is problematic.

[John Bauman]

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.

[James Robinson]

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.

[Vangelis Kokkevis]

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

[Stephen White]

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

[James Robinson]

(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

[James Robinson]

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

[John Bauman]

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.

[John Bauman]

Created attachment 103544 [details]
remove now-unnecessary DEPS roll

[James Robinson]

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.

[James Robinson]

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

[John Bauman]

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

[James Robinson]

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

[John Bauman]

Created attachment 103715 [details]
Fix layering issues

[James Robinson]

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, [e-mail], and [e-mail]

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

[John Bauman]

(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, [e-mail], and [e-mail]
> 
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.

[Kenneth Russell]

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.

[John Bauman]

Created attachment 103722 [details]
use OwnArrayPtr

[James Robinson]

Comment on attachment 103722 [details]
use OwnArrayPtr

restoring kbr's r+ cq+

[WebKit Review Bot]

Comment on attachment 103722 [details]
use OwnArrayPtr

Clearing flags on attachment: 103722

Committed r92908: <http://trac.webkit.org/changeset/92908>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.