sync compositor: Software draw

sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, share it to renderer to draw, and then
draw the memory into the canvas passed in.

BUG=545611
Committed: https://crrev.com/f6d263e9b4a207273d81f25c33bf62330287693a
Cr-Commit-Position: refs/heads/master@{#361564}

[boliu, 2015-11-23 17:23:01]

Description was changed from

==========
sync compositor: Software draw

BUG=
==========

to

==========
sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, zero it, share it to renderer to draw,
and then draw the memory into the canvas passed in.

There are lots of room for optimizations in the future.

BUG=545611
==========

[boliu, 2015-11-23 17:23:11]

boliu@chromium.org changed reviewers:
+ sievers@chromium.org

[boliu, 2015-11-23 17:23:11]

wdyt?

[boliu, 2015-11-23 19:40:25]

Description was changed from

==========
sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, zero it, share it to renderer to draw,
and then draw the memory into the canvas passed in.

There are lots of room for optimizations in the future.

BUG=545611
==========

to

==========
sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, share it to renderer to draw, and then
draw the memory into the canvas passed in.

There are lots of room for optimizations in the future.

BUG=545611
==========

[boliu, 2015-11-23 19:40:54]

Description was changed from

==========
sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, share it to renderer to draw, and then
draw the memory into the canvas passed in.

There are lots of room for optimizations in the future.

BUG=545611
==========

to

==========
sync compositor: Software draw

Minimal change to implement software draw in ipc-based
synchronous compositor. Changes are all internal to sync
compositor.

Allocate a new shared memory the size of the canvas in
ARGB_8888 format, share it to renderer to draw, and then
draw the memory into the canvas passed in.

BUG=545611
==========

[boliu, 2015-11-23 23:04:19]

boliu@chromium.org changed reviewers:
+ dcheng@chromium.org

[boliu, 2015-11-23 23:04:20]

+dcheng for security review

So tomorrow is my last day before a 1.5 week long vacation. This is not very
time sensitive, but would unblock some other work for others.

Hopefully not that controversial? :)

[dcheng, 2015-11-23 23:26:36]

Just an initial glance.

https://codereview.chromium.org/1469773002/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1469773002/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:145:
IPC_SYNC_MESSAGE_ROUTED2_3(SyncCompositorMsg_DemandDrawSw,
Can we add section comments like other IPC messages?

e.g. these messages go from browser process -> compositor process

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:219: if
(!shm.Map(params.size.width() * params.size.height() * 4))
This should probably check for overflow.

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:226: if
(!bitmap.installPixels(info, shm.memory(), params.size.width() * 4))
As should this.

[boliu, 2015-11-24 00:16:34]

https://codereview.chromium.org/1469773002/diff/60001/content/common/android/...
File content/common/android/sync_compositor_messages.h (right):

https://codereview.chromium.org/1469773002/diff/60001/content/common/android/...
content/common/android/sync_compositor_messages.h:145:
IPC_SYNC_MESSAGE_ROUTED2_3(SyncCompositorMsg_DemandDrawSw,
On 2015/11/23 23:26:35, dcheng wrote:
> Can we add section comments like other IPC messages?
> 
> e.g. these messages go from browser process -> compositor process

Copied the same section headings from content/common/view_messages.h

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:219: if
(!shm.Map(params.size.width() * params.size.height() * 4))
On 2015/11/23 23:26:35, dcheng wrote:
> This should probably check for overflow.

Done.

https://codereview.chromium.org/1469773002/diff/60001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:226: if
(!bitmap.installPixels(info, shm.memory(), params.size.width() * 4))
On 2015/11/23 23:26:35, dcheng wrote:
> As should this.

Done.

[boliu, 2015-11-24 20:36:12]

sievers: half-day-left kind of ping? :p

[no sievers, 2015-11-24 23:38:40]

lgtm

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:97:
canvas->getDeviceSize().height());
nit: indent

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:103: size_t buffer_size =
params.size.width() * params.size.height() * 4;
nit: dcheck the pixel format?

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:110: base::ProcessHandle
renderer_process_handle =
no recycling? is this only supposed to be for once frame once in a while?

https://codereview.chromium.org/1469773002/diff/80001/content/renderer/androi...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/80001/content/renderer/androi...
content/renderer/android/synchronous_compositor_proxy.cc:221: if
(!stride.IsValid())
you don't trust the browser :)

[boliu, 2015-11-25 00:00:14]

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:97:
canvas->getDeviceSize().height());
On 2015/11/24 23:38:40, sievers wrote:
> nit: indent

Done. Also use getBaseLayerSize since getDeviceSize is deprecated.

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:103: size_t buffer_size =
params.size.width() * params.size.height() * 4;
On 2015/11/24 23:38:40, sievers wrote:
> nit: dcheck the pixel format?

Fun fact. The canvas passed in does has kUnknown_SkColorType.

But on the other hand, shm size has nothing to do with canvas color type either
in theory. It could be converted in drawBitmap if they don't match. What I
should DCHECK is SkImageInfo below has the expected format, which I've added.

https://codereview.chromium.org/1469773002/diff/80001/content/browser/android...
content/browser/android/synchronous_compositor_host.cc:110: base::ProcessHandle
renderer_process_handle =
On 2015/11/24 23:38:39, sievers wrote:
> no recycling? is this only supposed to be for once frame once in a while?

No. But that optimization can come later. It's somewhat complicated, since in
case this is only once in awhile, then we should no hold onto a giant piece of
memory forever.

[boliu, 2015-11-25 00:27:37]

dcheng: ping? now I'm excited if I can get this in today, maybe... :)

[dcheng, 2015-11-25 00:55:17]

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:107: size_t buffer_size =
params.size.width() * params.size.height() * 4;
info.getSafeSize()? Optionally, bail out if buffer_size == 0 (I'm not sure if
you can end up in a situation where that's possible)

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:136: if
(!bitmap.installPixels(info, shm.memory(), params.size.width() * 4))
Similarly, info.minRowBytes() here?

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:219:
base::CheckedNumeric<size_t> stride = 4;
I didn't realize this is in the renderer, so the CheckedNumeric here is
pointless. Sorry! Let's just revert this to the original.

[dcheng, 2015-11-25 00:56:18]

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:234:
SkImageInfo::MakeN32Premul(params.size.width(), params.size.height());
Similarly, if you calculate this first, you can use the Skia helpers instead of
replication the calculations.

[boliu, 2015-11-25 01:13:41]

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
File content/browser/android/synchronous_compositor_host.cc (right):

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:107: size_t buffer_size =
params.size.width() * params.size.height() * 4;
On 2015/11/25 00:55:17, dcheng wrote:
> info.getSafeSize()?

Done

> Optionally, bail out if buffer_size == 0 (I'm not sure if
> you can end up in a situation where that's possible)

Broke this into two parts.

If the size is 0, then just return true. In that case there is nothing to draw
anyway.

But if getSafeSize returns 0, that means it's a overflow, in that case  we
should return false. Can't possibly allocate that much memory on android
anyway..

https://codereview.chromium.org/1469773002/diff/100001/content/browser/androi...
content/browser/android/synchronous_compositor_host.cc:136: if
(!bitmap.installPixels(info, shm.memory(), params.size.width() * 4))
On 2015/11/25 00:55:17, dcheng wrote:
> Similarly, info.minRowBytes() here?

Done.

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
File content/renderer/android/synchronous_compositor_proxy.cc (right):

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:219:
base::CheckedNumeric<size_t> stride = 4;
On 2015/11/25 00:55:17, dcheng wrote:
> I didn't realize this is in the renderer, so the CheckedNumeric here is
> pointless. Sorry! Let's just revert this to the original.

Done.

https://codereview.chromium.org/1469773002/diff/100001/content/renderer/andro...
content/renderer/android/synchronous_compositor_proxy.cc:234:
SkImageInfo::MakeN32Premul(params.size.width(), params.size.height());
On 2015/11/25 00:56:18, dcheng wrote:
> Similarly, if you calculate this first, you can use the Skia helpers instead
of
> replication the calculations.

Done.

[dcheng, 2015-11-25 01:43:45]

lgtm

[boliu, 2015-11-25 01:44:49]

Thanks both :)

[boliu, 2015-11-25 01:44:51]

The CQ bit was checked by boliu@chromium.org

[boliu, 2015-11-25 01:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org
Link to the patchset: https://codereview.chromium.org/1469773002/#ps120001
(title: "dcheng review")

[commit-bot: I haz the power, 2015-11-25 01:45:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1469773002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1469773002/120001

[commit-bot: I haz the power, 2015-11-25 03:54:06]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-25 03:55:22]

Patchset 7 (id:??) landed as
https://crrev.com/f6d263e9b4a207273d81f25c33bf62330287693a
Cr-Commit-Position: refs/heads/master@{#361564}