Summary: | WebGL rendering results must be made available to Canvas.toDataURL and 2D drawImage | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||||||||||||||||||
Component: | WebGL | Assignee: | Zhenyao Mo <zmo> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | cmarrin, gman, jamesr, japhet, mdelaney7, oliver, simon.fraser, vangelis, webkit-ews, zherczeg, zmo | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Kenneth Russell
2010-02-08 13:01:19 PST
The same problem exists with Canvas.toDataURL(). Both of these APIs must respect the premultipliedAlpha flag in the WebGLContextAttributes. Created attachment 60461 [details]
patch
Tested in both Safari and Chrome in Mac in Release mode.
Comment on attachment 60461 [details]
patch
paintRenderingResultsToCanvas is leaking 'pixels'.
Created attachment 60482 [details]
revised patch: responding to Simon Fraser's review
Use OwnArrayPtr for pixels to avoid leak.
Comment on attachment 60482 [details]
revised patch: responding to Simon Fraser's review
WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
+ // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
Either share the code, or remove the comment. As-is, the comment is not useful.
Can you share code with GraphicsContext3D::readPixels()?
WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:290
+ CGImageRef cgImage = CGImageCreate(m_currentWidth,
You should use RetainPtr for the dataProvider, colorSpace and cgImage.
Do you need to save/restore the state in the imageBuffer->context()?
WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307
+ // We want to completely overwrite the previous frame's rendering results.
Is that described in a spec anywhere?
r=me
Comment on attachment 60482 [details]
revised patch: responding to Simon Fraser's review
Generally looks good, but one high-level comment about code refactoring which I would prefer be done.
WebCore/platform/graphics/GraphicsContext3D.h:711
+ void paintRenderingResultsToCanvas(WebGLRenderingContext*context);
Formatting: WebGLRenderingContext* context
WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256
+ // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer
The CG portions of this code should be refactored rather than duplicated. Given that the original code came from GraphicsContext3D.cpp in WebKit/chromium/src, it should be possible to refactor it into WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp. It might be necessary to split it into two routines (before/after readback), or pass a callback function. If it seems reasonable to do the refactoring, please do so.
(In reply to comment #5) > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307 > + // We want to completely overwrite the previous frame's rendering results. > > Is that described in a spec anywhere? The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image. (In reply to comment #7) > (In reply to comment #5) > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307 > > + // We want to completely overwrite the previous frame's rendering results. > > > > Is that described in a spec anywhere? > > The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image. I realize that, but where is that behavior spec'd? I imagine this affect the outcome of mixing a 2D and 2D context on the same canvas, but I'd hope that transparent pixels in the 3D context wouldn't clobber earlier 2D drawing. (In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307 > > > + // We want to completely overwrite the previous frame's rendering results. > > > > > > Is that described in a spec anywhere? > > > > The comment refers to the fact that the kCGBlendModeCopy blend mode is used, so that transparent pixels aren't blended with any content previously in the image. > > I realize that, but where is that behavior spec'd? I imagine this affect the outcome of mixing a 2D and 2D context on the same canvas, but I'd hope that transparent pixels in the 3D context wouldn't clobber earlier 2D drawing. There have been some changes pending to the HTML Canvas specification for some time now regarding mixing of contexts, and I believe the latest resolution is that using the 2D and 3D contexts simultaneously will be explicitly disallowed in the spec. Regardless, even if you're just using the 3D context, it's still necessary to completely overwrite the rendering results. Consider the case where you draw one 3D scene with a certain set of pixels as transparent, and then perform a drawImage call with that canvas as source. Then draw a different 3D scene with a different set of transparent pixels, and perform another similar drawImage call (into a second, clean, canvas). It would be an error if portions of the first scene were visible where the pixels are transparent in the second scene because the data read back from the graphics card was blended with the previously read back data. Fair enough. This may also fix bug 30722. (In reply to comment #5) > (From update of attachment 60482 [details]) > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256 > + // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer > Either share the code, or remove the comment. As-is, the comment is not useful. Removed. > > Can you share code with GraphicsContext3D::readPixels()? readPixels() logic is different. Here we always read from backbuffer, whereas readPixels might read from user created fbo. > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:290 > + CGImageRef cgImage = CGImageCreate(m_currentWidth, > You should use RetainPtr for the dataProvider, colorSpace and cgImage. > > Do you need to save/restore the state in the imageBuffer->context()? Done. > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:307 > + // We want to completely overwrite the previous frame's rendering results. > > Is that described in a spec anywhere? > > r=me (In reply to comment #6) > (From update of attachment 60482 [details]) > Generally looks good, but one high-level comment about code refactoring which I would prefer be done. > > > WebCore/platform/graphics/GraphicsContext3D.h:711 > + void paintRenderingResultsToCanvas(WebGLRenderingContext*context); > Formatting: WebGLRenderingContext* context Done. > > WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:256 > + // BEGIN: the same code as in WebGraphicsContext3DDefaultImpl.cpp::readBackFramebuffer > The CG portions of this code should be refactored rather than duplicated. Given that the original code came from GraphicsContext3D.cpp in WebKit/chromium/src, it should be possible to refactor it into WebCore/platform/graphics/cg/GraphicsContext3DCG.cpp. It might be necessary to split it into two routines (before/after readback), or pass a callback function. If it seems reasonable to do the refactoring, please do so. Done. Created attachment 61159 [details]
revised patch: responding to Simon Fraser and kbr's review
Created attachment 61170 [details]
revised patch: fix a bug
in WebGLRenderingContext::markedContextChanged(), always set the dirty flag when context has changed.
Also, add another test canvas-test.html for this change. (This test is copied from WebGL conformance tests with minor modifications).
Yike, this patch is so out-of-date. CanvasSurface.h/cpp is gone. Have to manually merge. Add jamesr@chromium.org to cc list since he did the code refactoring. Created attachment 61172 [details]
revised patch: resolving svn merge conflicts
Created attachment 61447 [details]
revised patch: deal with Qt part of GraphicsContext3D
Try my luck!
Created attachment 61449 [details]
revised patch: fix a ChangeLog issue
Attachment 61449 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/3523150 Created attachment 61451 [details]
revised patch: fix the Qt bug
Created attachment 61457 [details]
revised patch
Seems like Qt/Gtk/Win bots failure may have nothing to do with CG stuff. Revert the changes I made based on the blame-CG assumption.
Created attachment 61712 [details]
revised patch
Using kbr's script to sync the two tests in this patch with khronos conformance tests. No other changes,
Comment on attachment 61712 [details]
revised patch
Looks good. Thanks for waiting and keeping the tests in sync.
Comment on attachment 61712 [details] revised patch > +#if ENABLE(3D_CANVAS) > + if (sourceCanvas->is3D()) { > + WebGLRenderingContext* context3d = reinterpret_cast<WebGLRenderingContext*>(sourceCanvas->renderingContext()); > + context3d->paintRenderingResultsToCanvas(); > + } > +#endif > + We probably shouldn't be doing 3D things in CanvasRenderingContext2D. > + intervalId = window.setInterval(function() { If there's any way to make this test work without setInterval, please do so, lest it be slow and flaky. Created attachment 61747 [details]
revised patch
Comment on attachment 61747 [details]
revised patch
This looks better to me.
I think the setInterval call is needed in the test (I'm not the test author but there's probably a reason it was used), but the modifications that have been made should prevent any flakiness.
Comment on attachment 61747 [details]
revised patch
Alright, sounds good.
Committed r63502: <http://trac.webkit.org/changeset/63502> This patch does not builds on mac-leopard: /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm: In member function 'void WebCore::GraphicsContext3D::paintRenderingResultsToCanvas(WebCore::WebGLRenderingContext*)': /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:257: warning: 'mustRestoreFBO' may be used uninitialized in this function I think it should be false by default, isn't it? (In reply to comment #30) > This patch does not builds on mac-leopard: > > /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm: In member function 'void WebCore::GraphicsContext3D::paintRenderingResultsToCanvas(WebCore::WebGLRenderingContext*)': > /Users/hzoli/WebKitHZ/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:257: warning: 'mustRestoreFBO' may be used uninitialized in this function > > I think it should be false by default, isn't it? Yes, you're right. I'll upload a patch fixing this. Not sure why it wasn't caught by the bots. Created attachment 61996 [details]
Patch fixing compiler warning
From the ChangeLog:
Fixed compiler warning introduced by original patch. No new tests; covered by existing tests.
Committed r63705: <http://trac.webkit.org/changeset/63705> Does this fix bug 30722? (In reply to comment #34) > Does this fix bug 30722? I'm not sure (and Mo is on vacation this week). Was there a test case for 30722? This caused bug 42852. (In reply to comment #35) > (In reply to comment #34) > > Does this fix bug 30722? > > I'm not sure (and Mo is on vacation this week). Was there a test case for 30722? See bug 42862. |