Bug 93761 - [chromium] Implement software renderer
Summary: [chromium] Implement software renderer
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexandre Elias
URL:
Keywords:
Depends on: 93677 93927
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-10 21:29 PDT by Alexandre Elias
Modified: 2013-04-15 08:11 PDT (History)
13 users (show)

See Also:


Attachments
Patch (59.79 KB, patch)
2012-08-10 21:35 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (24.30 KB, patch)
2012-08-13 23:05 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (23.54 KB, patch)
2012-08-21 16:30 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (22.01 KB, patch)
2012-08-27 17:03 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (22.39 KB, patch)
2012-08-28 16:09 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2012-08-29 12:40 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (23.18 KB, patch)
2012-08-29 13:46 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (24.54 KB, patch)
2012-08-29 15:08 PDT, Alexandre Elias
no flags Details | Formatted Diff | Diff
Patch (26.07 KB, patch)
2012-08-29 23:31 PDT, Alexandre Elias
aelias: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Elias 2012-08-10 21:29:12 PDT
[chromium] Implement software renderer
Comment 1 Alexandre Elias 2012-08-10 21:35:39 PDT
Created attachment 157857 [details]
Patch
Comment 2 Alexandre Elias 2012-08-10 21:43:53 PDT
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?).
Comment 3 Adrienne Walker 2012-08-13 09:20:05 PDT
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>
Comment 4 Alexandre Elias 2012-08-13 13:12:45 PDT
I'm thinking CCDirectRenderer and CCDelegatingRenderer.
Comment 5 Antoine Labour 2012-08-13 13:21:33 PDT
CCDelegatingRenderer sounds best to me.
Comment 6 Alexandre Elias 2012-08-13 23:05:11 PDT
Created attachment 158224 [details]
Patch

Rebased on Bug 93927
Comment 7 Nat Duca 2012-08-15 10:40:28 PDT
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?
Comment 8 Alexandre Elias 2012-08-15 11:29:43 PDT
> 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.
Comment 9 Nat Duca 2012-08-15 11:36:49 PDT
Chosing between the delegating renderer and direct can be done by checking the hasParentCompositor capability on the output surface, right?
Comment 10 Alexandre Elias 2012-08-21 16:30:20 PDT
Created attachment 159794 [details]
Patch

Rebased
Comment 11 Alexandre Elias 2012-08-27 17:03:41 PDT
Created attachment 160860 [details]
Patch
Comment 12 Alexandre Elias 2012-08-27 17:05:13 PDT
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?
Comment 13 WebKit Review Bot 2012-08-27 17:05:45 PDT
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment 14 Alexandre Elias 2012-08-28 16:09:26 PDT
Created attachment 161080 [details]
Patch

Rename everything to 'software'
Comment 15 Adrienne Walker 2012-08-29 10:41:38 PDT
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.
Comment 16 Adrienne Walker 2012-08-29 10:59:29 PDT
(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.
Comment 17 Alexandre Elias 2012-08-29 12:40:54 PDT
Created attachment 161288 [details]
Patch

Removed use of quadVisibleRect, implemented SolidColorQuad, and moved shared code into drawQuad()
Comment 18 Alexandre Elias 2012-08-29 12:43:32 PDT
(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.
Comment 19 Alexandre Elias 2012-08-29 13:46:50 PDT
Created attachment 161304 [details]
Patch

Added support for scissoring and opacity
Comment 20 Alexandre Elias 2012-08-29 15:08:07 PDT
Created attachment 161325 [details]
Patch

Implemented CCTextureDrawQuad (needed for scrollbars)
Comment 21 Alexandre Elias 2012-08-29 23:31:22 PDT
Created attachment 161407 [details]
Patch

Implemented CCDebugBorderDrawQuad and added flipped TextureDrawQuad support
Comment 22 Adrienne Walker 2012-09-05 14:40:49 PDT
Any more thoughts about testing?
Comment 23 Alexandre Elias 2012-09-05 14:46:04 PDT
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?
Comment 24 Alexandre Elias 2012-09-14 16:11:22 PDT
Comment on attachment 161407 [details]
Patch

R- as this will land in the Chromium tree.