Support single-threaded impl-side painting

Support single-threaded impl-side painting

This makes the synchronous composite path support impl-side painting.
This initial version is intended for converting layout tests, as a real
version for the browser would commit to the active tree directly.

This path is currently not used anywhere (it DCHECKed) and the only
consumer after this patch is a unittest to verify that it works.

Depends on https://codereview.chromium.org/672673003/ so that
WaitForTasksRunningToComplete includes all visible tiles.

Depends on https://codereview.chromium.org/668123003/ for correctness,
as zero copy and masks aren't supported quite yet.

R=danakj@chromium.org,reveman@chromium.org,vmpstr@chromium.org
BUG=381919
Committed: https://crrev.com/69277cb70300d50ef288cebb9f1383fe9c94121a
Cr-Commit-Position: refs/heads/master@{#301966}

[danakj, 2014-10-24 20:40:06]

I looked at cc/trees and content/ and that part LGTM. I think reveman/vmpstr
should look over this too.

[enne (OOO), 2014-10-24 23:44:39]

This patch could land now and I could start converting non-mask tests to use
impl-side painting.  So, if y'all have a chance to review it, please do, as it
unlocks a lot of work that can be done in parallel.

[vmpstr, 2014-10-27 18:29:49]

lgtm

[enne (OOO), 2014-10-27 19:05:27]

enne@chromium.org changed reviewers:
+ jbauman@chromium.org

[enne (OOO), 2014-10-27 19:05:42]

jbauman: content/renderer/gpu/render_widget_compositor.cc OWNERS?

[reveman, 2014-10-27 19:15:50]

sorry, not lgtm

https://codereview.chromium.org/667793004/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/667793004/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:187:
task_graph_runner_->WaitForTasksToFinishRunning(namespace_token_);
This doesn't really make sense. It will block on current thread for all tasks to
run on current thread. It's guaranteed to deadlock.

NOTREACHED() or NOTIMPLEMENTED() would be better but I think we should avoid
that too if possible. See my cc/resources/rasterizer.h comment.

https://codereview.chromium.org/667793004/diff/40001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/667793004/diff/40001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:291:
task_graph_runner_->WaitForTasksToFinishRunning(namespace_token_);
Note that this doesn't do what is expected. It will only wait for currently
scheduled tasks to finish running and not take throttling into account.

https://codereview.chromium.org/667793004/diff/40001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/667793004/diff/40001/cc/resources/rasterizer....
cc/resources/rasterizer.h:156: virtual void WaitForTasksToFinishRunning() = 0;
I don't think we should add this to the Rasterizer interface when it can only be
supported by the 0-copy implementation.

Here's an alternative solution:

1. In single-threaded impl-side painting mode, have LTHI create a
ZeroCopyRasterWorkerPool instance using a non-threaded TaskGraphRunner instance.

2. In SingleThreadProxy::CompositeImmediately, after calling
LayerTreeHostImpl::ManageTiles(), call LayerTreeHostImpl::InitializeAllTiles(),
which in turn will call TaskGraphRunner::RunUntilIdle() on the task graph runner
instance given to the ZeroCopyRasterWorkerPool instance.

This should have the same effect but would remove the need for this plumbing and
having incorrect implementations of this interface in all but one Rasterizer
implementation.

This would run all raster on the compositor thread instead of worker threads. If
it needs to run on worker threads then adding a WaitForTasksToFinishRunning to
only the ZeroCopyRasterWorkerPool class is an option.

[enne (OOO), 2014-10-27 20:23:24]

https://codereview.chromium.org/667793004/diff/40001/cc/resources/gpu_raster_...
File cc/resources/gpu_raster_worker_pool.cc (right):

https://codereview.chromium.org/667793004/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:187:
task_graph_runner_->WaitForTasksToFinishRunning(namespace_token_);
On 2014/10/27 19:15:49, reveman wrote:
> This doesn't really make sense. It will block on current thread for all tasks
to
> run on current thread. It's guaranteed to deadlock.
> 
> NOTREACHED() or NOTIMPLEMENTED() would be better but I think we should avoid
> that too if possible. See my cc/resources/rasterizer.h comment.

I'm happy to change this if this is not right, but this is what you directly
suggested to me to do.

https://codereview.chromium.org/667793004/diff/40001/cc/resources/rasterizer.h
File cc/resources/rasterizer.h (right):

https://codereview.chromium.org/667793004/diff/40001/cc/resources/rasterizer....
cc/resources/rasterizer.h:156: virtual void WaitForTasksToFinishRunning() = 0;
On 2014/10/27 19:15:49, reveman wrote:
> Here's an alternative solution:
> 
> 1. In single-threaded impl-side painting mode, have LTHI create a
> ZeroCopyRasterWorkerPool instance using a non-threaded TaskGraphRunner
instance.

What do you mean by non-threaded? Is this something I would add, or is there
some way to specify this that I can't see?

> 2. In SingleThreadProxy::CompositeImmediately, after calling
> LayerTreeHostImpl::ManageTiles(), call
LayerTreeHostImpl::InitializeAllTiles(),
> which in turn will call TaskGraphRunner::RunUntilIdle() on the task graph
runner
> instance given to the ZeroCopyRasterWorkerPool instance.

What do you mean by LayerTreeHostImpl::InitializeAllTiles?  Is this some new
function that I would write or are you referring to something?

[enne (OOO), 2014-10-27 21:57:46]

Thanks for all the feedback.  Here's a patch with a lot less plumbing.

It does turn out this depends on all of the in process command buffer / image
factory changes, since it's trying to test pixels in a cc unit test.

[reveman, 2014-10-28 03:46:33]

lgtm with one nit and one suggestion

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1749:
DCHECK(!settings_.single_thread_proxy_scheduler);
nit: can we DCHECK to make sure that settings.scheduled_raster_task_limit and
settings.use_zero_copy are set correctly too as this won't do what the name
indicate that it does otherwise?

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:2060: }
I think you can replace all this with:

synchronous_single_threaded
    ? &single_thread_synchronous_task_graph_runner_
    : RasterWorkerPool::GetTaskGraphRunner()

below if you just avoid using "new" to allocate
single_thread_synchronous_task_graph_runner_.

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.h (right):

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.h:700: scoped_ptr<TaskGraphRunner>
single_thread_synchronous_task_graph_runner_;
TaskGraphRunner is pretty lightweight. I think you can remove scoped_ptr<> and
make the code in the .cc file a bit cleaner by removing the possibility for this
to not be initialized.

[enne (OOO), 2014-10-28 16:49:56]

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1749:
DCHECK(!settings_.single_thread_proxy_scheduler);
On 2014/10/28 03:46:33, reveman wrote:
> nit: can we DCHECK to make sure that settings.scheduled_raster_task_limit and
> settings.use_zero_copy are set correctly too as this won't do what the name
> indicate that it does otherwise?

We only need the zero copy DCHECK if I follow your other nits.

The reason single_thread_synchronous_task_graph_runner_ is a scoped ptr is so
that it's only valid when it's actually being used.  If I make it a member
variable instead of a scoped_ptr, I can add a bool variable that says if it's
valid or not, but to me that seems easier to screw up.

[reveman, 2014-10-28 18:08:24]

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/667793004/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1749:
DCHECK(!settings_.single_thread_proxy_scheduler);
On 2014/10/28 16:49:56, enne wrote:
> On 2014/10/28 03:46:33, reveman wrote:
> > nit: can we DCHECK to make sure that settings.scheduled_raster_task_limit
and
> > settings.use_zero_copy are set correctly too as this won't do what the name
> > indicate that it does otherwise?
> 
> We only need the zero copy DCHECK if I follow your other nits.

hm, this will only initialize some tiles if settings.scheduled_raster_task_limit
is not high enough afaict. I guess we can change the name to
SynchronouslyInitializeAllTilesWithScheduledRasterTasks() or something like that
to make the behavior less strict.

> 
> The reason single_thread_synchronous_task_graph_runner_ is a scoped ptr is so
> that it's only valid when it's actually being used.  If I make it a member
> variable instead of a scoped_ptr, I can add a bool variable that says if it's
> valid or not, but to me that seems easier to screw up.

Do we really need more state to track if it's valid to use this than what we
already have? If we're going to use a scoped_ptr as a mechanism to tell if it's
valid to use this then shouldn't the DCHECKs above be replaced by
DCHECK(single_thread_synchronous_task_graph_runner_)?

scoped_ptr DCHECK or existing DCHECKs + zero-copy and task-limit DCHECKs to
validate correct behavior are both solutions I'm fine with. I you think it's
necessary to add a bool when removing the scoped_ptr then I'd rather you keep
the scoped_ptr.

[enne (OOO), 2014-10-28 18:24:04]

I do feel like it's necessary to add a bool when removing the scoped_ptr. 
There's currently no other way to make sure that the right raster worker pool
and task graph runner is being used as it's all abstracted away in the create
function.  I don't want me or anybody else to have to debug some situation in
the future where the wrong one is silently being used.  If you see a better way
to enforce this, please let me know.

I removed the redundant DCHECKs.  Rather than adding in the suggested DHCECK for
scheduled_raster_task_limit, I changed the code so that use_zero_copy and
scheduled_raster_task_limit=max are automatically used in CreateTileManager
rather than requiring embedders to do the right dance.

[enne (OOO), 2014-10-28 18:24:42]

enne@chromium.org changed reviewers:
- jbauman@chromium.org

[reveman, 2014-10-28 18:53:09]

Not sure what you mean by "the right raster worker pool
and task graph runner is being used". What's the use-case you're worried about?
The tile manager can't use anything but the Rasterizer given to it at
construction time. And if someone makes the mistake of calling
single_thread_synchronous_task_graph_runner_.RunUntilIdle() when not in
synchronous-single-threaded mode then they'd simple run all tasks pending for
synchronous-single-threaded mode, which in that case will be 0. Is the plan to
not always use single_thread_synchronous_task_graph_runner_ in
synchronous-single-threaded mode and that would make it invalid to use this even
though we're currently in synchronous-single-threaded mode?

This lgtm either way. I'm just curious as to what use-case you worry about that
the scoped_ptr is protecting against.

https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:2025: size_t scheduled_raster_task_limit =
settings_.scheduled_raster_task_limit;
do we still need the setting?

[enne (OOO), 2014-10-28 19:00:15]

On 2014/10/28 at 18:53:09, reveman wrote:
> Not sure what you mean by "the right raster worker pool
> and task graph runner is being used". What's the use-case you're worried
about? The tile manager can't use anything but the Rasterizer given to it at
construction time. And if someone makes the mistake of calling
single_thread_synchronous_task_graph_runner_.RunUntilIdle() when not in
synchronous-single-threaded mode then they'd simple run all tasks pending for
synchronous-single-threaded mode, which in that case will be 0. Is the plan to
not always use single_thread_synchronous_task_graph_runner_ in
synchronous-single-threaded mode and that would make it invalid to use this even
though we're currently in synchronous-single-threaded mode?
> 
> This lgtm either way. I'm just curious as to what use-case you worry about
that the scoped_ptr is protecting against.

My worry is that the selection of the raster worker pool is currently being done
by a complicated set of ordered conditionals based on settings and context
capabilities.  It seems very plausible that in the future somebody might reorder
or accidentally set/unset some setting or capability such that a different
raster worker pool gets chosen.  Since the synchronous path only works with one
raster worker pool with a specific task graph runner, I wanted to be able to
DCHECK that it was sanely using the raster worker pool it was expecting to be
able to.

>
https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_impl.cc:2025: size_t scheduled_raster_task_limit =
settings_.scheduled_raster_task_limit;
> do we still need the setting?

No, not necessarily.  But, it'd kind of nice to not have that be hidden?

[reveman, 2014-10-28 19:12:27]

On 2014/10/28 19:00:15, enne wrote:
> On 2014/10/28 at 18:53:09, reveman wrote:
> > Not sure what you mean by "the right raster worker pool
> > and task graph runner is being used". What's the use-case you're worried
> about? The tile manager can't use anything but the Rasterizer given to it at
> construction time. And if someone makes the mistake of calling
> single_thread_synchronous_task_graph_runner_.RunUntilIdle() when not in
> synchronous-single-threaded mode then they'd simple run all tasks pending for
> synchronous-single-threaded mode, which in that case will be 0. Is the plan to
> not always use single_thread_synchronous_task_graph_runner_ in
> synchronous-single-threaded mode and that would make it invalid to use this
even
> though we're currently in synchronous-single-threaded mode?
> > 
> > This lgtm either way. I'm just curious as to what use-case you worry about
> that the scoped_ptr is protecting against.
> 
> My worry is that the selection of the raster worker pool is currently being
done
> by a complicated set of ordered conditionals based on settings and context
> capabilities.  It seems very plausible that in the future somebody might
reorder
> or accidentally set/unset some setting or capability such that a different
> raster worker pool gets chosen.  Since the synchronous path only works with
one
> raster worker pool with a specific task graph runner, I wanted to be able to
> DCHECK that it was sanely using the raster worker pool it was expecting to be
> able to.

Ok, makes sense. Thanks.

> 
> >
>
https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
> > File cc/trees/layer_tree_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/667793004/diff/100001/cc/trees/layer_tree_hos...
> > cc/trees/layer_tree_host_impl.cc:2025: size_t scheduled_raster_task_limit =
> settings_.scheduled_raster_task_limit;
> > do we still need the setting?
> 
> No, not necessarily.  But, it'd kind of nice to not have that be hidden?

Ok, I would avoid exposing it in LayerTreeSettings unless needed but it's fine
to leave it there for now.

[enne (OOO), 2014-10-29 21:30:08]

https://codereview.chromium.org/667793004/diff/120001/cc/test/test_in_process...
File cc/test/test_in_process_context_provider.cc (right):

https://codereview.chromium.org/667793004/diff/120001/cc/test/test_in_process...
cc/test/test_in_process_context_provider.cc:130: capabilities.gpu.image = true;
reveman: This is one of the parts that was missing from your patches.

[enne (OOO), 2014-10-29 21:30:14]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-10-29 21:33:29]

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

[reveman, 2014-10-29 22:22:20]

https://codereview.chromium.org/667793004/diff/120001/cc/test/test_in_process...
File cc/test/test_in_process_context_provider.cc (right):

https://codereview.chromium.org/667793004/diff/120001/cc/test/test_in_process...
cc/test/test_in_process_context_provider.cc:130: capabilities.gpu.image = true;
On 2014/10/29 21:30:08, enne wrote:
> reveman: This is one of the parts that was missing from your patches.

Acknowledged.

[commit-bot: I haz the power, 2014-10-29 23:03:49]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-29 23:04:34]

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