Added preliminary support for tile rasterization with Ganesh

Added preliminary support for GPU tile rasterization.

The feature is behind the flag --enable-gpu-rasterizing.

BUG=316685

[nduca, 2013-11-22 20:52:54]

awesome, david and/or vmpstr should review.  enne might want to bless as well.

[nduca, 2013-11-22 20:53:23]

(you're gonna need unit tests)

[vmpstr, 2013-11-22 21:29:11]

I don't really have any substantial comments, since it seems that some more work
needs to be done to eliminate a bunch of todo's here and there. It looks like a
nice approach, since it's fairly isolated from the rest of the system (ie, it
doesn't complicate it that much). Some nits:

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.cc (right):

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.cc:20: if (!context_provider ||
Should these just be dchecks? or is there a valid case where you ask for the
rasterize and it returns null

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.cc:43: DCHECK(!client_);
With these two dchecks, it seems that client should just be a constructor
argument

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:53: RasterTask(Tile* tile,
scoped_ptr<ResourcePool::Resource> resource)
I think this (and destructor) will have to be declared out-of-line. I think
something will complain about it on some trybots.

[slavi, 2013-11-23 01:27:19]

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.cc (right):

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.cc:20: if (!context_provider ||
On 2013/11/22 21:29:12, vmpstr wrote:
> Should these just be dchecks? or is there a valid case where you ask for the
> rasterize and it returns null

Done.

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.cc:43: DCHECK(!client_);
On 2013/11/22 21:29:12, vmpstr wrote:
> With these two dchecks, it seems that client should just be a constructor
> argument

This class get's instantiated before TileManager (which is it's client) so it
couldn't be done in the constructor.

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/100001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:53: RasterTask(Tile* tile,
scoped_ptr<ResourcePool::Resource> resource)
On 2013/11/22 21:29:12, vmpstr wrote:
> I think this (and destructor) will have to be declared out-of-line. I think
> something will complain about it on some trybots.

Good catch. Done.

[nduca, 2013-11-24 07:29:29]

lets split out the flag patch from the other changes and also discuss broadly
whether any other work can be done to turn this into smaller patches

[reveman, 2013-11-24 21:56:56]

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.cc#newc...
cc/base/switches.cc:204: bool IsGpuRasterizingEnabled() {
Do we need this helper function when it just returns the value of
HasSwitch(cc::switches::kEnableGpuRasterizing)?

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.h#newco...
cc/base/switches.h:46: CC_EXPORT extern const char kEnableGpuRasterizing[];
GaneshRasterizer? Or is there a good reason for referring to this using two
different terms?

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2490: if (scissor_rect == scissor_rect_ &&
!scissor_rect_needs_reset_)
can you explain why we need this now but not before?

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:22: bool was_canceled) = 0;
why "was_canceled"? I don't see why that would ever be true as we run these
tasks synchronously. am I missing something?

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:34: ResourceFormat GetResourceFormat() const {
return BGRA_8888; }
This will be a problem after you rebase this patch as the ResourcePool will only
use one resource format. Ideally we would be able to use the same format for
better reuse of resources.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:38: RenderingStatsInstrumentation*
rendering_stats_instrumentation);
Instead of push/flush, what if we add a container (much like
RasterWorkerPool::RasterTask::Queue) and just pass this container to a
RunRasterTasks function?

Seems like that would also remove the need for the GaneshRasterizerClient
interface.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode.h
File cc/resources/raster_mode.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode...
cc/resources/raster_mode.h:24: GANESH_RASTER_MODE = 3,
I'm not sure this should be a raster mode. Raster modes exist to allow tiles to
transition from one type of raster to another. ie. low to high quality. It
doesn't seem like we would ever want a tile to transition for sw to ganesh
raster. Also, using raster mode prevents us from supporting low quality ganesh
raster.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile.h#newc...
cc/resources/tile.h:114: friend class GaneshRasterizer;
is this necessary?

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.h:60: bool use_ganesh,
how do we plan to mark layers/tiles as candidates for ganesh? do we need a
use_ganesh option here or can the system that marks layers as candidates take
care of this?

[Vangelis Kokkevis, 2013-11-25 17:14:25]

On 2013/11/24 07:29:29, nduca wrote:
> lets split out the flag patch from the other changes and also discuss broadly
> whether any other work can be done to turn this into smaller patches

I agree with Nat that we should separate out the flag part of this patch.
There's some interesting new code here:

https://codereview.chromium.org/69123002

that should help with caching the resulting flag in order to avoid expensive
lookups (when we start selectively ganesh-ing layers).

[vmpstr, 2013-11-25 18:11:48]

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode.h
File cc/resources/raster_mode.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode...
cc/resources/raster_mode.h:24: GANESH_RASTER_MODE = 3,
On 2013/11/24 21:56:57, David Reveman wrote:
> I'm not sure this should be a raster mode. Raster modes exist to allow tiles
to
> transition from one type of raster to another. ie. low to high quality. It
> doesn't seem like we would ever want a tile to transition for sw to ganesh
> raster. Also, using raster mode prevents us from supporting low quality ganesh
> raster.

I think this could be one of the ManageTileState::TileVersion::Mode enums (ie,
resource_mode, solid_color_mode, picture_pile_mode, ganesh_resource_mode?).

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

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:849:
ganesh_rasterizer_->FlushRasterTasks(rendering_stats_instrumentation_);
From the perspective of what TileManager current does, I kind of prefer having
the call to raster be somewhere else. TileManager currently takes tiles, sorts
them, and passes them to something that actually does rasterization. Then, tile
manager glues resources to tiles. Doing the flush here kind of breaks this. I do
like push/flush because it makes it possible to flush at any point in time,
whereas having a queue and passing it back will need more plumbing if this calls
moves somewhere else. 

I'm ok with this if this is truly the best place, but perhaps there is a better
place?

[reveman, 2013-11-25 21:24:09]

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

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:849:
ganesh_rasterizer_->FlushRasterTasks(rendering_stats_instrumentation_);
On 2013/11/25 18:11:48, vmpstr wrote:
> From the perspective of what TileManager current does, I kind of prefer having
> the call to raster be somewhere else. TileManager currently takes tiles, sorts
> them, and passes them to something that actually does rasterization. Then,
tile
> manager glues resources to tiles. Doing the flush here kind of breaks this. I
do
> like push/flush because it makes it possible to flush at any point in time,
> whereas having a queue and passing it back will need more plumbing if this
calls
> moves somewhere else. 
> 
> I'm ok with this if this is truly the best place, but perhaps there is a
better
> place?

It might be cleaner to tally up these tiles in a different vector (not
tiles_that_need_to_be_rasterized) and use a separate function (not
TileManager::ScheduleTasks) to execute them.

I think we should use a queue and pass it to the ganesh rasterizer in a way that
is consistent with how we interact with our RasterWorkerPool instance. Let's
keep things simple and intuitive for what the patch does now and not what it
might do in the future. ie. don't complicate things with a client interface
while gpu raster is synchronous.

[slavi, 2013-11-25 23:13:13]

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.cc#newc...
cc/base/switches.cc:204: bool IsGpuRasterizingEnabled() {
On 2013/11/24 21:56:57, David Reveman wrote:
> Do we need this helper function when it just returns the value of
> HasSwitch(cc::switches::kEnableGpuRasterizing)?

Also removed. See humper's patch.

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.h
File cc/base/switches.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/base/switches.h#newco...
cc/base/switches.h:46: CC_EXPORT extern const char kEnableGpuRasterizing[];
On 2013/11/24 21:56:57, David Reveman wrote:
> GaneshRasterizer? Or is there a good reason for referring to this using two
> different terms?

Removed the flag from this patch, since humper@ is landing it separately.

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2490: if (scissor_rect == scissor_rect_ &&
!scissor_rect_needs_reset_)
On 2013/11/24 21:56:57, David Reveman wrote:
> can you explain why we need this now but not before?

Ganesh and the GlRendeder are sharing the GL state. Also, Ganesh assumes that
the state will be the same as it was last left. This fixes such a state bug
having to do which scissoring. I believe there's another bug lurking having to
do with GL blend state.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
File cc/resources/ganesh_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:22: bool was_canceled) = 0;
On 2013/11/24 21:56:57, David Reveman wrote:
> why "was_canceled"? I don't see why that would ever be true as we run these
> tasks synchronously. am I missing something?

You're not. I modeled this after the other RasterWorkingPools which do have
was_canceled and I wanted to reuse that to signal a GL error or similar failure
to produce a tile. Although currently that's a TODO in my list, not really
expressed in the code.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/ganesh_rast...
cc/resources/ganesh_rasterizer.h:34: ResourceFormat GetResourceFormat() const {
return BGRA_8888; }
On 2013/11/24 21:56:57, David Reveman wrote:
> This will be a problem after you rebase this patch as the ResourcePool will
only
> use one resource format. Ideally we would be able to use the same format for
> better reuse of resources.

See the current way where in the rasterizer I read the format from the resource
that was passed. The format of the resources we're creating currently is being
read from the raster_worker_pool. I'm not certain how we should choose the best
format in hybrid settings where we have several rasterizers active. Maybe we
should support multiple resource formats in the same time? Or maybe we can
ignore it for now?

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile.h#newc...
cc/resources/tile.h:114: friend class GaneshRasterizer;
On 2013/11/24 21:56:57, David Reveman wrote:
> is this necessary?

Yes, I need access to part tile's state that's not exposed currently. I can
expose it publicly but it seems that's not what we want since all other friend
classes don't need to be friends any more.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.h:60: bool use_ganesh,
On 2013/11/24 21:56:57, David Reveman wrote:
> how do we plan to mark layers/tiles as candidates for ganesh? do we need a
> use_ganesh option here or can the system that marks layers as candidates take
> care of this?

We need a heuristic in tile manager (search TODO(skaslev)) whether a tile should
be sw or gpu rasterized. Deciding that on per layer level also makes sense but I
haven't pursued that much yet.

[enne (OOO), 2013-11-25 23:14:10]

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2490: if (scissor_rect == scissor_rect_ &&
!scissor_rect_needs_reset_)
On 2013/11/24 21:56:57, David Reveman wrote:
> can you explain why we need this now but not before?

Yeah, this looks like it should be a separate patch.  Could you just set the
scissor_rect_ to empty instead of adding a separate boolean?  Also, should
scissor_rect_ be renamed scissor_rect_shadow_ to match the other shadowing
variables and make it more clear?

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode.cc
File cc/resources/raster_mode.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode...
cc/resources/raster_mode.cc:24: case GANESH_RASTER_MODE:
This is kind of a bikeshed, but previously some Skia folks expressed a desire to
not include the word Ganesh in the compositor anywhere.  I think these should
all be changed to GPU_RASTER_MODE (with similar changes elsewhere).

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

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:189: GaneshRasterizer::Create(
It seems like extra complication to have this optional ganesh rasterizer.  Maybe
it'd simplify life to just have it always be created, but only sometimes used.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/tile_manage...
cc/resources/tile_manager.h:60: bool use_ganesh,
I would really not like to have a global mode flag on TileManager, even in the
first version of this patch.  I would really like to have layers say that their
tiles are Ganeshable, and then TileManager handle a set of tiles, some of which
can be Ganeshed.

Maybe in the first cut of this patch the "is ganeshable" decision can just be
true if and only if the setting is turned on, but then we can  refine that over
time.

[slavi, 2013-11-25 23:22:52]

I'll look into the remaining comments in a bit.

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:2490: if (scissor_rect == scissor_rect_ &&
!scissor_rect_needs_reset_)
On 2013/11/25 23:14:10, enne wrote:
> On 2013/11/24 21:56:57, David Reveman wrote:
> > can you explain why we need this now but not before?
> 
> Yeah, this looks like it should be a separate patch.  Could you just set the
> scissor_rect_ to empty instead of adding a separate boolean?  Also, should
> scissor_rect_ be renamed scissor_rect_shadow_ to match the other shadowing
> variables and make it more clear?

Ok. I'll prepare it as a separate patch.

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode.cc
File cc/resources/raster_mode.cc (right):

https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode...
cc/resources/raster_mode.cc:24: case GANESH_RASTER_MODE:
On 2013/11/25 23:14:10, enne wrote:
> This is kind of a bikeshed, but previously some Skia folks expressed a desire
to
> not include the word Ganesh in the compositor anywhere.  I think these should
> all be changed to GPU_RASTER_MODE (with similar changes elsewhere).

Already done in the next patch set.

[humper, 2013-11-26 21:00:39]

On 2013/11/25 23:22:52, slavi wrote:
> I'll look into the remaining comments in a bit.
> 
> https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/69343005/diff/310001/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:2490: if (scissor_rect == scissor_rect_ &&
> !scissor_rect_needs_reset_)
> On 2013/11/25 23:14:10, enne wrote:
> > On 2013/11/24 21:56:57, David Reveman wrote:
> > > can you explain why we need this now but not before?
> > 
> > Yeah, this looks like it should be a separate patch.  Could you just set the
> > scissor_rect_ to empty instead of adding a separate boolean?  Also, should
> > scissor_rect_ be renamed scissor_rect_shadow_ to match the other shadowing
> > variables and make it more clear?
> 
> Ok. I'll prepare it as a separate patch.
> 
>
https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode.cc
> File cc/resources/raster_mode.cc (right):
> 
>
https://codereview.chromium.org/69343005/diff/310001/cc/resources/raster_mode...
> cc/resources/raster_mode.cc:24: case GANESH_RASTER_MODE:
> On 2013/11/25 23:14:10, enne wrote:
> > This is kind of a bikeshed, but previously some Skia folks expressed a
desire
> to
> > not include the word Ganesh in the compositor anywhere.  I think these
should
> > all be changed to GPU_RASTER_MODE (with similar changes elsewhere).
> 
> Already done in the next patch set.

Flag split out into separate patch (https://codereview.chromium.org/85813003/)
-- awaiting LGTM from thakis on that one.

[reveman, 2013-11-27 00:21:37]

https://codereview.chromium.org/69343005/diff/540001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/540001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.h:34: ResourceFormat GetResourceFormat() const {
return BGRA_8888; }
The GpuRasterizer doesn't care what internal format is used. It will work with
whatever format the RasterWorkerPool instance decides is most efficient. The
difference is that we never need to swizzle when sampling from gpu rasterized
textures.

Looks like we just need to separate the return value of
TileVersion::contents_swizzled() from the format.

[slavi, 2013-11-27 22:57:56]

I've addressed most of the issues raised. PTAL

[enne (OOO), 2013-11-27 23:33:26]

https://codereview.chromium.org/69343005/diff/580001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/69343005/diff/580001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:485: use_gpu_rasterizer);
Thanks, I think this is much more in the direction I think we should go.

Based on https://code.google.com/p/skia/issues/detail?id=1849 I think we will
will probably want to go to a tri-state enum, but a simple boolean is fine for
now.

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.h:32: void SetClient(GpuRasterizerClient* client);
Should this be a constructor parameter?

[vmpstr, 2013-11-27 23:45:41]

just one small nit...

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:115: canvas.get(), tile->content_rect_,
tile->contents_scale_,
nit: tile provides public getters for picture pile, content rect, and contents
scale. I think it's better to use those and remove the rasterizer friendship
from the tile. The friends there are only to access the managed tile state,
which this class doesn't need.

[vangelis, 2013-11-28 00:35:01]

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:111: if (tile->opaque_rect().IsEmpty())
Shouldn't we be checking here for an opaque_rect() < tile_rect ? If the tile is
partially transparent we'll still need to clear it.

[slavi, 2013-11-28 00:47:48]

PTAL

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:111: if (tile->opaque_rect().IsEmpty())
On 2013/11/28 00:35:01, vangelis wrote:
> Shouldn't we be checking here for an opaque_rect() < tile_rect ? If the tile
is
> partially transparent we'll still need to clear it. 

This doesn't happen currently. The opaque_rect is either tile_rect or empty:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/picture_...

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:115: canvas.get(), tile->content_rect_,
tile->contents_scale_,
On 2013/11/27 23:45:42, vmpstr wrote:
> nit: tile provides public getters for picture pile, content rect, and contents
> scale. I think it's better to use those and remove the rasterizer friendship
> from the tile. The friends there are only to access the managed tile state,
> which this class doesn't need.

Done.

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
File cc/resources/gpu_rasterizer.h (right):

https://codereview.chromium.org/69343005/diff/580001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.h:32: void SetClient(GpuRasterizerClient* client);
On 2013/11/27 23:33:26, enne wrote:
> Should this be a constructor parameter?

Done.

[reveman, 2013-12-02 01:00:25]

How does this fit into our image decode story?

Maybe it'd be better to perform gpu raster behind the RasterWorkerPool
interface. That would allow us to handle image decode properly (even if for now
gpu raster would be limited to tasks without image decode dependencies). I don't
think that would change the patch much. It would reduce the changes to the
TileManager as gpu raster would just be a parameter to
RasterWorkerPool::CreateRasterTask that could be respected or ignored by the
RasterWorkerPool instance.

[vangelis, 2013-12-02 06:36:31]

On 2013/12/02 01:00:25, David Reveman wrote:
> How does this fit into our image decode story?

That's a great question. If I'm not mistaken, the way the patch currently works,
image decodes will happen at raster time, i.e. inline with the Ganesh calls,
increasing the probability of missed frames. Could this be setup so that we scan
tiles marked as ganesh-able for undecoded image dependencies, create decode
tasks for them and defer ganesh-ing until after the decodes are done? 

If so, could this be a follow up to this patch? 


> 
> Maybe it'd be better to perform gpu raster behind the RasterWorkerPool
> interface. That would allow us to handle image decode properly (even if for
now
> gpu raster would be limited to tasks without image decode dependencies). I
don't
> think that would change the patch much. It would reduce the changes to the
> TileManager as gpu raster would just be a parameter to
> RasterWorkerPool::CreateRasterTask that could be respected or ignored by the
> RasterWorkerPool instance.

[reveman, 2013-12-02 16:39:56]

On 2013/12/02 06:36:31, vangelis wrote:
> On 2013/12/02 01:00:25, David Reveman wrote:
> > How does this fit into our image decode story?
> 
> That's a great question. If I'm not mistaken, the way the patch currently
works,
> image decodes will happen at raster time, i.e. inline with the Ganesh calls,
> increasing the probability of missed frames. Could this be setup so that we
scan
> tiles marked as ganesh-able for undecoded image dependencies, create decode
> tasks for them and defer ganesh-ing until after the decodes are done? 

Yes, I think it could be done efficiently if ganesh-ing is moved behind the
RasterWorkerPool interface.

> 
> If so, could this be a follow up to this patch?

I'd be fine with that. Preferably we'd limit ganesh-ing to layers without images
for now.
 
> 
> 
> > 
> > Maybe it'd be better to perform gpu raster behind the RasterWorkerPool
> > interface. That would allow us to handle image decode properly (even if for
> now
> > gpu raster would be limited to tasks without image decode dependencies). I
> don't
> > think that would change the patch much. It would reduce the changes to the
> > TileManager as gpu raster would just be a parameter to
> > RasterWorkerPool::CreateRasterTask that could be respected or ignored by the
> > RasterWorkerPool instance.

[nduca, 2013-12-02 19:50:17]

im still a bit confused about the relationship between gpu rasterizer and the
other pools. Especially when we start talking about having pre-work for a gpu
rasterization task be a decode task issued to the raster worker pool, I wonder
if a better design is to teach raster worker pool more.

https://codereview.chromium.org/69343005/diff/620001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/69343005/diff/620001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:474: bool use_gpu_rasterizer =
Can you make this a function call please, up front? It can return bool but
shouldn't be hidden inline like this.

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h#newc...
cc/resources/tile.h:63: bool use_gpu_rasterizer() const {
should this be mutable?

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h#newc...
cc/resources/tile.h:130: bool use_gpu_rasterizer);
this is unreadable in the call sites. new Tile (..., true, false) means nothing.

please make an enum for TileRasterFlags { USE_GPU_RASTERIZER = 0x2, USE_LCD_TEXT
= 0x1 } and adjust call sites accordingly.

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

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:210: gpu_rasterizer_(GpuRasterizer::Create(
you should have unit tests that ensure that tiles enqueued to the tile manager
with raster get rasterized, but using a fake gpu rasterizer.

But, since you're constructing the GpuRasterizer directly in here, you're making
creating that test harder

Please add a unit test for tile manager and adjust the way this works
accordingly.

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

https://codereview.chromium.org/69343005/diff/620001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1681: context_provider,
I think you're better off passing in a GpuRasterizer than passing in the context
provider.

https://codereview.chromium.org/69343005/diff/620001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/69343005/diff/620001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:153:
settings.using_gpu_rasterization = cc::switches::IsGPURasterizationEnabled();
the switch is IsGpuRasterization. Why is the flag named differently, e.g.
using_gpu_rasterization?

[nduca, 2013-12-02 19:51:27]

it seems to me it might be good to split this patch further... there are some
easily landable parts and some less so parts. Splitting those out would be good
because we would keep making progress.

For instance, the change of tile constructors to take a gpu parameteter and
adjusting the picture layer to create with this argument. Then, the settings
changes, adding the type to the tiles.

[slavi, 2013-12-04 02:06:49]

Added a test and addressed the nits.

PTAL

https://codereview.chromium.org/69343005/diff/620001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/69343005/diff/620001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:474: bool use_gpu_rasterizer =
On 2013/12/02 19:50:17, nduca wrote:
> Can you make this a function call please, up front? It can return bool but
> shouldn't be hidden inline like this.

Done.

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h
File cc/resources/tile.h (right):

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h#newc...
cc/resources/tile.h:63: bool use_gpu_rasterizer() const {
On 2013/12/02 19:50:17, nduca wrote:
> should this be mutable?

Done.

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile.h#newc...
cc/resources/tile.h:130: bool use_gpu_rasterizer);
On 2013/12/02 19:50:17, nduca wrote:
> this is unreadable in the call sites. new Tile (..., true, false) means
nothing.
> 
> please make an enum for TileRasterFlags { USE_GPU_RASTERIZER = 0x2,
USE_LCD_TEXT
> = 0x1 } and adjust call sites accordingly.

Done.

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

https://codereview.chromium.org/69343005/diff/620001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:210: gpu_rasterizer_(GpuRasterizer::Create(
On 2013/12/02 19:50:17, nduca wrote:
> you should have unit tests that ensure that tiles enqueued to the tile manager
> with raster get rasterized, but using a fake gpu rasterizer.
> 
> But, since you're constructing the GpuRasterizer directly in here, you're
making
> creating that test harder
> 
> Please add a unit test for tile manager and adjust the way this works
> accordingly.

Done.

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

https://codereview.chromium.org/69343005/diff/620001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1681: context_provider,
On 2013/12/02 19:50:17, nduca wrote:
> I think you're better off passing in a GpuRasterizer than passing in the
context
> provider.

That was done per enne request to avoid the need of SetClient method. I have no
opinion eitherway. 

Switched it back to the SetClient API so that it's easier to test with
FakeGpuRasterizers and such.

https://codereview.chromium.org/69343005/diff/620001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/69343005/diff/620001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:153:
settings.using_gpu_rasterization = cc::switches::IsGPURasterizationEnabled();
On 2013/12/02 19:50:17, nduca wrote:
> the switch is IsGpuRasterization. Why is the flag named differently, e.g.
> using_gpu_rasterization?

Done.

[reveman, 2013-12-04 02:35:46]

I think we'll have to make gpu raster take place behind the RasterWorkerPool
interface before this can land. And to have it fit into our image decode story
we probably have to make it run as tasks on the workers too :( Those tasks could
however send synchronous gpu raster requests to the compositor thread instead of
actually performing gpu raster on the worker threads.

[vangelis, 2013-12-04 06:34:01]

On 2013/12/04 02:35:46, David Reveman wrote:
> I think we'll have to make gpu raster take place behind the RasterWorkerPool
> interface before this can land. And to have it fit into our image decode story
> we probably have to make it run as tasks on the workers too :( Those tasks
could
> however send synchronous gpu raster requests to the compositor thread instead
of
> actually performing gpu raster on the worker threads.

This brings up an interesting alternative, but maybe beyond the scope of this
patch. What if Ganesh were to use its own GL context, initialized in the worker
thread (and in the same share group as the compositor's context). The advantage
would be that it would help protect the compositor's thread from long running
CPU-bound Ganesh workloads.

[Vangelis Kokkevis, 2013-12-04 17:43:49]

On 2013/12/04 06:34:01, vangelis wrote:
> On 2013/12/04 02:35:46, David Reveman wrote:
> > I think we'll have to make gpu raster take place behind the RasterWorkerPool
> > interface before this can land. And to have it fit into our image decode
story
> > we probably have to make it run as tasks on the workers too :( Those tasks
> could
> > however send synchronous gpu raster requests to the compositor thread
instead
> of
> > actually performing gpu raster on the worker threads.
> 
> This brings up an interesting alternative, but maybe beyond the scope of this
> patch. What if Ganesh were to use its own GL context, initialized in the
worker
> thread (and in the same share group as the compositor's context). The
advantage
> would be that it would help protect the compositor's thread from long running
> CPU-bound Ganesh workloads.

Upon further reflection:

Since Ganesh is being put in place for cases where we want to produce tiles in a
hurry, and in a lot of these cases we will not be able to draw a frame until the
tiles are ready, deferring work to worker threads will likely only add overhead.
Ping-poinging between the impl and worker threads for high priority tasks leaves
us vulnerable to the kernel scheduler's whimsy. 

David, is there an architectural reason why image decodes cannot happen inline
in the main impl thread while we Ganesh the contents of a tile?

[reveman, 2013-12-04 19:36:22]

On 2013/12/04 17:43:49, Vangelis Kokkevis wrote:
> On 2013/12/04 06:34:01, vangelis wrote:
> > On 2013/12/04 02:35:46, David Reveman wrote:
> > > I think we'll have to make gpu raster take place behind the
RasterWorkerPool
> > > interface before this can land. And to have it fit into our image decode
> story
> > > we probably have to make it run as tasks on the workers too :( Those tasks
> > could
> > > however send synchronous gpu raster requests to the compositor thread
> instead
> > of
> > > actually performing gpu raster on the worker threads.
> > 
> > This brings up an interesting alternative, but maybe beyond the scope of
this
> > patch. What if Ganesh were to use its own GL context, initialized in the
> worker
> > thread (and in the same share group as the compositor's context). The
> advantage
> > would be that it would help protect the compositor's thread from long
running
> > CPU-bound Ganesh workloads.
> 
> Upon further reflection:
> 
> Since Ganesh is being put in place for cases where we want to produce tiles in
a
> hurry, and in a lot of these cases we will not be able to draw a frame until
the
> tiles are ready, deferring work to worker threads will likely only add
overhead.
> Ping-poinging between the impl and worker threads for high priority tasks
leaves
> us vulnerable to the kernel scheduler's whimsy. 
> 
> David, is there an architectural reason why image decodes cannot happen inline
> in the main impl thread while we Ganesh the contents of a tile?

No, if we're OK running expensive image decode on the compositor thread then I
don't see a problem.

[vangelis, 2013-12-04 21:39:09]

On 2013/12/04 19:36:22, David Reveman wrote:
> On 2013/12/04 17:43:49, Vangelis Kokkevis wrote:
> > On 2013/12/04 06:34:01, vangelis wrote:
> > > On 2013/12/04 02:35:46, David Reveman wrote:
> > > > I think we'll have to make gpu raster take place behind the
> RasterWorkerPool
> > > > interface before this can land. And to have it fit into our image decode
> > story
> > > > we probably have to make it run as tasks on the workers too :( Those
tasks
> > > could
> > > > however send synchronous gpu raster requests to the compositor thread
> > instead
> > > of
> > > > actually performing gpu raster on the worker threads.
> > > 
> > > This brings up an interesting alternative, but maybe beyond the scope of
> this
> > > patch. What if Ganesh were to use its own GL context, initialized in the
> > worker
> > > thread (and in the same share group as the compositor's context). The
> > advantage
> > > would be that it would help protect the compositor's thread from long
> running
> > > CPU-bound Ganesh workloads.
> > 
> > Upon further reflection:
> > 
> > Since Ganesh is being put in place for cases where we want to produce tiles
in
> a
> > hurry, and in a lot of these cases we will not be able to draw a frame until
> the
> > tiles are ready, deferring work to worker threads will likely only add
> overhead.
> > Ping-poinging between the impl and worker threads for high priority tasks
> leaves
> > us vulnerable to the kernel scheduler's whimsy. 
> > 
> > David, is there an architectural reason why image decodes cannot happen
inline
> > in the main impl thread while we Ganesh the contents of a tile?
> 
> No, if we're OK running expensive image decode on the compositor thread then I
> don't see a problem.

Maybe we could try this as a step #1 and see how things look?  If we were to
have the worker threads pre-emptively decode images that are present in the
tiles without making any hard dependencies between the decode tasks and the
ganesh raster tasks, could we get around the need to run gpu raster as tasks on
the workers?

[reveman, 2013-12-04 23:11:37]

On 2013/12/04 21:39:09, vangelis wrote:
> On 2013/12/04 19:36:22, David Reveman wrote:
> > On 2013/12/04 17:43:49, Vangelis Kokkevis wrote:
> > > On 2013/12/04 06:34:01, vangelis wrote:
> > > > On 2013/12/04 02:35:46, David Reveman wrote:
> > > > > I think we'll have to make gpu raster take place behind the
> > RasterWorkerPool
> > > > > interface before this can land. And to have it fit into our image
decode
> > > story
> > > > > we probably have to make it run as tasks on the workers too :( Those
> tasks
> > > > could
> > > > > however send synchronous gpu raster requests to the compositor thread
> > > instead
> > > > of
> > > > > actually performing gpu raster on the worker threads.
> > > > 
> > > > This brings up an interesting alternative, but maybe beyond the scope of
> > this
> > > > patch. What if Ganesh were to use its own GL context, initialized in the
> > > worker
> > > > thread (and in the same share group as the compositor's context). The
> > > advantage
> > > > would be that it would help protect the compositor's thread from long
> > running
> > > > CPU-bound Ganesh workloads.
> > > 
> > > Upon further reflection:
> > > 
> > > Since Ganesh is being put in place for cases where we want to produce
tiles
> in
> > a
> > > hurry, and in a lot of these cases we will not be able to draw a frame
until
> > the
> > > tiles are ready, deferring work to worker threads will likely only add
> > overhead.
> > > Ping-poinging between the impl and worker threads for high priority tasks
> > leaves
> > > us vulnerable to the kernel scheduler's whimsy. 
> > > 
> > > David, is there an architectural reason why image decodes cannot happen
> inline
> > > in the main impl thread while we Ganesh the contents of a tile?
> > 
> > No, if we're OK running expensive image decode on the compositor thread then
I
> > don't see a problem.
> 
> Maybe we could try this as a step #1 and see how things look?  If we were to
> have the worker threads pre-emptively decode images that are present in the
> tiles without making any hard dependencies between the decode tasks and the
> ganesh raster tasks, could we get around the need to run gpu raster as tasks
on
> the workers?

Running everything including decode on the compositor thread for now sgtm.

I'm not sure decoding images on worker threads but performing gpu raster that is
not represented as tasks that run on worker threads is a good idea. The problem
is that we want to pin decoded images before raster and unpin them after, which
is difficult when raster is not represented in the task graph.

[enne (OOO), 2014-02-01 01:00:17]

I think this patch can be closed now.