cc: GPU rasterize tiles synchronously in PrepareToDraw

cc: GPU rasterize tiles synchronously in PrepareToDraw

BUG=434889

[ernstm, 2014-11-20 00:58:30]

ernstm@chromium.org changed reviewers:
+ brianderson@chromium.org, enne@chromium.org, reveman@chromium.org,
vmiura@chromium.org

[ernstm, 2014-11-20 00:58:43]

PTAL

[brianderson, 2014-11-20 01:16:53]

Is the "prepare tasks for image decode" patch going to land on top of this
patch, at which point ManageTiles for the prepare tasks will come back?

[vmiura, 2014-11-20 01:21:10]

On 2014/11/20 01:16:53, brianderson wrote:
> Is the "prepare tasks for image decode" patch going to land on top of this
> patch, at which point ManageTiles for the prepare tasks will come back?

Yes, the image decode patch will re-enable ManageTiles for GPU raster under a
flag.

[vmiura, 2014-11-20 01:44:02]

For what it's worth, modulo any issues with the implementation, I really support
the separation of Ganesh rasterization from ManageTiles.

This will enable Ganesh image pre-decodes to use ManageTiles/RWP with minimal
differences from Software pre-painting.

If we don't separate the two then ManageTiles/RWP will have to grow the ability
to handle both cases differently anyway.

[vmiura, 2014-11-20 01:48:42]

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

https://codereview.chromium.org/733773005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:218: completed_tasks_.clear();
Since CompleteTasks() calls completed_tasks_.clear(), this could be removed.

[ernstm, 2014-11-20 01:57:01]

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

https://codereview.chromium.org/733773005/diff/40001/cc/resources/gpu_raster_...
cc/resources/gpu_raster_worker_pool.cc:218: completed_tasks_.clear();
On 2014/11/20 01:48:42, vmiura wrote:
> Since CompleteTasks() calls completed_tasks_.clear(), this could be removed.

Done.

[vmiura, 2014-11-20 02:27:11]

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:331: if (tile_manager_ &&
tile_priorities_dirty_ && !use_gpu_rasterization_)
Remove check for use_gpu_rasterization_, as we check it in ManageTiles.

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1853: if (!visible_ && !use_gpu_rasterization_)
Remove check for use_gpu_rasterization_, as we check it in ManageTiles.

[vmiura, 2014-11-20 02:29:20]

https://codereview.chromium.org/733773005/diff/60001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/733773005/diff/60001/cc/trees/thread_proxy.cc...
cc/trees/thread_proxy.cc:1056: if
(!impl().layer_tree_host_impl->use_gpu_rasterization())
nit: This should be redundant, as LTHI does the same check.

If we keep this, update single_thread_proxy too.

[brianderson, 2014-11-20 02:40:19]

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
Should we only RasterizeTiles if draw_result == DRAW_SUCCESS at then end?

Alternatively, does it work to call RasterizeTiles in DrawLayers after the
NoDamage early out?

[vmiura, 2014-11-20 02:53:45]

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:331: if (tile_manager_ &&
tile_priorities_dirty_ && !use_gpu_rasterization_)
I take that back, since we need to run the "else" part and activate right now.

[reveman, 2014-11-20 15:20:59]

If this is the direction we're going (which I like btw) then I think we should
remove GpuRasterWorkerPool and simply handle all gpu raster separately from the
tile manager. Maybe the tile manager can still assign memory to tiles using the
same mechanism as sw raster but the RWP used in the GPU raster case would not
actually perform any raster work.

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
How you considered rasterizing at draw time instead? It would guarantee that
we're no rasterizing any tiles that are not needed for the frame.

[vmiura, 2014-11-20 18:28:57]

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
On 2014/11/20 15:20:59, reveman wrote:
> How you considered rasterizing at draw time instead? It would guarantee that
> we're no rasterizing any tiles that are not needed for the frame.

I put it here in the prototype as it needs to be done after
active_tree->UpdateDrawProperties() but before CalculateRenderPasses().

[vmpstr, 2014-11-20 18:41:00]

https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:558:
client_->BuildRasterQueue(&raster_priority_queue_,
What's the motivation for pulling this out of this function?

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

https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:444: while (!raster_priority_queue_.IsEmpty()) {
I think you still need to reset/rebuilt raster_priority_queue. The situation
where it's important is if we get less memory than we've been using but all the
tiles are done rasterizing. In that case, raster queue will be empty, but
eviction will evict some tiles, which won't appear in the raster queue unless we
rebuild it.

[reveman, 2014-11-20 19:07:54]

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

https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
On 2014/11/20 18:28:57, vmiura wrote:
> On 2014/11/20 15:20:59, reveman wrote:
> > How you considered rasterizing at draw time instead? It would guarantee that
> > we're no rasterizing any tiles that are not needed for the frame.
> 
> I put it here in the prototype as it needs to be done after
> active_tree->UpdateDrawProperties() but before CalculateRenderPasses().

Why before CalculateRenderPasses()?

[vmiura, 2014-11-20 19:48:38]

On 2014/11/20 19:07:54, reveman wrote:
>
https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
> On 2014/11/20 18:28:57, vmiura wrote:
> > On 2014/11/20 15:20:59, reveman wrote:
> > > How you considered rasterizing at draw time instead? It would guarantee
that
> > > we're no rasterizing any tiles that are not needed for the frame.
> > 
> > I put it here in the prototype as it needs to be done after
> > active_tree->UpdateDrawProperties() but before CalculateRenderPasses().
> 
> Why before CalculateRenderPasses()?

Just based on making minimal changes, but I agree it could be done later with a
bit more work.  We currently decide what Quads to append in
CalculateRenderPasses().  If we didn't raster before CRP then I guess we would
get PictureDrawQuads which we could rasterize later.

Ganesh needs to collect all draws into an SkMultiPictureDraw, so it could be a
pass between CRP() and DrawFrame() that collects all PictureDrawQuads, assigns
resources, rasterizes and converts them to TileDrawQuads.  I guess it should be
Renderer independent.

It's a bit different from rasterize_on_demand in the sense that we want to
collect all quads and rasterize, whereas rasterize_on_demand can do incremental
raster.

I think we could also rasterize just tiles needed by building the right
raster_priority_queue_ in RasterizeTiles().

Rather than that, we could do raster before or inside CRP() and skip making
PictureDrawQuads() in the first place.  I guess we can still make it rasterize
just the tiles we need.

[danakj, 2014-11-20 19:59:36]

On Thu, Nov 20, 2014 at 2:48 PM, <vmiura@chromium.org> wrote:

> On 2014/11/20 19:07:54, reveman wrote:
>
> https://codereview.chromium.org/733773005/diff/60001/cc/
> trees/layer_tree_host_impl.cc
>
>> File cc/trees/layer_tree_host_impl.cc (right):
>>
>
>
> https://codereview.chromium.org/733773005/diff/60001/cc/
> trees/layer_tree_host_impl.cc#newcode1060
>
>> cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
>> On 2014/11/20 18:28:57, vmiura wrote:
>> > On 2014/11/20 15:20:59, reveman wrote:
>> > > How you considered rasterizing at draw time instead? It would
>> guarantee
>>
> that
>
>> > > we're no rasterizing any tiles that are not needed for the frame.
>> >
>> > I put it here in the prototype as it needs to be done after
>> > active_tree->UpdateDrawProperties() but before CalculateRenderPasses().
>>
>
>  Why before CalculateRenderPasses()?
>>
>
> Just based on making minimal changes, but I agree it could be done later
> with a
> bit more work.  We currently decide what Quads to append in
> CalculateRenderPasses().  If we didn't raster before CRP then I guess we
> would
> get PictureDrawQuads which we could rasterize later.
>
> Ganesh needs to collect all draws into an SkMultiPictureDraw, so it could
> be a
> pass between CRP() and DrawFrame() that collects all PictureDrawQuads,
> assigns
> resources, rasterizes and converts them to TileDrawQuads.  I guess it
> should be
> Renderer independent.
>
> It's a bit different from rasterize_on_demand in the sense that we want to
> collect all quads and rasterize, whereas rasterize_on_demand can do
> incremental
> raster.
>
> I think we could also rasterize just tiles needed by building the right
> raster_priority_queue_ in RasterizeTiles().
>
> Rather than that, we could do raster before or inside CRP() and skip making
> PictureDrawQuads() in the first place.  I guess we can still make it
> rasterize
> just the tiles we need.
>

Right, you'd just need to have state set up such that PLI::AppendQuads
would consider the tiles ok to append as a resource, even tho it's not
complete yet. Then you could walk through the generated RenderPasses and
raster all the resources that were appended? I think that's what David is
suggesting. We don't have Tile*'s by that point though.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2014-11-20 20:10:10]

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

https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:444: while (!raster_priority_queue_.IsEmpty()) {
On 2014/11/20 18:40:59, vmpstr wrote:
> I think you still need to reset/rebuilt raster_priority_queue. The situation
> where it's important is if we get less memory than we've been using but all
the
> tiles are done rasterizing. In that case, raster queue will be empty, but
> eviction will evict some tiles, which won't appear in the raster queue unless
we
> rebuild it.

Is it possible for some required_for_draw tiles to evict other required_for_draw
tiles?

[vmpstr, 2014-11-20 20:10:16]

On 2014/11/20 19:59:36, danakj wrote:
> On Thu, Nov 20, 2014 at 2:48 PM, <mailto:vmiura@chromium.org> wrote:
> 
> > On 2014/11/20 19:07:54, reveman wrote:
> >
> > https://codereview.chromium.org/733773005/diff/60001/cc/
> > trees/layer_tree_host_impl.cc
> >
> >> File cc/trees/layer_tree_host_impl.cc (right):
> >>
> >
> >
> > https://codereview.chromium.org/733773005/diff/60001/cc/
> > trees/layer_tree_host_impl.cc#newcode1060
> >
> >> cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
> >> On 2014/11/20 18:28:57, vmiura wrote:
> >> > On 2014/11/20 15:20:59, reveman wrote:
> >> > > How you considered rasterizing at draw time instead? It would
> >> guarantee
> >>
> > that
> >
> >> > > we're no rasterizing any tiles that are not needed for the frame.
> >> >
> >> > I put it here in the prototype as it needs to be done after
> >> > active_tree->UpdateDrawProperties() but before CalculateRenderPasses().
> >>
> >
> >  Why before CalculateRenderPasses()?
> >>
> >
> > Just based on making minimal changes, but I agree it could be done later
> > with a
> > bit more work.  We currently decide what Quads to append in
> > CalculateRenderPasses().  If we didn't raster before CRP then I guess we
> > would
> > get PictureDrawQuads which we could rasterize later.
> >
> > Ganesh needs to collect all draws into an SkMultiPictureDraw, so it could
> > be a
> > pass between CRP() and DrawFrame() that collects all PictureDrawQuads,
> > assigns
> > resources, rasterizes and converts them to TileDrawQuads.  I guess it
> > should be
> > Renderer independent.
> >
> > It's a bit different from rasterize_on_demand in the sense that we want to
> > collect all quads and rasterize, whereas rasterize_on_demand can do
> > incremental
> > raster.
> >
> > I think we could also rasterize just tiles needed by building the right
> > raster_priority_queue_ in RasterizeTiles().
> >
> > Rather than that, we could do raster before or inside CRP() and skip making
> > PictureDrawQuads() in the first place.  I guess we can still make it
> > rasterize
> > just the tiles we need.
> >
> 
> Right, you'd just need to have state set up such that PLI::AppendQuads
> would consider the tiles ok to append as a resource, even tho it's not
> complete yet. Then you could walk through the generated RenderPasses and
> raster all the resources that were appended? I think that's what David is
> suggesting. We don't have Tile*'s by that point though.
> 

I might be misunderstanding, but I think we'd end up in a state where we
generated resources for quads, but not for tiles in that case, no?

Like if you pretend in AppendQuads that tiles that aren't ready to draw
are in fact ready, and append their content rect and resource (null), 
then rasterize those resources for those content rects later, then how 
do we tie that back to tiles for caching?


> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-11-20 20:11:56]

On 2014/11/20 19:48:38, vmiura wrote:
> On 2014/11/20 19:07:54, reveman wrote:
> >
>
https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
> > File cc/trees/layer_tree_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/733773005/diff/60001/cc/trees/layer_tree_host...
> > cc/trees/layer_tree_host_impl.cc:1060: RasterizeTiles();
> > On 2014/11/20 18:28:57, vmiura wrote:
> > > On 2014/11/20 15:20:59, reveman wrote:
> > > > How you considered rasterizing at draw time instead? It would guarantee
> that
> > > > we're no rasterizing any tiles that are not needed for the frame.
> > > 
> > > I put it here in the prototype as it needs to be done after
> > > active_tree->UpdateDrawProperties() but before CalculateRenderPasses().
> > 
> > Why before CalculateRenderPasses()?
> 
> Just based on making minimal changes, but I agree it could be done later with
a
> bit more work.  We currently decide what Quads to append in
> CalculateRenderPasses().  If we didn't raster before CRP then I guess we would
> get PictureDrawQuads which we could rasterize later.

I wouldn't expect us to use PictureDrawQuads. And TileDrawQuad would probably be
incorrect too.

Would it make sense to add a new ManagedTileState::DrawInfo mode for this?
::GPU_RESOURCE_MODE maybe. And that mode would produce a new type of DrawQuad.

> 
> Ganesh needs to collect all draws into an SkMultiPictureDraw, so it could be a
> pass between CRP() and DrawFrame() that collects all PictureDrawQuads, assigns
> resources, rasterizes and converts them to TileDrawQuads.  I guess it should
be
> Renderer independent.

Could this be handled by DrawFrame?

> 
> It's a bit different from rasterize_on_demand in the sense that we want to
> collect all quads and rasterize, whereas rasterize_on_demand can do
incremental
> raster.

Yes, it wouldn't be the same as raster-on-demand. Raster would just be deferred
to DrawFrame time in a similar way.

> 
> I think we could also rasterize just tiles needed by building the right
> raster_priority_queue_ in RasterizeTiles().
> 
> Rather than that, we could do raster before or inside CRP() and skip making
> PictureDrawQuads() in the first place.  I guess we can still make it rasterize
> just the tiles we need.

I'm not sure what the best approach is but I think we'll need to get a lot of
the cleanup that is planned for the tile manager and rasterizer interface done
before we can be sure that integrating it at that level is the right approach.

[vmpstr, 2014-11-20 20:13:30]

On 2014/11/20 20:10:10, vmiura wrote:
>
https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/733773005/diff/60001/cc/resources/tile_manage...
> cc/resources/tile_manager.cc:444: while (!raster_priority_queue_.IsEmpty()) {
> On 2014/11/20 18:40:59, vmpstr wrote:
> > I think you still need to reset/rebuilt raster_priority_queue. The situation
> > where it's important is if we get less memory than we've been using but all
> the
> > tiles are done rasterizing. In that case, raster queue will be empty, but
> > eviction will evict some tiles, which won't appear in the raster queue
unless
> we
> > rebuild it.
> 
> Is it possible for some required_for_draw tiles to evict other
required_for_draw
> tiles?

No that's not possible, but what is possible is that our total memory budget
starts at some large value, gets filled up, and then gets cut to a smaller
value. In that case, the raster queue would be "all done", and eviction
would kick in afterwards to ensure that we're within the (smaller) budget,
and evict a bunch of things.

[reveman, 2014-11-20 20:45:39]

FWIW, here's how I think we could implement synchronous raster of tiles without
adding more complexity to tile management or diverge too much in how memory is
assigned to tiles.

- GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call it
DeferredRasterWorkerPool. This RWP implementation doesn't do any raster work
(but it would be doing prepare work such as image decode), it simply puts each
tile in deferred-raster-mode instead of performing raster. Think of this as
similar to how we currently put some tiles in solid-color mode.

- The tile manager will assign memory exactly the same way as now. Tiles that
get assigned memory can be put in deferred-raster mode by the deferred RWP
instance. Tiles that lose memory will lose whatever old deferred raster work
they might have done. From the tile manager's POV there's no difference compared
to sw raster. It assigns memory, figures out what image decode work needs to be
done and uses the Rasterizer interface to initialize the tiles.

- PictureLayerImpl will produce a new type of deferred raster quad for any tiles
in deferred-raster-mode. These deferred raster quads will contain the resource
assigned by the tile manager to the tile and that resource can be used as target
for deferred raster work.

- The Renderer can handle these deferred raster quads in whatever way it likes.
Do one pass over all quads to raster them or just raster them as it goes.

[danakj, 2014-11-20 20:54:56]

On Thu, Nov 20, 2014 at 3:45 PM, <reveman@chromium.org> wrote:

> FWIW, here's how I think we could implement synchronous raster of tiles
> without
> adding more complexity to tile management or diverge too much in how
> memory is
> assigned to tiles.
>
> - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call
> it
> DeferredRasterWorkerPool. This RWP implementation doesn't do any raster
> work
> (but it would be doing prepare work such as image decode), it simply puts
> each
> tile in deferred-raster-mode instead of performing raster. Think of this as
> similar to how we currently put some tiles in solid-color mode.
>
> - The tile manager will assign memory exactly the same way as now. Tiles
> that
> get assigned memory can be put in deferred-raster mode by the deferred RWP
> instance. Tiles that lose memory will lose whatever old deferred raster
> work
> they might have done. From the tile manager's POV there's no difference
> compared
> to sw raster. It assigns memory, figures out what image decode work needs
> to be
> done and uses the Rasterizer interface to initialize the tiles.
>
> - PictureLayerImpl will produce a new type of deferred raster quad for any
> tiles
> in deferred-raster-mode. These deferred raster quads will contain the
> resource
> assigned by the tile manager to the tile and that resource can be used as
> target
> for deferred raster work.
>
> - The Renderer can handle these deferred raster quads in whatever way it
> likes.
> Do one pass over all quads to raster them or just raster them as it goes.
>

What is going to tie that raster work back to the TIle*? And the
RasterSource*? We don't include those in DrawQuads, since DrawQuads are our
IPC types. I think doing it in DelegatingRenderer is the wrong place. It
shuold be higher up in shared code off LTHI.


>
> https://codereview.chromium.org/733773005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vmiura, 2014-11-21 00:11:13]

> FWIW, here's how I think we could implement synchronous raster of tiles
without
> adding more complexity to tile management or diverge too much in how memory is
> assigned to tiles.
> 
> - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call it
> DeferredRasterWorkerPool. This RWP implementation doesn't do any raster work
> (but it would be doing prepare work such as image decode), it simply puts each
> tile in deferred-raster-mode instead of performing raster. Think of this as
> similar to how we currently put some tiles in solid-color mode.
> 
> - The tile manager will assign memory exactly the same way as now. Tiles that
> get assigned memory can be put in deferred-raster mode by the deferred RWP
> instance. Tiles that lose memory will lose whatever old deferred raster work
> they might have done. From the tile manager's POV there's no difference
compared
> to sw raster. It assigns memory, figures out what image decode work needs to
be
> done and uses the Rasterizer interface to initialize the tiles.

Generally seems I think we can fit that, but a few things:

1. In order to make the Raster synchronous with in the Draw, we need to be able
to pass the resource to the tile synchronously.  Deferred RWP is not compatible
with that.  We need something like RasterizeTiles() in this patch which
synchronously assigns resources before rasterizing.

2. It would be nice to be able to skip a thread-hop to deferred RWP if we don't
have anything to prepare, so like (1) I don't think assigning tile resources
should require going via deferred RWP.

3. It would be nice to be able to run image pre-decode / tile-prepare tasks
independent of assigning tile texture memory.  I think AssignGpuMemoryToTiles()
needs to specialize a bit; e.g. AssignGpuMemoryToTiles() becomes something like
AssignTileMemoryForRaster() (which is identical in Software & Ganesh), and the
"prepare" path uses something like AssignTileMemoryForPrepare() with slightly
different logic.

4. If at some point we consider any pre-rasterizing in Ganesh (e.g. on idle
frames) we will need to rasterize tiles not required for Draw.  If we couple too
deeply to Draw, we'd have to backtrack later.

> - PictureLayerImpl will produce a new type of deferred raster quad for any
tiles
> in deferred-raster-mode. These deferred raster quads will contain the resource
> assigned by the tile manager to the tile and that resource can be used as
target
> for deferred raster work.
> 
> - The Renderer can handle these deferred raster quads in whatever way it
likes.
> Do one pass over all quads to raster them or just raster them as it goes.

Wouldn't the Rasterizer be a better place to abstract these raster specifics?

[reveman, 2014-11-21 16:03:54]

On 2014/11/21 00:11:13, vmiura wrote:
> > FWIW, here's how I think we could implement synchronous raster of tiles
> without
> > adding more complexity to tile management or diverge too much in how memory
is
> > assigned to tiles.
> > 
> > - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call it
> > DeferredRasterWorkerPool. This RWP implementation doesn't do any raster work
> > (but it would be doing prepare work such as image decode), it simply puts
each
> > tile in deferred-raster-mode instead of performing raster. Think of this as
> > similar to how we currently put some tiles in solid-color mode.
> > 
> > - The tile manager will assign memory exactly the same way as now. Tiles
that
> > get assigned memory can be put in deferred-raster mode by the deferred RWP
> > instance. Tiles that lose memory will lose whatever old deferred raster work
> > they might have done. From the tile manager's POV there's no difference
> compared
> > to sw raster. It assigns memory, figures out what image decode work needs to
> be
> > done and uses the Rasterizer interface to initialize the tiles.
> 
> Generally seems I think we can fit that, but a few things:
> 
> 1. In order to make the Raster synchronous with in the Draw, we need to be
able
> to pass the resource to the tile synchronously.  Deferred RWP is not
compatible
> with that.  We need something like RasterizeTiles() in this patch which
> synchronously assigns resources before rasterizing.

The tile manager assigns memory synchronously. However, the resource currently
travel with the raster task and make it back to the tile when the task completes
but that will change when we remove the RunReplyOnOriginThread from the
RasterizerTask interface: crbug.com/435557

> 
> 2. It would be nice to be able to skip a thread-hop to deferred RWP if we
don't
> have anything to prepare, so like (1) I don't think assigning tile resources
> should require going via deferred RWP.

That applies to SW raster as well. We could skip thread-hops to generate
ready-to-draw/activate signals if no raster or image decode work needs to be
done. I think we used to have some code that did this but it was removed to
reduce complexity. I'm fine adding that back after we've finished some of the
Rasterizer/TileManager cleanup work we're doing (crbug.com/435561) if we have
data showing that it's worthwhile.

> 
> 3. It would be nice to be able to run image pre-decode / tile-prepare tasks
> independent of assigning tile texture memory.  I think
AssignGpuMemoryToTiles()
> needs to specialize a bit; e.g. AssignGpuMemoryToTiles() becomes something
like
> AssignTileMemoryForRaster() (which is identical in Software & Ganesh), and the
> "prepare" path uses something like AssignTileMemoryForPrepare() with slightly
> different logic.

Being able to do prepare work for tiles that we have not yet assigned memory to
sounds reasonable although I don't fully understand how this would make a
significant difference. What would the savings be from doing this? Do we need to
handle this differently between SW raster and Ganesh?

> 
> 4. If at some point we consider any pre-rasterizing in Ganesh (e.g. on idle
> frames) we will need to rasterize tiles not required for Draw.  If we couple
too
> deeply to Draw, we'd have to backtrack later.

Ok, it might be better to keep this in layer code rather output code. ie. do
raster as part of Layer::AppendQuads.

> 
> > - PictureLayerImpl will produce a new type of deferred raster quad for any
> tiles
> > in deferred-raster-mode. These deferred raster quads will contain the
resource
> > assigned by the tile manager to the tile and that resource can be used as
> target
> > for deferred raster work.
> > 
> > - The Renderer can handle these deferred raster quads in whatever way it
> likes.
> > Do one pass over all quads to raster them or just raster them as it goes.
> 
> Wouldn't the Rasterizer be a better place to abstract these raster specifics?

I don't think so. Currently, I think the purpose of the Rasterizer interface is
to provide an asynchronous mechanism to raster content in which dependencies on
other work (ie. image decode) can be described. The tasks exists for the purpose
of being able to represent these dependencies and allow work to be done on
separate threads.

If we're just doing synchronous raster, then the Rasterizer interface and the
concept of a raster task doesn't make sense to me. We could add some other
interface for this if we need the use of Ganesh to be abstracted instead of
synchronous raster being equal to Ganesh but I'd avoid that if not necessary.

[reveman, 2014-11-21 16:10:32]

On 2014/11/20 20:54:56, danakj wrote:
> On Thu, Nov 20, 2014 at 3:45 PM, <mailto:reveman@chromium.org> wrote:
> 
> > FWIW, here's how I think we could implement synchronous raster of tiles
> > without
> > adding more complexity to tile management or diverge too much in how
> > memory is
> > assigned to tiles.
> >
> > - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call
> > it
> > DeferredRasterWorkerPool. This RWP implementation doesn't do any raster
> > work
> > (but it would be doing prepare work such as image decode), it simply puts
> > each
> > tile in deferred-raster-mode instead of performing raster. Think of this as
> > similar to how we currently put some tiles in solid-color mode.
> >
> > - The tile manager will assign memory exactly the same way as now. Tiles
> > that
> > get assigned memory can be put in deferred-raster mode by the deferred RWP
> > instance. Tiles that lose memory will lose whatever old deferred raster
> > work
> > they might have done. From the tile manager's POV there's no difference
> > compared
> > to sw raster. It assigns memory, figures out what image decode work needs
> > to be
> > done and uses the Rasterizer interface to initialize the tiles.
> >
> > - PictureLayerImpl will produce a new type of deferred raster quad for any
> > tiles
> > in deferred-raster-mode. These deferred raster quads will contain the
> > resource
> > assigned by the tile manager to the tile and that resource can be used as
> > target
> > for deferred raster work.
> >
> > - The Renderer can handle these deferred raster quads in whatever way it
> > likes.
> > Do one pass over all quads to raster them or just raster them as it goes.
> >
> 
> What is going to tie that raster work back to the TIle*? And the
> RasterSource*? We don't include those in DrawQuads, since DrawQuads are our
> IPC types. I think doing it in DelegatingRenderer is the wrong place. It
> shuold be higher up in shared code off LTHI.
> 

A new quad type might be a bad idea. I was thinking that as PictureDrawQuad
includes a raster source we could just create a new quad that includes both a
raster source and a resource. However, it might be best to keep this in the
layer/tree code until we actually want to handle it differently depending on the
Renderer.

[vmiura, 2014-11-21 18:35:28]

On 2014/11/21 16:03:54, reveman wrote:
> On 2014/11/21 00:11:13, vmiura wrote:
> > > FWIW, here's how I think we could implement synchronous raster of tiles
> > without
> > > adding more complexity to tile management or diverge too much in how
memory
> is
> > > assigned to tiles.
> > > 
> > > - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's call
it
> > > DeferredRasterWorkerPool. This RWP implementation doesn't do any raster
work
> > > (but it would be doing prepare work such as image decode), it simply puts
> each
> > > tile in deferred-raster-mode instead of performing raster. Think of this
as
> > > similar to how we currently put some tiles in solid-color mode.
> > > 
> > > - The tile manager will assign memory exactly the same way as now. Tiles
> that
> > > get assigned memory can be put in deferred-raster mode by the deferred RWP
> > > instance. Tiles that lose memory will lose whatever old deferred raster
work
> > > they might have done. From the tile manager's POV there's no difference
> > compared
> > > to sw raster. It assigns memory, figures out what image decode work needs
to
> > be
> > > done and uses the Rasterizer interface to initialize the tiles.
> > 
> > Generally seems I think we can fit that, but a few things:
> > 
> > 1. In order to make the Raster synchronous with in the Draw, we need to be
> able
> > to pass the resource to the tile synchronously.  Deferred RWP is not
> compatible
> > with that.  We need something like RasterizeTiles() in this patch which
> > synchronously assigns resources before rasterizing.
> 
> The tile manager assigns memory synchronously. However, the resource currently
> travel with the raster task and make it back to the tile when the task
completes
> but that will change when we remove the RunReplyOnOriginThread from the
> RasterizerTask interface: crbug.com/435557
>
> > 2. It would be nice to be able to skip a thread-hop to deferred RWP if we
> don't
> > have anything to prepare, so like (1) I don't think assigning tile resources
> > should require going via deferred RWP.
> 
> That applies to SW raster as well. We could skip thread-hops to generate
> ready-to-draw/activate signals if no raster or image decode work needs to be
> done. I think we used to have some code that did this but it was removed to
> reduce complexity. I'm fine adding that back after we've finished some of the
> Rasterizer/TileManager cleanup work we're doing (crbug.com/435561) if we have
> data showing that it's worthwhile.
> 
> > 
> > 3. It would be nice to be able to run image pre-decode / tile-prepare tasks
> > independent of assigning tile texture memory.  I think
> AssignGpuMemoryToTiles()
> > needs to specialize a bit; e.g. AssignGpuMemoryToTiles() becomes something
> like
> > AssignTileMemoryForRaster() (which is identical in Software & Ganesh), and
the
> > "prepare" path uses something like AssignTileMemoryForPrepare() with
slightly
> > different logic.
> 
> Being able to do prepare work for tiles that we have not yet assigned memory
to
> sounds reasonable although I don't fully understand how this would make a
> significant difference. What would the savings be from doing this? Do we need
to
> handle this differently between SW raster and Ganesh?

Just an optimization.  SW needs a tile resource for RasterTasks.  Ganesh doesn't
need a resource for PrepareTasks.

We'll generate PrepareTasks for tiles outside the viewport that might never
become visible, so assigning a resource will evict other tiles unnecessarily. 
Ganesh can save resources by allocating for just 'now required for draw' tiles.

> > 4. If at some point we consider any pre-rasterizing in Ganesh (e.g. on idle
> > frames) we will need to rasterize tiles not required for Draw.  If we couple
> too
> > deeply to Draw, we'd have to backtrack later.
> 
> Ok, it might be better to keep this in layer code rather output code. ie. do
> raster as part of Layer::AppendQuads.

Layer::AppendQuads is for things we want to composit, so not for
pre-rasterizing.


I'd suggest keeping it at a level where CC scheduler could run it, so something
on LTHI like Manfred's patch :-D.

> > > - PictureLayerImpl will produce a new type of deferred raster quad for any
> > tiles
> > > in deferred-raster-mode. These deferred raster quads will contain the
> resource
> > > assigned by the tile manager to the tile and that resource can be used as
> > target
> > > for deferred raster work.
> > > 
> > > - The Renderer can handle these deferred raster quads in whatever way it
> > likes.
> > > Do one pass over all quads to raster them or just raster them as it goes.
> > 
> > Wouldn't the Rasterizer be a better place to abstract these raster
specifics?
> 
> I don't think so. Currently, I think the purpose of the Rasterizer interface
is
> to provide an asynchronous mechanism to raster content in which dependencies
on
> other work (ie. image decode) can be described. The tasks exists for the
purpose
> of being able to represent these dependencies and allow work to be done on
> separate threads.
> 
> If we're just doing synchronous raster, then the Rasterizer interface and the
> concept of a raster task doesn't make sense to me. We could add some other
> interface for this if we need the use of Ganesh to be abstracted instead of
> synchronous raster being equal to Ganesh but I'd avoid that if not necessary.

It begs the question why Rasterizer interface is tasks which TileManager has to
know about.  Perhaps a longer discussion.

I'd like to make sure we have a way forward for this patch as Ganesh image
decode is depending on it.  I will try to share the Ganesh image decode patch
later today so we can discuss around.

[ernstm, 2014-11-21 18:46:28]

I have scheduled a meeting to discuss.

[reveman, 2014-11-21 20:21:26]

On 2014/11/21 18:35:28, vmiura wrote:
> On 2014/11/21 16:03:54, reveman wrote:
> > On 2014/11/21 00:11:13, vmiura wrote:
> > > > FWIW, here's how I think we could implement synchronous raster of tiles
> > > without
> > > > adding more complexity to tile management or diverge too much in how
> memory
> > is
> > > > assigned to tiles.
> > > > 
> > > > - GpuRasterWorkerPool is replaced by a new RWP implementation. Let's
call
> it
> > > > DeferredRasterWorkerPool. This RWP implementation doesn't do any raster
> work
> > > > (but it would be doing prepare work such as image decode), it simply
puts
> > each
> > > > tile in deferred-raster-mode instead of performing raster. Think of this
> as
> > > > similar to how we currently put some tiles in solid-color mode.
> > > > 
> > > > - The tile manager will assign memory exactly the same way as now. Tiles
> > that
> > > > get assigned memory can be put in deferred-raster mode by the deferred
RWP
> > > > instance. Tiles that lose memory will lose whatever old deferred raster
> work
> > > > they might have done. From the tile manager's POV there's no difference
> > > compared
> > > > to sw raster. It assigns memory, figures out what image decode work
needs
> to
> > > be
> > > > done and uses the Rasterizer interface to initialize the tiles.
> > > 
> > > Generally seems I think we can fit that, but a few things:
> > > 
> > > 1. In order to make the Raster synchronous with in the Draw, we need to be
> > able
> > > to pass the resource to the tile synchronously.  Deferred RWP is not
> > compatible
> > > with that.  We need something like RasterizeTiles() in this patch which
> > > synchronously assigns resources before rasterizing.
> > 
> > The tile manager assigns memory synchronously. However, the resource
currently
> > travel with the raster task and make it back to the tile when the task
> completes
> > but that will change when we remove the RunReplyOnOriginThread from the
> > RasterizerTask interface: crbug.com/435557
> >
> > > 2. It would be nice to be able to skip a thread-hop to deferred RWP if we
> > don't
> > > have anything to prepare, so like (1) I don't think assigning tile
resources
> > > should require going via deferred RWP.
> > 
> > That applies to SW raster as well. We could skip thread-hops to generate
> > ready-to-draw/activate signals if no raster or image decode work needs to be
> > done. I think we used to have some code that did this but it was removed to
> > reduce complexity. I'm fine adding that back after we've finished some of
the
> > Rasterizer/TileManager cleanup work we're doing (crbug.com/435561) if we
have
> > data showing that it's worthwhile.
> > 
> > > 
> > > 3. It would be nice to be able to run image pre-decode / tile-prepare
tasks
> > > independent of assigning tile texture memory.  I think
> > AssignGpuMemoryToTiles()
> > > needs to specialize a bit; e.g. AssignGpuMemoryToTiles() becomes something
> > like
> > > AssignTileMemoryForRaster() (which is identical in Software & Ganesh), and
> the
> > > "prepare" path uses something like AssignTileMemoryForPrepare() with
> slightly
> > > different logic.
> > 
> > Being able to do prepare work for tiles that we have not yet assigned memory
> to
> > sounds reasonable although I don't fully understand how this would make a
> > significant difference. What would the savings be from doing this? Do we
need
> to
> > handle this differently between SW raster and Ganesh?
> 
> Just an optimization.  SW needs a tile resource for RasterTasks.  Ganesh
doesn't
> need a resource for PrepareTasks.
> 
> We'll generate PrepareTasks for tiles outside the viewport that might never
> become visible, so assigning a resource will evict other tiles unnecessarily. 
> Ganesh can save resources by allocating for just 'now required for draw'
tiles.
> 
> > > 4. If at some point we consider any pre-rasterizing in Ganesh (e.g. on
idle
> > > frames) we will need to rasterize tiles not required for Draw.  If we
couple
> > too
> > > deeply to Draw, we'd have to backtrack later.
> > 
> > Ok, it might be better to keep this in layer code rather output code. ie. do
> > raster as part of Layer::AppendQuads.
> 
> Layer::AppendQuads is for things we want to composit, so not for
> pre-rasterizing.
> 
> 
> I'd suggest keeping it at a level where CC scheduler could run it, so
something
> on LTHI like Manfred's patch :-D.

Keeping it in LTHI is fine with me if that's more appropriate. I'm more
concerned about how we assign memory and how to not add more complexity to the
tile manager and the Rasterize interface.

> 
> > > > - PictureLayerImpl will produce a new type of deferred raster quad for
any
> > > tiles
> > > > in deferred-raster-mode. These deferred raster quads will contain the
> > resource
> > > > assigned by the tile manager to the tile and that resource can be used
as
> > > target
> > > > for deferred raster work.
> > > > 
> > > > - The Renderer can handle these deferred raster quads in whatever way it
> > > likes.
> > > > Do one pass over all quads to raster them or just raster them as it
goes.
> > > 
> > > Wouldn't the Rasterizer be a better place to abstract these raster
> specifics?
> > 
> > I don't think so. Currently, I think the purpose of the Rasterizer interface
> is
> > to provide an asynchronous mechanism to raster content in which dependencies
> on
> > other work (ie. image decode) can be described. The tasks exists for the
> purpose
> > of being able to represent these dependencies and allow work to be done on
> > separate threads.
> > 
> > If we're just doing synchronous raster, then the Rasterizer interface and
the
> > concept of a raster task doesn't make sense to me. We could add some other
> > interface for this if we need the use of Ganesh to be abstracted instead of
> > synchronous raster being equal to Ganesh but I'd avoid that if not
necessary.
> 
> It begs the question why Rasterizer interface is tasks which TileManager has
to
> know about.  Perhaps a longer discussion.

The tasks exists to make it clear to the Rasterizer implementation and the
Tile/TileManager what's allowed to be used from a worker thread. We could just
be passing Tile instances to the Rasterizer but it would make it less obvious
what might be used on a worker thread and what might not.

> 
> I'd like to make sure we have a way forward for this patch as Ganesh image
> decode is depending on it.  I will try to share the Ganesh image decode patch
> later today so we can discuss around.

[ernstm, 2014-11-26 01:54:58]

I've created a GpuRasterizer that does the synchronous rasterization. Please
take another look.

[ernstm, 2014-11-26 18:29:38]

On 2014/11/26 01:54:58, ernstm wrote:
> I've created a GpuRasterizer that does the synchronous rasterization. Please
> take another look.

btw: GpuRasterWorkerPool could be removed with this patch, but Victor will need
it for the prepare tasks, so I left it unchanged for now.

[vmiura, 2014-12-24 00:17:49]

Closing, as superceded by https://codereview.chromium.org/820743002/.