cc: GPU rasterize tiles synchronously in PrepareToDraw.

cc: GPU rasterize tiles synchronously in PrepareToDraw

This is another version of Manfred's patch to rasterize GPU tiles
synchronously.

Original: https://codereview.chromium.org/733773005/
vmiura's merge (plus fixes): https://codereview.chromium.org/807233002/

* Adds Rasterizer base class, derives a Software and Gpu rasterizer
* GpuRasterizer rasterizes the tiles synchronously
* SoftwareRasterizer does basically nothing (but returns the prepare mode)
* Gutted parts of GpuTileTaskWorkerPool, this probably needs to be removed
  completely.  It doesn't do real work anymore.  There was investigation into
  doing the raster work in this class, but it looks like we might start using
  one or zero copy for gpu rasterization in the future.
* TileManager has two modes now:
  * create raster tasks and do nothing
  * don't create tasks and rasterize synchronously
  - there was some investigation into separating these better but vmiura's
    next change will start adding decode tasks.  Some of this may become
    more clear when his stuff is committed.
  synchronously,
* There's some code duplication in TileManager, but vmiura assures me that
  his patch will address that.  More specifically, some of the code in
  PrepareTiles will always be executed, and then removed from
  SynchronouslyRasterizeTiles
* Added a test that exercises GpuRasterizer and verifies that it creates
  resources

BUG=434889
Committed: https://crrev.com/2f32e4ee3c21e0bd9ca26d7fa63cf14ed49d8a3b
Cr-Commit-Position: refs/heads/master@{#309866}

[hendrikw, 2014-12-23 02:42:19]

hendrikw@chromium.org changed reviewers:
+ vmiura@chromium.org, vmpstr@chromium.org

[hendrikw, 2014-12-23 02:42:33]

PTAL, thanks

[vmpstr, 2014-12-23 09:51:03]

The PrepareTilesMode feels like it could be cleaner if it was just two bools:
something like RasterizeDuringPrepare, and ShouldPrepareTiles with a
dcheck_implies(RaterizeDuringPrepare, ShouldPrepareTiles) somewhere (maybe not,
but I find it a bit awkward that the checks are mostly of the TileMode !=
something variety (instead of TileMode == something)).

It also feels like we need to deconstruct tile manager into parts that make
sense independently of rasterization mode, make those functions well named and
isolated, and then have a few ifs on rasterizer bools in few other functions
that control the flow. I guess this does this to some degree, but it seems a bit
messy. Maybe after my comments are fixed, this would look better, but feel free
to fix things that you think might be better as well.

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:32: const bool kUseColorEstimator = true;
I know we have this elsewhere, but can you remove this as part of this change.
Since we control this in PLI now, I don't see a reason for having another
constant.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:62: return tile_prepare_enabled_ ?
PrepareTilesMode::PREPARE_PRIORITIZED_TILES
It feels a bit awkward to pass this bool, since we will always have it as false
(or true after vmiura's patch). Is there some reason why it's a run time check?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:68: TileManager* tile_manager) {
:\ I'm not really a fan of rasterizer being owned by tile manager, and it having
a reference back to it. It almost feels like a callback would be better

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:82: Raster(tile, const_resource);
Should this be called something like "AppendToMultiPictureDraw"?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_tile_ta...
File cc/resources/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_tile_ta...
cc/resources/gpu_tile_task_worker_pool.cc:47: // TODO(hendrikw): This class
should be removed.
Is this hard to do? That is, if it's a lot of changes elsewhere to get this
removed, then this is fine. If it's straight-forward then it feels like we
should do it here, since otherwise this class is pretty gutted.

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:440: ready_to_activate_notifier_.Schedule();
I'm not really a fan of these callbacks. Can we just call the function directly
here?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:464:
client_->BuildRasterQueue(&raster_priority_queue_,
I think I've mentioned this on the original patch, but this isn't sufficient. We
have to rebuild the queue before marking tiles rasterize on demand. The reason
for this is that AssignGpuMemory, as one of its last actions ensures that we're
within memory limit. This means that we might release some times unconditionally
after we've already created the raster queue. As a result, the queue will miss
those tiles below.

It's probably pretty are that we're going to release down to required for
activation tiles, but I would still rather be safe.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:641: raster_priority_queue_.Pop();
I think this needs some thought. I don't like the fact that we run through the
whole raster queue every time. The easiest thing would be to build a queue with
smoothness takes priority mode and break out after the bin isn't NOW. I don't
know if that's the best solution though. Maybe we need to do some redesign of
the iterators to allow for this type of query. Is there a long term plan for
this? That is, is there a planned follow-up? If so, what is it?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:862: void TileManager::CompleteRasterTask(
Why not just call OnRasterTaskCompleted with false? :)

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:869: void TileManager::OnRasterTaskCompleted(
Maybe a better split would be if OnRasterTaskCompleted dealt with raster_tasks
stuff and then called InitializeTile(tile, resource, analysis) and gpu
rasterizer can call that function directly (replace InitializeTile with whatever
name might be better)

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:963: if (did_notify_ready_to_draw_)
We check this twice now on the software path..

[vmpstr, 2014-12-23 09:54:43]

Hit send after reviewing tile_manager :P but here's one more comment

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
..AndRasterizer? :) Can TileManager own the rasterizer instead?

[danakj, 2014-12-23 16:16:13]

On Mon, Dec 22, 2014 at 6:42 PM, <hendrikw@chromium.org> wrote:

> LayerTreeHostOnDemandRasterPixelTestWithGpuRasterizationForc
> ed.RasterPictureLayer
>

This tests on-demand-raster which is in GLRenderer.


> LayerTreeHostTestDeferredInitializeWithGpuRasterization.
> RunMultiThread_DirectRenderer_MainThreadPaint
> LayerTreeHostTestDeferredInitializeWithGpuRasterization.
> RunMultiThread_DirectRenderer_ImplSidePaint
> LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_
> DelegatingRenderer_MainThreadPaint
> LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_
> DelegatingRenderer_ImplSidePaint
>

This test appears to do nothing but set some settings and make sure we
don't crash? Probably this test should just go away...


> LayerTreeHostTestGpuRasterizationForced.RunSingleThread_
> DirectRenderer_ImplSidePaint
> LayerTreeHostTestGpuRasterizationForced.RunMultiThread_
> DirectRenderer_ImplSidePaint
> LayerTreeHostTestGpuRasterizationForced.RunSingleThread_
> DelegatingRenderer_ImplSidePaint
> LayerTreeHostTestGpuRasterizationForced.RunMultiThread_DelegatingRenderer_
> ImplSidePaint
>

This checks that we choose to use gpu rasterization but doesn't check what
the gpu raster code does at all.


> LayerTreeHostPictureTestResizeViewportWithGpuRaster.
> RunMultiThread_DirectRenderer_ImplSidePaint
>

This tests PictureLayerImpl's tile size choosing, doesn't verify anything
from the rasterizer.


> LayerTreeHostMasksPixelTest.* also does, but is not run on windows :/
>

:/


>
> Please let me know if you don't think this is sufficient, and I'll do my
> best '
> to add another test.


IWBN to add a (non-pixel) unit test that uses GPU raster and verifies that
tiles with resources come out of the system. I am thinking something like
the new pinchzoom smoke test that was recently
added, LayerTreeHostTestCrispUpAfterPinchEndsWithOneCopy (probably without
the delaying raster forever part, and verifying tiles have resources?).


https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

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

[danakj, 2014-12-23 16:54:29]

On Tue, Dec 23, 2014 at 8:15 AM, Dana Jansens <danakj@chromium.org> wrote:

> On Mon, Dec 22, 2014 at 6:42 PM, <hendrikw@chromium.org> wrote:
>
>> LayerTreeHostOnDemandRasterPixelTestWithGpuRasterizationForc
>> ed.RasterPictureLayer
>>
>
> This tests on-demand-raster which is in GLRenderer.
>
>
>> LayerTreeHostTestDeferredInitializeWithGpuRasterization.
>> RunMultiThread_DirectRenderer_MainThreadPaint
>> LayerTreeHostTestDeferredInitializeWithGpuRasterization.
>> RunMultiThread_DirectRenderer_ImplSidePaint
>> LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_
>> DelegatingRenderer_MainThreadPaint
>> LayerTreeHostTestDeferredInitializeWithGpuRasterization.RunMultiThread_
>> DelegatingRenderer_ImplSidePaint
>>
>
> This test appears to do nothing but set some settings and make sure we
> don't crash? Probably this test should just go away...
>

Oh I half take this back, this is a webview test making sure we can init
from software to GL on the compositor thread when using GPU raster. I'm not
really.. sure.. how GPU raster matters for this test compared to the
non-GPU raster version, but maybe it does.


>
>
>> LayerTreeHostTestGpuRasterizationForced.RunSingleThread_
>> DirectRenderer_ImplSidePaint
>> LayerTreeHostTestGpuRasterizationForced.RunMultiThread_
>> DirectRenderer_ImplSidePaint
>> LayerTreeHostTestGpuRasterizationForced.RunSingleThread_
>> DelegatingRenderer_ImplSidePaint
>> LayerTreeHostTestGpuRasterizationForced.RunMultiThread_
>> DelegatingRenderer_ImplSidePaint
>>
>
> This checks that we choose to use gpu rasterization but doesn't check what
> the gpu raster code does at all.
>
>
>> LayerTreeHostPictureTestResizeViewportWithGpuRaster.
>> RunMultiThread_DirectRenderer_ImplSidePaint
>>
>
> This tests PictureLayerImpl's tile size choosing, doesn't verify anything
> from the rasterizer.
>
>
>> LayerTreeHostMasksPixelTest.* also does, but is not run on windows :/
>>
>
> :/
>
>
>>
>> Please let me know if you don't think this is sufficient, and I'll do my
>> best '
>> to add another test.
>
>
> IWBN to add a (non-pixel) unit test that uses GPU raster and verifies that
> tiles with resources come out of the system. I am thinking something like
> the new pinchzoom smoke test that was recently
> added, LayerTreeHostTestCrispUpAfterPinchEndsWithOneCopy (probably without
> the delaying raster forever part, and verifying tiles have resources?).
>
>
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...
>

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

[hendrikw, 2014-12-23 18:10:33]

I made a few of the requested changes.

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:32: const bool kUseColorEstimator = true;
On 2014/12/23 09:51:02, vmpstr wrote:
> I know we have this elsewhere, but can you remove this as part of this change.
> Since we control this in PLI now, I don't see a reason for having another
> constant.

Sure, do you want me to remove the one in tilemanager too?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:62: return tile_prepare_enabled_ ?
PrepareTilesMode::PREPARE_PRIORITIZED_TILES
On 2014/12/23 09:51:02, vmpstr wrote:
> It feels a bit awkward to pass this bool, since we will always have it as
false
> (or true after vmiura's patch). Is there some reason why it's a run time
check?

I don't know what the plans for this are, I thought both modes would be used.
Victor, can you jump in?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:68: TileManager* tile_manager) {
On 2014/12/23 09:51:02, vmpstr wrote:
> :\ I'm not really a fan of rasterizer being owned by tile manager, and it
having
> a reference back to it. It almost feels like a callback would be better

Not a fan?  We do this _EVERYWHERE_!  :)

Sure, I'll change this to a callback, though I don't think it improves things
much (sometimes too much abstraction makes it more difficult to understand).

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:82: Raster(tile, const_resource);
On 2014/12/23 09:51:02, vmpstr wrote:
> Should this be called something like "AppendToMultiPictureDraw"?

Yes!

I'll also rename Analyze to PerformSolidColorAnalysis

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_tile_ta...
File cc/resources/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_tile_ta...
cc/resources/gpu_tile_task_worker_pool.cc:47: // TODO(hendrikw): This class
should be removed.
On 2014/12/23 09:51:02, vmpstr wrote:
> Is this hard to do? That is, if it's a lot of changes elsewhere to get this
> removed, then this is fine. If it's straight-forward then it feels like we
> should do it here, since otherwise this class is pretty gutted.

Yes, too hard to do.  Many tests are still using it (and fail if it ceases to
exist) Also I don't know what vmiura will need to do when he starts making tasks
again.

Better that we do this as a followup after his change lands + this change is
already getting big.

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:440: ready_to_activate_notifier_.Schedule();
On 2014/12/23 09:51:02, vmpstr wrote:
> I'm not really a fan of these callbacks. Can we just call the function
directly
> here?

Nope, they must be scheduled.  vmiura, can you jump in and explain?

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:464:
client_->BuildRasterQueue(&raster_priority_queue_,
On 2014/12/23 09:51:03, vmpstr wrote:
> I think I've mentioned this on the original patch, but this isn't sufficient.
We
> have to rebuild the queue before marking tiles rasterize on demand. The reason
> for this is that AssignGpuMemory, as one of its last actions ensures that
we're
> within memory limit. This means that we might release some times
unconditionally
> after we've already created the raster queue. As a result, the queue will miss
> those tiles below.
> 
> It's probably pretty are that we're going to release down to required for
> activation tiles, but I would still rather be safe.

I guess I'll talk with you about this when you get in.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:862: void TileManager::CompleteRasterTask(
On 2014/12/23 09:51:03, vmpstr wrote:
> Why not just call OnRasterTaskCompleted with false? :)

Changing to a callback, so it doesn't matter.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:869: void TileManager::OnRasterTaskCompleted(
On 2014/12/23 09:51:02, vmpstr wrote:
> Maybe a better split would be if OnRasterTaskCompleted dealt with raster_tasks
> stuff and then called InitializeTile(tile, resource, analysis) and gpu
> rasterizer can call that function directly (replace InitializeTile with
whatever
> name might be better)

I'm not sure what InitializeTile is, how it relates, or what you want split.
again, I'll wait for you to get in.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:963: if (did_notify_ready_to_draw_)
On 2014/12/23 09:51:02, vmpstr wrote:
> We check this twice now on the software path..

I only see one (REQUIRED_FOR_DRAW), where's the second?

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
On 2014/12/23 09:54:42, vmpstr wrote:
> ..AndRasterizer? :) Can TileManager own the rasterizer instead?

Do you want TileManager to take a enum (GPU or software), and have it create the
rasterizer?

[hendrikw, 2014-12-23 18:15:17]

hendrikw@chromium.org changed reviewers:
+ ernstm@chromium.org

[hendrikw, 2014-12-23 18:23:17]

On 2014/12/23 09:51:03, vmpstr wrote:
> It also feels like we need to deconstruct tile manager into parts that make
> sense independently of rasterization mode, make those functions well named and
> isolated, and then have a few ifs on rasterizer bools in few other functions
> that control the flow. I guess this does this to some degree, but it seems a
bit
> messy. Maybe after my comments are fixed, this would look better, but feel
free
> to fix things that you think might be better as well.

I tried to make the separation clearer, but in the end, when victor's patch
lands,
there's only one function (SynchronouslyRasterizeTiles) that is gpu specific,
the rest 
will be shared between both gpu and software, though gpu will only create decode
tasks
and software will create decode+raster tasks

[vmiura, 2014-12-23 18:33:14]

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:62: return tile_prepare_enabled_ ?
PrepareTilesMode::PREPARE_PRIORITIZED_TILES
On 2014/12/23 09:51:02, vmpstr wrote:
> It feels a bit awkward to pass this bool, since we will always have it as
false
> (or true after vmiura's patch). Is there some reason why it's a run time
check?

In my patch it's turned on by a "--enable-tile-prepare" flag.  We might want to
keep it behind a flag initially, so we can default on and have an option to turn
it off if anything blows up in wide release.

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:440: ready_to_activate_notifier_.Schedule();
On 2014/12/23 09:51:02, vmpstr wrote:
> I'm not really a fan of these callbacks. Can we just call the function
directly
> here?

The client can end up recursing back into TM::PrepareTiles(), so this was aimed
at running on an unwinded stack, similar to
ready_to_activate/draw_check_notifier_.

After landing GPU Prepare tasks, we should remove this code.

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:641: raster_priority_queue_.Pop();
On 2014/12/23 09:51:02, vmpstr wrote:
> I think this needs some thought. I don't like the fact that we run through the
> whole raster queue every time. The easiest thing would be to build a queue
with
> smoothness takes priority mode and break out after the bin isn't NOW. I don't
> know if that's the best solution though. Maybe we need to do some redesign of
> the iterators to allow for this type of query. Is there a long term plan for
> this? That is, is there a planned follow-up? If so, what is it?

Initially I think we might want to have fewer changes in this patch,  so we
should remove this check and rasterize all the tiles (i.e. still include border
tiles).

As a follow up we should make this draw only required_for_draw tiles based on
Vlad's suggestion (either using SMOOTHNESS_TAKES_PRIORITY mode iterator, or a
new iterator constraint).

In this patch, we still have a skewport of Zero for GPU Raster, and we should be
Activating right after PrepareTiles (modulo scheduling), so we won't be
iterating through excess tiles, however with the follow-up GPU Prepare patch I'm
enabling the skewport, and PrepareTiles goes back to doing deferred work, so
then having a way to iterate only 'required_for_draw' tiles will be required.

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
TileManager doesn't have access to context_provider, resource_provider,
use_dff...

  *rasterizer =
         GpuRasterizer::Create(context_provider, resource_provider_.get(),
                                           settings_.use_distance_field_text,
false);

[hendrikw, 2014-12-23 18:54:30]

removed the border raster stuff vmiura suggested should be moved into followup

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

https://codereview.chromium.org/820743002/diff/80001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:641: raster_priority_queue_.Pop();
On 2014/12/23 18:33:13, vmiura wrote:
> On 2014/12/23 09:51:02, vmpstr wrote:
> > I think this needs some thought. I don't like the fact that we run through
the
> > whole raster queue every time. The easiest thing would be to build a queue
> with
> > smoothness takes priority mode and break out after the bin isn't NOW. I
don't
> > know if that's the best solution though. Maybe we need to do some redesign
of
> > the iterators to allow for this type of query. Is there a long term plan for
> > this? That is, is there a planned follow-up? If so, what is it?
> 
> Initially I think we might want to have fewer changes in this patch,  so we
> should remove this check and rasterize all the tiles (i.e. still include
border
> tiles).
> 
> As a follow up we should make this draw only required_for_draw tiles based on
> Vlad's suggestion (either using SMOOTHNESS_TAKES_PRIORITY mode iterator, or a
> new iterator constraint).
> 
> In this patch, we still have a skewport of Zero for GPU Raster, and we should
be
> Activating right after PrepareTiles (modulo scheduling), so we won't be
> iterating through excess tiles, however with the follow-up GPU Prepare patch
I'm
> enabling the skewport, and PrepareTiles goes back to doing deferred work, so
> then having a way to iterate only 'required_for_draw' tiles will be required.

Done.

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
On 2014/12/23 18:33:13, vmiura wrote:
> TileManager doesn't have access to context_provider, resource_provider,
> use_dff...
> 
>   *rasterizer =
>          GpuRasterizer::Create(context_provider, resource_provider_.get(),
>                                            settings_.use_distance_field_text,
> false);

oh fun.

[vmiura, 2014-12-23 19:08:25]

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
On 2014/12/23 18:54:29, hendrikw wrote:
> On 2014/12/23 18:33:13, vmiura wrote:
> > TileManager doesn't have access to context_provider, resource_provider,
> > use_dff...
> > 
> >   *rasterizer =
> >          GpuRasterizer::Create(context_provider, resource_provider_.get(),
> >                                           
settings_.use_distance_field_text,
> > false);
> 
> oh fun.

Flowing rasterizer specific settings through TileManager doesn't sound right. 
If anything, it seems to me that either Rasterizer should own TileManager, or
they should both remain peers on LTHI.

[vmpstr, 2014-12-23 20:00:02]

(just a quick response)

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

https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:1959:
CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
On 2014/12/23 19:08:25, vmiura wrote:
> On 2014/12/23 18:54:29, hendrikw wrote:
> > On 2014/12/23 18:33:13, vmiura wrote:
> > > TileManager doesn't have access to context_provider, resource_provider,
> > > use_dff...
> > > 
> > >   *rasterizer =
> > >          GpuRasterizer::Create(context_provider, resource_provider_.get(),
> > >                                           
> settings_.use_distance_field_text,
> > > false);
> > 
> > oh fun.
> 
> Flowing rasterizer specific settings through TileManager doesn't sound right. 
> If anything, it seems to me that either Rasterizer should own TileManager, or
> they should both remain peers on LTHI.

I meant to say we can create it here, but why store it on LTHI? Can we just pass
the ownership to tile manager? It doesn't really matter, I guess, since we do a
similar thing with tile task worker pool, but in either case
CreateResourceAndTileTaskWorkerPool dosen't seem like the right name anymore.
Maybe we can split this up into more functions or rename it to something more
appropriate.

https://codereview.chromium.org/820743002/diff/100001/cc/resources/gpu_tile_t...
File cc/resources/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/820743002/diff/100001/cc/resources/gpu_tile_t...
cc/resources/gpu_tile_task_worker_pool.cc:47: // TODO(hendrikw): This class
should be removed.
File a bug for this please.

[hendrikw, 2014-12-23 22:42:39]

On 2014/12/23 20:00:02, vmpstr wrote:
> (just a quick response)
> 
>
https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
> File cc/trees/layer_tree_host_impl.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/80001/cc/trees/layer_tree_host...
> cc/trees/layer_tree_host_impl.cc:1959:
> CreateResourceAndTileTaskWorkerPool(&tile_task_worker_pool_, &rasterizer_,
> On 2014/12/23 19:08:25, vmiura wrote:
> > On 2014/12/23 18:54:29, hendrikw wrote:
> > > On 2014/12/23 18:33:13, vmiura wrote:
> > > > TileManager doesn't have access to context_provider, resource_provider,
> > > > use_dff...
> > > > 
> > > >   *rasterizer =
> > > >          GpuRasterizer::Create(context_provider,
resource_provider_.get(),
> > > >                                           
> > settings_.use_distance_field_text,
> > > > false);
> > > 
> > > oh fun.
> > 
> > Flowing rasterizer specific settings through TileManager doesn't sound
right. 
> > If anything, it seems to me that either Rasterizer should own TileManager,
or
> > they should both remain peers on LTHI.
> 
> I meant to say we can create it here, but why store it on LTHI? Can we just
pass
> the ownership to tile manager? It doesn't really matter, I guess, since we do
a
> similar thing with tile task worker pool, but in either case
> CreateResourceAndTileTaskWorkerPool dosen't seem like the right name anymore.
> Maybe we can split this up into more functions or rename it to something more
> appropriate.
> 
>
https://codereview.chromium.org/820743002/diff/100001/cc/resources/gpu_tile_t...
> File cc/resources/gpu_tile_task_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/100001/cc/resources/gpu_tile_t...
> cc/resources/gpu_tile_task_worker_pool.cc:47: // TODO(hendrikw): This class
> should be removed.
> File a bug for this please.

I've added a test that uses the GpuRasterizer, and checks that it created
resources

I started from the test danakj suggested.  Seems a bit simple, hopefully it's
close to what danakj likes.  Let me know how I can improve it (I feel like I'm
doing reaching into a black box when coding this)

I've logged the issue to remove the gpu worker pool and added the number to the
comment.

I've also split off the CreateRasterizer from
CreateResourceAndTileTaskWorkerPool.

I guess the only thing that remains is the issue with us allocating too much
memory (running all tasks, perhaps when we can't, assuming I understand vmpstr's
concern correctly).

[hendrikw, 2014-12-23 23:24:58]

vmiura helped me hopefully address vmpstr's concern.  PTAL

[vmiura, 2014-12-29 22:58:10]

https://codereview.chromium.org/820743002/diff/260001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/260001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:458: SynchronouslyRasterizeTiles(state);
I think it would be better to put the SynchronouslyRasterizeTiles() call after
the CheckForCompletedTasks() check, that way the latest asynchronous tasks that
complete are finalized before the sync-raster.

[hendrikw, 2014-12-29 23:28:19]

On 2014/12/29 22:58:10, vmiura wrote:
>
https://codereview.chromium.org/820743002/diff/260001/cc/resources/tile_manag...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/260001/cc/resources/tile_manag...
> cc/resources/tile_manager.cc:458: SynchronouslyRasterizeTiles(state);
> I think it would be better to put the SynchronouslyRasterizeTiles() call after
> the CheckForCompletedTasks() check, that way the latest asynchronous tasks
that
> complete are finalized before the sync-raster.

done!

[vmiura, 2014-12-30 22:24:38]

LGTM for me, this'll work for the image prepare patch.  Thanks!

A few comments, in particular please check "layer_tree_host_impl.cc:1867".

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:66: scoped_ptr<ScopedResource> resource =
We should be able to skip acquiring a resource for solid color tiles.  Maybe add
a TODO?

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:111: ResourceProvider::ScopedWriteLockGr
lock(resource_provider_, resource->id());
Not new code, but isn't it a bit odd that we lock & unlock the resource for
writing here, but rasterize to the surface later?

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.h (right):

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.h:33: const OnTaskCompleteCallback&
OnRasterTaskCompleted) override;
nit: OnRasterTaskCompleted -> on_raster_task_completed?

https://codereview.chromium.org/820743002/diff/300001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/300001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:87: void FakeTileManager::AssignMemoryToTiles(
Remove this unused method?

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

https://codereview.chromium.org/820743002/diff/300001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(vmiura): Make sure we do evict
tiles in all modes.
Please check this with vmpstr.  In case we use PrepareTilesMode::PREPARE_NONE,
TileManager::PrepareTiles() won't evict anything here.  How do we handle that
case?

[vmpstr, 2014-12-30 22:55:23]

https://codereview.chromium.org/820743002/diff/320001/cc/resources/software_r...
File cc/resources/software_rasterizer.h (right):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/software_r...
cc/resources/software_rasterizer.h:15: class CC_EXPORT SoftwareRasterizer :
public Rasterizer {
Make a comment here describing what this class does (and doesn't do)

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:491:
client_->BuildRasterQueue(&raster_priority_queue_,
It seems that this change can stay, since we always do Build/Assign/Reset
outside (we might as well keep it inside)

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:352: if (prepare_tiles_mode !=
PrepareTilesMode::PREPARE_NONE) {
Can you add a TODO here to consider moving some of this code into rasterizer? 

I'm concerned that this function is basically the following:

PrepareTiles {
 if (!PREPARE_NONE) {
   ...
 } else {
   ...
 }
}, which kind of says to me that this should be a split at the rasterizer level.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:410: // We must reduce the amount of unused
resoruces before calling
typo: resources

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:808: if (tile->raster_task_.get()) {
This can stay a DCHECK now, I think.

https://codereview.chromium.org/820743002/diff/320001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/320001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:86: // TODO(vmiura): This seems unused.  Remove it?
You can remove if it's unused.

https://codereview.chromium.org/820743002/diff/320001/cc/test/layer_tree_pixe...
File cc/test/layer_tree_pixel_resource_test.h (right):

https://codereview.chromium.org/820743002/diff/320001/cc/test/layer_tree_pixe...
cc/test/layer_tree_pixel_resource_test.h:38: void
CreateRasterizer(LayerTreeHostImpl* host_impl,
This is kind of awkward, maybe scoped_ptr<Rasterizer> CreateRasterizer(LTHI*
host_impl) override?

or is there a reason for this particular way?

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

https://codereview.chromium.org/820743002/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(vmiura): Make sure we do evict
tiles in all modes.
This needs to be addressed in this patch (ideally with a test).

[hendrikw, 2014-12-31 00:03:56]

PTAL

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:66: scoped_ptr<ScopedResource> resource =
On 2014/12/30 22:24:38, vmiura wrote:
> We should be able to skip acquiring a resource for solid color tiles.  Maybe
add
> a TODO?

Done.

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:111: ResourceProvider::ScopedWriteLockGr
lock(resource_provider_, resource->id());
On 2014/12/30 22:24:38, vmiura wrote:
> Not new code, but isn't it a bit odd that we lock & unlock the resource for
> writing here, but rasterize to the surface later?

Done.

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.h (right):

https://codereview.chromium.org/820743002/diff/300001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.h:33: const OnTaskCompleteCallback&
OnRasterTaskCompleted) override;
On 2014/12/30 22:24:38, vmiura wrote:
> nit: OnRasterTaskCompleted -> on_raster_task_completed?

Done.

https://codereview.chromium.org/820743002/diff/300001/cc/test/fake_tile_manag...
File cc/test/fake_tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/300001/cc/test/fake_tile_manag...
cc/test/fake_tile_manager.cc:87: void FakeTileManager::AssignMemoryToTiles(
On 2014/12/30 22:24:38, vmiura wrote:
> Remove this unused method?

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/software_r...
File cc/resources/software_rasterizer.h (right):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/software_r...
cc/resources/software_rasterizer.h:15: class CC_EXPORT SoftwareRasterizer :
public Rasterizer {
On 2014/12/30 22:55:23, vmpstr wrote:
> Make a comment here describing what this class does (and doesn't do)

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:491:
client_->BuildRasterQueue(&raster_priority_queue_,
On 2014/12/30 22:55:23, vmpstr wrote:
> It seems that this change can stay, since we always do Build/Assign/Reset
> outside (we might as well keep it inside)

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:352: if (prepare_tiles_mode !=
PrepareTilesMode::PREPARE_NONE) {
On 2014/12/30 22:55:23, vmpstr wrote:
> Can you add a TODO here to consider moving some of this code into rasterizer? 
> 
> I'm concerned that this function is basically the following:
> 
> PrepareTiles {
>  if (!PREPARE_NONE) {
>    ...
>  } else {
>    ...
>  }
> }, which kind of says to me that this should be a split at the rasterizer
level.

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:410: // We must reduce the amount of unused
resoruces before calling
On 2014/12/30 22:55:23, vmpstr wrote:
> typo: resources

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:808: if (tile->raster_task_.get()) {
On 2014/12/30 22:55:23, vmpstr wrote:
> This can stay a DCHECK now, I think.

Done.

https://codereview.chromium.org/820743002/diff/320001/cc/test/layer_tree_pixe...
File cc/test/layer_tree_pixel_resource_test.h (right):

https://codereview.chromium.org/820743002/diff/320001/cc/test/layer_tree_pixe...
cc/test/layer_tree_pixel_resource_test.h:38: void
CreateRasterizer(LayerTreeHostImpl* host_impl,
On 2014/12/30 22:55:23, vmpstr wrote:
> This is kind of awkward, maybe scoped_ptr<Rasterizer> CreateRasterizer(LTHI*
> host_impl) override?
> 
> or is there a reason for this particular way?

Done.

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

https://codereview.chromium.org/820743002/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl.cc:1867: // TODO(vmiura): Make sure we do evict
tiles in all modes.
On 2014/12/30 22:55:23, vmpstr wrote:
> This needs to be addressed in this patch (ideally with a test).

Done.

[vmpstr, 2014-12-31 00:22:56]

Wouldn't this also rasterize all prepaint tiles (and pending tree tiles)? What
happened to the code that filtered out required for draw tiles? I'm fairly sure
this would be a pretty big regression.

https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:115: ResourceProvider::ScopedWriteLockGr* lock =
This should already be a scoped ptr, and Pass to locks

[hendrikw, 2014-12-31 00:31:04]

On 2014/12/31 00:22:56, vmpstr wrote:
> Wouldn't this also rasterize all prepaint tiles (and pending tree tiles)? What
> happened to the code that filtered out required for draw tiles? I'm fairly
sure
> this would be a pretty big regression.
> 
>
https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
> File cc/resources/gpu_rasterizer.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
> cc/resources/gpu_rasterizer.cc:115: ResourceProvider::ScopedWriteLockGr* lock
=
> This should already be a scoped ptr, and Pass to locks

Yes, very bad.

[vmpstr, 2014-12-31 00:31:41]

On 2014/12/31 00:31:04, hendrikw wrote:
> On 2014/12/31 00:22:56, vmpstr wrote:
> > Wouldn't this also rasterize all prepaint tiles (and pending tree tiles)?
What
> > happened to the code that filtered out required for draw tiles? I'm fairly
> sure
> > this would be a pretty big regression.
> > 
> >
>
https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
> > File cc/resources/gpu_rasterizer.cc (right):
> > 
> >
>
https://codereview.chromium.org/820743002/diff/340001/cc/resources/gpu_raster...
> > cc/resources/gpu_rasterizer.cc:115: ResourceProvider::ScopedWriteLockGr*
lock
> =
> > This should already be a scoped ptr, and Pass to locks
> 
> Yes, very bad.

Although, we currently don't do any filtering, so I'm guessing it's the fact
that our memory policy is set to prepaint only somewhere?

[vmiura, 2014-12-31 00:48:22]

On 2014/12/31 00:22:56, vmpstr wrote:
> Wouldn't this also rasterize all prepaint tiles (and pending tree tiles)? What
> happened to the code that filtered out required for draw tiles? I'm fairly
sure
> this would be a pretty big regression.

Currently Ganesh rasterizes all prioritized tiles, so this would be the same. 
Filtering required for draw tiles should be an improvement to performance, but
we wanted to do that as a follow up patch since sync-raster changes already may
change performance.

Removing pre-painting will change what tiles get rasterized during a scroll
action, so benchmarks will change, possibly regressing since initial page
pre-painting is currently "free" in our benchmarks.  I think we should land that
as a standalone change and watch the benchmarks.

[vmiura, 2014-12-31 00:59:24]

> Although, we currently don't do any filtering, so I'm guessing it's the fact
> that our memory policy is set to prepaint only somewhere?

Skewport target time of 0.0s for gpu rasterization.

[vmpstr, 2014-12-31 01:05:59]

On 2014/12/31 00:48:22, vmiura wrote:
> On 2014/12/31 00:22:56, vmpstr wrote:
> > Wouldn't this also rasterize all prepaint tiles (and pending tree tiles)?
What
> > happened to the code that filtered out required for draw tiles? I'm fairly
> sure
> > this would be a pretty big regression.
> 
> Currently Ganesh rasterizes all prioritized tiles, so this would be the same. 
> Filtering required for draw tiles should be an improvement to performance, but
> we wanted to do that as a follow up patch since sync-raster changes already
may
> change performance.
> 
> Removing pre-painting will change what tiles get rasterized during a scroll
> action, so benchmarks will change, possibly regressing since initial page
> pre-painting is currently "free" in our benchmarks.  I think we should land
that
> as a standalone change and watch the benchmarks.

The difference is that we would schedule 32 tiles at a time, yielding to
whatever else needs to be done, but now we will schedule all tiles (including
prepaint and pending tree). I'm fine with that as long as it's a conscious
decision and not an accidental "whoops".

[vmiura, 2014-12-31 01:27:05]

> The difference is that we would schedule 32 tiles at a time, yielding to
> whatever else needs to be done, but now we will schedule all tiles (including
> prepaint and pending tree). I'm fine with that as long as it's a conscious
> decision and not an accidental "whoops".

Good point about the 32 tile limit.  Current Ganesh would also blast through a
series of ScheduleTasks, but potentially slice to other impl thread tasks in
between.

[vmpstr, 2015-01-02 17:43:22]

https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
File cc/resources/gpu_rasterizer.cc (right):

https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:65: ResourceWriteLocks locks;
nit: ScopedResourceWriteLocks

https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
cc/resources/gpu_rasterizer.cc:68: // TODO(hendrikw): Don't create resources for
solid color tiles.
File a bug for this please.

https://codereview.chromium.org/820743002/diff/360001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/820743002/diff/360001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:228: ready_to_activate_notifier_(
From my understanding, we use these notifiers if tiles_prepare_mode ==
PREPARE_NONE and otherwise we use the two notifiers below. Since they call very
similar functions (one checks and notifies, one just notifies), is it possible
to add the tiles_prepare_mode check into the "check" function and just have one
set of notifiers? Or is there a future plan for this that makes it not possible?

[hendrikw, 2015-01-02 18:55:10]

On 2015/01/02 17:43:22, vmpstr wrote:
>
https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
> File cc/resources/gpu_rasterizer.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
> cc/resources/gpu_rasterizer.cc:65: ResourceWriteLocks locks;
> nit: ScopedResourceWriteLocks
> 
Done

>
https://codereview.chromium.org/820743002/diff/360001/cc/resources/gpu_raster...
> cc/resources/gpu_rasterizer.cc:68: // TODO(hendrikw): Don't create resources
for
> solid color tiles.
> File a bug for this please.
> 
Done

>
https://codereview.chromium.org/820743002/diff/360001/cc/resources/tile_manag...
> File cc/resources/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/820743002/diff/360001/cc/resources/tile_manag...
> cc/resources/tile_manager.cc:228: ready_to_activate_notifier_(
> From my understanding, we use these notifiers if tiles_prepare_mode ==
> PREPARE_NONE and otherwise we use the two notifiers below. Since they call
very
> similar functions (one checks and notifies, one just notifies), is it possible
> to add the tiles_prepare_mode check into the "check" function and just have
one
> set of notifiers? Or is there a future plan for this that makes it not
possible?

This would move the logic from one location to three.  I'm hopeful that we can
improve the design in a followup patch, and possibly
move some of this logic to the rasterizers.

[vmpstr, 2015-01-02 20:36:53]

lgtm from the point of view of correctness. I'm ok with landing this, with the
assumption that work will continue in this space to see if we can find a better
design (as we've discussed numerous times offline). Also, this should allow
vmiura's image decode patch to move forward, so that we can make progress on GPU
rasterization's performance. However, again, I'd like to stress that I want to
continue working on this to refactor/clean this area of the code a bit more.

[hendrikw, 2015-01-02 20:37:39]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-02 20:38:42]

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

[commit-bot: I haz the power, 2015-01-02 20:49:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-02 20:49:08]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[hendrikw, 2015-01-02 22:35:24]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-02 22:35:44]

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

[commit-bot: I haz the power, 2015-01-02 22:53:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-02 22:53:51]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[hendrikw, 2015-01-02 23:30:55]

The CQ bit was checked by hendrikw@chromium.org

[commit-bot: I haz the power, 2015-01-02 23:31:37]

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

[commit-bot: I haz the power, 2015-01-03 00:23:12]

Committed patchset #23 (id:440001)

[commit-bot: I haz the power, 2015-01-03 00:24:06]

Patchset 23 (id:??) landed as
https://crrev.com/2f32e4ee3c21e0bd9ca26d7fa63cf14ed49d8a3b
Cr-Commit-Position: refs/heads/master@{#309866}