Bug 93927 – [chromium] Move non-GL-specific code out of LayerRendererChromium

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

[Alexandre Elias]

[chromium] Move non-GL-specific code out of LayerRendererChromium

[Alexandre Elias]

Created attachment 158198 [details]
Patch

[WebKit Review Bot]

Attachment 158198 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:11:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:12:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[James Robinson]

Comment on attachment 158198 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:40
> +static WebTransformationMatrix projectionMatrix(float left, float right, float bottom, float top)

Are non-GL renderers going to need this 3d math?  I thought that any software backend we put in would have to ignore 3d content - we don't have a perspective-aware software rasterizer.

[Alexandre Elias]

In my software rasterizer prototype, I've been multiplying all the 4x4 matrices in the usual way and then collapsing to 3x3 at the last minute by cutting the 3rd row and column (see toSkMatrix() in Bug 93761 ).  Because neither the input quads nor the output surface have a nonzero Z coordinate, it looks like every supported 3d transformation can be expressed that way.  The projection matrix already ensures that any z-axis movement is collapsed into x,y space.

[Nat Duca]

Perspective correction?

[Alexandre Elias]

Created attachment 159191 [details]
Patch

Rebased on Bug 94050

[Adrienne Walker]

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

I'm ok with 3D software layers that don't have perspective correct texture interpolation.  I think that's better than not having 3D layers at all, honestly.

The direction of this patch seems great to me.  I just have a bunch of additional style and bikeshed nits:

> Source/WebCore/platform/graphics/chromium/TextureUploader.h:49
> +class UnthrottledTextureUploader : public TextureUploader {

Put this in its own header file if it's not just being used internal to a single class, please.

>> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:40
>> +static WebTransformationMatrix projectionMatrix(float left, float right, float bottom, float top)
> 
> Are non-GL renderers going to need this 3d math?  I thought that any software backend we put in would have to ignore 3d content - we don't have a perspective-aware software rasterizer.

Did you need to rename this? What about orthoProjectionMatrix?

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:61
> +static WebTransformationMatrix canvasMatrix(int x, int y, int width, int height)

Did you need to rename this too? windowMatrix makes more sense to me than canvasMatrix, since "window coordinates" are often talked about in OpenGL.  I'll also suggest viewportMatrix as an option?

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:78
> +void CCDirectRenderer::DrawingFrame::initializeMatrices(const IntRect& drawRect, bool flipY)

I don't like the way that this struct now has member functions.  This should either be "just a struct" and CCDirectRenderer::initializeMatrices should take a DrawingFrame& or it should be a class and have m_-named member variables so that it's obvious at a glance what side effects these functions have.

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.h:52
> +    explicit CCDirectRenderer(CCRendererClient* client, CCResourceProvider* resourceProvider)

No explicit.

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.h:111
> +    inline static WebKit::WebTransformationMatrix quadRectTransform(const WebKit::WebTransformationMatrix& quadTransform, const FloatRect& quadRect)
> +    {
> +        WebKit::WebTransformationMatrix quadRectTransform = quadTransform;
> +        quadRectTransform.translate(0.5 * quadRect.width() + quadRect.x(), 0.5 * quadRect.height() + quadRect.y());
> +        quadRectTransform.scaleNonUniform(quadRect.width(), quadRect.height());
> +        return quadRectTransform;
> +    }

inline, really?

[Alexandre Elias]

Created attachment 159227 [details]
Patch

All suggestions applied, and added "const" to most drawQuad DrawingFrame params

[Alexandre Elias]

In addition to applying your comments, I changed most drawQuad methods taking DrawingFrame& to take a const DrawingFrame&.  The exception is drawRenderPassQuad, which needs to modify it due to some weird tricks involving the background pass.  I don't know quite what to do with that; that's an area I'd like to see cleaned up in the future as the call structure is quite tangled and I don't fully understand it.

(In reply to comment #7)
> View in context: https://bugs.webkit.org/attachment.cgi?id=158198&action=review
> 
> I'm ok with 3D software layers that don't have perspective correct texture interpolation.  I think that's better than not having 3D layers at all, honestly.

Happily, Skia matrices are better than expected and poster circle displays decently in my software renderer prototype.  I sent a screenshot to chrome-gpu@ ("First look at software compositing")

> > Source/WebCore/platform/graphics/chromium/TextureUploader.h:49
> > +class UnthrottledTextureUploader : public TextureUploader {
> 
> Put this in its own header file if it's not just being used internal to a single class, please.
> 

Done

> Did you need to rename this? What about orthoProjectionMatrix?
> 

Renamed to orthoProjectionMatrix() and windowMatrix()

> > Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:78
> > +void CCDirectRenderer::DrawingFrame::initializeMatrices(const IntRect& drawRect, bool flipY)
> 
> I don't like the way that this struct now has member functions.  This should either be "just a struct" and CCDirectRenderer::initializeMatrices should take a DrawingFrame& or it should be a class and have m_-named member variables so that it's obvious at a glance what side effects these functions have.
> 

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.h:52
> > +    explicit CCDirectRenderer(CCRendererClient* client, CCResourceProvider* resourceProvider)
> 
> No explicit.
> 

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.h:111
> > +    inline static WebKit::WebTransformationMatrix quadRectTransform(const WebKit::WebTransformationMatrix& quadTransform, const FloatRect& quadRect)
> > +    {
> > +        WebKit::WebTransformationMatrix quadRectTransform = quadTransform;
> > +        quadRectTransform.translate(0.5 * quadRect.width() + quadRect.x(), 0.5 * quadRect.height() + quadRect.y());
> > +        quadRectTransform.scaleNonUniform(quadRect.width(), quadRect.height());
> > +        return quadRectTransform;
> > +    }
> 
> inline, really?

I mainly want to avoid copying around a WebTransformationMatrix for every drawQuad.  I moved it to the .cpp file and changed the function to take a pointer to the target matrix instead of returning one.

[Alexandre Elias]

Created attachment 159245 [details]
Patch

Rebased

[Dana Jansens]

(In reply to comment #9)
> In addition to applying your comments, I changed most drawQuad methods taking DrawingFrame& to take a const DrawingFrame&.  The exception is drawRenderPassQuad, which needs to modify it due to some weird tricks involving the background pass.  I don't know quite what to do with that; that's an area I'd like to see cleaned up in the future as the call structure is quite tangled and I don't fully understand it.

The reason is DrawingFrame holds the current render target, and background filters are drawn in a different render target, then merged with content back into the original render target. So the current target changes during this.

In order to avoid this, you'd have to do the background filter drawing steps in another place that has a non-const DrawingFrame, or not keep the current target in the DrawingFrame, or something else. But I'm not sure it's worth it for the const, and might make things more convoluted.

[Adrienne Walker]

Comment on attachment 159245 [details]
Patch

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

R=me.

> Source/WebCore/platform/graphics/chromium/cc/CCDirectRenderer.cpp:16
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

Please use the 2-clause license like you do in UnthrottledTextureUploader.h and CCDirectRenderer.h instead here.

[Alexandre Elias]

Created attachment 159473 [details]
Patch

Updated CCDirectRenderer.cpp license header

[Adrienne Walker]

Comment on attachment 159473 [details]
Patch

R=meagain.

[WebKit Review Bot]

Comment on attachment 159473 [details]
Patch

Clearing flags on attachment: 159473

Committed r126052: <http://trac.webkit.org/changeset/126052>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.