ppapi: Send sync point whenever the pepper texture changes.

ppapi: Send sync point whenever the pepper texture changes.

When giving a texture mailbox to the compositor, it's important
to give a sync point along with it, so the browser compositor does
not display the texture before it is ready.

Currently pepper is giving the mailbox to the compositor once
(without a sync point) and then just telling it that the texture
changed, without specifying a sync point for those changes.

Change it to always send a sync point with the mailbox, and when
the texture changes, reset the mailbox with a new sync point for
each frame.

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

[danakj, 2014-03-03 22:33:00]

I can't upload base files, so the side by side doesn't work right now.

[brianderson, 2014-03-03 22:49:19]

https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:736: 
It's kind of hard to follow how mailbox_ gets set. Is it possible to to pass in
the mailbox to this method too?

Or maybe call context->GetBackingMailbox here and insert they syncpoint within
this method? That would make the syncpoint returned from
context->GetBackingMailbox useless, but I guess that's what's already happening
in a more round about way?

[danakj, 2014-03-03 22:53:53]

https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:736: 
On 2014/03/03 22:49:19, brianderson wrote:
> It's kind of hard to follow how mailbox_ gets set. Is it possible to to pass
in
> the mailbox to this method too?
> 
> Or maybe call context->GetBackingMailbox here and insert they syncpoint within
> this method? That would make the syncpoint returned from
> context->GetBackingMailbox useless, but I guess that's what's already
happening
> in a more round about way?

The sync point from GetBackingMailbox is not useless I think. It is the sync
point you should wait on for the mailbox originally given to the compositor. Not
waiting on it could mean the mailbox does not even exist yet (you'll get GL
errors).

The sync point here is after a swap occured, so slightly different. I could call
GetBackingMailbox() here, though if you have a texture_layer_, you will also
have a mailbox_ already since the layer is only created if you have a mailbox_.
That'd be a slightly more risky merge as it has more pointer dereffing also,
though would not require adding a mailbox_ member to this class anymore.

What do you think John?

[jbauman, 2014-03-03 23:01:16]

On 2014/03/03 22:53:53, danakj wrote:
>
https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
> 
>
https://codereview.chromium.org/185363009/diff/60001/content/renderer/pepper/...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:736: 
> On 2014/03/03 22:49:19, brianderson wrote:
> > It's kind of hard to follow how mailbox_ gets set. Is it possible to to pass
> in
> > the mailbox to this method too?
> > 
> > Or maybe call context->GetBackingMailbox here and insert they syncpoint
within
> > this method? That would make the syncpoint returned from
> > context->GetBackingMailbox useless, but I guess that's what's already
> happening
> > in a more round about way?
> 
> The sync point from GetBackingMailbox is not useless I think. It is the sync
> point you should wait on for the mailbox originally given to the compositor.
Not
> waiting on it could mean the mailbox does not even exist yet (you'll get GL
> errors).
> 
> The sync point here is after a swap occured, so slightly different. I could
call
> GetBackingMailbox() here, though if you have a texture_layer_, you will also
> have a mailbox_ already since the layer is only created if you have a
mailbox_.
> That'd be a slightly more risky merge as it has more pointer dereffing also,
> though would not require adding a mailbox_ member to this class anymore.
> 
> What do you think John?

I'd be a bit more comfortable if we updated the sync_point_ on the
PlatformContext3D, as then detaching and reattaching the Graphics3D from the
plugin instance would be guaranteed to give the right result. Then we could call
GetBackingMailbox in CommitBackingTexture.

[danakj, 2014-03-03 23:30:42]

PTAL

[danakj, 2014-03-03 23:31:41]

https://codereview.chromium.org/185363009/diff/180001/content/renderer/pepper...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/185363009/diff/180001/content/renderer/pepper...
content/renderer/pepper/ppb_graphics_3d_impl.cc:201: InsertSyncPoint());
Oops, this gotta go.

[danakj, 2014-03-03 23:33:15]

https://codereview.chromium.org/185363009/diff/180001/content/renderer/pepper...
File content/renderer/pepper/ppb_graphics_3d_impl.cc (right):

https://codereview.chromium.org/185363009/diff/180001/content/renderer/pepper...
content/renderer/pepper/ppb_graphics_3d_impl.cc:201: InsertSyncPoint());
On 2014/03/03 23:31:42, danakj wrote:
> Oops, this gotta go.

Fixed.

[brianderson, 2014-03-03 23:36:17]

This version is much easier for me to understand. Thanks John and Dana. I'm not
an owner for any of these files, but lgtm!

[jbauman, 2014-03-03 23:38:34]

lgtm

[danakj, 2014-03-03 23:41:44]

+dmichael: OWNERS review please

[dmichael (off chromium), 2014-03-04 16:49:07]

lgtm w/nit

https://codereview.chromium.org/185363009/diff/150002/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/185363009/diff/150002/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1940:
DCHECK_EQ(!mailbox.IsZero(), sync_point != 0);
nit:
So you're asserting that either:
the mailbox and the sync_point are *both* valid
or
the mailbox and the sync_point are both zero?
...is that the intent?

I think it would be clearer to either just do those cases explicitly:
DCHECK((mailbox.IsZero() && sync_point == 0) ||
       (!mailbox.IsZero() && sync_point != 0));
or at least remove the negations so there's less to think about:
DCHECK_EQ(mailbox.IsZero(), sync_point == 0);

[danakj, 2014-03-04 16:55:09]

https://codereview.chromium.org/185363009/diff/150002/content/renderer/pepper...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/185363009/diff/150002/content/renderer/pepper...
content/renderer/pepper/pepper_plugin_instance_impl.cc:1940:
DCHECK_EQ(!mailbox.IsZero(), sync_point != 0);
On 2014/03/04 16:49:08, dmichael wrote:
> nit:
> So you're asserting that either:
> the mailbox and the sync_point are *both* valid
> or
> the mailbox and the sync_point are both zero?
> ...is that the intent?

Yep.

> I think it would be clearer to either just do those cases explicitly:
> DCHECK((mailbox.IsZero() && sync_point == 0) ||
>        (!mailbox.IsZero() && sync_point != 0));
> or at least remove the negations so there's less to think about:
> DCHECK_EQ(mailbox.IsZero(), sync_point == 0);

Ok!

[danakj, 2014-03-04 16:55:59]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-04 16:56:44]

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

[commit-bot: I haz the power, 2014-03-04 18:05:17]

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

[commit-bot: I haz the power, 2014-03-04 18:05:18]

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

[danakj, 2014-03-04 18:11:57]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:13:58]

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

[commit-bot: I haz the power, 2014-03-04 21:59:53]

Change committed as 254840