cc: Only make num_threads - 1 SkPicture clones.

cc: Only make num_threads - 1 SkPicture clones.

Run on-demand raster on worker threads. This allows us to create
one fewer SkPicture clone as raster will never be done on the
compositor thread.

BUG=333855
TEST=cc_unittests --gtest_filter=LayerTreeHostOnDemandRasterPixelTest.RasterPictureLayer

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

[vmpstr, 2014-02-11 19:54:41]

lgtm. I was worried about getclone returning a * instead of a scoped_refptr, but
I think we always hang on to at least one scoped refptr in all cases. Quad has
it a scoped refptr, and raster task has it as that as well. I'm not missing any
cases, right?

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (left):

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc#oldc...
cc/resources/picture.cc:95: if (num_raster_threads > 0)
Was this check ever valid?

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:204: // We can re-use this picture for the first raster
worker thread.
Technically last, not first. Or just say for "one of raster worker threads"

[Dominik Grewe, 2014-02-11 20:33:35]

Thanks! Glad we could do this optimization after all.

cc::Picture lgtm.

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (left):

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc#oldc...
cc/resources/picture.cc:95: if (num_raster_threads > 0)
On 2014/02/11 19:54:42, vmpstr wrote:
> Was this check ever valid?

Some tests actually pass 0 because we don't need clones in those cases. We could
easily change that though.

[vmpstr, 2014-02-11 21:20:31]

On 2014/02/11 20:33:35, Dominik Grewe wrote:
> Thanks! Glad we could do this optimization after all.
> 
> cc::Picture lgtm.
> 
> https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc
> File cc/resources/picture.cc (left):
> 
>
https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc#oldc...
> cc/resources/picture.cc:95: if (num_raster_threads > 0)
> On 2014/02/11 19:54:42, vmpstr wrote:
> > Was this check ever valid?
> 
> Some tests actually pass 0 because we don't need clones in those cases. We
could
> easily change that though.

Ah I see. Thanks.

[reveman, 2014-02-11 21:22:28]

enne, please take a look.

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/160023002/diff/1/cc/resources/picture.cc#newc...
cc/resources/picture.cc:204: // We can re-use this picture for the first raster
worker thread.
On 2014/02/11 19:54:42, vmpstr wrote:
> Technically last, not first. Or just say for "one of raster worker threads"

Done.

[enne (OOO), 2014-02-11 22:30:45]

lgtm

Is there any way that we can enforce that pictures are never played back on the
compositor thread?

https://codereview.chromium.org/160023002/diff/90001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/160023002/diff/90001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1778: // Schedule task and wait for task grap runner to
finish running it.
typo

https://codereview.chromium.org/160023002/diff/90001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/160023002/diff/90001/cc/resources/picture.cc#...
cc/resources/picture.cc:219: clone->EmitTraceSnapshotAlias(this);
Do you need to emit the trace snapshot alias for the 0th picture too?

[reveman, 2014-02-11 23:40:42]

Added SoftwareRenderer support. The raster_thread_checker_.CalledOnValidThread()
DCHECKs in Picture class will fail if called on the wrong thread. That seems to
be good protection against invalid use.

PTAL.

https://codereview.chromium.org/160023002/diff/90001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/160023002/diff/90001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1778: // Schedule task and wait for task grap runner to
finish running it.
On 2014/02/11 22:30:45, enne wrote:
> typo

Done.

https://codereview.chromium.org/160023002/diff/90001/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/160023002/diff/90001/cc/resources/picture.cc#...
cc/resources/picture.cc:219: clone->EmitTraceSnapshotAlias(this);
On 2014/02/11 22:30:45, enne wrote:
> Do you need to emit the trace snapshot alias for the 0th picture too?

Not needed as discussed.

[enne (OOO), 2014-02-11 23:43:49]

lgtmstill

[reveman, 2014-02-11 23:46:24]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-11 23:48:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/160023002/230001

[commit-bot: I haz the power, 2014-02-12 00:29:07]

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

[commit-bot: I haz the power, 2014-02-12 00:29:07]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests,
check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests,
components_unittests, compositor_unittests, content_browsertests,
content_unittests, crypto_unittests, events_unittests, google_apis_unittests,
gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, ppapi_unittests,
printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests,
sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[reveman, 2014-02-12 01:08:04]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-12 01:09:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/160023002/230001

[commit-bot: I haz the power, 2014-02-12 09:35:19]

Change committed as 250664