Bug 51928 – [chromium] Enable reuse of RenderSurface for drawing.

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

[W. James MacLean]

[chromium] Enable reuse of RenderSurface for drawing.

[W. James MacLean]

Created attachment 78006 [details]
Patch

[W. James MacLean]

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

This patch suggest a modification to RenderSurfaceChromium that allows the object to be selected for drawing multiple times via LayerRendererChromium::useRenderSurface. This use case arises when compositing to an offscreen texture (atlhough not often it appears ...the test compositing/geometry/fixed-position.html is the only case I know of thus far).

This patch also contains two minor modifications to 
LayerRendererChromium::drawLayers(), which I would like to *discuss* (I'm not necessarily proposing they would be part of the final patch).

The mods are marked with comments that begin "// WJM". While testing with fixed-position.html and using offscreen compositing, I found that there was a problem with the alpha layers, and it seems to be solved if we turn on the alpha channel mask during drawLayers. The test places two transparent rectangles on the tab, and a piece of text below them. As the text is scrolled up into the region of the rectangles, it is visible 'through the colored layers', and the black text takes on the colours of the rectangles. Without the two commented lines, the text changes color, but the rectangles are not visible, and thus the test fails. Again, this only occurs during offscreen compositing. The change does not change test results for any other GPU test *during offscreen compositing*.

I'd be interested in thoughts/suggestions regarding this apparent difference between compositing directly to the display versus to an offscreen texture. It may be we will need to address this in a separate patch.

[David Levin]

Not what you're asking for, but something to consider for a another revision: colorMask is a bad api.  Those bools should be enums as the current form is unreadable and fragile.
  "m_context->colorMask(true, true, true, true)"

[W. James MacLean]

(In reply to comment #3)
> Not what you're asking for, but something to consider for a another revision: colorMask is a bad api.  Those bools should be enums as the current form is unreadable and fragile.
>   "m_context->colorMask(true, true, true, true)"

Thanks David. If these lines do end up in the final patch, I will look into converting to using enums.

[Kenneth Russell]

(In reply to comment #4)
> (In reply to comment #3)
> > Not what you're asking for, but something to consider for a another revision: colorMask is a bad api.  Those bools should be enums as the current form is unreadable and fragile.
> >   "m_context->colorMask(true, true, true, true)"
> 
> Thanks David. If these lines do end up in the final patch, I will look into converting to using enums.

Please don't do that. The GraphicsContext3D methods mirror the OpenGL ES 2.0 API and for better or worse this is how OpenGL phrases this call. Please do not make spurious changes.

[David Levin]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > Not what you're asking for, but something to consider for a another revision: colorMask is a bad api.  Those bools should be enums as the current form is unreadable and fragile.
> > >   "m_context->colorMask(true, true, true, true)"
> > 
> > Thanks David. If these lines do end up in the final patch, I will look into converting to using enums.
> 
> Please don't do that. The GraphicsContext3D methods mirror the OpenGL ES 2.0 API and for better or worse this is how OpenGL phrases this call. Please do not make spurious changes.

Thanks for explaining.

fwiw, I think you're repeating yourself: "Please don't do that. ... Please do not make spurious changes."

[Simon Fraser (smfr)]

It's unfortunate that the Render* prefix is used in this code (not new in this patch though), since it's well-established for RenderObject and subclasses.

[Stephen White]

This change seems to violate the comment above it:

    // Mask out writes to alpha channel: subpixel antialiasing via Skia results in invalid
    // zero alpha values on text glyphs. The root layer is always opaque.
    m_context->colorMask(true, true, true, false);

Either this change is wrong, or the comment is wrong.  Vangelis, what do you think?

[James Robinson]

(In reply to comment #7)
> It's unfortunate that the Render* prefix is used in this code (not new in this patch though), since it's well-established for RenderObject and subclasses.

I think the risk of confusion is low here given that RenderSurface exists in platform/ and is never referenced outside of platform/, so there can't be any code that uses RenderObject or RenderLayer and RenderSurface at the same time.

[W. James MacLean]

(In reply to comment #8)
> This change seems to violate the comment above it:
> 
>     // Mask out writes to alpha channel: subpixel antialiasing via Skia results in invalid
>     // zero alpha values on text glyphs. The root layer is always opaque.
>     m_context->colorMask(true, true, true, false);
> 
> Either this change is wrong, or the comment is wrong.  Vangelis, what do you think?

I've moved the comment along with the change to the color mask, but I do not know about the origin of the comment.

I'm happy to prepare a test-patch people could apply to ToT to see for themselves the difference between leaving the alpha mask on and off, if that would help. As mentioned earlier, so far it's just the one test case I know of that triggers it.

[Vincent Scheib]

(In reply to comment #10)
> (In reply to comment #8)
> > This change seems to violate the comment above it:
> > 
> >     // Mask out writes to alpha channel: subpixel antialiasing via Skia results in invalid
> >     // zero alpha values on text glyphs. The root layer is always opaque.
> >     m_context->colorMask(true, true, true, false);
> > 
> > Either this change is wrong, or the comment is wrong.  Vangelis, what do you think?
> 
> I've moved the comment along with the change to the color mask, but I do not know about the origin of the comment.
> 
> I'm happy to prepare a test-patch people could apply to ToT to see for themselves the difference between leaving the alpha mask on and off, if that would help. As mentioned earlier, so far it's just the one test case I know of that triggers it.


The comment originated with a change I made to remove zero values from the alpha channel. Writing to the alpha channel is momentarily disabled:
http://trac.webkit.org/changeset/69266
+    // Mask out writes to alpha channel: ClearType via Skia results in invalid
+    // zero alpha values on text glyphs. The root layer is always opaque.
+    GLC(m_context, m_context->colorMask(true, true, true, false));
     TransformationMatrix layerMatrix;
     layerMatrix.translate3d( ... );
     LayerChromium::drawTexturedQuad( ... );
+    GLC(m_context, m_context->colorMask(true, true, true, true));

A trivial change, then this change that modified logic and comment:
http://trac.webkit.org/changeset/74568
The color mask is modified to only ever be set to false for alpha.

[Vangelis Kokkevis]

(In reply to comment #10)
> (In reply to comment #8)
> > This change seems to violate the comment above it:
> > 
> >     // Mask out writes to alpha channel: subpixel antialiasing via Skia results in invalid
> >     // zero alpha values on text glyphs. The root layer is always opaque.
> >     m_context->colorMask(true, true, true, false);
> > 
> > Either this change is wrong, or the comment is wrong.  Vangelis, what do you think?
> 
> I've moved the comment along with the change to the color mask, but I do not know about the origin of the comment.
> 
> I'm happy to prepare a test-patch people could apply to ToT to see for themselves the difference between leaving the alpha mask on and off, if that would help. As mentioned earlier, so far it's just the one test case I know of that triggers it.

Here's my suspicion as to what's happening:  The RenderSurface's texture starts out with Alpha = 1 for all pixels (which is guaranteed from setting the colorMask to (true, true, true, true) and clearing).  The code subsequently masks out alpha writes which should guarantee that the RenderSurface texture keeps alpha = 1 for all pixels all the way through.

In the offscreen rendering path, if your window backbuffer was never cleared, it could have non-one alpha values for pixels so when you render the one large quad with the RenderSurface's contents, since you are masking alpha, you won't affect the existing alpha values.  

Can you try setting the color mask back to (true, true, true, true) before drawing the offscreen texture into the final window backbuffer?

[Vincent Scheib]

(In reply to comment #11)
[sorry, adding CC sent comment early, continuing] 

enne made that logic change.

That logic was then adjusted with a clear that is permitted to write to the alpha channel by [e-mail]
http://trac.webkit.org/changeset/75030

So, it seems we desire:
alpha writes enabled for clear
alpha writes disabled at all other times

[W. James MacLean]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #8)
> > > This change seems to violate the comment above it:
> > > 
> > >     // Mask out writes to alpha channel: subpixel antialiasing via Skia results in invalid
> > >     // zero alpha values on text glyphs. The root layer is always opaque.
> > >     m_context->colorMask(true, true, true, false);
> > > 
> > > Either this change is wrong, or the comment is wrong.  Vangelis, what do you think?
> > 
> > I've moved the comment along with the change to the color mask, but I do not know about the origin of the comment.
> > 
> > I'm happy to prepare a test-patch people could apply to ToT to see for themselves the difference between leaving the alpha mask on and off, if that would help. As mentioned earlier, so far it's just the one test case I know of that triggers it.
> 
> Here's my suspicion as to what's happening:  The RenderSurface's texture starts out with Alpha = 1 for all pixels (which is guaranteed from setting the colorMask to (true, true, true, true) and clearing).  The code subsequently masks out alpha writes which should guarantee that the RenderSurface texture keeps alpha = 1 for all pixels all the way through.
> 
> In the offscreen rendering path, if your window backbuffer was never cleared, it could have non-one alpha values for pixels so when you render the one large quad with the RenderSurface's contents, since you are masking alpha, you won't affect the existing alpha values.  
> 
> Can you try setting the color mask back to (true, true, true, true) before drawing the offscreen texture into the final window backbuffer?

I'd be happy to try that --- it sounds like a good idea. It will have to wait until Monday, as my z600 is somewhere in transit while we do our office move. If the change is successful, I'll re-submit the patch with the colorMask lines removed.

[Adrienne Walker]

Are there any plans for adding automated testing of offscreen compositing so that this doesn't break again in the future? As I'm sure the compositor will get refactored again in the future, it'd be nice to have some tests as a warning.

[W. James MacLean]

(In reply to comment #15)
> Are there any plans for adding automated testing of offscreen compositing so that this doesn't break again in the future? As I'm sure the compositor will get refactored again in the future, it'd be nice to have some tests as a warning.

You read my mind on this one.

Is it worth (i) adding a command-line switch (--test-offscreen-compositing or something similar), and (ii) in debug mode compiling in the test function which copies the offscreen texture back to the display buffer? Then the test suite could be automatically run on the offscreen compositing version as well.

If this seems like a reasonable approach, I can file a new bug and submit the appropriate patch at the beginning of next week.

[W. James MacLean]

Created attachment 78407 [details]
Patch

[W. James MacLean]

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

Vangelis' solution seems to have fixed it. In fact I had been clearing the display buffer, but I was only clearing the alpha channel. Clearing all channels seems to be necessary.

I will check in the command-line switch for testing of offscreen rendering in a separate bug.

[Kenneth Russell]

Comment on attachment 78407 [details]
Patch

Looks fine.

[Vangelis Kokkevis]

(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=78407) [details] [details]
> > Patch
> 
> Vangelis' solution seems to have fixed it. In fact I had been clearing the display buffer, but I was only clearing the alpha channel. Clearing all channels seems to be necessary.
> 

Glad to hear it works. From the behavior you're describing it sounds like when you do the final draw to the backbuffer you have blending enabled.  Unless you're expecting to composite multiple results, it will be a big performance win if you disable blend before rendering.  At that point you should in theory only have to clear the alpha channel of the backbuffer once.

[W. James MacLean]

(In reply to comment #20)
> (In reply to comment #18)
> > (In reply to comment #17)
> > > Created an attachment (id=78407) [details] [details] [details]
> > > Patch
> > 
> > Vangelis' solution seems to have fixed it. In fact I had been clearing the display buffer, but I was only clearing the alpha channel. Clearing all channels seems to be necessary.
> > 
> 
> Glad to hear it works. From the behavior you're describing it sounds like when you do the final draw to the backbuffer you have blending enabled.  Unless you're expecting to composite multiple results, it will be a big performance win if you disable blend before rendering.  At that point you should in theory only have to clear the alpha channel of the backbuffer once.

I think then end goal is to composite multiple results, although there will likely be times when only one offscreen texture is being copied to the screen. In those cases we could disable blending before display.

[WebKit Commit Bot]

Comment on attachment 78407 [details]
Patch

Clearing flags on attachment: 78407

Committed r75410: <http://trac.webkit.org/changeset/75410>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.