Add CC software renderer.

Add CC software renderer.

This new renderer adds support for the compositor to output to a
software-bitmap surface instead of a GL context.  It uses Skia to
interpret the CCDrawQuads and configures the resource provider to only
use bitmap-backed resources.  During initialization, the host checks
whether a GL or software output surface is available and initializes the
appropriate renderer.

The basic texture and solid-color based quad types are supported, along
with alpha, clipping and 3d transformations.  Render surfaces and video
are not yet supported.

BUG=124671
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=158396

[aelias_OOO_until_Jul13, 2012-09-15 03:44:11]

Here it is again in the Chromium tree.  It's close to what I uploaded before,
except that it no longer draws with a flipped the Y axis on the framebuffer, and
getFramebufferPixels() is implemented.  I think it's ready for landing.

[jamesr, 2012-09-17 20:57:23]

Seems pretty reasonable, but I'd like to get at least some sort of smoke tests
up+running. Otherwise it'll be really hard for folks who want to change the code
in any way to know if they are busting this path or not.

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp
File cc/CCRendererSoftware.cpp (right):

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp#new...
cc/CCRendererSoftware.cpp:44: using namespace WebKit;
I know we have various bits of code that do this but this is against chromium
style (and technically WebKit style as well).  Could you add using declarations
for the specific WebKit types you're using?  Is it just WebTransformationMatrix
/ WebSize / WebImage?

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp#new...
cc/CCRendererSoftware.cpp:169: m_skCurrentCanvas->clear(SK_ColorGREEN);
this will have to be more advanced once you start supporting render surfaces -
could you leave a FIXME?

[nduca, 2012-09-17 21:07:28]

https://codereview.chromium.org/10918258/diff/1/cc/CCDirectRenderer.h
File cc/CCDirectRenderer.h (right):

https://codereview.chromium.org/10918258/diff/1/cc/CCDirectRenderer.h#newcode99
cc/CCDirectRenderer.h:99: virtual bool flippedFramebuffer() const = 0;
didn't we just eradicate yflip recently?

[aelias_OOO_until_Jul13, 2012-09-17 21:24:34]

On 2012/09/17 20:57:23, jamesr wrote:
> Seems pretty reasonable, but I'd like to get at least some sort of smoke tests
> up+running. Otherwise it'll be really hard for folks who want to change the
code
> in any way to know if they are busting this path or not.

My idea was that I would land the DRT layout test patch at the same time as
this.  About half of the pixel tests in compositing/ are passing.  

On 2012/09/17 21:07:28, nduca wrote:
> didn't we just eradicate yflip recently?

No, the flipping is still in the code.  You might be thinking of a patch I
submitted last week changing the flip behavior during readback, but it didn't
affect rendering.

[nduca, 2012-09-17 21:28:11]

There should be some unit tests that draw some quads to a bitmap skpicture and
verify that it was drawn.

[aelias_OOO_until_Jul13, 2012-09-17 21:42:01]

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp
File cc/CCRendererSoftware.cpp (right):

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp#new...
cc/CCRendererSoftware.cpp:44: using namespace WebKit;
On 2012/09/17 20:57:24, jamesr wrote:
> I know we have various bits of code that do this but this is against chromium
> style (and technically WebKit style as well).  Could you add using
declarations
> for the specific WebKit types you're using?  Is it just
WebTransformationMatrix
> / WebSize / WebImage?

Done.

https://codereview.chromium.org/10918258/diff/1/cc/CCRendererSoftware.cpp#new...
cc/CCRendererSoftware.cpp:169: m_skCurrentCanvas->clear(SK_ColorGREEN);
On 2012/09/17 20:57:24, jamesr wrote:
> this will have to be more advanced once you start supporting render surfaces -
> could you leave a FIXME?

I'm not sure why this particular part would have to be more advanced. 
m_skCurrentCanvas will point to the render surface.

[danakj, 2012-09-19 18:35:14]

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:691: renderer = CCRendererSoftware::create(this,
resourceProvider.get(), contextRef->softwareDevice());
Should there be an else { ASSERT_NOT_REACHED(); } here? Or remove the if on the
else and ASSERT(softwareDevice)?

Or it ok to leave this with renderer == NULL? The release() below makes me think
it's not.

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp
File cc/CCRendererSoftware.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp#...
cc/CCRendererSoftware.cpp:123:
m_outputDevice->didChangeViewportSize(WebSize(viewportSize().width(),
viewportSize().height()));
How come the software renderer needs a call like this but the GL one does not?
I'm wondering especially cuz CCRendererGL doesn't have an m_outputDevice at all.

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp#...
cc/CCRendererSoftware.cpp:258: // FIXME: Add support for non-premultiplied
alpha.
Do you want an assert quad->premultipliedAlpha() or return?

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp#...
cc/CCResourceProvider.cpp:344: m_maxTextureSize = INT_MAX;
If you do something like

if (textureSize > m_maxTextureSize)

and they are both ints, and m_maxTextureSize == INT_MAX, isn't that going to
just going to be always true?

If this is going to be INT_MAX should m_maxTextureSize be an unsigned (this
would force other places to be unsigned in order to compare)?

It reminds me of all the overflow issues caused by INT_MAX stuff in the subpixel
layout code. The checks we have in place for max texture size would be no-ops in
this case.

[aelias_OOO_until_Jul13, 2012-09-19 19:58:26]

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:691: renderer = CCRendererSoftware::create(this,
resourceProvider.get(), contextRef->softwareDevice());
On 2012/09/19 18:35:14, danakj wrote:
> Should there be an else { ASSERT_NOT_REACHED(); } here? Or remove the if on
the
> else and ASSERT(softwareDevice)?
> 
> Or it ok to leave this with renderer == NULL? The release() below makes me
think
> it's not.

The release() will not crash given a NULL pointer.  This function returns false
if the renderer could not be initialized.

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp
File cc/CCRendererSoftware.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp#...
cc/CCRendererSoftware.cpp:123:
m_outputDevice->didChangeViewportSize(WebSize(viewportSize().width(),
viewportSize().height()));
On 2012/09/19 18:35:14, danakj wrote:
> How come the software renderer needs a call like this but the GL one does not?
> I'm wondering especially cuz CCRendererGL doesn't have an m_outputDevice at
all.

For the test harness and GL adapter, they need to know what size bitmap to
create.  With the platform surface, the bitmap will be created somewhere else,
but it will probably still be useful for the output device to know what the
renderer thinks the size of the viewport is.  We may want to assert they're the
same for example.

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp#...
cc/CCResourceProvider.cpp:344: m_maxTextureSize = INT_MAX;
On 2012/09/19 18:35:14, danakj wrote:
> If you do something like
> 
> if (textureSize > m_maxTextureSize)
> 
> and they are both ints, and m_maxTextureSize == INT_MAX, isn't that going to
> just going to be always true?
> 
> If this is going to be INT_MAX should m_maxTextureSize be an unsigned (this
> would force other places to be unsigned in order to compare)?
> 
> It reminds me of all the overflow issues caused by INT_MAX stuff in the
subpixel
> layout code. The checks we have in place for max texture size would be no-ops
in
> this case.

(textureSize > m_maxTextureSize) would always be false, as desired.  That said
you have a point that INT_MAX has some insane properties, for example if we ever
add it to any positive number.  I mainly wanted to avoid a magic number.  I
guess we could go with INT_MAX / 2 or 100000000?

[danakj, 2012-09-19 20:06:17]

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp
File cc/CCLayerTreeHostImpl.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCLayerTreeHostImpl.cpp...
cc/CCLayerTreeHostImpl.cpp:691: renderer = CCRendererSoftware::create(this,
resourceProvider.get(), contextRef->softwareDevice());
On 2012/09/19 19:58:27, aelias wrote:
> On 2012/09/19 18:35:14, danakj wrote:
> > Should there be an else { ASSERT_NOT_REACHED(); } here? Or remove the if on
> the
> > else and ASSERT(softwareDevice)?
> > 
> > Or it ok to leave this with renderer == NULL? The release() below makes me
> think
> > it's not.
> 
> The release() will not crash given a NULL pointer.  This function returns
false
> if the renderer could not be initialized.

ah ok! right it didn't worry about else before either. "else if" without else is
just a warning trigger in my internal compiler :)

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp
File cc/CCRendererSoftware.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCRendererSoftware.cpp#...
cc/CCRendererSoftware.cpp:123:
m_outputDevice->didChangeViewportSize(WebSize(viewportSize().width(),
viewportSize().height()));
On 2012/09/19 19:58:27, aelias wrote:
> On 2012/09/19 18:35:14, danakj wrote:
> > How come the software renderer needs a call like this but the GL one does
not?
> > I'm wondering especially cuz CCRendererGL doesn't have an m_outputDevice at
> all.
> 
> For the test harness and GL adapter, they need to know what size bitmap to
> create.  With the platform surface, the bitmap will be created somewhere else,
> but it will probably still be useful for the output device to know what the
> renderer thinks the size of the viewport is.  We may want to assert they're
the
> same for example.

I see, thanks.

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp
File cc/CCResourceProvider.cpp (right):

https://codereview.chromium.org/10918258/diff/6001/cc/CCResourceProvider.cpp#...
cc/CCResourceProvider.cpp:344: m_maxTextureSize = INT_MAX;
On 2012/09/19 19:58:27, aelias wrote:
> On 2012/09/19 18:35:14, danakj wrote:
> > If you do something like
> > 
> > if (textureSize > m_maxTextureSize)
> > 
> > and they are both ints, and m_maxTextureSize == INT_MAX, isn't that going to
> > just going to be always true?
> > 
> > If this is going to be INT_MAX should m_maxTextureSize be an unsigned (this
> > would force other places to be unsigned in order to compare)?
> > 
> > It reminds me of all the overflow issues caused by INT_MAX stuff in the
> subpixel
> > layout code. The checks we have in place for max texture size would be
no-ops
> in
> > this case.
> 
> (textureSize > m_maxTextureSize) would always be false, as desired.  That said
> you have a point that INT_MAX has some insane properties, for example if we
ever
> add it to any positive number.  I mainly wanted to avoid a magic number.  I
> guess we could go with INT_MAX / 2 or 100000000?

er.. false. yes. I'd prefer a magic number, yeh. Maybe something based on
INT_MAX more than an independent constant, so INT_MAX/2.

[aelias_OOO_until_Jul13, 2012-09-20 23:09:03]

OK, I added some smoke tests.  Please take another look.

[enne (OOO), 2012-09-21 21:35:10]

You're going to need to fix Clang errors from your inlined virtuals, but LGTM
other than that.

[commit-bot: I haz the power, 2012-09-24 19:07:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/10918258/24001

[commit-bot: I haz the power, 2012-09-24 19:21:26]

Retried try job too often for step(s) build

[commit-bot: I haz the power, 2012-09-24 19:32:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aelias@chromium.org/10918258/22023

[commit-bot: I haz the power, 2012-09-24 21:32:59]

Change committed as 158396