cc: Refactor WorkerPoolTaskClient::AcquireBufferForRaster

cc: Refactor WorkerPoolTaskClient::AcquireBufferForRaster

With the introduction of GPU rasterization, we no longer rasterize to a bitmap. We rasterize directly to a texture-backed SkCanvas. This patch refactors AcquireBufferForRaster into AcquireCanvasForRaster to unify the API used by software and gpu rasterization.

It also allows us to cache the gpu canvas with the resource so that we do not need to re-create it whenever a tile needs to be repainted. Creating a new gpu canvas is expensive on command-buffer - both on client and service side. The client side adds a flush every time an FBO is created or destroyed. The service side checks for FBO completeness. This patch eliminates the setup/destroy cost of gpu canvas.

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

[alokp, 2014-02-11 00:24:52]

I still need to update the unittests. Please let me know if the API seems
reasonable.

To keep the patch small, I have left a few TODOs, like implementing
Acquire/Release/Map/Unmap in terms of old functions. I have just moved the old
functions to be private member functions for now. A few struct Resource member
variables will also move to appropriate subclass of RasterBuffer when I move
those implementations.

[reveman, 2014-02-11 02:00:43]

This looks nice.

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (left):

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:315: }
Where does this bitmap conversion code go?

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

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:547: }
I think RasterWorkerPool should be an interface and not an implementation. It's
sort of half right now and that's proven confusing and sometimes hard to work
with. This is my fault but I'm hoping we can correct it.

I'd like to get RasterWorkerPool to a place where it's just the interface and a
set of static helper functions and helper classes. All members need to be
implemented by Image or PixelBuffer instances.

In this case, I think you can just remove these member functions and move the
code to Image/PixelBufferRWP and not add any helper functions.

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

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:34: virtual SkCanvas*
AcquireCanvasForRaster(RasterWorkerPoolTask* task) = 0;
I'd like to see this take a basic WorkerPoolTask* in the future and eliminate
the need for both a RasterWorkerPoolTask and a WorkerPoolTask class. You don't
have to change that in this patch but it would be nice if it lined up with that
idea.

The result is that the WorkerPoolTaskClient doesn't know much about the task.
ie. it can't call use_gpu_rasterization(). The decision to use gpu raster would
instead have to be driven by the task.

So maybe we could make this:

virtual SkCanvas* AcquireCanvasForRaster(const WorkerPoolTask* task,
                                         const Resource* resource,
                                         bool use_gpu_rasterization);

And I think OnRasterCompleted should be removed altogether but we could keep it
as:

virtual void OnRasterCompleted(internal::WorkerPoolTask* task,
                               const Resource* resource,
                               const PicturePileImpl::Analysis&,
                               bool use_gpu_rasterization);

for now.

https://codereview.chromium.org/157293002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/157293002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.h:418: linked_ptr<PixelRasterBuffer>
pixel_raster_buffer;
scoped_ptr would be preferred but I assume this needs to be copyable, right?

How about making all these linked_ptr<RasterBuffer> and move the declaration of
all the different implementations into the .cc file? That would clean up the .h
quite a bit.

[alokp, 2014-02-11 07:05:21]

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (left):

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:315: }
On 2014/02/11 02:00:44, David Reveman wrote:
> Where does this bitmap conversion code go?

It somehow got lost while moving code! Added in BitmapRasterBuffer.

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

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:547: }
On 2014/02/11 02:00:44, David Reveman wrote:
> I think RasterWorkerPool should be an interface and not an implementation.
It's
> sort of half right now and that's proven confusing and sometimes hard to work
> with. This is my fault but I'm hoping we can correct it.
> 
> I'd like to get RasterWorkerPool to a place where it's just the interface and
a
> set of static helper functions and helper classes. All members need to be
> implemented by Image or PixelBuffer instances.
> 
> In this case, I think you can just remove these member functions and move the
> code to Image/PixelBufferRWP and not add any helper functions.

Done.

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

https://codereview.chromium.org/157293002/diff/40001/cc/resources/raster_work...
cc/resources/raster_worker_pool.h:34: virtual SkCanvas*
AcquireCanvasForRaster(RasterWorkerPoolTask* task) = 0;
On 2014/02/11 02:00:44, David Reveman wrote:
> I'd like to see this take a basic WorkerPoolTask* in the future and eliminate
> the need for both a RasterWorkerPoolTask and a WorkerPoolTask class. You don't
> have to change that in this patch but it would be nice if it lined up with
that
> idea.
> 
> The result is that the WorkerPoolTaskClient doesn't know much about the task.
> ie. it can't call use_gpu_rasterization(). The decision to use gpu raster
would
> instead have to be driven by the task.
> 
> So maybe we could make this:
> 
> virtual SkCanvas* AcquireCanvasForRaster(const WorkerPoolTask* task,
>                                          const Resource* resource,
>                                          bool use_gpu_rasterization);
> 
> And I think OnRasterCompleted should be removed altogether but we could keep
it
> as:
> 
> virtual void OnRasterCompleted(internal::WorkerPoolTask* task,
>                                const Resource* resource,
>                                const PicturePileImpl::Analysis&,
>                                bool use_gpu_rasterization);
> 
> for now.

Sure. This can be easily done when you decide to actually change
RasterWorkerPoolTask to WorkerPoolTask.

https://codereview.chromium.org/157293002/diff/40001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/157293002/diff/40001/cc/resources/resource_pr...
cc/resources/resource_provider.h:418: linked_ptr<PixelRasterBuffer>
pixel_raster_buffer;
On 2014/02/11 02:00:44, David Reveman wrote:
> scoped_ptr would be preferred but I assume this needs to be copyable, right?

Yes. It needs to be copyable.

> How about making all these linked_ptr<RasterBuffer> and move the declaration
of
> all the different implementations into the .cc file? That would clean up the
.h
> quite a bit.

Once this is landed, I plan to move the implementation of
Acquire/Release/Map/Unmap Image/Pixel into these classes. I also plan to  move a
few member variables of Resource into these classes, in which case just using
the RasterBuffer API may not be sufficient. I do not want to static_cast to use
a particular Image/PixelRasterBuffer member function.

Having said that I may be able to move the RasterBuffer outside ResourceProvider
if RasterBuffer can simply use cc::Resource. I would like to concentrate on
cleaning up the RWP in this patch. I will clean up the internal API in later
patches.

[alokp, 2014-02-11 22:26:59]

Dave: This is ready to review. I have also updated the affected unit-tests to
use the new API.

[reveman, 2014-02-12 01:45:12]

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
cc/resources/resource_provider.h:314: void UnmapDirectRasterBuffer(ResourceId
id);
Not clear to me what the "Direct" means here. Writes direct to the resource? So
does ImageRasterBuffer. Seems like this provides duplicate functionality in case
of sw resources...

What difference can I expect from this compared ImageRasterBuffer version? For
image version the raster cost is payed during draw. Is that the important
difference? In that case, DirectRasterBuffer with sw resources is not really
doing the right thing..

PixelBuffer version and Direct version in some ways have more in common. They
both have async behavior that require something like "DidSetPixelsComplete" to
determine when done..

Would be nice to establish the difference between these functions and document
it here.

[alokp, 2014-02-12 07:17:42]

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
cc/resources/resource_provider.h:314: void UnmapDirectRasterBuffer(ResourceId
id);
On 2014/02/12 01:45:12, David Reveman wrote:
> Not clear to me what the "Direct" means here. Writes direct to the resource?
So
> does ImageRasterBuffer. Seems like this provides duplicate functionality in
case
> of sw resources...

I consider image and pixel raster buffers indirect because you first write to an
intermediate buffer and then bind or upload the buffer. Though bind can be quite
cheap and direct as you pointed out. On those platforms I guess it is just
semantics. Direct raster is straight to the actual resource-backed canvas.
Suggestions welcome on better names.

Both - image and pixel - raster buffers support sw resource. So I thought it
might be appropriate for the direct raster buffer to support them too. There is
one slight difference - the direct path assumes 8888 config. I was hoping to use
it to implement ScopedWriteLockSoftware.

> What difference can I expect from this compared ImageRasterBuffer version? For
> image version the raster cost is payed during draw. Is that the important
> difference? In that case, DirectRasterBuffer with sw resources is not really
> doing the right thing..
> 
> PixelBuffer version and Direct version in some ways have more in common. They
> both have async behavior that require something like "DidSetPixelsComplete" to
> determine when done..

I do not know how the software resource is used by the compositor. I am assuming
the software compositor is using either IRWP or PRWP for rasterization. I will
ask @jbauman.

> Would be nice to establish the difference between these functions and document
> it here.

[reveman, 2014-02-12 17:23:39]

Sorry for delay here and thanks for being patient with me. I think I just needed
some time to see how this ties into our different RasterWorkerPool instances and
where all this is going long term.

I think we should reduce sw support to only one of these RasterBuffer types in a
follow up. Moving the use of DirectRasterBuffer to a new DirectRasterWorkerPool
implementation will be a nice cleanup and easy to do once I finish the
RasterWorkerPool task refactor I'm currently working on.

Please follow up with the TODOs you added to resource_provider.h asap.

LGTM.

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/157293002/diff/100002/cc/resources/resource_p...
cc/resources/resource_provider.h:314: void UnmapDirectRasterBuffer(ResourceId
id);
On 2014/02/12 07:17:42, Alok Priyadarshi wrote:
> On 2014/02/12 01:45:12, David Reveman wrote:
> > Not clear to me what the "Direct" means here. Writes direct to the resource?
> So
> > does ImageRasterBuffer. Seems like this provides duplicate functionality in
> case
> > of sw resources...
> 
> I consider image and pixel raster buffers indirect because you first write to
an
> intermediate buffer and then bind or upload the buffer. Though bind can be
quite
> cheap and direct as you pointed out. On those platforms I guess it is just
> semantics. Direct raster is straight to the actual resource-backed canvas.
> Suggestions welcome on better names.
> 
> Both - image and pixel - raster buffers support sw resource. So I thought it
> might be appropriate for the direct raster buffer to support them too. There
is
> one slight difference - the direct path assumes 8888 config. I was hoping to
use
> it to implement ScopedWriteLockSoftware.
> 
> > What difference can I expect from this compared ImageRasterBuffer version?
For
> > image version the raster cost is payed during draw. Is that the important
> > difference? In that case, DirectRasterBuffer with sw resources is not really
> > doing the right thing..
> > 
> > PixelBuffer version and Direct version in some ways have more in common.
They
> > both have async behavior that require something like "DidSetPixelsComplete"
to
> > determine when done..
> 
> I do not know how the software resource is used by the compositor. I am
assuming
> the software compositor is using either IRWP or PRWP for rasterization. I will
> ask @jbauman.
> 
> > Would be nice to establish the difference between these functions and
document
> > it here.
>  

Each RasterWorkerPool implementation is responsible for how to throttle and
schedule tasks based on the type of interface they use for writing to a
resource.

Gpu raster was tacked on to existing RWP implementations but I think that was a
mistake. This way of writing to a resource requires a different mechanism for
throttling and that justifies a new type of RWP implementation;
"DirectRasterWorkerPool".

If we can come up with a better name later, we can change it. "Direct" is OK
with me for now.

FYI, I think we should restrict sw support to only 1 of these 3 types of writing
to a resource. Should probably be "Image" version as the throttling logic
matches what is needed for sw best.

[alokp, 2014-02-12 18:13:00]

+jbauman

I agree. We should move GPU raster to a separate RWP. Please let me know when
you are done with the RWP task refactor.

I can think of one issue with having only one type of RasterBuffer support sw
resources. The type - GL/SW - should be transparent to the RWP. But again I do
not know how software compositor works. I wonder if rasterizing to a separate
buffer and BeginSetPixels is required.

[alokp, 2014-02-12 19:39:48]

The CQ bit was checked by alokp@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:55:42]

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

[commit-bot: I haz the power, 2014-02-13 03:48:19]

Change committed as 250924