Implemented browser side software compositing.

Implemented browser side software compositing behind the --ui-enable-software-compositing flag.


BUG=124671, 161008
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187123

[piman, 2013-03-06 19:09:41]

You added a lot of reviewers. Who do you expect to review what part?

[slavi, 2013-03-06 19:20:50]

On 2013/03/06 19:09:41, piman wrote:
> You added a lot of reviewers. Who do you expect to review what part?

I added the respective OWNERS from the tree.

I guess a minimal list would look like:
ben for content/browser/render_host/
piman for ui/compositor/ and content/
danakj for ui/gfx/
davemoore for ui/base/x/

[piman, 2013-03-06 19:22:30]

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/software_output_device.h (right):

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/software_output_device.h:24:
SoftwareOutputDevice(ui::Compositor* compositor);
nit: explicit

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/software_output_device.h:24:
SoftwareOutputDevice(ui::Compositor* compositor);
When threaded compositing is on, This class will be called from the thread, from
where you can't call into the ui::Compositor (which is a main thread class
only).

Note: it sounds like you only need the HWND / X Drawable. Maybe you can extract
that at construction time (which happens on the main thread), and keep it
around.

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/software_output_device_linux.cc (right):

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/software_output_device_linux.cc:25:
XFreeGC(display_, gc_);
When threaded compositing is on, this, and multiple calls below, is called on
the thread. The display isn't thread safe, so it's problematic to do X calls
from the other thread with the main thread display.

You really have 3 options:
- say threaded compositing doesn't work with the software device. This may be
fine for an initial approach, but you need to add checks for this, and come up
with a plan about how to enable it.
- post tasks to the main thread to do the blitting there.
- open a second display on the compositor thread, though you need to handle
synchronization between the 2 displays

I don't know whether Windows has similar restrictions or not.

[danakj, 2013-03-06 19:29:40]

On Wed, Mar 6, 2013 at 2:20 PM, <skaslev@chromium.org> wrote:

> On 2013/03/06 19:09:41, piman wrote:
>
>> You added a lot of reviewers. Who do you expect to review what part?
>>
>
> I added the respective OWNERS from the tree.
>
> I guess a minimal list would look like:
> ben for content/browser/render_host/
> piman for ui/compositor/ and content/
> danakj for ui/gfx/
>

Sorry, can you add an owner of ui/ instead? I only own the geometry types
there.


> davemoore for ui/base/x/
>
>
https://codereview.chromium.**org/12542006/<https://codereview.chromium.org/1...
>

[piman, 2013-03-06 19:31:18]

On Wed, Mar 6, 2013 at 11:20 AM, <skaslev@chromium.org> wrote:

> On 2013/03/06 19:09:41, piman wrote:
>
>> You added a lot of reviewers. Who do you expect to review what part?
>>
>
> I added the respective OWNERS from the tree.
>

Note: in general don't rely on OWNERs to do the first review. Find a
reviewer (if possible a single one) that is familiar with the area(s)
concerned, and ask them to review first. Then add OWNERs for the respective
areas if needed.

In this case, I'm happy to do most of the review here. However I'm no
windows expert, nor am I an owner for ui/gfx or ui/base.
The best would probably be to separate out the StretchDIBits move into a
separate CL, and get a reviewer/owner for that, that then doesn't need to
be involved with this CL.

Thanks,
Antoine


>
> I guess a minimal list would look like:
> ben for content/browser/render_host/
> piman for ui/compositor/ and content/
> danakj for ui/gfx/
> davemoore for ui/base/x/
>
>
https://codereview.chromium.**org/12542006/<https://codereview.chromium.org/1...
>

[slavi, 2013-03-06 21:51:19]

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/software_output_device.h (right):

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/software_output_device.h:24:
SoftwareOutputDevice(ui::Compositor* compositor);
On 2013/03/06 19:22:30, piman wrote:
> nit: explicit

Done

https://codereview.chromium.org/12542006/diff/1/content/browser/renderer_host...
content/browser/renderer_host/software_output_device.h:24:
SoftwareOutputDevice(ui::Compositor* compositor);
On 2013/03/06 19:22:30, piman wrote:
> When threaded compositing is on, This class will be called from the thread,
from
> where you can't call into the ui::Compositor (which is a main thread class
> only).
> 
> Note: it sounds like you only need the HWND / X Drawable. Maybe you can
extract
> that at construction time (which happens on the main thread), and keep it
> around.

I think it's fine not to support in-browser threaded composting until all the
pieces fit together.

I've added a CHECK to make sure we're not in threaded mode and created a bug
about the issue at crbug.com/180702

[slavi, 2013-03-06 21:58:01]

On 2013/03/06 19:29:40, danakj wrote:
> Sorry, can you add an owner of ui/ instead? I only own the geometry types
> there.

+sky Scott, can you review the ui/ changes?

[sky, 2013-03-06 22:40:51]

sky->darin (ui/gfx/gdi_util*)

[piman, 2013-03-07 00:25:51]

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/software_output_device.h (right):

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:22: class
SoftwareOutputDevice : public cc::SoftwareOutputDevice {
note: you have 2 completely separate implementations of this class (a win one
and a linux one). They don't share anything.

It would be better to have 2 explicit classes (SoftwareOutputDeviceWin and
SoftwareOutputDeviceLinux), and a single #ifdef in
SoftwareTransportFactory::CreateOutputSurface

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:37: BITMAPINFOHEADER
bitmap_header_;
Shouldn't this be a BITMAPINFO? The reinterpret_cast in EndPaint is invalid
(BITMAPINFO is a superset of BITMAPINFOHEADER).

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/software_output_device_linux.cc (right):

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device_linux.cc:23: // TODO Remove
this when crbug.com/180702 is fixed
nit: syntax is // TODO(skaslev): Remove this when crbug.com/180702 is fixed.
(trailing period included)

Note, the problem isn't so much that there's a bug with the non-Ash case (what
crbug.com/180702 is about). This wouldn't work on Ash/chromeos=1.

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device_linux.cc:25:
switches::kUIEnableThreadedCompositing));
How about moving this check to image_transport_factory before creating the
SoftwareTransportFactory?
This feels really out of place here

https://codereview.chromium.org/12542006/diff/8001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12542006/diff/8001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:380:
command_line->HasSwitch(switches::kUIEnableSoftwareCompositing);
This shouldn't be here. It should be set by SoftwareRenderer if the
OutputSurface isn't capable of partial updates. Maybe adding a cap bit into the
OutputSurface.

[slavi, 2013-03-07 01:48:29]

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/software_output_device.h (right):

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:22: class
SoftwareOutputDevice : public cc::SoftwareOutputDevice {
On 2013/03/07 00:25:51, piman wrote:
> note: you have 2 completely separate implementations of this class (a win one
> and a linux one). They don't share anything.
> 
> It would be better to have 2 explicit classes (SoftwareOutputDeviceWin and
> SoftwareOutputDeviceLinux), and a single #ifdef in
> SoftwareTransportFactory::CreateOutputSurface

Done.

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:37: BITMAPINFOHEADER
bitmap_header_;
On 2013/03/07 00:25:51, piman wrote:
> Shouldn't this be a BITMAPINFO? The reinterpret_cast in EndPaint is invalid
> (BITMAPINFO is a superset of BITMAPINFOHEADER).

That's fine since gfx::CreateBitmapHeader sets |.biSize| to
sizeof(BITMAPINFOHEADER). It's rather a winapi quirk.

BackingStoreWin does the same thing:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Maybe change gfx::StretchDIBits to accept BITMAPINFOHEADER and have the
reinterpret_cast inside instead of in users?

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:37: BITMAPINFOHEADER
bitmap_header_;
On 2013/03/07 00:25:51, piman wrote:
> Shouldn't this be a BITMAPINFO? The reinterpret_cast in EndPaint is invalid
> (BITMAPINFO is a superset of BITMAPINFOHEADER).

That's fine since gfx::CreateBitmapHeader sets |.biSize| to
sizeof(BITMAPINFOHEADER). It's a winapi quirk.

BackingStoreWin does the same thing:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

Maybe I should change gfx::StretchDIBits to accept BITMAPINFOHEADER and have the
reinterpret_cast inside instead of in users?

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/software_output_device_linux.cc (right):

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device_linux.cc:23: // TODO Remove
this when crbug.com/180702 is fixed
On 2013/03/07 00:25:51, piman wrote:
> nit: syntax is // TODO(skaslev): Remove this when crbug.com/180702 is fixed.
> (trailing period included)

Done.

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device_linux.cc:25:
switches::kUIEnableThreadedCompositing));
On 2013/03/07 00:25:51, piman wrote:
> How about moving this check to image_transport_factory before creating the
> SoftwareTransportFactory?
> This feels really out of place here

Changed that to an explicit DCHECK we're called on the correct thread.

https://codereview.chromium.org/12542006/diff/8001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/12542006/diff/8001/ui/compositor/compositor.c...
ui/compositor/compositor.cc:380:
command_line->HasSwitch(switches::kUIEnableSoftwareCompositing);
On 2013/03/07 00:25:51, piman wrote:
> This shouldn't be here. It should be set by SoftwareRenderer if the
> OutputSurface isn't capable of partial updates. Maybe adding a cap bit into
the
> OutputSurface.

I found that this is not really necessary since the software renderer doesn't
respect that override at all:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/software_render...

[piman, 2013-03-07 01:57:36]

LGTM for content/ and ui/compositor.

Please have a windows expert/owner look at the gfx::StretchDIBits changes/usage
pattern.

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
File content/browser/renderer_host/software_output_device.h (right):

https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
content/browser/renderer_host/software_output_device.h:37: BITMAPINFOHEADER
bitmap_header_;
On 2013/03/07 01:48:30, slavi wrote:
> On 2013/03/07 00:25:51, piman wrote:
> > Shouldn't this be a BITMAPINFO? The reinterpret_cast in EndPaint is invalid
> > (BITMAPINFO is a superset of BITMAPINFOHEADER).
> 
> That's fine since gfx::CreateBitmapHeader sets |.biSize| to
> sizeof(BITMAPINFOHEADER). It's a winapi quirk.
> 
> BackingStoreWin does the same thing:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> 
> Maybe I should change gfx::StretchDIBits to accept BITMAPINFOHEADER and have
the
> reinterpret_cast inside instead of in users?

I'm no expert in the area - whatever the resident expert wants.
Still, you can make a BITMAPINFO, and then pass bitmap_header_.bmiHeader to
gfx::StretchDIBits and not have the ugly reinterpret_cast.

[slavi, 2013-03-08 03:01:45]

On 2013/03/07 01:57:36, piman wrote:
> LGTM for content/ and ui/compositor.
> 
> Please have a windows expert/owner look at the gfx::StretchDIBits
changes/usage
> pattern.
> 
>
https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
> File content/browser/renderer_host/software_output_device.h (right):
> 
>
https://codereview.chromium.org/12542006/diff/8001/content/browser/renderer_h...
> content/browser/renderer_host/software_output_device.h:37: BITMAPINFOHEADER
> bitmap_header_;
> On 2013/03/07 01:48:30, slavi wrote:
> > On 2013/03/07 00:25:51, piman wrote:
> > > Shouldn't this be a BITMAPINFO? The reinterpret_cast in EndPaint is
invalid
> > > (BITMAPINFO is a superset of BITMAPINFOHEADER).
> > 
> > That's fine since gfx::CreateBitmapHeader sets |.biSize| to
> > sizeof(BITMAPINFOHEADER). It's a winapi quirk.
> > 
> > BackingStoreWin does the same thing:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...
> > 
> > Maybe I should change gfx::StretchDIBits to accept BITMAPINFOHEADER and have
> the
> > reinterpret_cast inside instead of in users?
> 
> I'm no expert in the area - whatever the resident expert wants.
> Still, you can make a BITMAPINFO, and then pass bitmap_header_.bmiHeader to
> gfx::StretchDIBits and not have the ugly reinterpret_cast.

Done. There're no reinterpret_cast now.

[Nico, 2013-03-08 23:51:17]

gfx/ lgtm

[commit-bot: I haz the power, 2013-03-09 00:02:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skaslev@chromium.org/12542006/42001

[commit-bot: I haz the power, 2013-03-09 02:51:17]

Change committed as 187123