cc: Add threaded GPU rasterization.

cc: Add threaded GPU rasterization.

Enable flag: --enable-threaded-gpu-rasterization

This patch enables threaded rasterization in GpuTileTaskWorkerPool.

There are some not yet landed/rolled dependencies:

  InProcessContextProvider Context Lost handling (jbauman@).
  Without this the feature should be considered unstable on WebViews.

BUG=454500
Committed: https://crrev.com/78b692864e7745993b8be343f67968ca071e0642
Cr-Commit-Position: refs/heads/master@{#316333}

[vmiura, 2015-02-11 03:05:18]

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

[vmiura, 2015-02-11 03:05:19]

PTAL.  Almost have the pieces to turn on the threaded raster.

vmpstr@chromium.org, hendrikw@chromium.org: CC, GpuRasterWorkerPool,
GpuRasterizer factoring.

danakj@chromium.org: cc_unittests

jbauman@chromium.org: ContextProviders / OutputSurfaces setup.

sievers@chromium.org: OutputSurfaces related to Android.

[danakj, 2015-02-11 17:56:26]

danakj@chromium.org changed reviewers:
+ enne@chromium.org
- danakj@chromium.org

[enne (OOO), 2015-02-11 18:19:51]

enne@chromium.org changed reviewers:
+ danakj@chromium.org

[enne (OOO), 2015-02-11 18:19:52]

cc and cc unit tests look fine % vmpstr and hendrikw

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1142:
settings.threaded_gpu_rasterization_enabled ? 0.2f : 0.0f;
Maybe if threaded_gpu_rasterization_enabled, the embedder should just change
skewport_target_time_in_seconds, and then just set this to 0 if
!gpu_rasterization?  That way we can keep these skewports somewhat more
independent.  I don't feel strongly about this, so feel free to ignore.

https://codereview.chromium.org/916723002/diff/1/cc/test/failure_output_surfa...
File cc/test/failure_output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/test/failure_output_surfa...
cc/test/failure_output_surface.cc:10: :
FakeOutputSurface((ContextProvider*)nullptr, is_delegating) {
static_cast, if you really need a cast.

[vmpstr, 2015-02-11 19:00:52]

Is threaded gpu rasterization better than synchronous gpu rasterization in all
cases? If so, then let's just do that instead of supporting both ways. 

If there are different cases, what will determine whether this flag is on or
off?

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1142:
settings.threaded_gpu_rasterization_enabled ? 0.2f : 0.0f;
On 2015/02/11 18:19:51, enne wrote:
> Maybe if threaded_gpu_rasterization_enabled, the embedder should just change
> skewport_target_time_in_seconds, and then just set this to 0 if
> !gpu_rasterization?  That way we can keep these skewports somewhat more
> independent.  I don't feel strongly about this, so feel free to ignore.

I agree, I think this could be nicer if the setting changed when the flag
changes (if it's a command line flag, then it should just determine what
skewport target time at that time as well).

https://codereview.chromium.org/916723002/diff/1/cc/resources/gpu_tile_task_w...
File cc/resources/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/resources/gpu_tile_task_w...
cc/resources/gpu_tile_task_worker_pool.cc:67: // TODO(hendrikw): This class
should be removed.  See crbug.com/444938.
Is this still true?

https://codereview.chromium.org/916723002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/916723002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:1008: LOG(ERROR) << "Failed to create worker
context provider!!!!!!!!!";
:) !!!!!!!

[vmiura, 2015-02-11 20:13:18]

> Is threaded gpu rasterization better than synchronous gpu
> rasterization in all cases? If so, then let's just do that
> instead of supporting both ways. 

The general thing is we're conscious and accepting that we'll give up some peak
throughput for some dynamic content, in exchange for smoothness on a lot of
other content.

Before sync-raster (as far back as December :-D) we were pre-painting border
tiles active + pending trees, etc.  Sync-raster improved some polymer benchmarks
a little, but made some smoothness & latency benchmarks a little worse.

My expectation is we will be giving back the gains we had with sync-raster, but
seeing big improvements on most other benchmarks.

> If there are different cases, what will determine whether this flag is on or
> off?

The flag will be fixed one way or the other.  As this is an involved change, we
want to be able to have an option to back-out safely during M42 beta, hence
supporting both until the new mode sticks.  Then we should remove sync-raster
mode.

I'm not totally giving up on the option of sync-raster, but maybe we can do it
on the thread and just Sync for the thread to compelete.  Nat is still hopeful
we can optimize & avoid thread hops & such, in future++.

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1142:
settings.threaded_gpu_rasterization_enabled ? 0.2f : 0.0f;
On 2015/02/11 19:00:51, vmpstr wrote:
> On 2015/02/11 18:19:51, enne wrote:
> > Maybe if threaded_gpu_rasterization_enabled, the embedder should just change
> > skewport_target_time_in_seconds, and then just set this to 0 if
> > !gpu_rasterization?  That way we can keep these skewports somewhat more
> > independent.  I don't feel strongly about this, so feel free to ignore.
> 
> I agree, I think this could be nicer if the setting changed when the flag
> changes (if it's a command line flag, then it should just determine what
> skewport target time at that time as well).

Because of vetoing we want to be able to switch from
(threaded_gpu_rasterization, skewport = 0.2 seconds) to (software_rasterization,
skewport = 1.0 seconds).

So if we code the skewport to 0.2 in RenderWidgetCompositor then that's not
good.

Is there a better place we should do this, or perhaps I should add a separate
settings.gpu_skewport_target_time_in_seconds?

https://codereview.chromium.org/916723002/diff/1/cc/resources/gpu_tile_task_w...
File cc/resources/gpu_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/resources/gpu_tile_task_w...
cc/resources/gpu_tile_task_worker_pool.cc:67: // TODO(hendrikw): This class
should be removed.  See crbug.com/444938.
On 2015/02/11 19:00:52, vmpstr wrote:
> Is this still true?

Removed comment.

https://codereview.chromium.org/916723002/diff/1/cc/test/failure_output_surfa...
File cc/test/failure_output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/test/failure_output_surfa...
cc/test/failure_output_surface.cc:10: :
FakeOutputSurface((ContextProvider*)nullptr, is_delegating) {
On 2015/02/11 18:19:52, enne wrote:
> static_cast, if you really need a cast.

Done.  It's needed to disambiguate between ContextProvider &
SoftwareOutputDevice versions of the constructors.

https://codereview.chromium.org/916723002/diff/1/content/renderer/render_widg...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/916723002/diff/1/content/renderer/render_widg...
content/renderer/render_widget.cc:1008: LOG(ERROR) << "Failed to create worker
context provider!!!!!!!!!";
On 2015/02/11 19:00:52, vmpstr wrote:
> :) !!!!!!!

Removed :)

[enne (OOO), 2015-02-11 20:45:16]

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1142:
settings.threaded_gpu_rasterization_enabled ? 0.2f : 0.0f;
On 2015/02/11 20:13:18, vmiura wrote:
> On 2015/02/11 19:00:51, vmpstr wrote:
> > On 2015/02/11 18:19:51, enne wrote:
> > > Maybe if threaded_gpu_rasterization_enabled, the embedder should just
change
> > > skewport_target_time_in_seconds, and then just set this to 0 if
> > > !gpu_rasterization?  That way we can keep these skewports somewhat more
> > > independent.  I don't feel strongly about this, so feel free to ignore.
> > 
> > I agree, I think this could be nicer if the setting changed when the flag
> > changes (if it's a command line flag, then it should just determine what
> > skewport target time at that time as well).
> 
> Because of vetoing we want to be able to switch from
> (threaded_gpu_rasterization, skewport = 0.2 seconds) to
(software_rasterization,
> skewport = 1.0 seconds).
> 
> So if we code the skewport to 0.2 in RenderWidgetCompositor then that's not
> good.
> 
> Is there a better place we should do this, or perhaps I should add a separate
> settings.gpu_skewport_target_time_in_seconds?

Yeah, a different setting seems better, in that case.

(Aren't we getting rid of vetoing?)

[vmiura, 2015-02-11 22:12:50]

PTAL

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/916723002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:1142:
settings.threaded_gpu_rasterization_enabled ? 0.2f : 0.0f;
On 2015/02/11 20:45:15, enne wrote:
> On 2015/02/11 20:13:18, vmiura wrote:
> > On 2015/02/11 19:00:51, vmpstr wrote:
> > > On 2015/02/11 18:19:51, enne wrote:
> > > > Maybe if threaded_gpu_rasterization_enabled, the embedder should just
> change
> > > > skewport_target_time_in_seconds, and then just set this to 0 if
> > > > !gpu_rasterization?  That way we can keep these skewports somewhat more
> > > > independent.  I don't feel strongly about this, so feel free to ignore.
> > > 
> > > I agree, I think this could be nicer if the setting changed when the flag
> > > changes (if it's a command line flag, then it should just determine what
> > > skewport target time at that time as well).
> > 
> > Because of vetoing we want to be able to switch from
> > (threaded_gpu_rasterization, skewport = 0.2 seconds) to
> (software_rasterization,
> > skewport = 1.0 seconds).
> > 
> > So if we code the skewport to 0.2 in RenderWidgetCompositor then that's not
> > good.
> > 
> > Is there a better place we should do this, or perhaps I should add a
separate
> > settings.gpu_skewport_target_time_in_seconds?
> 
> Yeah, a different setting seems better, in that case.

Done.

> (Aren't we getting rid of vetoing?)

We should, but not ready to go all-in just yet :).

[enne (OOO), 2015-02-11 22:23:38]

https://codereview.chromium.org/916723002/diff/40001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/916723002/diff/40001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.cc:33: gpu_rasterization_skewport_factor(0.0f),
The reason I wanted this change in the first place was so that software and
hardware skewports weren't tied together.  Can you just make this
gpu_rasterization_skewport_target_time_in_seconds and not relative to the other
skewport?

[vmpstr, 2015-02-11 22:27:25]

On 2015/02/11 20:13:18, vmiura wrote:
> > Is threaded gpu rasterization better than synchronous gpu
> > rasterization in all cases? If so, then let's just do that
> > instead of supporting both ways. 
> 
> The general thing is we're conscious and accepting that we'll give up some
peak
> throughput for some dynamic content, in exchange for smoothness on a lot of
> other content.
> 
> Before sync-raster (as far back as December :-D) we were pre-painting border
> tiles active + pending trees, etc.  Sync-raster improved some polymer
benchmarks
> a little, but made some smoothness & latency benchmarks a little worse.
> 
> My expectation is we will be giving back the gains we had with sync-raster,
but
> seeing big improvements on most other benchmarks.
> 
> > If there are different cases, what will determine whether this flag is on or
> > off?
> 
> The flag will be fixed one way or the other.  As this is an involved change,
we
> want to be able to have an option to back-out safely during M42 beta, hence
> supporting both until the new mode sticks.  Then we should remove sync-raster
> mode.
> 
> I'm not totally giving up on the option of sync-raster, but maybe we can do it
> on the thread and just Sync for the thread to compelete.  Nat is still hopeful
> we can optimize & avoid thread hops & such, in future++.
> 

Ok, thanks for the explanation. Can you please file a bug to remove one of the
modes, whichever loses on the bots or whichever one seems worse than the other?
I want to avoid it falling off the radar and having two modes indefinitely, even
if only one of those can be enabled.

[vmiura, 2015-02-11 23:32:43]

https://codereview.chromium.org/916723002/diff/40001/cc/trees/layer_tree_sett...
File cc/trees/layer_tree_settings.cc (right):

https://codereview.chromium.org/916723002/diff/40001/cc/trees/layer_tree_sett...
cc/trees/layer_tree_settings.cc:33: gpu_rasterization_skewport_factor(0.0f),
On 2015/02/11 22:23:38, enne wrote:
> The reason I wanted this change in the first place was so that software and
> hardware skewports weren't tied together.  Can you just make this
> gpu_rasterization_skewport_target_time_in_seconds and not relative to the
other
> skewport?

Done.

[enne (OOO), 2015-02-11 23:33:40]

Thanks! lgtm

[jbauman, 2015-02-11 23:46:09]

OutputSurface/ContextProvider lgtm

[vmpstr, 2015-02-11 23:53:38]

Thanks for filing the crbug, lgtm with some nits

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

https://codereview.chromium.org/916723002/diff/60001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:124:
multi_picture_draw.add(write_lock->sk_surface()->getCanvas(),
Do we have to use a multi picture draw here? What's the reason (I don't know
that much about it)

https://codereview.chromium.org/916723002/diff/60001/cc/test/failure_output_s...
File cc/test/failure_output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/60001/cc/test/failure_output_s...
cc/test/failure_output_surface.cc:10: :
FakeOutputSurface(static_cast<ContextProvider*>(nullptr), is_delegating) {
Do you need a cast here? nullptr_t is supposed to be implicitly convertible to
all raw pointers.

https://codereview.chromium.org/916723002/diff/60001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/916723002/diff/60001/content/renderer/render_...
content/renderer/render_widget.cc:1024: routing_id(), output_surface_id, NULL,
NULL, software_device.Pass(),
nullptr still, right?

[vmiura, 2015-02-12 00:00:12]

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

https://codereview.chromium.org/916723002/diff/60001/cc/resources/gpu_rasteri...
cc/resources/gpu_rasterizer.cc:124:
multi_picture_draw.add(write_lock->sk_surface()->getCanvas(),
On 2015/02/11 23:53:37, vmpstr wrote:
> Do we have to use a multi picture draw here? What's the reason (I don't know
> that much about it)

There's a bug in Ganesh layer hoisting without it :(.

We should be able to do this when that is fixed:
 picture->playback(write_lock->sk_surface()->getCanvas(), nullptr);

[vmiura, 2015-02-12 00:11:10]

New patchsets have been uploaded after l-g-t-m from
enne@chromium.org,jbauman@chromium.org,vmpstr@chromium.org

[vmiura, 2015-02-12 00:12:03]

https://codereview.chromium.org/916723002/diff/60001/cc/test/failure_output_s...
File cc/test/failure_output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/60001/cc/test/failure_output_s...
cc/test/failure_output_surface.cc:10: :
FakeOutputSurface(static_cast<ContextProvider*>(nullptr), is_delegating) {
On 2015/02/11 23:53:37, vmpstr wrote:
> Do you need a cast here? nullptr_t is supposed to be implicitly convertible to
> all raw pointers. 

FakeOutputSurface has two constructors that take a pointer and a bool, so the
compiler barfs at the ambiguity.

https://codereview.chromium.org/916723002/diff/60001/content/renderer/render_...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/916723002/diff/60001/content/renderer/render_...
content/renderer/render_widget.cc:1024: routing_id(), output_surface_id, NULL,
NULL, software_device.Pass(),
On 2015/02/11 23:53:37, vmpstr wrote:
> nullptr still, right?

Done.

[vmpstr, 2015-02-12 00:21:06]

On 2015/02/12 00:00:12, vmiura wrote:
>
https://codereview.chromium.org/916723002/diff/60001/cc/resources/gpu_rasteri...
> File cc/resources/gpu_rasterizer.cc (right):
> 
>
https://codereview.chromium.org/916723002/diff/60001/cc/resources/gpu_rasteri...
> cc/resources/gpu_rasterizer.cc:124:
> multi_picture_draw.add(write_lock->sk_surface()->getCanvas(),
> On 2015/02/11 23:53:37, vmpstr wrote:
> > Do we have to use a multi picture draw here? What's the reason (I don't know
> > that much about it)
> 
> There's a bug in Ganesh layer hoisting without it :(.
> 
> We should be able to do this when that is fixed:
>  picture->playback(write_lock->sk_surface()->getCanvas(), nullptr);

Ah, then maybe add like a TODO there, but feel free to leave it as is as well :)

[no sievers, 2015-02-12 00:45:04]

content/renderer lgtm

[vmiura, 2015-02-12 07:49:54]

New patchsets have been uploaded after l-g-t-m from sievers@chromium.org

[vmiura, 2015-02-12 23:31:34]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-12 23:32:42]

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

[commit-bot: I haz the power, 2015-02-13 01:23:09]

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

[commit-bot: I haz the power, 2015-02-13 01:23:11]

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

[vmiura, 2015-02-13 01:30:53]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-13 01:31:29]

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

[commit-bot: I haz the power, 2015-02-13 03:11:16]

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

[commit-bot: I haz the power, 2015-02-13 03:11:18]

Try jobs failed on following builders:
  android_rel_tests_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_rel_tes...)

[vmiura, 2015-02-13 03:16:42]

vmiura@chromium.org changed reviewers:
+ boliu@chromium.org, epenner@chromium.org

[vmiura, 2015-02-13 03:16:43]

I need someone to look at the newest SynchronousCompositor changes.

Looks like sievers is OOO.  Bo, Eric could you take a look?

[boliu, 2015-02-13 18:04:05]

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:84:
true /* share_resources */,
This is causing video context to fail (the call from
TryCreateStreamTextureFactory)

chromium: [FATAL:in_process_command_buffer.cc(347)] Check failed: !share_group
|| service_.get() == share_group->service_.get().

You could add a bool share_resources parameter to this I guess.

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:158:
g_factory.Get().CreateOnscreenContextProviderForCompositorThread();
Is it possible/safe to key this off of a command line switch? For webview,
creating and destroying context happens as normal part of operation, not just on
start up/tear down/error. Creating a context is pretty expensive, so it could
have perf impact.

Also just rename the method to CreateContextProviderForCompositor.

[vmiura, 2015-02-13 19:52:35]

PTAL

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:84:
true /* share_resources */,
On 2015/02/13 18:04:05, boliu wrote:
> This is causing video context to fail (the call from
> TryCreateStreamTextureFactory)
> 
> chromium: [FATAL:in_process_command_buffer.cc(347)] Check failed: !share_group
> || service_.get() == share_group->service_.get().
> 
> You could add a bool share_resources parameter to this I guess.

Done.

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:158:
g_factory.Get().CreateOnscreenContextProviderForCompositorThread();
On 2015/02/13 18:04:05, boliu wrote:
> Is it possible/safe to key this off of a command line switch? For webview,
> creating and destroying context happens as normal part of operation, not just
on
> start up/tear down/error. Creating a context is pretty expensive, so it could
> have perf impact.

We need it whenever GPU rasterization is enabled.  Is it safe to check something
like Content::IsGpuRasterizationEnabled() here?.

> Also just rename the method to CreateContextProviderForCompositor.

Done.

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

https://codereview.chromium.org/916723002/diff/240001/cc/resources/gpu_tile_t...
cc/resources/gpu_tile_task_worker_pool.cc:43: base::AutoLock
scoped_lock(*context_provider->GetLock());
jbauman's patch for ContextProvider::GetLock() landed, so I'm rolling this
safety lock here instead of as a separate patch.

[boliu, 2015-02-13 20:25:53]

A note about context loss handling in webview. Webview never handled context
loss, and would probalby just crash in that case, because it's not possible to
recover when android view's context gets lost. (Our crash rate is still a lot
lower than the old webview, so wasn't that bad.)

I think the right answer for webview there is
1) switch (renderer) compositor to use a gpu thread created and owned by chrome,
which is done very recently: crbug.com/448168
2) switch from InProcessCommandBuffer to GpuCommandBufferStub (not done yet)

Then webview should just get chrome's implementation of context loss handling
automatically.

So I wouldn't worry about context loss handling in webview unless you believe
this change could cause more frequent context loss.

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
File content/browser/android/in_process/synchronous_compositor_impl.cc (right):

https://codereview.chromium.org/916723002/diff/200001/content/browser/android...
content/browser/android/in_process/synchronous_compositor_impl.cc:158:
g_factory.Get().CreateOnscreenContextProviderForCompositorThread();
On 2015/02/13 19:52:34, vmiura wrote:
> On 2015/02/13 18:04:05, boliu wrote:
> > Is it possible/safe to key this off of a command line switch? For webview,
> > creating and destroying context happens as normal part of operation, not
just
> on
> > start up/tear down/error. Creating a context is pretty expensive, so it
could
> > have perf impact.
> 
> We need it whenever GPU rasterization is enabled.  Is it safe to check
something
> like Content::IsGpuRasterizationEnabled() here?.

Oh I thought the new context will only be used with
--enable-threaded-gpu-rasterization. If it's needed for ganesh in general, then
I don't think it's worth it.

I think the answer to IsGpuRasterizationEnabled is yes. It is safe since it's
just reading a bunch of command line switches.

> 
> > Also just rename the method to CreateContextProviderForCompositor.
> 
> Done.
>

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
cc/output/output_surface.cc:149: success =
worker_context_provider->BindToCurrentThread();
Was trying this in webview with a ganesh page, and failed in a thread check in
ContextProviderInProcess::ContextGL.

This method is called on the thread, so we can't bind the worker context here
and then use it on a worker thread.

But InitializeAndSetContext3d needs to synchronous return false if this fails.
I'm not sure if it's possible to block the compositor thread on worker threads?

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
cc/output/output_surface.cc:155:
worker_context_provider_->SetLostContextCallback(base::Bind(
Same threading problem on the context loss callback I think.

[vmiura, 2015-02-13 21:28:32]

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
File cc/output/output_surface.cc (right):

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
cc/output/output_surface.cc:149: success =
worker_context_provider->BindToCurrentThread();
On 2015/02/13 20:25:52, boliu wrote:
> Was trying this in webview with a ganesh page, and failed in a thread check in
> ContextProviderInProcess::ContextGL.
> 
> This method is called on the thread, so we can't bind the worker context here
> and then use it on a worker thread.
> 
> But InitializeAndSetContext3d needs to synchronous return false if this fails.
> I'm not sure if it's possible to block the compositor thread on worker
threads?

I think I need to implement ContextProviderInProcess::DetachFromThread(), as we
did for ContextProviderCommandBuffer::DetachFromThread().

https://codereview.chromium.org/916723002/diff/240001/cc/output/output_surfac...
cc/output/output_surface.cc:155:
worker_context_provider_->SetLostContextCallback(base::Bind(
On 2015/02/13 20:25:52, boliu wrote:
> Same threading problem on the context loss callback I think.

Same here.

We bind to the compositor thread here, but then GpuTileTaskWorkerPool
constructor calls ContextProvider::DetachFromThread() to take ownership, and
again in it's destructor to release ownership.

[vmiura, 2015-02-13 22:06:43]

Done, PTAL.

https://codereview.chromium.org/916723002/diff/260001/webkit/common/gpu/conte...
File webkit/common/gpu/context_provider_in_process.cc (right):

https://codereview.chromium.org/916723002/diff/260001/webkit/common/gpu/conte...
webkit/common/gpu/context_provider_in_process.cc:112: void
ContextProviderInProcess::DetachFromThread() {
Added this.

[danakj, 2015-02-13 22:11:20]

webkit/ LGTM

[boliu, 2015-02-13 22:17:45]

in_process and general webview bits lgtm

[vmiura, 2015-02-13 22:29:38]

The CQ bit was checked by vmiura@chromium.org

[commit-bot: I haz the power, 2015-02-13 22:30:59]

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

[commit-bot: I haz the power, 2015-02-14 00:01:40]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-02-14 00:02:28]

Patchset 14 (id:??) landed as
https://crrev.com/78b692864e7745993b8be343f67968ca071e0642
Cr-Commit-Position: refs/heads/master@{#316333}