Bug 93761 – [chromium] Implement software renderer

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

[Alexandre Elias]

[chromium] Implement software renderer

[Alexandre Elias]

Created attachment 157857 [details]
Patch

[Alexandre Elias]

Proof of concept, not yet for review.

I wanted to put this up particularly for comments on the shared code between the GL and software renderers.  In the current patch, I put it inside CCRenderer, but I'm thinking it won't be useful to CCExternalRenderer and that a class in between CCRenderer and the GL/software implementations might make more sense.  I can't think of a great name for it though (CCInternalRenderer?).

[Adrienne Walker]

Ooh, thanks for putting this up to look at.  I'm happy to see so much common code move out of LRC.

I'm guessing that CCExternalRenderer is the renderer that sends a frame to the host compositor? If you don't like internal vs. external, I think direct vs. indirect or immediate vs. deferred also might be reasonable names.  </bikeshed>

[Alexandre Elias]

I'm thinking CCDirectRenderer and CCDelegatingRenderer.

[Antoine Labour]

CCDelegatingRenderer sounds best to me.

[Alexandre Elias]

Created attachment 158224 [details]
Patch

Rebased on Bug 93927

[Nat Duca]

Comment on attachment 158224 [details]
Patch

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

Pretty awesome.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:701
> +#else

This can be done by looking at presence of a context3d on the output surface, right?

[Alexandre Elias]

> This can be done by looking at presence of a context3d on the output surface, right?

In the short term this patch still depends on a GL context to output anything.  In the long term, in the absence of a context3d we'll still need to choose between CCSoftwareRenderer and CCDelegatingRenderer.  So this isn't quite settled yet.

[Nat Duca]

Chosing between the delegating renderer and direct can be done by checking the hasParentCompositor capability on the output surface, right?

[Alexandre Elias]

Created attachment 159794 [details]
Patch

Rebased

[Alexandre Elias]

Created attachment 160860 [details]
Patch

[Alexandre Elias]

This is getting into a reviewable state, please take a look.  The preliminary implementation of the 2d tearoff is at https://chromiumcodereview.appspot.com/10873099

Anyone have thoughts on testing strategy?

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Alexandre Elias]

Created attachment 161080 [details]
Patch

Rename everything to 'software'

[Adrienne Walker]

Comment on attachment 161080 [details]
Patch

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

This patch is exciting to see.

> Source/WebCore/platform/graphics/chromium/cc/CCRendererSoftware.cpp:177
> +    case CCDrawQuad::TiledContent:
> +        drawTileQuad(frame, CCTileDrawQuad::materialCast(quad));
> +        break;

You might also want to add solid color quad handling in this first rev so you can get gutter quads on page zoom.

> Source/WebCore/platform/graphics/chromium/cc/CCRendererSoftware.cpp:189
> +    SkRect vertexRect = toSkRect(quadVertexRect());
> +    SkIRect uvRect = toSkIRect(IntRect(quad->textureOffset(), quad->quadVisibleRect().size()));
> +
> +    WebTransformationMatrix quadRectMatrix;
> +    quadRectTransform(&quadRectMatrix, quad->quadTransform(), quad->quadRect());

I think this is wrong, because calculating the visible rect from the quad rect doesn't update the texture offset.  You need to use IntRect(quad->textureOffset(), quad->quadRect().size()) if you're going to use quad->quadRect() later.

Alternatively, you can pass quad->quadVisibleRect() to quadRectTransform and make the uvRect IntRect(quad->textureOffset() + quad->quadVisibleRect().location() - quad->quadRect.location(), quad->quadVisibleRect.size()) instead.

[Adrienne Walker]

(In reply to comment #12)
>
> Anyone have thoughts on testing strategy?

What about running this path through a small subset of layout tests with pixel results?  It's not ideal, but it's probably the least amount of work to get tests up and running.

It looks like you have an adapter to just upload the software bitmap to the screen and a command line flag to enable it.  Once that and this are landed, you could add a virtual layout test directory for software compositing to run existing tests through DRT with that flag set.

[Alexandre Elias]

Created attachment 161288 [details]
Patch

Removed use of quadVisibleRect, implemented SolidColorQuad, and moved shared code into drawQuad()

[Alexandre Elias]

(In reply to comment #15)
> > Source/WebCore/platform/graphics/chromium/cc/CCRendererSoftware.cpp:189
> > +    SkRect vertexRect = toSkRect(quadVertexRect());
> > +    SkIRect uvRect = toSkIRect(IntRect(quad->textureOffset(), quad->quadVisibleRect().size()));
> > +
> > +    WebTransformationMatrix quadRectMatrix;
> > +    quadRectTransform(&quadRectMatrix, quad->quadTransform(), quad->quadRect());
> 
> I think this is wrong, because calculating the visible rect from the quad rect doesn't update the texture offset.  You need to use IntRect(quad->textureOffset(), quad->quadRect().size()) if you're going to use quad->quadRect() later.
> 
> Alternatively, you can pass quad->quadVisibleRect() to quadRectTransform and make the uvRect IntRect(quad->textureOffset() + quad->quadVisibleRect().location() - quad->quadRect.location(), quad->quadVisibleRect.size()) instead.

OK, in getting it up and running a few weeks ago, I remember needing to use quadVisibleRect to get correct texture coordinates, but now just using quadRect in both places works just as well, so I switched to that.

[Alexandre Elias]

Created attachment 161304 [details]
Patch

Added support for scissoring and opacity

[Alexandre Elias]

Created attachment 161325 [details]
Patch

Implemented CCTextureDrawQuad (needed for scrollbars)

[Alexandre Elias]

Created attachment 161407 [details]
Patch

Implemented CCDebugBorderDrawQuad and added flipped TextureDrawQuad support

[Adrienne Walker]

Any more thoughts about testing?

[Alexandre Elias]

I think pixel layout tests are the right way to test this, but it might take a few weeks before I have time to tackle the test infrastructure.  Do you want to hold off on landing this until we have them?

[Alexandre Elias]

Comment on attachment 161407 [details]
Patch

R- as this will land in the Chromium tree.