Draw the offscreen texture to reflector's surface without extra copy

Draw the offscreen texture to reflector's surface without extra copy

This CL also refactor Reflector/BrowserCompositorOutputSurface as follows.

* BrowserCompositorOutputSurface ('s subclass) is responsible for creating Texture mailbox for mirroring
 - when reflector is set
 - or texture size is changed
* OutputSurface notifies reflector when mailbox is updated.

BUG=365662
Committed: https://crrev.com/592eb864e7c5d1c89d56756e98161f4f809b2669
Cr-Commit-Position: refs/heads/master@{#330042}

[oshima, 2015-05-05 23:07:46]

Patchset #1 (id:1) has been deleted

[oshima, 2015-05-05 23:07:53]

Patchset #1 (id:20001) has been deleted

[oshima, 2015-05-05 23:08:00]

Patchset #1 (id:40001) has been deleted

[oshima, 2015-05-05 23:08:07]

Patchset #1 (id:60001) has been deleted

[oshima, 2015-05-06 00:46:23]

Patchset #1 (id:80001) has been deleted

[oshima, 2015-05-06 00:46:27]

Patchset #1 (id:100001) has been deleted

[oshima, 2015-05-06 00:49:24]

oshima@chromium.org changed reviewers:
+ piman@chromium.org

[oshima, 2015-05-06 00:49:32]

Patchset #1 (id:120001) has been deleted

[oshima, 2015-05-06 00:52:58]

Patchset #1 (id:140001) has been deleted

[oshima, 2015-05-06 00:53:03]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-06 00:53:23]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123763003/160001

[oshima, 2015-05-06 01:03:35]

https://codereview.chromium.org/1123763003/diff/160001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/160001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:131:
offline_compositor_gl_helper_->InsertOrderingBarrier();
I'm not sure if this is necessary. It seems to be working without this, but
added as reflector has it for mirror mode. Please let me know if I should remove
this.

[piman, 2015-05-07 01:56:45]

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:60:
mailbox_ = new OwnedMailbox(shared_helper);
Why not creating mailbox_ with the offline_compositor_gl_helper_? That way you
don't need to ConsumeMailboxToTexture it - you can simply use
mailbox_->texture_id(). (but then you want to release mailbox_ in
DiscardBackbuffer)


I wonder if it might be possible to converge both paths. Especially now that the
thread is gone, it may be easier to always have the
BrowserCompositorOutputSurface produce the mailbox (i.e. in the old path, do the
readback), and then the Reflector just manages the interaction with the
mirroring layers, forwarding the mailbox and damage. What do you think?

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:121:
backing_texture_id_, 0);
I just realized this, you end up calling this every frame, and it's somewhat
expensive. In truth, you can just create the fbo_ together with the texture, it
would simplify.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:131:
offline_compositor_gl_helper_->InsertOrderingBarrier();
The InsertOrderingBarrier is needed to order between 2 contexts. Here we just
used the output surface's context. In OnSourceSwapBuffers, it's also the same
context that we use, so I don't think the InsertOrderingBarrier is needed here.
It is needed in OnSourceSwapBuffers to order wrt the mirroring compositor's
context.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:145:
EnsureBackbuffer();
Is this needed? The compositor is not allowed to draw / call SwapBuffers if it
hasn't called EnsureBackbuffer first. Then maybe you can just call
OnSourceTextureMailboxUpdated directly from EnsureBackbuffer, and not need
needs_update_reflector_.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/reflector_impl.cc:123:
layer->GetCompositor()->AddObserver(this);
Is the layer guaranteed to be in the tree at that point? Can you add a DCHECK?
Is it guaranteed it's not removed from the tree until RemoveMirroringLayer?

[oshima, 2015-05-07 16:43:25]

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:60:
mailbox_ = new OwnedMailbox(shared_helper);
On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> Why not creating mailbox_ with the offline_compositor_gl_helper_? That way you
> don't need to ConsumeMailboxToTexture it - you can simply use
> mailbox_->texture_id(). (but then you want to release mailbox_ in
> DiscardBackbuffer)
> 
> 
> I wonder if it might be possible to converge both paths. Especially now that
the
> thread is gone, it may be easier to always have the
> BrowserCompositorOutputSurface produce the mailbox (i.e. in the old path, do
the
> readback), and then the Reflector just manages the interaction with the
> mirroring layers, forwarding the mailbox and damage. What do you think?

ok, let me rework on it.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:121:
backing_texture_id_, 0);
On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> I just realized this, you end up calling this every frame, and it's somewhat
> expensive. In truth, you can just create the fbo_ together with the texture,
it
> would simplify.

thanks, will do.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:145:
EnsureBackbuffer();
On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> Is this needed? The compositor is not allowed to draw / call SwapBuffers if it
> hasn't called EnsureBackbuffer first. Then maybe you can just call
> OnSourceTextureMailboxUpdated directly from EnsureBackbuffer, and not need
> needs_update_reflector_.

I need GetTextureMailbox call so that I can get the mailbox in
OnSourceOutputSurfaceReady. I'll rework to consolidate two path and hopefully we
can clean this up.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/reflector_impl.cc:123:
layer->GetCompositor()->AddObserver(this);
On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> Is the layer guaranteed to be in the tree at that point? Can you add a DCHECK?
> Is it guaranteed it's not removed from the tree until RemoveMirroringLayer?

Yes, it's supposed to be in the tree. I'll add DCHECKs and comments.

[piman, 2015-05-07 22:43:21]

On Thu, May 7, 2015 at 9:43 AM, <oshima@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> File
> content/browser/compositor/offscreen_browser_compositor_output_surface.cc
> (right):
>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
>
> content/browser/compositor/offscreen_browser_compositor_output_surface.cc:60:
> mailbox_ = new OwnedMailbox(shared_helper);
> On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
>
>> Why not creating mailbox_ with the offline_compositor_gl_helper_? That
>>
> way you
>
>> don't need to ConsumeMailboxToTexture it - you can simply use
>> mailbox_->texture_id(). (but then you want to release mailbox_ in
>> DiscardBackbuffer)
>>
>
>
>  I wonder if it might be possible to converge both paths. Especially
>>
> now that the
>
>> thread is gone, it may be easier to always have the
>> BrowserCompositorOutputSurface produce the mailbox (i.e. in the old
>>
> path, do the
>
>> readback), and then the Reflector just manages the interaction with
>>
> the
>
>> mirroring layers, forwarding the mailbox and damage. What do you
>>
> think?
>
> ok, let me rework on it.
>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
>
>
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:121:
> backing_texture_id_, 0);
> On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
>
>> I just realized this, you end up calling this every frame, and it's
>>
> somewhat
>
>> expensive. In truth, you can just create the fbo_ together with the
>>
> texture, it
>
>> would simplify.
>>
>
> thanks, will do.
>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
>
>
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:145:
> EnsureBackbuffer();
> On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
>
>> Is this needed? The compositor is not allowed to draw / call
>>
> SwapBuffers if it
>
>> hasn't called EnsureBackbuffer first. Then maybe you can just call
>> OnSourceTextureMailboxUpdated directly from EnsureBackbuffer, and not
>>
> need
>
>> needs_update_reflector_.
>>
>
> I need GetTextureMailbox call so that I can get the mailbox in
> OnSourceOutputSurfaceReady. I'll rework to consolidate two path and
> hopefully we can clean this up.


Why though?
If we don't have a mailbox yet, we can leave the Layer in a default state
(solid color).


>
>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> File content/browser/compositor/reflector_impl.cc (right):
>
>
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> content/browser/compositor/reflector_impl.cc:123:
> layer->GetCompositor()->AddObserver(this);
> On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
>
>> Is the layer guaranteed to be in the tree at that point? Can you add a
>>
> DCHECK?
>
>> Is it guaranteed it's not removed from the tree until
>>
> RemoveMirroringLayer?
>
> Yes, it's supposed to be in the tree. I'll add DCHECKs and comments.
>
> https://codereview.chromium.org/1123763003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-05-07 23:25:53]

On 2015/05/07 22:43:21, piman (Very slow to review) wrote:
> On Thu, May 7, 2015 at 9:43 AM, <mailto:oshima@chromium.org> wrote:
> 
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> > File
> > content/browser/compositor/offscreen_browser_compositor_output_surface.cc
> > (right):
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> >
> >
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:60:
> > mailbox_ = new OwnedMailbox(shared_helper);
> > On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> >
> >> Why not creating mailbox_ with the offline_compositor_gl_helper_? That
> >>
> > way you
> >
> >> don't need to ConsumeMailboxToTexture it - you can simply use
> >> mailbox_->texture_id(). (but then you want to release mailbox_ in
> >> DiscardBackbuffer)
> >>
> >
> >
> >  I wonder if it might be possible to converge both paths. Especially
> >>
> > now that the
> >
> >> thread is gone, it may be easier to always have the
> >> BrowserCompositorOutputSurface produce the mailbox (i.e. in the old
> >>
> > path, do the
> >
> >> readback), and then the Reflector just manages the interaction with
> >>
> > the
> >
> >> mirroring layers, forwarding the mailbox and damage. What do you
> >>
> > think?
> >
> > ok, let me rework on it.
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> >
> >
> content/browser/compositor/offscreen_browser_compositor_output_surface.cc:121:
> > backing_texture_id_, 0);
> > On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> >
> >> I just realized this, you end up calling this every frame, and it's
> >>
> > somewhat
> >
> >> expensive. In truth, you can just create the fbo_ together with the
> >>
> > texture, it
> >
> >> would simplify.
> >>
> >
> > thanks, will do.
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> >
> >
> content/browser/compositor/offscreen_browser_compositor_output_surface.cc:145:
> > EnsureBackbuffer();
> > On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> >
> >> Is this needed? The compositor is not allowed to draw / call
> >>
> > SwapBuffers if it
> >
> >> hasn't called EnsureBackbuffer first. Then maybe you can just call
> >> OnSourceTextureMailboxUpdated directly from EnsureBackbuffer, and not
> >>
> > need
> >
> >> needs_update_reflector_.
> >>
> >
> > I need GetTextureMailbox call so that I can get the mailbox in
> > OnSourceOutputSurfaceReady. I'll rework to consolidate two path and
> > hopefully we can clean this up.
> 
> 
> Why though?
> If we don't have a mailbox yet, we can leave the Layer in a default state
> (solid color).

Because it creates a mailbox for mirroring otherwise. In anycase,
new CL won't have this problem.

> 
> 
> >
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> > File content/browser/compositor/reflector_impl.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
> > content/browser/compositor/reflector_impl.cc:123:
> > layer->GetCompositor()->AddObserver(this);
> > On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> >
> >> Is the layer guaranteed to be in the tree at that point? Can you add a
> >>
> > DCHECK?
> >
> >> Is it guaranteed it's not removed from the tree until
> >>
> > RemoveMirroringLayer?
> >
> > Yes, it's supposed to be in the tree. I'll add DCHECKs and comments.
> >
> > https://codereview.chromium.org/1123763003/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[oshima, 2015-05-13 07:47:04]

Uploaded new patch, and updated the description. PTAL.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
File content/browser/compositor/offscreen_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:60:
mailbox_ = new OwnedMailbox(shared_helper);
On 2015/05/07 16:43:24, oshima wrote:
> On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> > Why not creating mailbox_ with the offline_compositor_gl_helper_? That way
you
> > don't need to ConsumeMailboxToTexture it - you can simply use
> > mailbox_->texture_id(). (but then you want to release mailbox_ in
> > DiscardBackbuffer)
> > 
> > 
> > I wonder if it might be possible to converge both paths. Especially now that
> the
> > thread is gone, it may be easier to always have the
> > BrowserCompositorOutputSurface produce the mailbox (i.e. in the old path, do
> the
> > readback), and then the Reflector just manages the interaction with the
> > mirroring layers, forwarding the mailbox and damage. What do you think?
> 
> ok, let me rework on it.

Looks like I still need to use shared helper because mailbox needs to be alive
until
the release callback is called, which is set in Layer::SetTextureMailbox.

I confirmed this with danakj@.

https://codereview.chromium.org/1123763003/diff/200001/content/browser/compos...
content/browser/compositor/offscreen_browser_compositor_output_surface.cc:131:
offline_compositor_gl_helper_->InsertOrderingBarrier();
On 2015/05/07 01:56:45, piman (Very slow to review) wrote:
> The InsertOrderingBarrier is needed to order between 2 contexts. Here we just
> used the output surface's context. In OnSourceSwapBuffers, it's also the same
> context that we use, so I don't think the InsertOrderingBarrier is needed
here.
> It is needed in OnSourceSwapBuffers to order wrt the mirroring compositor's
> context.

Removed

[piman, 2015-05-13 19:46:23]

I like this, LGTM

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:92: //
mailbox being held by the |mirroring_layer_|.
can you add a DCHECK(!mirrored_compositor_gl_helper_texture_id_)?

I'm assuming we only transition from !reflector_ to !!reflector_ and vice versa.
Otherwise we leak the texture.

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
content/browser/compositor/reflector_impl.cc:26: //
mirrored_compositor_gl_helper_texture_id_(0),
nit: remove

[oshima, 2015-05-14 14:44:58]

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
File content/browser/compositor/gpu_browser_compositor_output_surface.cc
(right):

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
content/browser/compositor/gpu_browser_compositor_output_surface.cc:92: //
mailbox being held by the |mirroring_layer_|.
On 2015/05/13 19:46:23, piman (Very slow to review) wrote:
> can you add a DCHECK(!mirrored_compositor_gl_helper_texture_id_)?
> 
> I'm assuming we only transition from !reflector_ to !!reflector_ and vice
versa.
> Otherwise we leak the texture.

Done.

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/1123763003/diff/220001/content/browser/compos...
content/browser/compositor/reflector_impl.cc:26: //
mirrored_compositor_gl_helper_texture_id_(0),
On 2015/05/13 19:46:23, piman (Very slow to review) wrote:
> nit: remove

Done.

[oshima, 2015-05-14 14:45:13]

The CQ bit was checked by oshima@chromium.org

[oshima, 2015-05-14 14:45:14]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1123763003/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-05-14 14:45:38]

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

[commit-bot: I haz the power, 2015-05-14 15:29:37]

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

[commit-bot: I haz the power, 2015-05-14 15:29:38]

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

[oshima, 2015-05-15 01:05:40]

Created separate class to handle texture creation/deletion (not sure if
ReflectorTexture is a good name. let me know if you have suggestion) and fixed
the text. PTAL.

[oshima, 2015-05-15 01:05:47]

The CQ bit was checked by oshima@chromium.org to run a CQ dry run

[oshima, 2015-05-15 01:05:47]

The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1123763003/#ps280001
(title: "moved texture code to ReflectorTexture")

[commit-bot: I haz the power, 2015-05-15 01:07:50]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1123763003/280001

[piman, 2015-05-15 01:13:57]

lgtm

[commit-bot: I haz the power, 2015-05-15 01:53:46]

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

[commit-bot: I haz the power, 2015-05-15 01:53:47]

Dry run: This issue passed the CQ dry run.

[oshima, 2015-05-15 04:45:09]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2015-05-15 04:45:23]

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

[commit-bot: I haz the power, 2015-05-15 04:48:56]

Committed patchset #7 (id:280001)

[commit-bot: I haz the power, 2015-05-15 04:49:43]

Patchset 7 (id:??) landed as
https://crrev.com/592eb864e7c5d1c89d56756e98161f4f809b2669
Cr-Commit-Position: refs/heads/master@{#330042}