Add signalSyncPoint to the WebGraphicsContext3D command buffer impls.

Add signalSyncPoint to the WebGraphicsContext3D command buffer impls.

Adds signalSyncPoint() support to our command buffer implementations
of WebGraphicsContext3D. Because we have to use a raw callback pointer
for WebGraphicsContext3D and this is not great, I've added a
SyncPointHelper class that adapts a base::Closure() for use with the
signalSyncPoint() method.

Tests:
GLRendererPixelTest.SignalSyncPoint
GLRendererPixelTest.SignalSyncPointOnLostContext

R=jamesr, piman
BUG=179896
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196930

[danakj, 2013-04-26 00:52:52]

Oh, sorted cc.gyp while I was there.

[piman, 2013-04-26 01:20:52]

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
File cc/resources/sync_point_helper.cc (right):

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
cc/resources/sync_point_helper.cc:14: SignalSyncPointCallbackClass(const
base::Closure& closure)
nit: explicit

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
cc/resources/sync_point_helper.cc:15: : closure_(closure) {}
nit: indent (':' at +4)

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
File cc/resources/sync_point_helper.h (right):

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
cc/resources/sync_point_helper.h:25: // the |context3d| is destroyed.
Could we make WGC3D::signalSyncPoint take ownership of it instead (sticking it
into the SignalSyncPointCallback)? That way there's no way to get it wrong.

[danakj, 2013-04-26 01:39:29]

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
File cc/resources/sync_point_helper.h (right):

https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_he...
cc/resources/sync_point_helper.h:25: // the |context3d| is destroyed.
On 2013/04/26 01:20:52, piman wrote:
> Could we make WGC3D::signalSyncPoint take ownership of it instead (sticking it
> into the SignalSyncPointCallback)? That way there's no way to get it wrong.

That was my original plan but if the context is destroyed and never calls it
then you like the object instance and that seems crappy. To avoid that wed need
the context to call all pending signals when its destroyed, I think.

[danakj1, 2013-04-26 01:40:23]

s/like/leak/
On Apr 25, 2013 9:39 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>
resources/sync_point_helper.h<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h>
> File cc/resources/sync_point_**helper.h (right):
>
> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>
resources/sync_point_helper.h#**newcode25<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h#newcode25>
> cc/resources/sync_point_**helper.h:25: // the |context3d| is destroyed.
> On 2013/04/26 01:20:52, piman wrote:
>
>> Could we make WGC3D::signalSyncPoint take ownership of it instead
>>
> (sticking it
>
>> into the SignalSyncPointCallback)? That way there's no way to get it
>>
> wrong.
>
> That was my original plan but if the context is destroyed and never
> calls it then you like the object instance and that seems crappy. To
> avoid that wed need the context to call all pending signals when its
> destroyed, I think.
>
>
https://codereview.chromium.**org/14126014/<https://codereview.chromium.org/1...
>

[piman, 2013-04-26 03:04:14]

On Thu, Apr 25, 2013 at 6:39 PM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>
resources/sync_point_helper.h<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h>
> File cc/resources/sync_point_**helper.h (right):
>
> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>
resources/sync_point_helper.h#**newcode25<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h#newcode25>
> cc/resources/sync_point_**helper.h:25: // the |context3d| is destroyed.
> On 2013/04/26 01:20:52, piman wrote:
>
>> Could we make WGC3D::signalSyncPoint take ownership of it instead
>>
> (sticking it
>
>> into the SignalSyncPointCallback)? That way there's no way to get it
>>
> wrong.
>
> That was my original plan but if the context is destroyed and never
> calls it then you like the object instance and that seems crappy. To
> avoid that wed need the context to call all pending signals when its
> destroyed, I think.
>

CommandBufferProxyImpl keeps ownership of the callbacks.
If SignalSyncPointCallback keeps a scoped_ptr<SyncPointHelper> as a
parameter, the ownership is transferred to the Callback. The Callback will
release the SyncPointHelper either after it sent it
to SignalSyncPointCallback or when the Callback is destroyed (without
running) when CommandBufferProxyImpl is destroyed, so no leak.


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

[danakj, 2013-04-26 13:15:33]

On Thu, Apr 25, 2013 at 11:03 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Thu, Apr 25, 2013 at 6:39 PM, <danakj@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>>
resources/sync_point_helper.h<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h>
>> File cc/resources/sync_point_**helper.h (right):
>>
>> https://codereview.chromium.**org/14126014/diff/6001/cc/**
>>
resources/sync_point_helper.h#**newcode25<https://codereview.chromium.org/14126014/diff/6001/cc/resources/sync_point_helper.h#newcode25>
>> cc/resources/sync_point_**helper.h:25: // the |context3d| is destroyed.
>> On 2013/04/26 01:20:52, piman wrote:
>>
>>> Could we make WGC3D::signalSyncPoint take ownership of it instead
>>>
>> (sticking it
>>
>>> into the SignalSyncPointCallback)? That way there's no way to get it
>>>
>> wrong.
>>
>> That was my original plan but if the context is destroyed and never
>> calls it then you like the object instance and that seems crappy. To
>> avoid that wed need the context to call all pending signals when its
>> destroyed, I think.
>>
>
> CommandBufferProxyImpl keeps ownership of the callbacks.
> If SignalSyncPointCallback keeps a scoped_ptr<SyncPointHelper> as a
> parameter, the ownership is transferred to the Callback. The Callback will
> release the SyncPointHelper either after it sent it
> to SignalSyncPointCallback or when the Callback is destroyed (without
> running) when CommandBufferProxyImpl is destroyed, so no leak.
>

We can't give a callback to CommandBufferProxyImpl directly, because it has
the WGC3D api, which takes a WGSignalSyncPointCallback*, not a
base::Callback :(

However, I thought on this some more and it's no good because if the
gl_renderer goes away and destroys the callback helper before the WGC3D is
destroyed/lost, we'll crash, so compositor shutdown could be racey. I'll
rearrange this a bit, I think I was going toward the right solution
yesterday and then got sidetracked.


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

[danakj, 2013-04-26 14:04:51]

PTAL.

Now the callback is destroyed when the WGC3D calls it. We keep around a WeakPtr
so we can still clean it up if we lose/destroy the WGC3D.

(Can't wait till we can get rid of the WGC3D in the middle here.)

[piman, 2013-04-26 15:55:55]

https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1629:
WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback* callback) {
I don't understand... Why can't we replace
WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback* callback by
scoped_ptr<WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback> here?

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

https://codereview.chromium.org/14126014/diff/20003/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1804:
callback->onSyncPointReached();
And delete callback here (well, regardless of whether the context was lost)

[danakj, 2013-04-26 15:59:06]

https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client...
File content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc
(right):

https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client...
content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc:1629:
WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback* callback) {
On 2013/04/26 15:55:55, piman wrote:
> I don't understand... Why can't we replace
> WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback* callback by
> scoped_ptr<WebKit::WebGraphicsContext3D::WebGraphicsSyncPointCallback> here?

Oh hm..

I guess the WGC3DCBI can store a list of the signal callbacks and make sure they
get freed when it goes away.

[piman, 2013-04-26 16:02:21]

On Fri, Apr 26, 2013 at 8:59 AM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.**org/14126014/diff/20003/**
>
content/common/gpu/client/**webgraphicscontext3d_command_**buffer_impl.cc<https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc>
> File
> content/common/gpu/client/**webgraphicscontext3d_command_**buffer_impl.cc
> (right):
>
> https://codereview.chromium.**org/14126014/diff/20003/**
> content/common/gpu/client/**webgraphicscontext3d_command_**
>
buffer_impl.cc#newcode1629<https://codereview.chromium.org/14126014/diff/20003/content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.cc#newcode1629>
> content/common/gpu/client/**webgraphicscontext3d_command_**
> buffer_impl.cc:1629:
> WebKit::WebGraphicsContext3D::**WebGraphicsSyncPointCallback* callback) {
> On 2013/04/26 15:55:55, piman wrote:
>
>> I don't understand... Why can't we replace
>> WebKit::WebGraphicsContext3D::**WebGraphicsSyncPointCallback* callback
>>
> by
>
>> scoped_ptr<WebKit::**WebGraphicsContext3D::**
>> WebGraphicsSyncPointCallback>
>>
> here?
>
> Oh hm..
>
> I guess the WGC3DCBI can store a list of the signal callbacks and make
> sure they get freed when it goes away.
>

What for? command_buffer_ (a CommandBufferProxyImpl) will do that for you.

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

[danakj, 2013-04-26 16:45:45]

Oh! I see what you mean, pass ownership of the raw pointer to the WGC3D. Clever!

Done, tho it depends on https://codereview.chromium.org/13935015/ to make this
change work.

[piman, 2013-04-26 17:11:07]

LGTM, yes, that's what I meant!

[danakj, 2013-04-26 18:57:08]

james please have a look at the webkit/ stuff.

[danakj, 2013-04-26 19:18:42]

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

https://codereview.chromium.org/14126014/diff/19011/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1810:
gl_->LoseContextCHROMIUM(current, other);
Hm, after doing this, trying to create and use a new WGC3DIPCBI ends up causing
GPU PARSE ERROR: 4 (kInvalidArguments).

[piman, 2013-04-26 21:15:01]

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

https://codereview.chromium.org/14126014/diff/19011/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1810:
gl_->LoseContextCHROMIUM(current, other);
On 2013/04/26 19:18:42, danakj wrote:
> Hm, after doing this, trying to create and use a new WGC3DIPCBI ends up
causing
> GPU PARSE ERROR: 4 (kInvalidArguments).

Do you know which call/in which conditions?

[danakj, 2013-04-26 21:23:50]

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

https://codereview.chromium.org/14126014/diff/19011/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:1810:
gl_->LoseContextCHROMIUM(current, other);
On 2013/04/26 21:15:01, piman wrote:
> On 2013/04/26 19:18:42, danakj wrote:
> > Hm, after doing this, trying to create and use a new WGC3DIPCBI ends up
> causing
> > GPU PARSE ERROR: 4 (kInvalidArguments).
> 
> Do you know which call/in which conditions?

CreateFramebuffer(), it was because losing the context didnt lose all the
contexts in the share group. So a new context would reuse a lost decoder. I
fixed this in the latest CL. Working on the ordering/callback for the signal
now.

[danakj, 2013-04-26 22:51:56]

This all seems to be working now! PTAL james

[jamesr, 2013-04-26 22:57:30]

lgtm

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

https://codereview.chromium.org/14126014/diff/35002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:310: using
WebKit::WebGraphicsContext3D;
do you need this using statement now?

[danakj, 2013-04-27 00:05:16]

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

https://codereview.chromium.org/14126014/diff/35002/webkit/gpu/webgraphicscon...
webkit/gpu/webgraphicscontext3d_in_process_command_buffer_impl.cc:310: using
WebKit::WebGraphicsContext3D;
On 2013/04/26 22:57:30, jamesr wrote:
> do you need this using statement now?

Nope! thanks.

[commit-bot: I haz the power, 2013-04-27 00:06:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14126014/44001

[commit-bot: I haz the power, 2013-04-27 00:07:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14126014/44001

[commit-bot: I haz the power, 2013-04-27 00:16:27]

Retried try job too often on linux_chromeos for step(s) ash_unittests,
aura_unittests, base_unittests, browser_tests, cacheinvalidation_unittests,
check_deps, chromeos_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, dbus_unittests, device_unittests,
gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, net_unittests, ppapi_unittests, printing_unittests,
sandbox_linux_unittests, sql_unittests, sync_unit_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-04-27 00:26:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/14126014/44001

[commit-bot: I haz the power, 2013-04-27 03:24:51]

Change committed as 196930