Add preliminary support for hw-accelerated tile rasterization.

Add preliminary support for hw-accelerated tile rasterization.

BUG=316685
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243836

[alokp, 2013-12-16 19:57:35]

David/Vlad: This is not ready to be committed. I still need to add a few tests.
But could you please comment on the overall approach if this is something you
had in mind.

This patch pushes gpu rasterization behind RasterWorkerPool and gets rid of the
complexity in TileManager. I personally am not too happy with the awkwardness in
two specific areas:

1. RasterWorkerPoolTask needs to be aware if it is running on a worker thread or
origin thread. I think it should just expose Run(SkCanvas*) and Complete APIs.
The wrapper classes PixelBufferWorkerPoolTaskImpl and ImageWorkerPoolTaskImpl
should worry about threading or creating SkCanvas. That way I can create another
wrapper class that sets up GPU rasterization.

2. Either RasterWorkerPool::CompleteOriginThreadTasks should be renamed more
specifically to gpu rasterization or it should not be making contexts current or
flushing them.

I think we can clean this up in follow-up patches. But if you have suggestions
on addressing this easily, we can do it in this patch.

[reveman, 2013-12-17 00:28:48]

https://codereview.chromium.org/110883015/diff/20001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:74: void
ImageRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
you're missing a call to CompleteOriginThreadTasks in this function.

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

https://codereview.chromium.org/110883015/diff/20001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:141:
RasterWorkerPool::CompleteOriginThreadTasks(queue);
Should we call this after ScheduleCheckForCompletedRasterTasks()? That would
allow worker threads and uploads to run at the same time.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:260: class RasterWorkerPoolTaskGPU : public
internal::RasterWorkerPoolTask {
Could we use RasterWorkerPoolTaskImpl for this instead? And just add a
gpu_rasterization flag and appropriate DCHECKs to RunOnOriginThread() and
RunOnOriginThread()?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:298: desc.fConfig =
ToGrFormat(resource()->format());
How are GrPixelConfig formats used by Ganesh? Some of our shaders depend on
components having to be swizzled if
"!PlatformColor::SameComponentOrder(resource_->format())" is true. Is that also
true for when we raster with Ganesh?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:314: gr_context->flush();
as you have a flush in CompleteOriginThreadTasks, should we avoid the flush
here?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:600: void
RasterWorkerPool::CompleteOriginThreadTasks(RasterTask::Queue* queue) {
I think this needs to be RunTasksOnOriginThread (Or
RunGpuRasterizationTasksOnOriginThread). See my comment below.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:613: task->CompleteOnOriginThread();
Task completion needs to be delayed until ::CheckForCompletedTasks() is called.
The RasterWorkerPool implementation will need to take care of this.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:149: TaskVector origin_thread_tasks_;
Maybe we should put all tasks in |tasks_| for now and pull them out and run them
on the origin thread more as an implementation detail of the RasterWorkerPool.
Wdyt?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:174: bool must_run_on_origin_thread_;
Why must_run_on_origin_thread here but use_gpu_rasterization when passed to
CreateRasterTask? Also, could this be a internal::RasterWorkerPoolTask member
instead?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:236: void
CompleteOriginThreadTasks(RasterTask::Queue* queue);
Do you have to pass a RasterTask::Queue* to this function? Could you instead
pass a TaskVector?

[vmpstr, 2013-12-17 21:04:36]

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:560: return RasterTask(impl,
use_gpu_rasterization);
This might be echoing some of David's comments, but why do we need to pass
use_gpu_rasterization here if impl is already set to be the correct thing?

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:906: return raster_worker_pool_->CreateRasterTask(
At some point in time we've decided to stick to static functions (i can't find
the CL that had that discussion atm), but if this is a required change can you
make CreateImageDecodeTask be consistent (ie non static). As an unrelated patch,
if they are non statics we can put things like rendering_stats_instrumentation_
in there.

[reveman, 2013-12-17 21:25:14]

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:906: return raster_worker_pool_->CreateRasterTask(
Oh, I didn't notice this change. The static vs non-static debate is really about
whether tasks should be tied to a specific raster worker pool implementation or
not. We've been avoiding that so far and like to keep it that way if possible
for now.

If we decide to make tasks RWP impl specific then I'd like to do that separately
as I think there are some alternatives for how we could do that and there's a
number of things that I'd like to see change as part of that.

[alokp, 2013-12-17 22:12:15]

https://codereview.chromium.org/110883015/diff/20001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:74: void
ImageRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
On 2013/12/17 00:28:48, David Reveman wrote:
> you're missing a call to CompleteOriginThreadTasks in this function.

Done.

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

https://codereview.chromium.org/110883015/diff/20001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:141:
RasterWorkerPool::CompleteOriginThreadTasks(queue);
On 2013/12/17 00:28:48, David Reveman wrote:
> Should we call this after ScheduleCheckForCompletedRasterTasks()? That would
> allow worker threads and uploads to run at the same time.

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:260: class RasterWorkerPoolTaskGPU : public
internal::RasterWorkerPoolTask {
On 2013/12/17 00:28:48, David Reveman wrote:
> Could we use RasterWorkerPoolTaskImpl for this instead? And just add a
> gpu_rasterization flag and appropriate DCHECKs to RunOnOriginThread() and
> RunOnOriginThread()?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:298: desc.fConfig =
ToGrFormat(resource()->format());
On 2013/12/17 00:28:48, David Reveman wrote:
> How are GrPixelConfig formats used by Ganesh? Some of our shaders depend on
> components having to be swizzled if
> "!PlatformColor::SameComponentOrder(resource_->format())" is true. Is that
also
> true for when we raster with Ganesh?

Swizzling is never required when rasterizing with Ganesh. I will make sure that
this assertion is true.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:314: gr_context->flush();
On 2013/12/17 00:28:48, David Reveman wrote:
> as you have a flush in CompleteOriginThreadTasks, should we avoid the flush
> here?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:560: return RasterTask(impl,
use_gpu_rasterization);
On 2013/12/17 21:04:37, vmpstr wrote:
> This might be echoing some of David's comments, but why do we need to pass
> use_gpu_rasterization here if impl is already set to be the correct thing?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:600: void
RasterWorkerPool::CompleteOriginThreadTasks(RasterTask::Queue* queue) {
On 2013/12/17 00:28:48, David Reveman wrote:
> I think this needs to be RunTasksOnOriginThread (Or
> RunGpuRasterizationTasksOnOriginThread). See my comment below.

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:613: task->CompleteOnOriginThread();
On 2013/12/17 00:28:48, David Reveman wrote:
> Task completion needs to be delayed until ::CheckForCompletedTasks() is
called.
> The RasterWorkerPool implementation will need to take care of this.

AFAICT ImageRasterWorkerPool never calls CheckForCompletedTasks(). Also there is
no RasterWorkerPool::CheckForCompletedTasks(), it is in fact
WorkerPool::CheckForCompletedTasks(), which only evaluates tasks in the
task-graph. Since we do not put gpu-raster tasks in the graph, it is not
directly applicable here.

I have implemented it in a slightly different way, please check the latest patch
if it seems reasonable.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:149: TaskVector origin_thread_tasks_;
On 2013/12/17 00:28:48, David Reveman wrote:
> Maybe we should put all tasks in |tasks_| for now and pull them out and run
them
> on the origin thread more as an implementation detail of the RasterWorkerPool.
> Wdyt?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:174: bool must_run_on_origin_thread_;
On 2013/12/17 00:28:48, David Reveman wrote:
> Why must_run_on_origin_thread here but use_gpu_rasterization when passed to
> CreateRasterTask? Also, could this be a internal::RasterWorkerPoolTask member
> instead?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:236: void
CompleteOriginThreadTasks(RasterTask::Queue* queue);
On 2013/12/17 00:28:48, David Reveman wrote:
> Do you have to pass a RasterTask::Queue* to this function? Could you instead
> pass a TaskVector?

Done.

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/110883015/diff/20001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:906: return raster_worker_pool_->CreateRasterTask(
Changed it back to static.

[vmpstr, 2013-12-17 22:52:45]

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

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:227: task->DidRun(false);
In ImageRasterWorkerPool, similar block does a OnRasterTaskCompleted (which is
also changed to do special logic for gpu rasterization tasks). In here, you call
task->DidRun/WillComplete/etc directly. I think we should add some consistency
between the two methods.

I would vote for calling OnRasterTaskCompleted in both cases and handling gpu
rasterization tasks specially (this would allow us to still have the trace event
that is triggered from there)

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:556:
task->RunOnOriginThread(resource_provider_, context_provider_);
This kind of looks weird to me... We have a task->RunOnOriginThread (a generic
name) which takes specific objects, because we of course know what we need to
give for gpu rasterization :) 

It's not so much a comment on this patch, since I can see that RunOnWorkerThread
also takes some items that are not exactly generic.

In any case, there seems to be a bit of struggle between using generic types
that are opaque and will complete their work when needed (ie tasks / task vector
/ raster task / etc), and doing things like
DCHECK(task->use_gpu_rasterization()) and RunGpuRasterizationTasks where it's
not so much opaque anymore as to what is happening.


Do you think it might be better if we had a flag "should run on main thread" vs
"should run on worker thread" (i thought i saw something like this in here
already?), and keep the rest of the details opaque? Maybe add a new type of task
that is kind of like RasterTask except implemented with ganesh?

I'll admit that this code was already a bit too involved for me to really
understand the nuances of interacting between task vector/queues and RWP and
PBRWP and WP, so I'm reluctantly OK with this approach if that's what people
have decided (ie, this comment is mostly a structural comment, not
implementation details comment).

[reveman, 2013-12-17 23:03:16]

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:17: class ImageWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
FYI, you can exercise this RasterWorkerPool impl on desktop using
--enable-map-image

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:164:
RasterWorkerPool::RunGpuRasterizationTasks(gpu_rasterization_tasks);
RasterWorkerPool:: suffix unnecessary. Move comment to header.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:168: }
You'll have to implement CheckForCompletedTasks and run these callbacks at that
time.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:205: if (task->use_gpu_rasterization())
{
Hm, as you're not running gpu raster in the thread pool can't this just be
DCHECK(!task->use_gpu_rasterization())?

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

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:227: task->DidRun(false);
Should this be done by RasterWorkerPool::RunGpuRasterizationTasks?

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:230: task->DidComplete();
I think these 3 lines need to be moved to ::CheckForCompletedTasks().

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:160: void Raster(PicturePileImpl*
picture_pile, SkCanvas* canvas) {
can this be private?

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:545: if (tasks.empty()) return;
nit: move "return" to next line.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:238: void
RunGpuRasterizationTasks(RasterTaskVector& tasks);
can this be "const RasterTaskVector&"? otherwise "RasterTaskVector*" is probably
better.

[alokp, 2013-12-17 23:26:07]

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:17: class ImageWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
On 2013/12/17 23:03:16, David Reveman wrote:
> FYI, you can exercise this RasterWorkerPool impl on desktop using
> --enable-map-image

does it work on windows?

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:168: }
On 2013/12/17 23:03:16, David Reveman wrote:
> You'll have to implement CheckForCompletedTasks and run these callbacks at
that
> time.

I am confused. Do you mean implement RasterWorkerPool::CheckForCompletedTasks?
Where is it called in ImageRasterWorkerPool? How would
Image/PixelBufferRasterWorkerPool be notified of completed raster tasks without
a callback? The callback is only registered for tasks in the task-graph.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:205: if (task->use_gpu_rasterization())
{
On 2013/12/17 23:03:16, David Reveman wrote:
> Hm, as you're not running gpu raster in the thread pool can't this just be
> DCHECK(!task->use_gpu_rasterization())?

Sorry. I am confused. Are you suggesting not calling
ImageRasterWorkerPool::OnRasterTaskCompleted for gpu-raster tasks?

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

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:227: task->DidRun(false);
On 2013/12/17 23:03:16, David Reveman wrote:
> Should this be done by RasterWorkerPool::RunGpuRasterizationTasks?

Yes. It can be. I was just following the pattern used by other tasks.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:230: task->DidComplete();
On 2013/12/17 23:03:16, David Reveman wrote:
> I think these 3 lines need to be moved to ::CheckForCompletedTasks().

Are you suggesting implementing RasterWorkerPool::CheckForCompletedTasks, which
will iterate through the gpu-raster tasks and call these 3 functions?

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:556:
task->RunOnOriginThread(resource_provider_, context_provider_);
I admit - it is awkward. I also do not like must-run-on-origin-thread flag. It
does not say anything about GPU rasterization. I think longer term,
RasterWorkerPoolTask should not worry about origin/worker threads. It would just
Run and Complete on whatever thread they are called. The wrapper classes would
setup the thread and canvas.

I believe this kind of refactoring can be done in subsequent patches. This patch
at least pushes this complexity from TileManager to RasterWorkerPool and lets us
experiment with gpu rasterization.

[reveman, 2013-12-18 00:09:51]

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:17: class ImageWorkerPoolTaskImpl :
public internal::WorkerPoolTask {
On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> On 2013/12/17 23:03:16, David Reveman wrote:
> > FYI, you can exercise this RasterWorkerPool impl on desktop using
> > --enable-map-image
> 
> does it work on windows?

It should. If it doesn't, please file a bug. It should also work with the
software compositor.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:168: }
On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> On 2013/12/17 23:03:16, David Reveman wrote:
> > You'll have to implement CheckForCompletedTasks and run these callbacks at
> that
> > time.
> 
> I am confused. Do you mean implement RasterWorkerPool::CheckForCompletedTasks?
> Where is it called in ImageRasterWorkerPool? How would
> Image/PixelBufferRasterWorkerPool be notified of completed raster tasks
without
> a callback? The callback is only registered for tasks in the task-graph.

As discussed offline. TileManager calls RasterWorkerPool::CheckForCompletedTasks
and expects callbacks to only run at that time.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:205: if (task->use_gpu_rasterization())
{
On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> On 2013/12/17 23:03:16, David Reveman wrote:
> > Hm, as you're not running gpu raster in the thread pool can't this just be
> > DCHECK(!task->use_gpu_rasterization())?
> 
> Sorry. I am confused. Are you suggesting not calling
> ImageRasterWorkerPool::OnRasterTaskCompleted for gpu-raster tasks?

I missed that you were calling this function directly from
ImageRasterWorkerPool::ScheduleTasks. I don't think you should do this as it's
meant for tasks that run on worker threads. You can instead handle completion of
gpu tasks separately in a similar way to how you're already doing this for
PBRWP.

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

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:230: task->DidComplete();
On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> On 2013/12/17 23:03:16, David Reveman wrote:
> > I think these 3 lines need to be moved to ::CheckForCompletedTasks().
> 
> Are you suggesting implementing RasterWorkerPool::CheckForCompletedTasks,
which
> will iterate through the gpu-raster tasks and call these 3 functions?

You already have an impl of this function below:
PixelBufferRasterWorkerPool::CheckForCompletedTasks()
just do it from there instead.

And for ImageRasterWorkerPool you'll need to add:
ImageRasterWorkerPool::CheckForCompletedTasks() as it was not needed before
(WorkerPool::CheckForCompletedTasks() was already doing the right thing there
before).

[alokp, 2013-12-18 22:49:21]

I believe I have addressed all commments. Please le tme know if I missed
anything or there are things that should be done differently. I will add a few
unittests next if everyone agrees with the approach.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:164:
RasterWorkerPool::RunGpuRasterizationTasks(gpu_rasterization_tasks);
On 2013/12/17 23:03:16, David Reveman wrote:
> RasterWorkerPool:: suffix unnecessary. Move comment to header.

Done.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:168: }
On 2013/12/18 00:09:51, David Reveman wrote:
> On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> > On 2013/12/17 23:03:16, David Reveman wrote:
> > > You'll have to implement CheckForCompletedTasks and run these callbacks at
> > that
> > > time.
> > 
> > I am confused. Do you mean implement
RasterWorkerPool::CheckForCompletedTasks?
> > Where is it called in ImageRasterWorkerPool? How would
> > Image/PixelBufferRasterWorkerPool be notified of completed raster tasks
> without
> > a callback? The callback is only registered for tasks in the task-graph.
> 
> As discussed offline. TileManager calls
RasterWorkerPool::CheckForCompletedTasks
> and expects callbacks to only run at that time.

Done.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:205: if (task->use_gpu_rasterization())
{
On 2013/12/18 00:09:51, David Reveman wrote:
> On 2013/12/17 23:26:08, Alok Priyadarshi wrote:
> > On 2013/12/17 23:03:16, David Reveman wrote:
> > > Hm, as you're not running gpu raster in the thread pool can't this just be
> > > DCHECK(!task->use_gpu_rasterization())?
> > 
> > Sorry. I am confused. Are you suggesting not calling
> > ImageRasterWorkerPool::OnRasterTaskCompleted for gpu-raster tasks?
> 
> I missed that you were calling this function directly from
> ImageRasterWorkerPool::ScheduleTasks. I don't think you should do this as it's
> meant for tasks that run on worker threads. You can instead handle completion
of
> gpu tasks separately in a similar way to how you're already doing this for
> PBRWP.

Done.

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

https://codereview.chromium.org/110883015/diff/60001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:230: task->DidComplete();
I added an implementation of RWP::CheckForCompletedTasks instead.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:160: void Raster(PicturePileImpl*
picture_pile, SkCanvas* canvas) {
On 2013/12/17 23:03:16, David Reveman wrote:
> can this be private?

Done.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:545: if (tasks.empty()) return;
On 2013/12/17 23:03:16, David Reveman wrote:
> nit: move "return" to next line.

Done.

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/60001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:238: void
RunGpuRasterizationTasks(RasterTaskVector& tasks);
On 2013/12/17 23:03:16, David Reveman wrote:
> can this be "const RasterTaskVector&"? otherwise "RasterTaskVector*" is
probably
> better.

Done.

[vmpstr, 2013-12-19 00:30:33]

This approach makes sense to me.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:563: gpu_raster_tasks_.swap(*tasks);
Can you DCHECK that current gpu_raster_tasks_ is empty? Or is that not a
requirement?

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:576: DCHECK(task->use_gpu_rasterization());
This is already DCHECKed in RunOnOriginThread, unless this is a permanent check
and the other one is temporary?

[reveman, 2013-12-19 01:36:47]

Approach is good.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:540: it != gpu_raster_tasks_.end(); ++it) {
nit: indent should align with RasterTaskV.. not 4 spaces

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:563: gpu_raster_tasks_.swap(*tasks);
On 2013/12/19 00:30:33, vmpstr wrote:
> Can you DCHECK that current gpu_raster_tasks_ is empty? Or is that not a
> requirement?

Yes, this function being called multiple times without a call to
CheckForCompletedTasks is going to be a problem. I think we either have to allow
that or add some checks to prevent it. Allowing it by adding a
completed_gpu_raster_tasks_ vector might be cleanest imo.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:574: it != gpu_raster_tasks_.end(); ++it) {
nit: indent should align with RasterTaskV.. not 4 spaces

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:577:
task->RunOnOriginThread(resource_provider_, context_provider_);
Can we call DidSchedule() and WillRun() above this?

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:181: // scheduled tasks. Reply callbacks are
still guaranteed to run.
Do you mind replacing this comment with "// Overridden from WorkerPool:" while
here? As any documentation needed for this function should be on the WorkerPool
interface, not here.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:186: virtual void CheckForCompletedTasks()
OVERRIDE;
Hm, this will still result in raster task completion callbacks taking place
outside the scope of the CheckForCompletedTasks() call made by the tile manager
as PBRWP will call this periodically to kick off more uploads. Maybe have IRWP
implement this instead and add a protected RWP::CheckForCompletedGpuRasterTasks
function that PBRWP and IRWP can call.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:242: // Run raster tasks that use GPU on impl
thread.
nit: s/impl/current/

[alokp, 2013-12-19 06:25:41]

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:540: it != gpu_raster_tasks_.end(); ++it) {
On 2013/12/19 01:36:47, David Reveman wrote:
> nit: indent should align with RasterTaskV.. not 4 spaces

Done. Is this in style guide?

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:563: gpu_raster_tasks_.swap(*tasks);
On 2013/12/19 01:36:47, David Reveman wrote:
> On 2013/12/19 00:30:33, vmpstr wrote:
> > Can you DCHECK that current gpu_raster_tasks_ is empty? Or is that not a
> > requirement?
> 
> Yes, this function being called multiple times without a call to
> CheckForCompletedTasks is going to be a problem. I think we either have to
allow
> that or add some checks to prevent it. Allowing it by adding a
> completed_gpu_raster_tasks_ vector might be cleanest imo.

Done.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:574: it != gpu_raster_tasks_.end(); ++it) {
On 2013/12/19 01:36:47, David Reveman wrote:
> nit: indent should align with RasterTaskV.. not 4 spaces

Done.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:576: DCHECK(task->use_gpu_rasterization());
On 2013/12/19 00:30:33, vmpstr wrote:
> This is already DCHECKed in RunOnOriginThread, unless this is a permanent
check
> and the other one is temporary?

This is permanent (if there is such a thing). I am not sure about the other one.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:577:
task->RunOnOriginThread(resource_provider_, context_provider_);
On 2013/12/19 01:36:47, David Reveman wrote:
> Can we call DidSchedule() and WillRun() above this?

There are no RasterWorkerPoolTask::WillRun/DidSchedule. These tasks are not
WorkerPoolTasks.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:181: // scheduled tasks. Reply callbacks are
still guaranteed to run.
On 2013/12/19 01:36:47, David Reveman wrote:
> Do you mind replacing this comment with "// Overridden from WorkerPool:" while
> here? As any documentation needed for this function should be on the
WorkerPool
> interface, not here.

Done.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:186: virtual void CheckForCompletedTasks()
OVERRIDE;
Hm. It does not sound like the right thing to do. Could PBRWP use a different
function to periodically collect completed tasks instead? I can look into it as
a separate patch.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:242: // Run raster tasks that use GPU on impl
thread.
On 2013/12/19 01:36:47, David Reveman wrote:
> nit: s/impl/current/

Done.

[reveman, 2013-12-19 17:52:02]

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:186: virtual void CheckForCompletedTasks()
OVERRIDE;
On 2013/12/19 06:25:42, Alok Priyadarshi wrote:
> Hm. It does not sound like the right thing to do. Could PBRWP use a different
> function to periodically collect completed tasks instead? I can look into it
as
> a separate patch.

This doesn't have to be overridden by RWP implementations if RWP is implementing
a "Check" function. How about we rename this to CheckForCompletedRasterTasks.
And WorkerPool::CheckForCompletedTasks becomes a protected non-virtual method
that PBRWP can call as much as it likes without having to worry about triggering
completion of gpu raster tasks?

[reveman, 2013-12-19 18:43:35]

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:540: it != gpu_raster_tasks_.end(); ++it) {
On 2013/12/19 06:25:42, Alok Priyadarshi wrote:
> On 2013/12/19 01:36:47, David Reveman wrote:
> > nit: indent should align with RasterTaskV.. not 4 spaces
> 
> Done. Is this in style guide?

Maybe not but it's consistent with the rest of the worker pool code, if not also
most of the chromium code.

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:577:
task->RunOnOriginThread(resource_provider_, context_provider_);
On 2013/12/19 06:25:42, Alok Priyadarshi wrote:
> On 2013/12/19 01:36:47, David Reveman wrote:
> > Can we call DidSchedule() and WillRun() above this?
> 
> There are no RasterWorkerPoolTask::WillRun/DidSchedule. These tasks are not
> WorkerPoolTasks.

Ah, ok.

[alokp, 2013-12-19 21:37:51]

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:186: virtual void CheckForCompletedTasks()
OVERRIDE;
Good Idea! With this change we are moving away from inheritance to composition.
Is there any reason for RWP to inherit from WP? It could easily have WP as a
data member and IMO it would be cleaner.

[reveman, 2013-12-19 22:05:25]

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/110883015/diff/80001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:186: virtual void CheckForCompletedTasks()
OVERRIDE;
On 2013/12/19 21:37:51, Alok Priyadarshi wrote:
> Good Idea! With this change we are moving away from inheritance to
composition.
> Is there any reason for RWP to inherit from WP? It could easily have WP as a
> data member and IMO it would be cleaner.

Yes, I think we'll do that but I like to first land all changes needed to share
the thread pool across LTHIs (https://codereview.chromium.org/73923003). That
will likely result in the base WorkerPool class being removed completely and the
RasterWorkerPool having a TaskGraphRunner as data member.

[alokp, 2013-12-20 07:33:06]

I opted to rename WorkerPool::CheckForCompletedTasks to
WorkerPool::CheckForCompletedWorkerTasks because:
1. It was much less code.
2. I noticed a nice distinction between raster and upload tasks in PBRWP that I
did not want to disturb.

[reveman, 2013-12-20 13:44:42]

This looks great! Add a test or two to raster_worker_pool_unittest.cc and I
think this is ready to land.

https://codereview.chromium.org/110883015/diff/140001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/140001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:547: completed_gpu_raster_tasks_.clear();
nit: could you make this consistent with
PixelBufferRasterWorkerPool::CheckForCompletedTasks() by using a deque to store
completed gpu raster tasks and change the loop to be a "while (!empty) {
front(); pop_front(); }" style thing. You can move the TaskDeque type from PBRWP
to RWP.

[alokp, 2014-01-03 22:09:44]

Also added tests. PTAL.

https://codereview.chromium.org/110883015/diff/140001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/140001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:547: completed_gpu_raster_tasks_.clear();
On 2013/12/20 13:44:42, David Reveman wrote:
> nit: could you make this consistent with
> PixelBufferRasterWorkerPool::CheckForCompletedTasks() by using a deque to
store
> completed gpu raster tasks and change the loop to be a "while (!empty) {
> front(); pop_front(); }" style thing. You can move the TaskDeque type from
PBRWP
> to RWP.

Done.

[reveman, 2014-01-03 23:29:55]

lgtm

https://codereview.chromium.org/110883015/diff/350001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/110883015/diff/350001/skia/skia_library.gypi#...
skia/skia_library.gypi:133: '<@(skgpu_null_gl_sources)',
what is this for?

[alokp, 2014-01-03 23:56:16]

Vlad: do you see any issues?

https://codereview.chromium.org/110883015/diff/350001/skia/skia_library.gypi
File skia/skia_library.gypi (right):

https://codereview.chromium.org/110883015/diff/350001/skia/skia_library.gypi#...
skia/skia_library.gypi:133: '<@(skgpu_null_gl_sources)',
On 2014/01/03 23:29:55, David Reveman wrote:
> what is this for?

To pick up the implementation for GrGLCreateNullInterface(), which is used in
TestContextProvider::GrContext().

[vmpstr, 2014-01-06 19:15:44]

lgtm with a few comments

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:249: RenderingStatsInstrumentation* stats =
tile_resolution_ == HIGH_RESOLUTION ?
nit: methinks clang-format would do 

    RenderingStatsInstrumentation* stats =
        tile_resolution_ == HIGH_RESOLUTION ? rendering_stats_ : NULL;

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:569: context->makeContextCurrent();
You should check the return value... although I don't really know what you would
do on failure here (if that's possible).

https://codereview.chromium.org/110883015/diff/350001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/110883015/diff/350001/cc/test/test_context_pr...
cc/test/test_context_provider.h:72: skia::RefPtr<class GrContext> gr_context_;
You should just forward declare at the top? I don't think it matters in test
though.

[alokp, 2014-01-06 22:05:23]

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:249: RenderingStatsInstrumentation* stats =
tile_resolution_ == HIGH_RESOLUTION ?
On 2014/01/06 19:15:45, vmpstr wrote:
> nit: methinks clang-format would do 
> 
>     RenderingStatsInstrumentation* stats =
>         tile_resolution_ == HIGH_RESOLUTION ? rendering_stats_ : NULL;

Done.

https://codereview.chromium.org/110883015/diff/350001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:569: context->makeContextCurrent();
On 2014/01/06 19:15:45, vmpstr wrote:
> You should check the return value... although I don't really know what you
would
> do on failure here (if that's possible).

Done. Added a return.

https://codereview.chromium.org/110883015/diff/350001/cc/test/test_context_pr...
File cc/test/test_context_provider.h (right):

https://codereview.chromium.org/110883015/diff/350001/cc/test/test_context_pr...
cc/test/test_context_provider.h:72: skia::RefPtr<class GrContext> gr_context_;
On 2014/01/06 19:15:45, vmpstr wrote:
> You should just forward declare at the top? I don't think it matters in test
> though.

I will just follow the convention used in the base class header and other
implementations. If we decide to fix it, I will fix other locations as well in a
separate patch.

[commit-bot: I haz the power, 2014-01-06 22:07:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/110883015/580001

[commit-bot: I haz the power, 2014-01-06 23:26:24]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-07 00:18:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/110883015/830001

[commit-bot: I haz the power, 2014-01-07 01:17:04]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-07 09:54:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/110883015/830001

[commit-bot: I haz the power, 2014-01-07 11:10:47]

Retried try job too often on linux_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 06:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/110883015/1190001

[commit-bot: I haz the power, 2014-01-09 10:39:49]

Change committed as 243836