Make ReflectorImpl use mailboxes

Make ReflectorImpl use mailboxes

- fixes ui::Layer::SetTextureMailbox to not be hard-coded for software
  frames.
- adds a ui::Layer::SetTextureSize to update the size without sending a
  new mailbox (since it doesn't change).
- entirely removes ui::Layer::SetExternalTexture and ui::Texture, and
  derived classes.
- significantly changes the ReflectorImpl implementation:
  * an OwnedMailbox is created on the main thread, using the shared main
    thread context.
  * the mailbox is passed to the impl thread, that creates a texture out
    of it with the OutputSurface context, and does operations on that.
  * the mailbox is passed to the layer.
  * if the OutputSurface is recreated (assuming, after a lost context),
    the OwnedMailbox is recreated, and passed to the impl side again,
    together with the new surface.
  * the OwnedMailbox takes care of deleting the texture, whether because
    of a lost context, or when the ReflectorImpl is shut down.

BUG=333408
R=danakj@chromium.org, oshima@chromium.org, sky@chromium.org

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

[piman, 2014-04-08 05:02:25]

This is on top of https://codereview.chromium.org/227683006/

danakj: please review the overall CL
oshima: please review ReflectorImpl
sky: OWNER for ui/aura/bench/bench_main.cc and ui/views/view.cc

[sky, 2014-04-08 15:33:46]

ui/aura/bench/bench_main.cc and ui/views/view.cc LGTM

[oshima, 2014-04-08 15:57:24]

reflector_impl lgtm

[danakj, 2014-04-08 16:08:00]

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.cc:174:
main_thread_mailbox_->mailbox().IsZero())
I'm not sure where this method is all called, but if the mailbox is empty (say
it failed in ReflectorImpl::ReattachToOutputSurfaceFromMainThread) should we be
SetShowPaintedContent somewhere?

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.cc:183: } else if (size !=
texture_size_) {
This texture_size_ is only here to prevent extra calls to SetTextureSize()?
Would it make sense to have ui::Layer early out instead, if this is even a
problem?

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.h:101: // Must be accessed only on
ImplThread.
This starts to remind me of cc::ThreadProxy. Consider putting these into a impl
and main structs, and DCHECKing the access to the struct via impl() and main()
accessors to them? Possibly as another CL?

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.h:115:
scoped_refptr<base::MessageLoopProxy> main_message_loop_;
Can this only be accessed on MainThread also? Maybe the grouping here is a bit
off, or an extra comment is needed?

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc
File ui/aura/bench/bench_main.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:142: unsigned int texture,
nit: uint32?

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:146: return;
Don't you want to delete the texture here, just because?

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:216: unsigned int sync_point =
gl->InsertSyncPointCHROMIUM();
uint32? (trying to keep "unsigned int" inside cc/, we should swap them all
sometime..)

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:511: DCHECK(release_callback.get());
nit: no .get() needed for scoped_ptr

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:519: if (mailbox_release_callback_.get())
nit: no .get() needed for scoped_ptr

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:558: mailbox_ = cc::TextureMailbox();
should we call the release callback here if there is one? (and maybe add a test
for that)

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:664: if (!mailbox_release_callback_.get())
nit: no .get()

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer_unittest.cc
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:665: void ReturnMailbox(uint32 sync_point, bool
is_lost) {}
We should make sure this gets called in tests in the places you've added Run()
in layer.cc?

[piman, 2014-04-08 23:17:46]

(PS2 has fixes, PS3 is rebase)

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
File content/browser/compositor/reflector_impl.cc (right):

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.cc:174:
main_thread_mailbox_->mailbox().IsZero())
On 2014/04/08 16:08:01, danakj wrote:
> I'm not sure where this method is all called, but if the mailbox is empty (say
> it failed in ReflectorImpl::ReattachToOutputSurfaceFromMainThread) should we
be
> SetShowPaintedContent somewhere?

It can't fail in ReattachToOutputSurfaceFromMainThread (GenMailbox doesn't fail
any more), the only case where the mailbox is NULL is after a Shutdown, when we
already do SetShowPaintedContent.

The main case for the IsZero check is because OwnedTexture clears its mailbox
when the context is lost - that's the desired behavior. In that case, we don't
want to do anything until we recreate the output surface anyway (giving a new
mailbox in ReattachToOutputSurfaceFromMainThread). The compositor won't draw,
etc.

I added a SetShowPaintedContent in ReattachToOutputSurfaceFromMainThread so that
we release the old mailbox as early as possible in all cases (timing between
ReattachToOutputSurfaceFromMainThread and the OnLostResources is fundamentally
racy, no clear way around that).

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.cc:183: } else if (size !=
texture_size_) {
On 2014/04/08 16:08:01, danakj wrote:
> This texture_size_ is only here to prevent extra calls to SetTextureSize()?
> Would it make sense to have ui::Layer early out instead, if this is even a
> problem?

I wanted to avoid full layer invalidations if the size doesn't change, caused by
either SetTextureSize or SetBounds.

SetBounds already catches redundant sizes, so I moved the check in
SetTextureSize to save the field here.

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.h:101: // Must be accessed only on
ImplThread.
On 2014/04/08 16:08:01, danakj wrote:
> This starts to remind me of cc::ThreadProxy. Consider putting these into a
impl
> and main structs, and DCHECKing the access to the struct via impl() and main()
> accessors to them? Possibly as another CL?

Done.

https://codereview.chromium.org/228083002/diff/1/content/browser/compositor/r...
content/browser/compositor/reflector_impl.h:115:
scoped_refptr<base::MessageLoopProxy> main_message_loop_;
On 2014/04/08 16:08:01, danakj wrote:
> Can this only be accessed on MainThread also? Maybe the grouping here is a bit
> off, or an extra comment is needed?

Done.

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc
File ui/aura/bench/bench_main.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:142: unsigned int texture,
On 2014/04/08 16:08:01, danakj wrote:
> nit: uint32?

GLuint

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:146: return;
On 2014/04/08 16:08:01, danakj wrote:
> Don't you want to delete the texture here, just because?

Done.

https://codereview.chromium.org/228083002/diff/1/ui/aura/bench/bench_main.cc#...
ui/aura/bench/bench_main.cc:216: unsigned int sync_point =
gl->InsertSyncPointCHROMIUM();
On 2014/04/08 16:08:01, danakj wrote:
> uint32? (trying to keep "unsigned int" inside cc/, we should swap them all
> sometime..)

GLuint

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc
File ui/compositor/layer.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:511: DCHECK(release_callback.get());
On 2014/04/08 16:08:01, danakj wrote:
> nit: no .get() needed for scoped_ptr

Done.

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:519: if (mailbox_release_callback_.get())
On 2014/04/08 16:08:01, danakj wrote:
> nit: no .get() needed for scoped_ptr

Done.

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:558: mailbox_ = cc::TextureMailbox();
On 2014/04/08 16:08:01, danakj wrote:
> should we call the release callback here if there is one? (and maybe add a
test
> for that)

Done.

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer.cc#newco...
ui/compositor/layer.cc:664: if (!mailbox_release_callback_.get())
On 2014/04/08 16:08:01, danakj wrote:
> nit: no .get()

Done.

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer_unittest.cc
File ui/compositor/layer_unittest.cc (right):

https://codereview.chromium.org/228083002/diff/1/ui/compositor/layer_unittest...
ui/compositor/layer_unittest.cc:665: void ReturnMailbox(uint32 sync_point, bool
is_lost) {}
On 2014/04/08 16:08:01, danakj wrote:
> We should make sure this gets called in tests in the places you've added Run()
> in layer.cc?

Done.

[danakj, 2014-04-09 17:30:00]

LGTM, thanks!

https://codereview.chromium.org/228083002/diff/40001/content/browser/composit...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/228083002/diff/40001/content/browser/composit...
content/browser/compositor/reflector_impl.h:126: ImplThreadData impl_;
name this impl_unsafe_ or something, to make it clear people should be using
GetImpl() to get to it?

[piman, 2014-04-09 20:09:06]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-04-09 20:09:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/228083002/70001

[piman, 2014-04-09 22:14:38]

https://codereview.chromium.org/228083002/diff/40001/content/browser/composit...
File content/browser/compositor/reflector_impl.h (right):

https://codereview.chromium.org/228083002/diff/40001/content/browser/composit...
content/browser/compositor/reflector_impl.h:126: ImplThreadData impl_;
On 2014/04/09 17:30:00, danakj wrote:
> name this impl_unsafe_ or something, to make it clear people should be using
> GetImpl() to get to it?

Done.

[piman, 2014-04-09 22:15:02]

Committed patchset #5 manually as r262836 (presubmit successful).