Bug 52311

Created attachment 78708 [details] Patch

Attachment 78708 [details] did not build on qt: Build output: http://queues.webkit.org/results/7476094

Attachment 78708 [details] did not build on win: Build output: http://queues.webkit.org/results/7473119

Created attachment 78717 [details] Patch

This patch provides plumbing for a new command line switch: --enable-composite-to-texture I'm still doing some testing on it (hence the lack of an review-request flag), but I'd appreciate any feedback on whether I'm doing it correctly, or if there's a better way. In particular, is my use of page()->settings() in WebViewImpl.cpp OK, or should I plumb this differently? The function copyOffscreenTextureToDisplay() is (nearly) the same as in https://bugs.webkit.org/show_bug.cgi?id=50833, and is only required to allow automated testing with the new flag.

Comment on attachment 78717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78717&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643 > + GLC(m_context, m_context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, 0)); It would be nice to re-factor LayerRendererChromium::useRenderSurface() such that when you pass zero as your render surface and m_compositeOffscreen == true then it renders to the display. I think it would be fairly straightforward. It looks like all it would take would be to: Replace: if (renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) by: if ((renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) || (!renderSurface && m_compositeOffscreen)) Then you won't need to call bindFramebuffer, setDrawViewportRect or m_currentRenderSurface == 0 here. > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:647 > + m_context->colorMask(true, true, true, true); You can avoid doing this clear every frame if you just do it once when setCompositeOffscreen(true) is called and keep the color mask set to (true, true, true, false). > WebKit/chromium/src/WebSettingsImpl.cpp:287 > +void WebSettingsImpl::setCompositeToTextureEnabled(bool enabled) Since this is a setting that's only applicable to Chromium and not the other WebKit ports, I don't think it should be propagated down to the Settings class. The right way to do this would be to add another member to WebSettingsImpl and store the setting value there. WebViewImpl has access to WebSettingsImpl via WebViewImpl::settings()

(In reply to comment #6) > (From update of attachment 78717 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78717&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643 > > + GLC(m_context, m_context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, 0)); > > It would be nice to re-factor LayerRendererChromium::useRenderSurface() such that when you pass zero as your render surface and m_compositeOffscreen == true then it renders to the display. I think it would be fairly straightforward. It looks like all it would take would be to: > > Replace: > if (renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) > > by: > if ((renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) || (!renderSurface && m_compositeOffscreen)) > > Then you won't need to call bindFramebuffer, setDrawViewportRect or m_currentRenderSurface == 0 here. I can make this change, although at present I don't think it's possible to call useRenderSurface() with a zero-valued surface if m_compositeOffscreen is true. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:647 > > + m_context->colorMask(true, true, true, true); > > You can avoid doing this clear every frame if you just do it once when setCompositeOffscreen(true) is called and keep the color mask set to (true, true, true, false). > > > WebKit/chromium/src/WebSettingsImpl.cpp:287 > > +void WebSettingsImpl::setCompositeToTextureEnabled(bool enabled) > > Since this is a setting that's only applicable to Chromium and not the other WebKit ports, I don't think it should be propagated down to the Settings class. The right way to do this would be to add another member to WebSettingsImpl and store the setting value there. WebViewImpl has access to WebSettingsImpl via WebViewImpl::settings() No problem ... I've uploaded a new patch ... let me know if the new patch is what you have in mind.

Created attachment 78738 [details] Patch

(In reply to comment #7) Sorry, forgot one bullet item: > > You can avoid doing this clear every frame if you just do it once when setCompositeOffscreen(true) is called and keep the color mask set to (true, true, true, false). > > Will this work if setCompositeOffscreen(true) isn't called inside of doComposite(), i.e. it's called once when the render layer is created an then never again?

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 78717 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=78717&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643 > > > + GLC(m_context, m_context->bindFramebuffer(GraphicsContext3D::FRAMEBUFFER, 0)); > > > > It would be nice to re-factor LayerRendererChromium::useRenderSurface() such that when you pass zero as your render surface and m_compositeOffscreen == true then it renders to the display. I think it would be fairly straightforward. It looks like all it would take would be to: > > > > Replace: > > if (renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) > > > > by: > > if ((renderSurface == m_defaultRenderSurface && !m_compositeOffscreen) || (!renderSurface && m_compositeOffscreen)) > > > > Then you won't need to call bindFramebuffer, setDrawViewportRect or m_currentRenderSurface == 0 here. > > I can make this change, although at present I don't think it's possible to call useRenderSurface() with a zero-valued surface if m_compositeOffscreen is true. What I was suggesting is that if you make this change, you should be able to call useRenderSurface(0) here instead of making all the individual calls. Would that work? > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:647 > > > + m_context->colorMask(true, true, true, true); > > > > You can avoid doing this clear every frame if you just do it once when setCompositeOffscreen(true) is called and keep the color mask set to (true, true, true, false). > > > > > WebKit/chromium/src/WebSettingsImpl.cpp:287 > > > +void WebSettingsImpl::setCompositeToTextureEnabled(bool enabled) > > > > Since this is a setting that's only applicable to Chromium and not the other WebKit ports, I don't think it should be propagated down to the Settings class. The right way to do this would be to add another member to WebSettingsImpl and store the setting value there. WebViewImpl has access to WebSettingsImpl via WebViewImpl::settings() > > No problem ... I've uploaded a new patch ... let me know if the new patch is what you have in mind.

(In reply to comment #9) > (In reply to comment #7) > > Sorry, forgot one bullet item: > > > > You can avoid doing this clear every frame if you just do it once when setCompositeOffscreen(true) is called and keep the color mask set to (true, true, true, false). > > > > > Will this work if setCompositeOffscreen(true) isn't called inside of doComposite(), i.e. it's called once when the render layer is created an then never again? It should, in theory. If you clear the alpha channel of the backbuffer once at the very beginning and then mask alpha writes off, then the alpha channel should stay clean.

Created attachment 78809 [details] Patch

(In reply to comment #10) > What I was suggesting is that if you make this change, you should be able to call useRenderSurface(0) here instead of making all the individual calls. Would that work? Ahh, so you're suggesting using it to simplify copyOffscreenTextureToDisplay()? If so, then yes, it seems to work well (see new patch). If this is not what you had in mind let me know. (In reply to comment #11) > > > > Will this work if setCompositeOffscreen(true) isn't called inside of doComposite(), i.e. it's called once when the render layer is created an then never again? > > It should, in theory. If you clear the alpha channel of the backbuffer once at the very beginning and then mask alpha writes off, then the alpha channel should stay clean. OK, I've implemented this, and it seems to test OK. I've moved the call to setCompositeOffscreen() to WebViewImpl::resize() to avoid clearing the screen on each and every call to doComposite(), but making sure to clear whenever the screen size changes. Question: is WebViewImpl::resize() guaranteed to be called at least once before doComposite()? I'm guessing 'yes', and it seems to test OK, but I'm not 100% sure.

Comment on attachment 78809 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78809&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:639 > + setDrawViewportRect(IntRect(0, 0, m_rootLayerTextureWidth, m_rootLayerTextureHeight), true); Could the two calls above be replaced by "useRenderSurface(0)" ? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643 > + m_context->clearColor(1, 0, 0, 1); // Clear to red to detect regions not composite. "not composite" -> "not composited" ? > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:658 > + m_currentRenderSurface = 0; // Need to do this, or else next call to useRenderSurface will still be bound to I don't believe you need to make this call now. It should be taken care of when calling useRenderSurface(0) > WebKit/chromium/src/WebViewImpl.cpp:972 > + m_layerRenderer->setCompositeOffscreen(settings()->compositeToTextureEnabled()); // Clears display. You probably need to define a separate clearDisplay() method in m_layerRenderer as setCompositeOffScreen uses m_rootLayerTextureWidth/Height which won't be correct while resizing. However, I'm getting a bit confused now as to why we need to clear the window.. DrawLayers does a clear that affects the alpha channel so the resulting offscreen texture should always have alpha = 1. Why do we need to clear again?

Created attachment 78846 [details] Patch

(In reply to comment #14) > (From update of attachment 78809 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78809&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:639 > > + setDrawViewportRect(IntRect(0, 0, m_rootLayerTextureWidth, m_rootLayerTextureHeight), true); > > Could the two calls above be replaced by "useRenderSurface(0)" ? Yes, but these calls are now gone - see below. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:643 > > + m_context->clearColor(1, 0, 0, 1); // Clear to red to detect regions not composite. > > "not composite" -> "not composited" ? fixed. > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:658 > > + m_currentRenderSurface = 0; // Need to do this, or else next call to useRenderSurface will still be bound to > > I don't believe you need to make this call now. It should be taken care of when calling useRenderSurface(0) Yes, removed. > > WebKit/chromium/src/WebViewImpl.cpp:972 > > + m_layerRenderer->setCompositeOffscreen(settings()->compositeToTextureEnabled()); // Clears display. > > You probably need to define a separate clearDisplay() method in m_layerRenderer as setCompositeOffScreen uses m_rootLayerTextureWidth/Height which won't be correct while resizing. However, I'm getting a bit confused now as to why we need to clear the window.. DrawLayers does a clear that affects the alpha channel so the resulting offscreen texture should always have alpha = 1. Why do we need to clear again? The need to clear arose as part of fixing an incorrect test result for compositing/geometry/fixed-position.html (only during offscreen compositing), but recent changes (possibly outside this patch?) seem to have made clearing unnecessary. I have removed this code from setCompositeOffscreen() and moved the call to setCompositeOffscreen() back to doComposite(), as it no longer has high overhead associated with it.

Just to update: I have successfully tested the current version of the patch on both Mac and Linux (with compositing-to-texture both 'on' and 'off').

Comment on attachment 78846 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=78846&action=review > WebKit/chromium/src/WebViewImpl.cpp:972 > + m_layerRenderer->setCompositeOffscreen(settings()->compositeToTextureEnabled()); Is this the right place to make this call? Shouldn't it be made inside doComposite()?

Created attachment 78854 [details] Patch

(In reply to comment #18) > (From update of attachment 78846 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=78846&action=review > > > WebKit/chromium/src/WebViewImpl.cpp:972 > > + m_layerRenderer->setCompositeOffscreen(settings()->compositeToTextureEnabled()); > > Is this the right place to make this call? Shouldn't it be made inside doComposite()? It was, but somehow when I created a git branch to commit from, it didn't get that change (I tried rebasing, which apparently didn't do what I expected). Should be OK now.

Comment on attachment 78854 [details] Patch One more question: Does setCompositeOffScreen() still need to set the layerRenderer for the root layer? When we're compositing off screen, we'll pass 0 as the argument to this function so prepareContentsTexture() will never be called. I think this could simplify setCompositeOffScreen to the point that it just sets the value of m_compositeOffscreen. Or maybe I'm missing something here.

(In reply to comment #21) > (From update of attachment 78854 [details]) > One more question: Does setCompositeOffScreen() still need to set the layerRenderer for the root layer? When we're compositing off screen, we'll pass 0 as the argument to this function so prepareContentsTexture() will never be called. I think this could simplify setCompositeOffScreen to the point that it just sets the value of m_compositeOffscreen. Or maybe I'm missing something here. I don't think this will work. When compositing to a texture, instead of to the screen, the RenderSurfaceChromium object that manages the texture we're compositing into needs to create a GL texture, and to do this it needs to call prepareContentsTexture() and this in turn requires a non-null pointer to a layerRenderer so that it knows what texture manager and which context to associate the texture with ... Commenting out the call "m_rootLayer->setLayerRenderer(this);" in setCompositeOffscreen() leads to a null-pointer dereference in the current code and thus "Aw, snap!". I assume when you say "we'll pass 0 as the argument to this function" you mean that final display of the texture (in the testing code path) will call useRenderSurface(0).

> ... the RenderSurfaceChromium object that manages the texture we're compositing into needs to create a GL texture, and to do this it needs to call prepareContentsTexture() and this in turn requires a non-null pointer to a layerRenderer so that it knows what texture manager and which context to associate the texture with ... > I was thinking ... it seems odd to be able to create a RenderSurfaceChromium with no LayerRenderer since subsequent calls to prepareContentTexture don't check for this condition, and thus die. Is there any way to have a default LayerRenderer, perhaps created and stored as a static var in RenderSurfaceChromium, which is used if no LayerRenderer is specified?

(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 78854 [details] [details]) > > One more question: Does setCompositeOffScreen() still need to set the layerRenderer for the root layer? When we're compositing off screen, we'll pass 0 as the argument to this function so prepareContentsTexture() will never be called. I think this could simplify setCompositeOffScreen to the point that it just sets the value of m_compositeOffscreen. Or maybe I'm missing something here. > > I don't think this will work. > > When compositing to a texture, instead of to the screen, the RenderSurfaceChromium object that manages the texture we're compositing into needs to create a GL texture, and to do this it needs to call prepareContentsTexture() and this in turn requires a non-null pointer to a layerRenderer so that it knows what texture manager and which context to associate the texture with ... > > Commenting out the call "m_rootLayer->setLayerRenderer(this);" in setCompositeOffscreen() leads to a null-pointer dereference in the current code and thus "Aw, snap!". Oh, I see. Good point. Here's something we could do: Modify LayerRendererChromium::setRootLayer() to call setLayerRenderer. This seems like a more natural place to make the call, and hopefully it will be made only as needed. If that works then LayerRendererChromium::setCompositeOffscreen() could be simplified as: void LayerRendererChromium::setCompositeOffscreen(bool compositeOffscreen) { if (m_compositeOffscreen == compositeOffscreen) return; m_compositeOffscreen = compositeOffscreen; if (!m_compositeOffscreen) m_rootLayer->m_renderSurface.clear(); } > > I assume when you say "we'll pass 0 as the argument to this function" you mean that final display of the texture (in the testing code path) will call useRenderSurface(0).

(In reply to comment #24) > > Oh, I see. Good point. Here's something we could do: Modify LayerRendererChromium::setRootLayer() to call setLayerRenderer. This seems like a more natural place to make the call, and hopefully it will be made only as needed. If that works then LayerRendererChromium::setCompositeOffscreen() could be simplified as: > > void LayerRendererChromium::setCompositeOffscreen(bool compositeOffscreen) > { > if (m_compositeOffscreen == compositeOffscreen) > return; > > m_compositeOffscreen = compositeOffscreen; > > if (!m_compositeOffscreen) > m_rootLayer->m_renderSurface.clear(); > } Sounds good, although I've run into one slight hitch: when I make the modification (see diff below), one test starts failing --- compositing/iframes/composited-iframe-alignment.html renders upside down! It renders right-side up without this change. Off the top of my head, I don't know why this is. diff --git a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp index ce7b868..7c163ad 100644 --- a/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp +++ b/Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp @@ -340,6 +340,8 @@ void LayerRendererChromium::present() void LayerRendererChromium::setRootLayer(PassRefPtr<LayerChromium> layer) { m_rootLayer = layer; + if (m_rootLayer) + m_rootLayer->setLayerRenderer(this); m_rootLayerTiler->invalidateEntireLayer(); if (m_horizontalScrollbarTiler) m_horizontalScrollbarTiler->invalidateEntireLayer(); @@ -621,18 +623,12 @@ void LayerRendererChromium::updateLayersRecursive(LayerChromium* layer, const Tr void LayerRendererChromium::setCompositeOffscreen(bool compositeOffscreen) { - m_compositeOffscreen = compositeOffscreen; + if (m_compositeOffscreen == compositeOffscreen) + return; - if (!m_rootLayer) { - m_compositeOffscreen = false; - return; - } + m_compositeOffscreen = compositeOffscreen; - if (m_compositeOffscreen) { - // Need to explicitly set a LayerRendererChromium for the layer with the offscreen texture, - // or else the call to prepareContentsTexture() in useRenderSurface() will fail. - m_rootLayer->setLayerRenderer(this); - } else + if (!m_compositeOffscreen && m_rootLayer) m_rootLayer->m_renderSurface.clear(); }

> Sounds good, although I've run into one slight hitch: when I make the modification (see diff below), one test starts failing --- compositing/iframes/composited-iframe-alignment.html renders upside down! It renders right-side up without this change. Off the top of my head, I don't know why this is. Strange... I don't think I see why that would happen.. Is this repeatable or is there maybe a race condition somewhere that this seems to tickle?

(In reply to comment #26) > > Sounds good, although I've run into one slight hitch: when I make the modification (see diff below), one test starts failing --- compositing/iframes/composited-iframe-alignment.html renders upside down! It renders right-side up without this change. Off the top of my head, I don't know why this is. > > Strange... I don't think I see why that would happen.. Is this repeatable or is there maybe a race condition somewhere that this seems to tickle? I think I'm suffering from a git problem. Now the previous version of the patch is giving me problems. I'll get back to this Monday morning :-(

Created attachment 79007 [details] Patch

(In reply to comment #28) > Created an attachment (id=79007) [details] > Patch OK, I've un-snarled my repo ... here's the patch, but I haven't fully tested it (compositing/iframes/composited-iframe-alignment.html looks OK in Debug/chrome, but I haven't tried it in DumpRenderTree). Will continue this Monday ...

OK, here's the latest update: - I updated my repo to pull in the WebKit re-structuring changes from the weekend - I've re-tested my patch - DumpRenderTree seems to crash on many tests ... - ... but those same tests seem to pass on chrome This doesn't seem to be related to moving the call to setLayerRenderer() into setRootLayer() - and DRT was working with offscreen compositing last week -has DRT changed recently? Is there any reason why it would behave this differently?

(In reply to comment #30) I've found the problem - good news is I was mistaken, and it's not DRT's fault. The problem stems from using useRenderSurface(0); in the call to copyOffscreenTextureToDisplay(). This has the side effect of setting m_currentRenderSurface = renderSurface; // = 0 which then causes a null pointer dereference in when we call m_defaultRenderSurface->draw(), since it calls setScissorToRect(), which attempts int scissorX = scissorRect.x() - m_currentRenderSurface->m_contentRect.x(); Should I modify setScissorToRect() to handle this case (use rootLayer rect instead?), or stop using useRenderSurface(0)?

(In reply to comment #31) > (In reply to comment #30) > > I've found the problem - good news is I was mistaken, and it's not DRT's fault. > > The problem stems from using > > useRenderSurface(0); > > in the call to copyOffscreenTextureToDisplay(). This has the side effect of setting > > m_currentRenderSurface = renderSurface; // = 0 > > which then causes a null pointer dereference in when we call m_defaultRenderSurface->draw(), since it calls setScissorToRect(), which attempts > > int scissorX = scissorRect.x() - m_currentRenderSurface->m_contentRect.x(); > > Should I modify setScissorToRect() to handle this case (use rootLayer rect instead?), or stop using useRenderSurface(0)? Oops. Good catch. Modifying setScissorRect is reasonable as the method needs to be able to deal with drawing directly to the window.

Created attachment 79326 [details] Patch

(In reply to comment #33) > Created an attachment (id=79326) [details] > Patch OK, this patch is an attempt to recover after the confusion of the last patch, where I suspect I was quite testing what I thought I was testing ... Comments: 1) I have added a function called clearFramebuffer() to LayerRendererChromium and called it from initializeSharedObjects() to clear the display. Without this, a large number of tests fail on a Skia assertion SkASSERT(r < a) suggesting the alpha channel is unhappy. Placing it in initializeSharedObjects() means it gets cleared only once. Question: is there any reason we should also do clearFramebuffer(m_offscreenFramebufferId)? 2) I have proposed a fix for setScissorRect() for the use case useRenderSurface(0) ... let me know if using thew contentRect from m_defaultRenderSurface seems correct. It seems to display OK for all cases I've tried. 3) Similarly, I've had to test for renderSurface == 0 in useRenderSurface and default to setting the drawViewPortRect according to m_defaultRenderSurface when renderSurface is 0. 4) A small number of test cases seem to have slight changes in pixel intensities --- imperceptible to my eye --- that seem to again be related to alpha channel values. One in particular, compositing/iframes/composited-iframe-alignment.html, tests fine if I delete the line clearFramebuffer(0) from initializeSharedObjects(), but this of course messes up a bunch of other tests (see (1)). I would like to proceed with the patch despite these tests (can we temporarily mark them as expected-fail for offscreen compositing, or rebaseline for the offscreen case?). I will continue to try and hunt down the cause, but I think this will be easier once offscreen-compositing is part of the test suite (to prevent any other regressions).

Comment on attachment 79326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79326&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:732 > + IntRect contentRect(m_defaultRenderSurface->m_contentRect); To avoid double assignment you could write this as: IntRect contentRect = (m_currentRenderSurface ? m_currentRenderSurface->m_contentRect : m_defaultRenderSurface->m_contentRect)

(In reply to comment #34) > (In reply to comment #33) > > Created an attachment (id=79326) [details] [details] > > Patch > > OK, this patch is an attempt to recover after the confusion of the last patch, where I suspect I was quite testing what I thought I was testing ... > > Comments: > > 1) I have added a function called clearFramebuffer() to LayerRendererChromium and called it from initializeSharedObjects() to clear the display. Without this, a large number of tests fail on a Skia assertion > > SkASSERT(r < a) > I think there's a good opportunity now that you're getting the assert to understand what's going wrong here. Clearing the framebuffer at init time seems to mostly work but I'm curious why (and why it causes that one failure) and whether it's masking some other problem. Have you tried calling: m_context->colorMask(true, true, true, true) in copyOffscreenTextureToDisplay() before drawing ? The texture attached to the m_defaultRenderSurface should have a clear alpha channel. > suggesting the alpha channel is unhappy. Placing it in initializeSharedObjects() means it gets cleared only once. > > Question: is there any reason we should also do clearFramebuffer(m_offscreenFramebufferId)? > > 2) I have proposed a fix for setScissorRect() for the use case useRenderSurface(0) ... let me know if using thew contentRect from m_defaultRenderSurface seems correct. It seems to display OK for all cases I've tried. > > 3) Similarly, I've had to test for renderSurface == 0 in useRenderSurface and default to setting the drawViewPortRect according to m_defaultRenderSurface when renderSurface is 0. > > 4) A small number of test cases seem to have slight changes in pixel intensities --- imperceptible to my eye --- that seem to again be related to alpha channel values. One in particular, compositing/iframes/composited-iframe-alignment.html, tests fine if I delete the line clearFramebuffer(0) from initializeSharedObjects(), but this of course messes up a bunch of other tests (see (1)). > > I would like to proceed with the patch despite these tests (can we temporarily mark them as expected-fail for offscreen compositing, or rebaseline for the offscreen case?). I will continue to try and hunt down the cause, but I think this will be easier once offscreen-compositing is part of the test suite (to prevent any other regressions).

Created attachment 79426 [details] Patch

(In reply to comment #35) > (From update of attachment 79326 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79326&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:732 > > + IntRect contentRect(m_defaultRenderSurface->m_contentRect); > > To avoid double assignment you could write this as: > IntRect contentRect = (m_currentRenderSurface ? m_currentRenderSurface->m_contentRect : m_defaultRenderSurface->m_contentRect) Thanks ... I always seem to forget that assignment in a declaration invokes the copy-constructor. (In reply to comment #36) >I think there's a good opportunity now that you're getting the assert to understand what's going wrong here. Clearing the framebuffer at init time seems to mostly work but I'm curious why (and why it causes that one failure) and whether it's masking some other problem. > >Have you tried calling: >m_context->colorMask(true, true, true, true) >in copyOffscreenTextureToDisplay() before drawing ? The texture attached to the m_defaultRenderSurface should have a clear alpha channel. This helps in that I can remove the code that clears the display in initializeSharedObjects(); I have updated to do this and remove clearFramebuffer() as it is not used now. However, it doesn't clear up why we are getting those sporadic intensity changes. I suspect somehow the alpha channel is behind this, but I haven't figured out where/how this happens. I wondered if the call to clear() in drawLayers() might be somehow treating the texture differently from the display, although I can find no evidence for this. The two interesting features are: 1) The sporadic pixels are most often (but not always) associated with text, and 2) their frequency tends to increase as x increases.

(In reply to comment #38) Another data point: these sporadic pixel intensity changes don't seem to occur on Mac, except in one test --- fast/canvas/shadow-offset-7.html : this test works fine on Linux, and on Mac comparing actual vs. expected output gives no perceptible difference).

(In reply to comment #38) > (In reply to comment #35) > > (From update of attachment 79326 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=79326&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:732 > > > + IntRect contentRect(m_defaultRenderSurface->m_contentRect); > > > > To avoid double assignment you could write this as: > > IntRect contentRect = (m_currentRenderSurface ? m_currentRenderSurface->m_contentRect : m_defaultRenderSurface->m_contentRect) > > Thanks ... I always seem to forget that assignment in a declaration invokes the copy-constructor. > > (In reply to comment #36) > > >I think there's a good opportunity now that you're getting the assert to understand what's going wrong here. Clearing the framebuffer at init time seems to mostly work but I'm curious why (and why it causes that one failure) and whether it's masking some other problem. > > > >Have you tried calling: > > >m_context->colorMask(true, true, true, true) > > >in copyOffscreenTextureToDisplay() before drawing ? The texture attached to the m_defaultRenderSurface should have a clear alpha channel. > > This helps in that I can remove the code that clears the display in initializeSharedObjects(); I have updated to do this and remove clearFramebuffer() as it is not used now. > > However, it doesn't clear up why we are getting those sporadic intensity changes. I suspect somehow the alpha channel is behind this, but I haven't figured out where/how this happens. I wondered if the call to clear() in drawLayers() might be somehow treating the texture differently from the display, although I can find no evidence for this. > When you say sporadic, do you mean different tests failing in every run or it always the same tests that fail? > The two interesting features are: > > 1) The sporadic pixels are most often (but not always) associated with text, and > > 2) their frequency tends to increase as x increases. What is "x" ? The horizontal coordinate in the page?

(In reply to comment #38) > (In reply to comment #35) > > (From update of attachment 79326 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=79326&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:732 > > > + IntRect contentRect(m_defaultRenderSurface->m_contentRect); > > > > To avoid double assignment you could write this as: > > IntRect contentRect = (m_currentRenderSurface ? m_currentRenderSurface->m_contentRect : m_defaultRenderSurface->m_contentRect) > > Thanks ... I always seem to forget that assignment in a declaration invokes the copy-constructor. > > (In reply to comment #36) > > >I think there's a good opportunity now that you're getting the assert to understand what's going wrong here. Clearing the framebuffer at init time seems to mostly work but I'm curious why (and why it causes that one failure) and whether it's masking some other problem. > > > >Have you tried calling: > > >m_context->colorMask(true, true, true, true) > > >in copyOffscreenTextureToDisplay() before drawing ? The texture attached to the m_defaultRenderSurface should have a clear alpha channel. > > This helps in that I can remove the code that clears the display in initializeSharedObjects(); I have updated to do this and remove clearFramebuffer() as it is not used now. > > However, it doesn't clear up why we are getting those sporadic intensity changes. I suspect somehow the alpha channel is behind this, but I haven't figured out where/how this happens. I wondered if the call to clear() in drawLayers() might be somehow treating the texture differently from the display, although I can find no evidence for this. Also, are the differences now limited to the color channels? Is alpha always 1.0? > > The two interesting features are: > > 1) The sporadic pixels are most often (but not always) associated with text, and Does it look like it's around the edges of letters? I wonder if it's somehow caused by subpixel AA in text... > > 2) their frequency tends to increase as x increases.

(In reply to comment #40) > > When you say sporadic, do you mean different tests failing in every run or it always the same tests that fail? Always the same tests. 'Sporadic' in the sense that there is no strong pattern as to which pixels are affected, although I suspect the output is identical from one trial to the next. > What is "x" ? The horizontal coordinate in the page? Yes .... sorry, I should have been more specific :-) (In reply to comment #41) >Also, are the differences now limited to the color channels? Is alpha always 1.0? I'm guessing ImageDiff just shows color channel differences ... I don't know for sure. I suppose it could be alpha ... but the -actual.png images appear to just be RGB, so if alpha is related it's been multiplied in. > Does it look like it's around the edges of letters? I wonder if it's somehow caused by subpixel AA in text... It looks like it, although it's hard to tell ... the testing font is so 'skinny' in most of the tests that it's not always obvious if there are any interior pixels. On fast/canvas/canvas-text-alignment-diff.png it certainly looks to be limited to exterior font pixels. I'll upload the actual output from this test as an attachment.

Created attachment 79479 [details] Example of image-mistach diff image for fast/canvas/canvas-text-alignment-diff.png

Any word on the status of this? Since the purpose of this CL is to get a command-line switch working for off-screen compositing (so it can be added to the testing system), I'd like to push ahead with this patch, and use test expectations to handle the subtle pixel differences until they can be resolved in a separate patch. To recap - there are 14 tests that have a small number of pixel differences that are imperceptible to the human eye (mine at least). Typically they occur near edges towards the right-hand side of the window, and involve intensity differences of 2 or less (out of 255). They only occur on Linux (Skia? nVidia graphics driver?) and not on Mac. I suspect they're somehow alpha-channel related, but have not yet figured out what the difference is between compositing to the screen vs. a texture bound to a frame buffer object.

Are you still seeing diffs, even after: http://trac.webkit.org/changeset/76299

Comment on attachment 79426 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=79426&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:645 > + m_context->colorMask(true, true, true, true); Now that the color mask is set to (t,t,t,t) in the LayerRendererChromium ln 266, you should avoid resetting it here. I wonder if this will clear the problems you're seeing..

(In reply to comment #45) > Are you still seeing diffs, even after: > > http://trac.webkit.org/changeset/76299 Yes. The changeset you mention was helpful in resolving the only pixel difference I was encountering on Mac, but it does not resolve the pixel differences I'm seeing on Linux. (In reply to comment #46) > (From update of attachment 79426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=79426&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:645 > > + m_context->colorMask(true, true, true, true); > > Now that the color mask is set to (t,t,t,t) in the LayerRendererChromium ln 266, you should avoid resetting it here. I wonder if this will clear the problems you're seeing.. No, it doesn't help. I tried all 4 combinations of including/removing each of the two calls to colorMask() and the results are the same in each case.

(In reply to comment #47) > (In reply to comment #45) > > Are you still seeing diffs, even after: > > > > http://trac.webkit.org/changeset/76299 > > Yes. > > The changeset you mention was helpful in resolving the only pixel difference I was encountering on Mac, but it does not resolve the pixel differences I'm seeing on Linux. > > (In reply to comment #46) > > (From update of attachment 79426 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=79426&action=review > > > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:645 > > > + m_context->colorMask(true, true, true, true); > > > > Now that the color mask is set to (t,t,t,t) in the LayerRendererChromium ln 266, you should avoid resetting it here. I wonder if this will clear the problems you're seeing.. > > No, it doesn't help. I tried all 4 combinations of including/removing each of the two calls to colorMask() and the results are the same in each case. They colorMask calls should be removed anyway as they're not consistent anymore with the state that the compositor uses (compositor expects color to be unmasked most of the time). My only other guess for the differences you're seeing is that something is off by a fraction of a pixel and you're seeing slight stretching. Our textures are by default using LINEAR filtering. You could try setting the TEXTURE_MIN_FILTER, TEXTURE_MAG_FILTER to NEAREST . You could do that in RenderSurface::prepareContentsTexture, after the call to reserve() you can bind the texture and set the filter modes. The reason I'd like to get to the bottom of this is that it would be nice to know that you can use the off-screen path as your primary render path which means that you'd need the layout tests to work.

(In reply to comment #48) > > They colorMask calls should be removed anyway as they're not consistent anymore with the state that the compositor uses (compositor expects color to be unmasked most of the time). Done. > My only other guess for the differences you're seeing is that something is off by a fraction of a pixel and you're seeing slight stretching. Our textures are by default using LINEAR filtering. You could try setting the TEXTURE_MIN_FILTER, TEXTURE_MAG_FILTER to NEAREST . You could do that in RenderSurface::prepareContentsTexture, after the call to reserve() you can bind the texture and set the filter modes. This seems to have helped. 12 of the 14 failing tests now pass, and the two remaining failures have considerably fewer pixels failing. The the remaining failures are fast/canvas/canvas-text-alignment.html fast/canvas/canvas-text-baseline.html All of the remaining pixel mis-matches are along high-contrast edges (vertical and horizontal lines). > The reason I'd like to get to the bottom of this is that it would be nice to know that you can use the off-screen path as your primary render path which means that you'd need the layout tests to work. Rest assured, I don't want *any* failing testswhen we finish, but I also want to head off regressions that might sneak in in the meantime since the offscreen path is not being tested at present. I'll test the new version of the patch on Mac and then upload it.

Created attachment 80782 [details] Patch

(In reply to comment #50) > Created an attachment (id=80782) [details] > Patch I tried changing LINEAR -> NEAREST in Canvas2DLayerChromium (and even tried disabling the "CLAMP_TO_EDGE" setting), but it doesn't seem to help with the two remaining failing tests. (I've tried playing with a number of other "LINEAR" -> "NEAREST" changes as ell, but nothing helped with these tests.)

Created attachment 81633 [details] Patch

(In reply to comment #52) > Created an attachment (id=81633) [details] > Patch Updated patch to account for recent changes, down to two failing pixel tests (2Dcanvas vs. wide CSS borders interaction).

Comment on attachment 81633 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=81633&action=review > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:761 > +// scissorY = m_currentRenderSurface->m_contentRect.height() - (scissorRect.bottom() - m_currentRenderSurface->m_contentRect.y()); // mine - old Commented out lines need to be removed. > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:180 > + GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::NEAREST)); I suggested turning the filtering to NEAREST just to help troubleshoot the failing tests. Generally we want LINEAR filtering when drawing render surfaces.

(In reply to comment #54) > (From update of attachment 81633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=81633&action=review > > > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:761 > > +// scissorY = m_currentRenderSurface->m_contentRect.height() - (scissorRect.bottom() - m_currentRenderSurface->m_contentRect.y()); // mine - old > > Commented out lines need to be removed. I will fix this. I forgot to remove comments I made while resolving a merge. > > Source/WebCore/platform/graphics/chromium/RenderSurfaceChromium.cpp:180 > > + GLC(context3D, context3D->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, GraphicsContext3D::NEAREST)); > > I suggested turning the filtering to NEAREST just to help troubleshoot the failing tests. Generally we want LINEAR filtering when drawing render surfaces. OK. I'll go back and look at it again.

Here's a shot in the dark - feedback is appreciated ... Hypothesis: the failing pixels are due to precision limitations in the shader computations (most likely the fragment shader, but possibly a combination of both). This becomes pronounced when trying to sample a large texture in a single chunk. For Desktop GL we get max 23 bits (float mantissa) for shader floats, although for OpenGLES we're only guaranteed 2^(-10) precision in fragment shaders using mediump (i.e. 10 bits, highp not guaranteed to be available). Observation: The failures always seem to occur when (i) rendering pixels with large x or y values (i.e. close to 1.0 normalized), (ii) always at high contrast edges (usually 0 -> 255 transitions from what I've seen), and (iii) in some cases it is most tenacious when two or more textures are composited together. (iv) All the tests render properly on my Mac workstation. Some Numbers (back-of-the-envelope calculations only): # of bits required (for the texture coordinate) to safely linearly sample across a 0->255 boundary, assuming rounding: 9 bits (must be off by less than 0.5 of an intensity value). # of bits to represent an 800 pixel-wide texture = 10 bits That's about 19 bits ... and we also have to account for errors in accumulating transforms, etc. Could we be just hitting the limit of precision of the GPUs? (Does the FX380 in the Linux z600 have poorer precision than the card in the Mac? The Radeon card in my Mac guarantees IEEE standard) Even if this is not the problem for the desktop GL cards, it may well be for OpenGL ES implementations on mobile devices. Rendering direct to display may suffer from the same problem, but it's not so obvious since the baselines are done w.r.t. this case. Note that as texture coordinates approach 1.0, the effective resolution drops (e.g. we can easily represent 0.0 + 2^(-23) but not so easily 0.9 + 2^(-23), so perhaps this explains why the problem becomes more prevalent as the coordinates move to the right/up (close to 1.0). If this is true, then the fact that NEAREST sampling solves most of the cases is not surprising. Conclusion: This *may* be a problem that is not due to any error in the code. It may also be that it will reduce/disappear when tiling is applied to all layers (although the notion of fuzzy testing may need consideration too). We can try to devise appropriate tests if the idea seems worth further investigation.

Hi James, Interesting analysis, however I don't think in this case it's a 23 vs 10 bit precision issue. Even though we pretend to be running OGL ES, on linux we're really running on desktop GL and the mediump qualifier gets ignored (as it's not supported in desktop GL). In addition, if you're running the tests via DRT I believe that even if you run them on your desktop we will be using MESA and not the real h/w. That said, I agree that the issues you're seeing could very well be related to numerical imprecision when sampling the render target texture to do the final render into the frame buffer. At this point I don't want to let them block this patch. (In reply to comment #56) > Here's a shot in the dark - feedback is appreciated ... > > Hypothesis: the failing pixels are due to precision limitations in the shader computations (most likely the fragment shader, but possibly a combination of both). This becomes pronounced when trying to sample a large texture in a single chunk. > > For Desktop GL we get max 23 bits (float mantissa) for shader floats, although for OpenGLES we're only guaranteed 2^(-10) precision in fragment shaders using mediump (i.e. 10 bits, highp not guaranteed to be available). >

Vangelis, can I consider your comment above an unofficial r+?

(In reply to comment #58) > Vangelis, can I consider your comment above an unofficial r+? There are a few comments in the last patch that need to be addressed.

Created attachment 82803 [details] Patch

(In reply to comment #57) > Hi James, > Interesting analysis, however I don't think in this case it's a 23 vs 10 bit precision issue. Even though we pretend to be running OGL ES, on linux we're really running on desktop GL and the mediump qualifier gets ignored (as it's not supported in desktop GL). In addition, if you're running the tests via DRT I believe that even if you run them on your desktop we will be using MESA and not the real h/w. Sorry, I didn't mean to suggest that in this case we we're dealing with the OpenGLES 10-bit minimum guarantee. I mentioned this only because future hardware we may target may have significantly lower precision than we are currently using. The analysis in my previous comment was only meant to show that even with full float precision we may be getting into the realm where precision is a limiting factor for larger textures. >That said, I agree that the issues you're seeing could very well be related to numerical imprecision when sampling the render target texture to do the final render into the frame buffer. At this point I don't want to let them block this patch. Great, thanks! (In reply to comment #59) >(In reply to comment #58) >> Vangelis, can I consider your comment above an unofficial r+? > > There are a few comments in the last patch that need to be addressed. I have submitted a new patch that 1) updates the patch to the current tree state, 2) removes the commented lines of code from the previous patch as requested, and 3) restores LINEAR texture sampling (or removes NEAREST, depending on how you want to look at it). I think this is everything that was outstanding. Let me know if there's anything else.

Comment on attachment 82803 [details] Patch (unofficial) r+ from me!

Comment on attachment 82803 [details] Patch Official r+ from me

The commit-queue encountered the following flaky tests while processing attachment 82803 [details]: http/tests/xmlhttprequest/remember-bad-password.html bug 51733 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 82803 [details] Patch Clearing flags on attachment: 82803 Committed r78853: <http://trac.webkit.org/changeset/78853>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/78853 might have broken Windows Release (Build)