Add option to use GLContextVirtual in WGC3DIPCBI

Add option to use GLContextVirtual in WGC3DIPCBI

BUG=230195
win_x64_rel busted. All other bots passed
NOTRY=true

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

[boliu, 2013-04-19 16:05:26]

Another change to WGC3DIPCBI for webview. PTAL

I made a mistake in patch set 3 that turned on GlContextVirtual for offscreen,
and it did not break layout tests on linux. Perhaps we don't need the option at
all?

[no sievers, 2013-04-19 18:38:40]

Cool, this will help with some of the state persistency problems across draw
calls. lgtm.

https://codereview.chromium.org/14318004/diff/4004/webkit/gpu/webgraphicscont...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/4004/webkit/gpu/webgraphicscont...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:497:
DCHECK(context_->GetHandle());
Won't this DCHECK() fail for WebView (where we never have a context)?
It's probably fine to not have that check...

[no sievers, 2013-04-19 18:39:45]

https://codereview.chromium.org/14318004/diff/4004/webkit/gpu/webgraphicscont...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/4004/webkit/gpu/webgraphicscont...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:501: if
(context_->Initialize(surface_, gpu_preference)) {
Oh, and you could fall back to the else-path in here, if the initialization
failed.
That would be the case on some platform where not all surfaces are compatible
with the shared context.

[boliu, 2013-04-19 19:08:05]

Addressed both comments. Also renamed vars to
use_virtualized_gl_context_if_possible to indicate that there is a fallback.

[jamesr, 2013-04-19 19:50:49]

lgtm

https://codereview.chromium.org/14318004/diff/15001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/15001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:500: LOG(INFO)
<< "Created virtual GL context.";
I think this is a bit too spammy - could you make it a VLOG instead?

[boliu, 2013-04-19 19:58:24]

https://codereview.chromium.org/14318004/diff/15001/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/15001/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:500: LOG(INFO)
<< "Created virtual GL context.";
On 2013/04/19 19:50:49, jamesr wrote:
> I think this is a bit too spammy - could you make it a VLOG instead?

Changed to VLOG(1)

[commit-bot: I haz the power, 2013-04-19 19:58:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14318004/16001

[commit-bot: I haz the power, 2013-04-19 20:58:09]

Retried try job too often on mac_rel for step(s) base_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-19 21:51:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14318004/16001

[boliu, 2013-04-19 22:45:08]

+piman for change in ui/compositor to fix compile

[piman, 2013-04-19 22:51:00]

https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:630:
attributes, true, gfx::kNullAcceleratedWidget, false);
Why false here?

[boliu, 2013-04-19 22:53:22]

https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc (right):

https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:630:
attributes, true, gfx::kNullAcceleratedWidget, false);
On 2013/04/19 22:51:00, piman wrote:
> Why false here?

Retaining old behavior to not use GLVirtualContext

[piman, 2013-04-19 22:55:22]

On 2013/04/19 22:53:22, boliu wrote:
>
https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
> File webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc
(right):
> 
>
https://codereview.chromium.org/14318004/diff/24002/webkit/gpu/webgraphicscon...
> webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:630:
> attributes, true, gfx::kNullAcceleratedWidget, false);
> On 2013/04/19 22:51:00, piman wrote:
> > Why false here?
> 
> Retaining old behavior to not use GLVirtualContext

But why not expose the option, and let the embedder decide - if it has to be an
option in one, why not the other?
Also, what's the point of enabling virtual contexts on only half of them? I'm
confused.

[boliu, 2013-04-19 22:59:17]

On 2013/04/19 22:55:22, piman wrote:
> But why not expose the option, and let the embedder decide - if it has to be
an
> option in one, why not the other?
> Also, what's the point of enabling virtual contexts on only half of them? I'm
> confused.

Arrr...because this used to be only used by tests and I thought they wouldn't be
interested in using virtual context, so to minimize scope of this patch.

Will add it and fix all the callsites. The value should be false in
ui/compositor, right?

[piman, 2013-04-19 23:03:32]

On Fri, Apr 19, 2013 at 3:59 PM, <boliu@chromium.org> wrote:

> On 2013/04/19 22:55:22, piman wrote:
>
>> But why not expose the option, and let the embedder decide - if it has to
>> be
>>
> an
>
>> option in one, why not the other?
>> Also, what's the point of enabling virtual contexts on only half of them?
>> I'm
>> confused.
>>
>
> Arrr...because this used to be only used by tests and I thought they
> wouldn't be
> interested in using virtual context, so to minimize scope of this patch.
>

Offscreen contexts are used for at least skia, video decoding and webgl,
which, I think, are in scope for android webview.
I'm not even sure what it means to have a context with virtual contexts in
the same share group as one without.

Will add it and fix all the callsites. The value should be false in
> ui/compositor, right?
>

Yes for now.


>
https://codereview.chromium.**org/14318004/<https://codereview.chromium.org/1...
>

[boliu, 2013-04-19 23:19:29]

Done.

James, you are owner of all the other callsites, might want to look this over
again. It's just adding false everywhere.

On 2013/04/19 23:03:32, piman wrote:
> Offscreen contexts are used for at least skia, video decoding and webgl,
> which, I think, are in scope for android webview.
> I'm not even sure what it means to have a context with virtual contexts in
> the same share group as one without.

I'm not quite sure what the plan is to implement webgl/video/canvas for merged
thread architecture, but I've been told they won't work as is...

[jamesr, 2013-04-19 23:27:50]

Hmmmmm, on further thought I think this needs more work.  Contexts in the same
share group have to either all be virtual or all be non-virtual.  Android
webview will need offscreen contexts to work for at least canvas 2d, so
offscreen contexts do need to be set up the same way as the compositor's
context.

[no sievers, 2013-04-20 00:07:16]

On 2013/04/19 23:27:50, jamesr wrote:
> Hmmmmm, on further thought I think this needs more work.  Contexts in the same
> share group have to either all be virtual or all be non-virtual.  Android
> webview will need offscreen contexts to work for at least canvas 2d, so
> offscreen contexts do need to be set up the same way as the compositor's
> context.

Please no WGC3DInProcessCB offscreen contexts for Android WebView :)

All main thread contexts incl. canvas should be normal cmdbuffer clients
(WGC3DCBI).

[boliu, 2013-04-20 00:18:59]

Use a static EnableVirtualizedContext method instead to set a global static
bool. All thread safety is based on the assumption that it is supposed to be set
before any contexts are created. Thoughts?

Also do not fallback to normal context, since it doesn't make sense anymore.

[piman, 2013-04-20 00:24:16]

LGTM. Why do we need this if not for Android WebView then?

[no sievers, 2013-04-20 00:29:12]

On 2013/04/20 00:24:16, piman wrote:
> LGTM. Why do we need this if not for Android WebView then?

We just want the state restore logic, because for WebView we draw inside a
UI-driven functor, with a UI framework context already current.
So we want to make sure our compositor has a consistent state across Draw calls.

[boliu, 2013-04-20 00:29:46]

On 2013/04/20 00:24:16, piman wrote:
> LGTM. Why do we need this if not for Android WebView then?

This *is* for android webview. It will be used on the ui, ie renderer
compositor, thread to do most draws.

I think the plan is to run another instance of command buffer server on the gpu
thread for canvas/webgl/etc, talking to renderer through ipc. Somehow we will
need to get the two command buffer servers to talk to each other. This is the
part I'm not clear about...

[no sievers, 2013-04-20 00:31:42]

On 2013/04/20 00:29:46, boliu wrote:
> On 2013/04/20 00:24:16, piman wrote:
> > LGTM. Why do we need this if not for Android WebView then?
> 
> This *is* for android webview. It will be used on the ui, ie renderer
> compositor, thread to do most draws.
> 
> I think the plan is to run another instance of command buffer server on the
gpu
> thread for canvas/webgl/etc, talking to renderer through ipc. Somehow we will
> need to get the two command buffer servers to talk to each other. This is the
> part I'm not clear about...

No the service-side contexts for the two different GL threads can be
independent. We'll figure out how to share resources (SurfaceTexture, gralloc,
EGLimage come to mind).

[commit-bot: I haz the power, 2013-04-20 00:46:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14318004/31001

[jamesr, 2013-04-20 01:11:25]

What about offscreen compositor thread contexts?  Do you want those to be normal
cmd buffer contexts as well?

[boliu, 2013-04-20 01:21:18]

On 2013/04/20 01:11:25, jamesr wrote:
> What about offscreen compositor thread contexts?  Do you want those to be
normal
> cmd buffer contexts as well?

This is the one from RenderThreadImpl::CreateOffscreenContext3d, right? I don't
know the answer, but short term it needs to be in process version before this
share resource code is implemented.

What is this context used for? I didn't override this one to be the in-process
version in my big hack patch, and I wonder how many things are broken because of
it...

[jamesr, 2013-04-20 01:24:55]

It's used for applying filters in the compositor thread.

[commit-bot: I haz the power, 2013-04-20 02:23:14]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-20 17:38:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/boliu@chromium.org/14318004/31001

[commit-bot: I haz the power, 2013-04-20 17:39:12]

Change committed as 195398