Bug 50833 – [chromium] Add support to compositor to composite to offscreen texture.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Add support to compositor to composite to offscreen texture.

[W. James MacLean]

Created attachment 76228 [details]
Patch

[W. James MacLean]

*** The initial patch for this issue is posted for comments/discussion only, and is not intended to represent the final interface. It has not been marked for review. ***

Please add names to CC list who may have important input to this discussion.

The attached patch is experimental code which causes the compositor to composite into an offscreen texture (attached to a RenderSurfaceChromium object attached to m_rootLayer in LayerRendererChromium). It is meant to show a possible method for extending the compositor to do this, and includes sample code in WebViewImpl to actually invoke the code for testing.

* Comments on the approach?

* Have I missed anything that might break compositing? So far testing has been limited to

http://webkit.org/blog-files/3d-transforms/poster-circle.html

http://webkit.org/blog/386/3d-transforms/

http://peter.sh/2010/06/chromium-now-features-gpu-acceleration-and-css-3d-transforms/

Interface functions definitely need careful consideration, and I would like your feedback on what is appropriate. Questions to answer:

* is one off-screen texture enough, or should double-buffering of textures be an option?

* is it sufficient to export the texture ID, or should the entire containing RenderSurfaceChromium object be returned?

* Should LayerRendererChromium::copyOffscreenTextureToDisplay() be retained in the final interface?

[W. James MacLean]

(In reply to comment #1)
> Created an attachment (id=76228) [details]
> Patch

Please ignore the line

m_rootContentRect.setY(15);

I forgot to take this out ... it works fine without.

[W. James MacLean]

Created attachment 76232 [details]
Patch, with some extraneous lines removed.

[W. James MacLean]

James R. -

When updating this morning, I see you are working along similar lines ... can we coordinate our efforts? Please let me know how you would like to proceed.

[James Robinson]

What's the motivation for this functionality?

[W. James MacLean]

(In reply to comment #6)
> What's the motivation for this functionality?

It's for "Baklava" ... we discussed this with KBR and Vangelis when we were down (invited you too, but you were unable to attend). It's to provide browser-level compositing of of a touch-screen keyboard, plus possibly slide-out omnibox/button bar.

[Vangelis Kokkevis]

Comment on attachment 76232 [details]
Patch, with some extraneous lines removed.

View in context: https://bugs.webkit.org/attachment.cgi?id=76232&action=review

Generally looks fine. I think if this were a real implementation you would eliminate the copyOffscreenTextureToDisplay call and you would allocate the additional texture for the default render surface only when the compositor is told to render off screen. Otherwise you would just render straight to the display backbuffer as it's done today.

I also think there might be some additional merging that needs to happen with jamesr's texture manager CL.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:201
> +        if (m_rootLayer->m_renderSurface.get())

you don't have to include the .get().  It's enough to test m_rootLayer->m_renderSurface .

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:294
> +    if (!m_rootLayer->m_renderSurface.get())

remove .get()

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:666
> +    m_renderOffscreen = compositeOffscreen;

nit: Ideally the member name and the variable name should match.  I think I prefer compositeOffscreen to renderOffscreen

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:696
> +        GLC(m_context, m_context->colorMask(true, true, true, false));

I don't know that masking is necessary as it should have been done when you drew into the default render surface so hopefully the alpha channel is ok.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:702
> +                                        contentLayerValues->shaderMatrixLocation(), contentLayerValues->shaderAlphaLocation());

You need to use m_textureLayerShaderMatrixLocation and m_textureLayerShaderAlphaLocation here. Also, after Jamesr patch landed, RenderSurfaceChromium has its own draw method I believe.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:796
> +    // But, if rendering to offscreen texture, we reverse our sense of 'upside down'.

This seems a little strange.  I can see how maybe you don't need to render upside down when rendering to the default render surface but I don't see why you need to flip all the other render surfaces too.

[W. James MacLean]

Created attachment 76549 [details]
Patch

[W. James MacLean]

(In reply to comment #8)
> (From update of attachment 76232 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76232&action=review
> 
> Generally looks fine. I think if this were a real implementation you would eliminate the copyOffscreenTextureToDisplay call and you would allocate the additional texture for the default render surface only when the compositor is told to render off screen. Otherwise you would just render straight to the display backbuffer as it's done today.
> 
> I also think there might be some additional merging that needs to happen with jamesr's texture manager CL.

This is done in the current patch. Most of the changes are in copyOffscreenTextureToDisplay(), which will disappear once this patch is past "discussion" and into "review".

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:201
> > +        if (m_rootLayer->m_renderSurface.get())
> 
> you don't have to include the .get().  It's enough to test m_rootLayer->m_renderSurface .

Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:294
> > +    if (!m_rootLayer->m_renderSurface.get())
> 
> remove .get()

Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:666
> > +    m_renderOffscreen = compositeOffscreen;
> 
> nit: Ideally the member name and the variable name should match.  I think I prefer compositeOffscreen to renderOffscreen

Agrred, I thought of this too after submitting the previous patches. Fixed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:696
> > +        GLC(m_context, m_context->colorMask(true, true, true, false));
> 
> I don't know that masking is necessary as it should have been done when you drew into the default render surface so hopefully the alpha channel is ok.

Removed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:702
> > +                                        contentLayerValues->shaderMatrixLocation(), contentLayerValues->shaderAlphaLocation());
> 
> You need to use m_textureLayerShaderMatrixLocation and m_textureLayerShaderAlphaLocation here. Also, after Jamesr patch landed, RenderSurfaceChromium has its own draw method I believe.

Redundant, as new patch uses RenderSurfaceChromium::draw() which looks after this.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:796
> > +    // But, if rendering to offscreen texture, we reverse our sense of 'upside down'.
> 
> This seems a little strange.  I can see how maybe you don't need to render upside down when rendering to the default render surface but I don't see why you need to flip all the other render surfaces too.

I'm guessing that the final drawing of the texture also gets flipped, so we need to do this so everything comes out right in the end. Without it, clipping on pages like http://webkit.org/blog/386/3d-transforms/ and http://peter.sh/2010/06/chromium-now-features-gpu-acceleration-and-css-3d-transforms/ are upside-down and translate the wrong way.

I'd like to submit this as a patch for review/commit, so please let me know if it's ready (assuming I'll remove copyOffscreenTextureToDisplay() and the two lines of test code in WebViewImpl.cpp first ...).

[W. James MacLean]

(In reply to comment #10)
> (In reply to comment #8)
> > (From update of attachment 76232 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=76232&action=review
> > 
> > ... and you would allocate the additional texture for the default render surface only when the compositor is told to render off screen. Otherwise you would just render straight to the display backbuffer as it's done today.

Forgot to talk about these two points:

- at present, I rely on the call to prepareContentsTexture() in useRenderSurface() to allocate the texture, and this only happens when drawLayers() is invoked().

- if m_compositeOffscreen is false, then compositing is done in the same way as at present, including compositing directly to the backbuffer.

[Vangelis Kokkevis]

Comment on attachment 76549 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76549&action=review

James, a couple more comments but overall I think it looks good.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:303
> +    m_rootLayer->m_renderSurface->m_contentRect = IntRect(0, 0, m_rootLayerTextureWidth, m_rootLayerTextureHeight);

I think we can move the m_rootLayer's render surface creation to ::prepareToDrawLayers() and set the m_contentRect only when the m_rootLayerTextureWidth changes.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
> +    m_rootLayer->m_renderSurface->m_scissorRect = rootScissorRect; // Do I need this?

No, you don't need to set the scissor rect here.  The render surface's scissor rect is the scissor rect that needs to be applied before drawing the completed surface to its own parent surface.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:786
> +        renderingUpsideDown = !renderingUpsideDown;

I believe the correct logic here would be:

if (m_currentRenderSurface == m_defaultRenderSurface && ! m_compositeOffscreen)
  // flip the scissor
else
  // don't flip the scissor

In other words, I don't think you should be flipping the scissor for all non-root renderSurface's when you're rendering offscreen.

> WebCore/platform/graphics/chromium/LayerRendererChromium.h:162
> +    bool m_compositeOffscreen;

Need to provide a default value for this in the constructor.

[W. James MacLean]

Created attachment 76767 [details]
Patch

[W. James MacLean]

(In reply to comment #12)
> (From update of attachment 76549 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76549&action=review
> 
> James, a couple more comments but overall I think it looks good.
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:303
> > +    m_rootLayer->m_renderSurface->m_contentRect = IntRect(0, 0, m_rootLayerTextureWidth, m_rootLayerTextureHeight);
> 
> I think we can move the m_rootLayer's render surface creation to ::prepareToDrawLayers() and set the m_contentRect only when the m_rootLayerTextureWidth changes.

I agree ... this makes more sense than testing again in drawLayers().

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:339
> > +    m_rootLayer->m_renderSurface->m_scissorRect = rootScissorRect; // Do I need this?
> 
> No, you don't need to set the scissor rect here.  The render surface's scissor rect is the scissor rect that needs to be applied before drawing the completed surface to its own parent surface.

Thanks - removed.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:786
> > +        renderingUpsideDown = !renderingUpsideDown;
> 
> I believe the correct logic here would be:
> 
> if (m_currentRenderSurface == m_defaultRenderSurface && ! m_compositeOffscreen)
>   // flip the scissor
> else
>   // don't flip the scissor
> 
> In other words, I don't think you should be flipping the scissor for all non-root renderSurface's when you're rendering offscreen.

Fixed - I've made this change and tested it, and it works as expected.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.h:162
> > +    bool m_compositeOffscreen;
> 
> Need to provide a default value for this in the constructor.

Fixed.

I've submitted the new patch, and asked for a review.

[James Robinson]

Comment on attachment 76767 [details]
Patch

R=me

[WebKit Commit Bot]

Comment on attachment 76767 [details]
Patch

Clearing flags on attachment: 76767

Committed r74278: <http://trac.webkit.org/changeset/74278>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Reverted r74278 for reason:

[chromium] Causes many layout tests to crash

Committed r74358: <http://trac.webkit.org/changeset/74358>

[W. James MacLean]

Created attachment 77114 [details]
Patch

[W. James MacLean]

(In reply to comment #19)
> Created an attachment (id=77114) [details]
> Patch

I've attached a revised patch which I've manually tested with the WebGL body-viewer demo.

I've attempted to run the full test suite: the update/build commands had problems (see below), but I manually built debug versions of chrome, test_shell and DumpRenderTree and ran:

$ ./Tools/Scripts/new-run-webkit-tests --chromium --debug --platform=chromium-gpu

and I got these results:

  canvas/philip/tests/2d.imageData.get.source.outside.html -> unexpected pass
  canvas/philip/tests/2d.shadow.transform.2.html -> unexpected pass
  fast/canvas/canvas-scale-fillPath-shadow.html -> unexpected pass
  fast/canvas/canvas-scale-fillRect-shadow.html -> unexpected pass
  fast/canvas/canvas-scale-strokePath-shadow.html -> unexpected pass
22727 tests ran as expected, 5 didn't:                  


Expected to fail, but passed: (5)
  canvas/philip/tests/2d.imageData.get.source.outside.html
  canvas/philip/tests/2d.shadow.transform.2.html
  fast/canvas/canvas-scale-fillPath-shadow.html
  fast/canvas/canvas-scale-fillRect-shadow.html
  fast/canvas/canvas-scale-strokePath-shadow.html

Now, I need to determine whether this is in fact a clean set of tests, or if the failure of the update/build commands means it is in fact not to be trusted.

Thoughts?

The build/update command failures are:
-------------------------------------------------------------------------------

./Tools/Scripts/update-webkit --chromium

If I do this on branch 'master' on a freshly checkout-out repo (updated once with git-pull), I get

Updating OpenSource

Unable to determine upstream SVN information from working tree history

Died at ./Tools/Scripts/update-webkit line 132

./Tools/Scripts/build-webkit --chromium --debug

If I try to run this on a branch containing my patch (keeping in mind I couldn't get the update command to run), I get

make -fMakefile.chromium -j16 BUILDTYPE=Debug all
make: Makefile.chromium: No such file or directory
make: *** No rule to make target `Makefile.chromium'. Stop.
-------------------------------------------------------------------------------

[Vangelis Kokkevis]

Comment on attachment 77114 [details]
Patch

The adjustment to avoid the crash from the previous patch looks good.

[Kenneth Russell]

(In reply to comment #20)
> (In reply to comment #19)
> > Created an attachment (id=77114) [details] [details]
> > Patch
> 
> I've attached a revised patch which I've manually tested with the WebGL body-viewer demo.
> 
> I've attempted to run the full test suite: the update/build commands had problems (see below), but I manually built debug versions of chrome, test_shell and DumpRenderTree and ran:
> 
> $ ./Tools/Scripts/new-run-webkit-tests --chromium --debug --platform=chromium-gpu
> 
> and I got these results:
> 
>   canvas/philip/tests/2d.imageData.get.source.outside.html -> unexpected pass
>   canvas/philip/tests/2d.shadow.transform.2.html -> unexpected pass
>   fast/canvas/canvas-scale-fillPath-shadow.html -> unexpected pass
>   fast/canvas/canvas-scale-fillRect-shadow.html -> unexpected pass
>   fast/canvas/canvas-scale-strokePath-shadow.html -> unexpected pass
> 22727 tests ran as expected, 5 didn't:                  
> 
> 
> Expected to fail, but passed: (5)
>   canvas/philip/tests/2d.imageData.get.source.outside.html
>   canvas/philip/tests/2d.shadow.transform.2.html
>   fast/canvas/canvas-scale-fillPath-shadow.html
>   fast/canvas/canvas-scale-fillRect-shadow.html
>   fast/canvas/canvas-scale-strokePath-shadow.html

I updated the GPU test expectations yesterday to expect that these five tests will pass, so this is fine.

> Now, I need to determine whether this is in fact a clean set of tests, or if the failure of the update/build commands means it is in fact not to be trusted.
> 
> Thoughts?
> 
> The build/update command failures are:
> -------------------------------------------------------------------------------
> 
> ./Tools/Scripts/update-webkit --chromium
> 
> If I do this on branch 'master' on a freshly checkout-out repo (updated once with git-pull), I get
> 
> Updating OpenSource
> 
> Unable to determine upstream SVN information from working tree history
> 
> Died at ./Tools/Scripts/update-webkit line 132
> 
> ./Tools/Scripts/build-webkit --chromium --debug
> 
> If I try to run this on a branch containing my patch (keeping in mind I couldn't get the update command to run), I get
> 
> make -fMakefile.chromium -j16 BUILDTYPE=Debug all
> make: Makefile.chromium: No such file or directory
> make: *** No rule to make target `Makefile.chromium'. Stop.
> ------------------------------------------------------------------------------

I don't know why this is failing. I use a Subversion checkout of the WebKit tree configured within my Chromium repo per http://www.chromium.org/developers/contributing-to-webkit .

[Kenneth Russell]

Comment on attachment 77114 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77114&action=review

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:682
> +    }

This logic only handles the transition from m_compositeOffscreen from false to true. What about the other direction?

[W. James MacLean]

(In reply to comment #23)
> (From update of attachment 77114 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77114&action=review
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:682
> > +    }
> 
> This logic only handles the transition from m_compositeOffscreen from false to true. What about the other direction?

It should work both ways. m_compositeOffscreen is set to whatever the value of the argument to setCompositeOffscreen says, but can be overriden is m_rootLayer is false.

Resources allocated going from false to true aren't necessarily de-allocated going from true to false, but it does go back to rendering direct to screen.

Is that what you were wondering about?

[Kenneth Russell]

(In reply to comment #24)
> (In reply to comment #23)
> > (From update of attachment 77114 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77114&action=review
> > 
> > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:682
> > > +    }
> > 
> > This logic only handles the transition from m_compositeOffscreen from false to true. What about the other direction?
> 
> It should work both ways. m_compositeOffscreen is set to whatever the value of the argument to setCompositeOffscreen says, but can be overriden is m_rootLayer is false.
> 
> Resources allocated going from false to true aren't necessarily de-allocated going from true to false, but it does go back to rendering direct to screen.
> 
> Is that what you were wondering about?

Yes, I was essentially wondering about the setting of the layer renderer for the root layer and any associated resources attached to the root layer. It would be good to clean up all of these resources upon the true -> false transition. What do you think about doing this work in this patch?

[W. James MacLean]

Created attachment 77257 [details]
Patch

[W. James MacLean]

Comment on attachment 77257 [details]
Patch

This new patch releases resources immediately if conpositing to an offscreen texture is disabled. It does so by releasing the render surface associated with m_rootLayer. This will also free the corresponding texture. The render surface, without an underlying texture, will be re-created the next time LayerRendererChromium::prepareToDrawLayers() is invoked, and the subsequent call to drawLayers will draw directly to the screenwith no offscreen texture.

[Kenneth Russell]

Comment on attachment 77257 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77257&action=review

Thanks for the cleanup. Basically looks good. I'm happy to r+/cq+ as is, but if you're willing to submit a revised patch that's good too. Assuming you tested the new code path?

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:684
> +        m_rootLayer->m_renderSurface.release();

You could use clear() here. Also, there's really no need to test m_renderSurface.

[W. James MacLean]

(In reply to comment #28)
> (From update of attachment 77257 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77257&action=review
> 
> Thanks for the cleanup. Basically looks good. I'm happy to r+/cq+ as is, but if you're willing to submit a revised patch that's good too. Assuming you tested the new code path?

Yes, I did test it - since the patch as it stands doesn't invoke the offscreen rendering, I created a test-patch that turned offscreen compositing on at the start of WebViewImpl::doComposite() and then off again after copying the texture back to the screen. I ran this on the entire test suite.

> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:684
> > +        m_rootLayer->m_renderSurface.release();
> 
> You could use clear() here. Also, there's really no need to test m_renderSurface.

I assume you mean clear() from LayerTexture? I thought about that and then shied away since (i) it requires another test to see if a texture is allocated, and then (ii) relies on the 'friend'-ness between LayerRendererChromium and RenderSurfaceChromium, as clear() is not exposed through the RenderSurfaceChromium interface. If this is considered OK, then I'm happy to use clear ... let me know. It's probably more efficient to use clear().

I get you comment about not needing to check m_renderSurface ... I keep thinking of it as a regular pointer :-) Although, I will need to check it in order to use clear().

I'm quite happy to iterate on the patch to make it better, so let me know what you think of my comments above.

[Kenneth Russell]

(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 77257 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=77257&action=review
> > 
> > Thanks for the cleanup. Basically looks good. I'm happy to r+/cq+ as is, but if you're willing to submit a revised patch that's good too. Assuming you tested the new code path?
> 
> Yes, I did test it - since the patch as it stands doesn't invoke the offscreen rendering, I created a test-patch that turned offscreen compositing on at the start of WebViewImpl::doComposite() and then off again after copying the texture back to the screen. I ran this on the entire test suite.

Great, sounds good.

> > > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:684
> > > +        m_rootLayer->m_renderSurface.release();
> > 
> > You could use clear() here. Also, there's really no need to test m_renderSurface.
> 
> I assume you mean clear() from LayerTexture? I thought about that and then shied away since (i) it requires another test to see if a texture is allocated, and then (ii) relies on the 'friend'-ness between LayerRendererChromium and RenderSurfaceChromium, as clear() is not exposed through the RenderSurfaceChromium interface. If this is considered OK, then I'm happy to use clear ... let me know. It's probably more efficient to use clear().
> 
> I get you comment about not needing to check m_renderSurface ... I keep thinking of it as a regular pointer :-) Although, I will need to check it in order to use clear().
> 
> I'm quite happy to iterate on the patch to make it better, so let me know what you think of my comments above.

No, I simply meant calling:
m_rootLayer->m_renderSurface.clear();
which has a void return value.

[W. James MacLean]

(In reply to comment #30)
> 
> No, I simply meant calling:
> m_rootLayer->m_renderSurface.clear();
> which has a void return value.

Ahh, OK. I didn't know about OwnPtr<>::clear(). Preparing/testing patch now ...

[W. James MacLean]

Created attachment 77368 [details]
Patch

[W. James MacLean]

(In reply to comment #32)
> Created an attachment (id=77368) [details]
> Patch

Revised as suggested, tested.

[Kenneth Russell]

Comment on attachment 77368 [details]
Patch

Thanks. r=me

[WebKit Commit Bot]

Comment on attachment 77368 [details]
Patch

Rejecting attachment 77368 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-attachment', '--no-update', '--non-interactive', 77368]" exit_code: 2
Last 500 characters of output:

Hunk #5 succeeded at 645 (offset -46 lines).
Hunk #6 succeeded at 713 (offset -46 lines).
1 out of 6 hunks FAILED -- saving rejects to file WebCore/platform/graphics/chromium/LayerRendererChromium.cpp.rej
patching file WebCore/platform/graphics/chromium/LayerRendererChromium.h
Hunk #1 succeeded at 87 (offset -1 lines).
Hunk #2 succeeded at 164 (offset 6 lines).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Kenneth Russell', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7323134

[Kenneth Russell]

It looks like you'll need to merge with top of tree and submit another patch.

[W. James MacLean]

(In reply to comment #36)
> It looks like you'll need to merge with top of tree and submit another patch.

Merging isn't enough - Enne's patch moved things around a lot, so I've had to refactor this patch to match. However, Enne's patch seems to have changed something that I don't yet understand, as my re-factored patch seems to render black pages very nicely, but nothing else :-(

I don't know if it's just my code to dump the rendered surfaces back to the display (during testing only), or if the behaviour of RenderLayerChromium::draw() has somehow changed, or something else. I'm looking at this now, but it may take a while to debug, esp. as I'm working from home and the GPU stuff doesn't display over NX (so far I'm just running new-run-webkit-tests and looking at the output for tests that fail).

[Adrienne Walker]

(In reply to comment #37)
> (In reply to comment #36)
> > It looks like you'll need to merge with top of tree and submit another patch.
> 
> Merging isn't enough - Enne's patch moved things around a lot, so I've had to refactor this patch to match. However, Enne's patch seems to have changed something that I don't yet understand, as my re-factored patch seems to render black pages very nicely, but nothing else :-(

I took a look through the last patch that you uploaded, but I don't see anything obvious that would interact poorly with the changes that I made.  I modified how the root layer was being drawn, but that shouldn't have affected the surface on which it was being drawn.

If there's something about that change that needs more explanation, feel free to ask here or send me an email or chat.

> I don't know if it's just my code to dump the rendered surfaces back to the display (during testing only), or if the behaviour of RenderLayerChromium::draw() has somehow changed, or something else. I'm looking at this now, but it may take a while to debug, esp. as I'm working from home and the GPU stuff doesn't display over NX (so far I'm just running new-run-webkit-tests and looking at the output for tests that fail).

You can use x11vnc to display GPU output remotely.  On the host, run 'x11vnc -xkb -display :0' to share your X display.  On the client, run 'vncviewer MACHINENAME:0' to connect to that host's display.  It's slower than NX in general, but you at least get the GPU output.

[W. James MacLean]

(In reply to comment #38)
> (In reply to comment #37)
> > (In reply to comment #36)
> > > It looks like you'll need to merge with top of tree and submit another patch.
> > 
> > Merging isn't enough - Enne's patch moved things around a lot, so I've had to refactor this patch to match. However, Enne's patch seems to have changed something that I don't yet understand, as my re-factored patch seems to render black pages very nicely, but nothing else :-(
> 
> I took a look through the last patch that you uploaded, but I don't see anything obvious that would interact poorly with the changes that I made.  I modified how the root layer was being drawn, but that shouldn't have affected the surface on which it was being drawn.

Agreed ... there doesn't seem to be anything obvious. That being said, the one thing I did find is that when running the tests (new-run-webkit-tests) the call to WebViewImpl::doPixelReadbackToCanvas() generates an error from the call to imageBuffer->putPremultipliedImageData (line 1026-ish) complaining that assertions are failing in Skia color, namely

SkASSERT(r < a);

for example (see partial dump below ...), which suggests that the data in the buffer is no longer pre-multiplied (this test appears to run fine with chrome through VNC). I don't know if this is related to your patch or some other recent change, although it seems to be something that landed on Dec 23.

If alpha = 0, then that might explain why DumpRenderTree is getting black images as well ...

> If there's something about that change that needs more explanation, feel free to ask here or send me an email or chat.

I'm going to keep looking at it, but may email/chat as I come up with questions.

> > I don't know if it's just my code to dump the rendered surfaces back to the display (during testing only), or if the behaviour of RenderLayerChromium::draw() has somehow changed, or something else. I'm looking at this now, but it may take a while to debug, esp. as I'm working from home and the GPU stuff doesn't display over NX (so far I'm just running new-run-webkit-tests and looking at the output for tests that fail).
> 
> You can use x11vnc to display GPU output remotely.  On the host, run 'x11vnc -xkb -display :0' to share your X display.  On the client, run 'vncviewer MACHINENAME:0' to connect to that host's display.  It's slower than NX in general, but you at least get the GPU output.

Thanks for this tip, although a bit slow, it does work! I can at least see that the on-screen version of the code does still seem to work OK.

Actual output for compositing/animation/state-at-end-event-transform-layer.html:

[4655:4655:434041820463:FATAL:SkColorPriv.h(216)] third_party/skia/include/core/SkColorPriv.h:216: failed assertion "r <= a"

Backtrace:
	base::debug::StackTrace::StackTrace() [0x8a537e]
	logging::LogMessage::~LogMessage() [0x848cda]
	SkDebugf_FileLine() [0x9218c7]
	SkPackARGB32() [0xc7f6e3]
	WebCore::putImageData<>() [0xc81edf]
	WebCore::ImageBuffer::putPremultipliedImageData() [0xc8038d]
	WebKit::WebViewImpl::doPixelReadbackToCanvas() [0x488297]
	WebKit::WebViewImpl::paint() [0x488491]

[W. James MacLean]

Another data point: when I run

Tools/Scripts/new-run-webkit-tests --chromium --debug --platform=chromium-gpu

then the failing tests (for example, fast/canvas/arc360.html) give an "actual" output of an all-black window, but if I run the same test directly in chrome (via VNC), I get what appears to be the correct output. I haven't check every single failing output manually, but everyone I've checked thus far follows this pattern ...

So it seems that the same input is generating very different output for chrome and DumpRenderTree.

Can anyone suggest what this might be due to? It only occurs when I composite to the offscreen texture, and then (immediately) copy that texture to the display.

[Kenneth Russell]

(In reply to comment #40)
> Another data point: when I run
> 
> Tools/Scripts/new-run-webkit-tests --chromium --debug --platform=chromium-gpu
> 
> then the failing tests (for example, fast/canvas/arc360.html) give an "actual" output of an all-black window, but if I run the same test directly in chrome (via VNC), I get what appears to be the correct output. I haven't check every single failing output manually, but everyone I've checked thus far follows this pattern ...
> 
> So it seems that the same input is generating very different output for chrome and DumpRenderTree.
> 
> Can anyone suggest what this might be due to? It only occurs when I composite to the offscreen texture, and then (immediately) copy that texture to the display.

Have you checked to see whether this behavior was introduced with your patch?

[W. James MacLean]

(In reply to comment #41)
> (In reply to comment #40)
> > Another data point: when I run
> > 
> > Tools/Scripts/new-run-webkit-tests --chromium --debug --platform=chromium-gpu
> > 
> > then the failing tests (for example, fast/canvas/arc360.html) give an "actual" output of an all-black window, but if I run the same test directly in chrome (via VNC), I get what appears to be the correct output. I haven't check every single failing output manually, but everyone I've checked thus far follows this pattern ...
> > 
> > So it seems that the same input is generating very different output for chrome and DumpRenderTree.
> > 
> > Can anyone suggest what this might be due to? It only occurs when I composite to the offscreen texture, and then (immediately) copy that texture to the display.
> 
> Have you checked to see whether this behavior was introduced with your patch?

I did not see this behaviour (with the same testing regimen) in this patch on or before Dec 23 (with or without compositing offscreen turned on). The current behaviour is that (1) with offscreen compositing turned on, and (2)  with DumpRenderTree, the output of certain tests is all black. When just running with chrome (and the appropriate flags), everything looks normal (offscreen compositing on or off). I did have change putPremultipliedImageData to putUnmultipliedImageData to get DumpRenderTree (see comment above) to run without asserting on these tests. It does not give a black image for all tests (I'm assuming here that everything in fast/compositing gets tested when --platform=chromium-gpu is enabled?).

I'll start looking at DumpRenderTree tomorrow to see what I can find.

[W. James MacLean]

I've tried a simple experiment that seems to confirm that the problem I'm seeing involves the alpha values in the display buffer (see hack-patch below). The experiment involves, during readback of the display buffer (used presumably by DumpRenderTree during the GPU tests), forcing all the alpha values to 255.

When I do this, I get the exact same test results I had on/before Dec 23.

Why the alpha values are wrong only when rendering to an offscreen texture is yet to be determined.

I'm guessing the display buffer ignores the alpha values, so things look right on the screen, but that DumpRenderTree multiplies by the alpha values before writing the PNG images used for comparison with the expected output images (creating a difference between manual observation and automated testing).

Patch:

diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
index 311b2c7..420e4d0 100644
--- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
+++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
@@ -359,6 +359,13 @@ void LayerRendererChromium::getFramebufferPixels(void *pixels, const IntRect& re
 
     GLC(m_context.get(), m_context->readPixels(rect.x(), rect.y(), rect.width(), rect.height(),
                                          GraphicsContext3D::RGBA, GraphicsContext3D::UNSIGNED_BYTE, pixels));
+
+    unsigned char* ptr = reinterpret_cast<unsigned char*>(pixels);
+    ptr += 3; // Offset to first alpha value.
+    // Set all alpha values to '1'.
+    unsigned numPixels = rect.width() * rect.height();
+    for (unsigned i = 0; i < numPixels; ++i, ptr += 4)
+        *ptr = 255;
 }
 
 // FIXME: This method should eventually be replaced by a proper texture manager.

[W. James MacLean]

OK, I think I've tracked down the change that is giving my patch difficulties. Before the root-layer tiling patch landed, the code that rendered the root layer use to call

GLC(m_context.get(), m_context->colorMask(true, true, true, false));

before the call to LayerChromium::drawTexturedQuad and call

GLC(m_context.get(), m_context->colorMask(true, true, true, true));

afterwards.

The root-layer tiling patch masks off the alpha channel (makes the first call), but never turns it back on (the second call). The patch below turns it back on after the call to updateAndDrawRootLayer(), and restores the results of this patch (rendering to offscreen texture) without breaking any tests when rendering directly to the display.

Should I file this as a separate WebKit bug, or make it part of my patch?

Patch:

diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
index 311b2c7..60bfd55 100644
--- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
+++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
@@ -268,6 +268,7 @@ void LayerRendererChromium::drawLayers(const IntRect& visibleRect, const IntRect
     m_context->clear(GraphicsContext3D::COLOR_BUFFER_BIT);
 
     updateAndDrawRootLayer(tilePaint, scrollbarPaint, visibleRect, contentRect);
+    GLC(m_context.get(), m_context->colorMask(true, true, true, true));
 
     // Set the root visible/content rects --- used by subsequent drawLayers calls.
     m_rootVisibleRect = visibleRect;

[Vangelis Kokkevis]

We really don't need non-one alpha values in the backbuffer as the composited page is assumed to be opaque. The blending mode we use (ONE, ONE_MINUS_SRC_ALPHA) doesn't use the alpha values stored in the backbuffer either so those values are essentially completely ignored. 

My suggestion would be to update the reference images generated by DRT rather than enable alpha writes.


(In reply to comment #44)
> OK, I think I've tracked down the change that is giving my patch difficulties. Before the root-layer tiling patch landed, the code that rendered the root layer use to call
> 
> GLC(m_context.get(), m_context->colorMask(true, true, true, false));
> 
> before the call to LayerChromium::drawTexturedQuad and call
> 
> GLC(m_context.get(), m_context->colorMask(true, true, true, true));
> 
> afterwards.
> 
> The root-layer tiling patch masks off the alpha channel (makes the first call), but never turns it back on (the second call). The patch below turns it back on after the call to updateAndDrawRootLayer(), and restores the results of this patch (rendering to offscreen texture) without breaking any tests when rendering directly to the display.
> 
> Should I file this as a separate WebKit bug, or make it part of my patch?
> 
> Patch:
> 
> diff --git a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> index 311b2c7..60bfd55 100644
> --- a/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> +++ b/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp
> @@ -268,6 +268,7 @@ void LayerRendererChromium::drawLayers(const IntRect& visibleRect, const IntRect
>      m_context->clear(GraphicsContext3D::COLOR_BUFFER_BIT);
> 
>      updateAndDrawRootLayer(tilePaint, scrollbarPaint, visibleRect, contentRect);
> +    GLC(m_context.get(), m_context->colorMask(true, true, true, true));
> 
>      // Set the root visible/content rects --- used by subsequent drawLayers calls.
>      m_rootVisibleRect = visibleRect;

[W. James MacLean]

(In reply to comment #45)
> We really don't need non-one alpha values in the backbuffer as the composited page is assumed to be opaque. The blending mode we use (ONE, ONE_MINUS_SRC_ALPHA) doesn't use the alpha values stored in the backbuffer either so those values are essentially completely ignored. 
> 
> My suggestion would be to update the reference images generated by DRT rather than enable alpha writes.
> 

Presumably this needs to be done in WebViewImpl::doPixelReadbackToCanvas before the call to ImageBuffer::putPremultipliedImageData to avoid getting asserts about alpha values less than r,g,b component values. ImageBuffer doesn't seem to have a function that resets the alpha values, and the code I used for my experiment is pretty hacky. Is there an elegant way to do this?

Would something like

glPixelTransferf(GL_ALPHA_BIAS, 1.0f);

before calling glReadPixels be OK?

[Vangelis Kokkevis]

(In reply to comment #46)
> (In reply to comment #45)
> > We really don't need non-one alpha values in the backbuffer as the composited page is assumed to be opaque. The blending mode we use (ONE, ONE_MINUS_SRC_ALPHA) doesn't use the alpha values stored in the backbuffer either so those values are essentially completely ignored. 
> > 
> > My suggestion would be to update the reference images generated by DRT rather than enable alpha writes.
> > 
> 
> Presumably this needs to be done in WebViewImpl::doPixelReadbackToCanvas before the call to ImageBuffer::putPremultipliedImageData to avoid getting asserts about alpha values less than r,g,b component values. ImageBuffer doesn't seem to have a function that resets the alpha values, and the code I used for my experiment is pretty hacky. Is there an elegant way to do this?
> 
> Would something like
> 
> glPixelTransferf(GL_ALPHA_BIAS, 1.0f);
> 
> before calling glReadPixels be OK?

If you clear the backbuffer with a color that has alpha = 1 then disable alpha writes via glColorMask(true, true, true, false) you will get what you need.  You need to remember to turn on alpha writes before calling glClear() otherwise the alpha channel won't be cleared. So the sequence in drawLayers() would be something like:

m_context->clearColor(0, 0, 1, 1);
m_context->colorMask(true, true, true, true);
m_context->clear(GraphicsContext3D::COLOR_BUFFER_BIT);
m_context->colorMask(true, true, true, false);

[W. James MacLean]

(In reply to comment #47)
> 
> If you clear the backbuffer with a color that has alpha = 1 then disable alpha writes via glColorMask(true, true, true, false) you will get what you need.  You need to remember to turn on alpha writes before calling glClear() otherwise the alpha channel won't be cleared. So the sequence in drawLayers() would be something like:
> 
> m_context->clearColor(0, 0, 1, 1);
> m_context->colorMask(true, true, true, true);
> m_context->clear(GraphicsContext3D::COLOR_BUFFER_BIT);
> m_context->colorMask(true, true, true, false);

I tried this in drawLayers (where the existing clear is), but it failed, giving Skia asserts on "r <= a". I think this means that some subsequent layer is getting drawn with the alpha mask turned on, and it has some alpha values < 1.0 - does that make sense? When rendering offscreen, this only applies to the offscreen RenderSurface.

If, on the other hand, I do this sequence on the display buffer (just before copying the pixels from the offscreen RenderSurface) then it works.

I would assume we should still leave the alpha-clear code at the top of drawLayers, but that we should also expect to have to do this clear again when drawing the offscreen RenderSurface to the display. Does that sound OK?

[W. James MacLean]

Created attachment 77636 [details]
Patch

[W. James MacLean]

Comment on attachment 77636 [details]
Patch

Patch for discussion purposes (do not review yet please).

Test code left in showing resetting of alpha values before copying offscreen RenderSurface to display.

[Vangelis Kokkevis]

Comment on attachment 77636 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=77636&action=review

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:270
> +    m_context->colorMask(true, true, true, false);

Since you set the color mask here, you should remove the colorMask call from updateAndDrawRootLayer()

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:701
> +        GLC(m_context.get(), m_context->colorMask(true, true, true, false));

the color mask should be already (true, true, true, false).  You shouldn't be modifying it here.

> WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:727
> +        GLC(m_context.get(), m_context->colorMask(true, true, true, false));

Again, the color mask should be correct.

[W. James MacLean]

(In reply to comment #51)
> (From update of attachment 77636 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77636&action=review
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:270
> > +    m_context->colorMask(true, true, true, false);
> 
> Since you set the color mask here, you should remove the colorMask call from updateAndDrawRootLayer()
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:701
> > +        GLC(m_context.get(), m_context->colorMask(true, true, true, false));
> 
> the color mask should be already (true, true, true, false).  You shouldn't be modifying it here.
> 
> > WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:727
> > +        GLC(m_context.get(), m_context->colorMask(true, true, true, false));
> 
> Again, the color mask should be correct.

Sure thing. The colorMasks on lines 701 and 727 weren't intended to be left in (forgot to take them out) ... I was just trying them to see if I could find where the non-unit alpha values were creeping in ...

I'll submit a proper patch once I've re-tested on a mac.

[W. James MacLean]

Created attachment 77640 [details]
Patch

[Kenneth Russell]

Comment on attachment 77640 [details]
Patch

The revised patch looks okay to me; if Vangelis can give it an LGTM I'll r+ it.

[Vangelis Kokkevis]

Comment on attachment 77640 [details]
Patch

Looks good.  Thanks!

[WebKit Commit Bot]

Comment on attachment 77640 [details]
Patch

Clearing flags on attachment: 77640

Committed r75030: <http://trac.webkit.org/changeset/75030>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/75030 might have broken Leopard Intel Debug (Tests)