Improve transition between opaque and translucent compositor views.

Improve transition between opaque and translucent compositor views.

[Re-land after revert]

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130
Review-Url: https://codereview.chromium.org/2201483002
Cr-Original-Commit-Position: refs/heads/master@{#456142}
Committed: https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadbe4e5f487c76e
Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#458486}
Committed: https://chromium.googlesource.com/chromium/src/+/3fbc8b7641ee4665f400dee3641e7362b581d09a

[liberato (no reviews please), 2016-08-25 22:28:41]

Description was changed from

==========
DO NOT COMMIT - hackily prevent transparency flash

BUG=
==========

to

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

BUG=629130
==========

[liberato (no reviews please), 2016-08-25 22:29:08]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

BUG=629130
==========

to

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

BUG=629130
==========

[liberato (no reviews please), 2016-08-25 22:29:08]

liberato@chromium.org changed reviewers:
+ sievers@chromium.org, watk@chromium.org

[liberato (no reviews please), 2016-08-25 22:31:23]

sievers, watk: it doesn't look like you own any of these files, but i'd value
your opinions before i bring in somebody from OWNERS.

thanks
-fl

[watk, 2016-08-25 23:16:09]

I'll wait for sievers to comment on the idea before doing a full review, but
seems like a good workaround to me. The only thing that I worry about is spiking
gralloc memory for a frame on low mem devices.

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:74:
private int mCurrentView = 0;
Not sure, but I kinda feel like the code will be more readable if you had two SV
members, like primarySurfaceView and secondarySurfaceView, and kept mCurrentView
updated to point to the right one.

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:228:
setBackgroundColor(Color.WHITE);
I wonder if we should make this black for the secondary SV? Not sure if we ever
see this white in particular :)

[liberato (no reviews please), 2016-08-25 23:35:30]

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:228:
setBackgroundColor(Color.WHITE);
On 2016/08/25 23:16:09, watk wrote:
> I wonder if we should make this black for the secondary SV? Not sure if we
ever
> see this white in particular :)

we do see the white before the first buffer is posted.  in fact, it prevents us
from seeing the first buffer until the background is cleared.  i found this out
the when the setBackgroundResource(0) in onSwapBuffersCompleted wasn't being set
on the SV properly, but the WHITE was being set on it.  all i could see was a
tab bar and a big white rectangle where the page was supposed to be.  :)

except for the very first buffer, i think that this should be unset.  i don't
believe that we'll ever want to set it on the background SV.  we use the
outgoing SV as the background, essentially, in that case.

[no sievers, 2016-08-30 23:01:20]

I mainly wanted to understand a little better why we have to do this.

For example, why does the transition with the surface format change cause a
glitch?

Is that because submitting the buffer that is now transparent or not transparent
anymore (or rather: it being presented) cannot be synchronized with some other
transition involved here that relates to the View layout (like the underlay)?
But if that's the case switching out a SurfaceView is also a change in the View
layout hierarchy which would have to be synchronized.

I don't understand what happens in the transition exactly, and also it's
different for transitioning in and out, so maybe you can help me wrap my head
around it.

[liberato (no reviews please), 2016-08-31 14:41:30]

On 2016/08/30 23:01:20, sievers wrote:
> I mainly wanted to understand a little better why we have to do this.
> 
> For example, why does the transition with the surface format change cause a
> glitch?

Calling SurfaceHolder::setFormat on the surface destroys and recreates the
surface.
For a frame or two, there's no layer in SurfaceFlinger that corresponds to the
CompositorView. That was part of what led me to look into using the egl config
to
update just the buffer format, which avoids calling setFormat entirely.  The
problem
was the weirdness with virtualized contexts and mismatched egl configs between
the
on-screen and off-screen contexts.
 
> I don't understand what happens in the transition exactly, and also it's
> different for transitioning in and out, so maybe you can help me wrap my head
> around it.

I don't know that I understand the whole process either.

The transition in and out are pretty close from the point of view of these
changes.
Once setFormat() is called is when things go haywire, from my (limited)
understanding.
setFormat destroys the outgoing surface, which (via surfaceDestroyed) triggers
the pending
compositor format change.  However, in either case, this CL orchestrates the
create /
destroy notifications such that the (old) surface isn't really destroyed until
the compositor
is using the new, correctly formatted surface.  Relies on the fact that Surfaces
without
any buffers pushed are ignored by SF.

As a test, I modified CompositorView to alternate its pixel format every few
seconds
on a timer.  Each transition caused a delay as the compositor was destroyed and
re-created (~same with or without the CL), but visually it was flicker-free. 
One could
tell that compositing stopped, but it didn't flash the screen to black.

I didn't get timing info for how long the compositor was out of commission with
and
without this CL to compare.  I'll try to do that, to be sure that it's not
making the
total time longer.  Didn't think of that until now.

There's also a corner case that watk pointed out that I need to check -- some
devices
display garbage for a frame during the transition due to optimizing out a
glClear, when
the compositor's notion of the opacity of the surface differs from android.  I
need to
make sure that this CL doesn't make that worse.

With the CL enabled (no timer), pressing the fullscreen button on a video
transitions
to full screen mode and the video stays visible during the transition.
Previously,
one saw a black full-screen flash for a frame or two.

In the future, this CL will also help us to delay the transition to SV until
we're
sure that it's safe to do so from a compositing perspective.  That's helpful for
getting SV for custom player controls, which don't full screen the video element
itself, but rather a containing element. That probably requires DialogSurface,
though.

[liberato (no reviews please), 2016-09-01 16:31:14]

ps3 fixes a bug -- i accidentally used TRANSPARENT instead of TRANSLUCENT in one
spot.  caused us never to notify the compositor about the transparent
background.  behavior is now the same as before.  includes, i'm afraid, the same
blue flash if one uses debug mode in gl_renderer.cc.

i think that we can remove that flash if we just delay turning off the glClear
for a few frames when gl_renderer transitions from transparent => opaque.  not
exactly pretty, and doesn't fix the underlying architectural issue.  however, i
don't fully grasp the underlying issue yet.  :)

either way, behavior is unchanged by this CL.

ps4 - removed the array of surface views.

thanks
-fl

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:74:
private int mCurrentView = 0;
On 2016/08/25 23:16:09, watk wrote:
> Not sure, but I kinda feel like the code will be more readable if you had two
SV
> members, like primarySurfaceView and secondarySurfaceView, and kept
mCurrentView
> updated to point to the right one.

i gave this a try in ps4.  seems fine to me.

[liberato (no reviews please), 2016-09-06 18:28:14]

liberato@chromium.org changed reviewers:
+ tedchoc@chromium.org

[liberato (no reviews please), 2016-09-06 18:32:59]

@tedc: PTAL.  Primary goal is to get rid of the black flash that shows up when
switching to using SurfaceView for full screen video playback.  It's caused by
the setFormat(), which destroys the SurfaceView entirely.

Secondary goal is to make the transition smooth enough that we don't need to
hide it during a full-screen transition to switch to video playback on
SurfaceView.  Might let us apply it in other cases, like many JS player controls
that don't ever full-screen the video element itself.

kinda fits well with https://codereview.chromium.org/2302023003/, which i'm
pretty happy about!

thanks
-fl

[Ted C, 2016-09-07 23:04:01]

tedchoc@chromium.org changed reviewers:
+ aelias@chromium.org

[Ted C, 2016-09-07 23:04:01]

+aelias, Are you at all familiar with the compositor layout such that you could
do a more meaningful review?

sievers@ is the best person for this, and I honestly couldn't give a strong
opinion of whether this is the proper approach or not

[aelias_OOO_until_Jul13, 2016-09-07 23:19:29]

I could, but since sievers@ has already given this some thought and this is due
for M55, I think we should wait for him to get back from vacation next Monday.

[liberato (no reviews please), 2016-10-05 16:34:42]

liberato@chromium.org changed reviewers:
+ boliu@chromium.org

[liberato (no reviews please), 2016-10-05 16:35:52]

mostly just a rebase.

boliu: i forgot to add you to this one.  sorry about that.

thanks
-fl

[boliu, 2016-10-05 18:03:38]

So.. I'm learning all of this for like the first time too \o/
cc khushalsagar too, since he's taking over some of sievers's repsonsibility too

My main concern is memory usage. Does SurfaceView provide hard guarantees that
GONE implies surface will be destroyed later soon, or is this just a particular
android implementation? Probably safer to detach the surface view? Also I think
peak memory usage actually does go up, because during the transition, both
surfaces are allocated. I think peak memory inrcrease, and the additional
complexity, need to be weighed against fixing this glitch, and I'm not really
sure where that decision should fall.

On the same note, iirc android only requires hardware to support 4 surfaces. And
since the top and bottom bars each take one, and apps take one for its views,
app only are really only guaranteed one extra surface. I have no idea what
happens when app runs out of surfaces, if android has a slower fallback in GL,
or things just break.. And I'm not sure either how true that number 4 is in
practice, I read it somewhere awhile ago. These are questions to the android
team, or someone more knowledgeable with surface views already :p


Also I think the piece of code I'm missing is what happens in the gpu stack, ie
with the onscreen compositor context, when surface changes. I would imagine it
needs to be re-created? But if that actually is a delayed signal, maybe that
could be improved, if we know for sure compositor needs to be start using a new
context for the next for, for example.


I left minor comments while I was reading the code, but those aren't relevant
yet to the overall discussion here.

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:64:
return super.onTouchEvent(e);
so don't need this override?, in which case why is class needed?

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:82:
private boolean mHideBackgroundPending = false;
drop "= false"

instance variables are default initialized to false/0 etc, but adding = false
here actually generates more bytecode due to corner case subclass interactions.
There was an internal "PSA: Don't initialize default values in Java" thread on
g/clank-team about this.

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:85:
private int mFramesUntilHide = 0;
ditto

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
private boolean mWillNotDraw = false;
ditto

[no sievers, 2016-10-05 18:04:22]

sievers@chromium.org changed reviewers:
- sievers@chromium.org

[liberato (no reviews please), 2016-10-05 19:03:52]

thanks for the feedback.  replies inline.

On 2016/10/05 18:03:38, boliu wrote:
> So.. I'm learning all of this for like the first time too \o/
> cc khushalsagar too, since he's taking over some of sievers's repsonsibility
too
> 
> My main concern is memory usage. Does SurfaceView provide hard guarantees that
> GONE implies surface will be destroyed later soon, or is this just a
particular
> android implementation?

I think that it does the same thing in all implementations, but just detaching
makes more sense.  i'll try that out.

> Also I think
> peak memory usage actually does go up, because during the transition, both
> surfaces are allocated. I think peak memory inrcrease, and the additional
> complexity, need to be weighed against fixing this glitch, and I'm not really
> sure where that decision should fall.

yes, memory usage does go up for a few frames.  the main reason why this CL
might still be useful is that it lets us decouple transitioning to a SurfaceView
from the full screen transition / relayout.  if we want to defer a bit to see if
there are read backs / other things that SV doesn't support, then we'll incur
the glitch at some seemingly arbitrary time from the user's perspective.

> On the same note, iirc android only requires hardware to support 4 surfaces.

I think that it's required to support only four hardware composer lanes.  when
it overruns them, SurfaceFlinger just falls back to GL composition for some of
the surfaces until the number of surfaces gets back into line with what the
hardware composer supports.  it's essentially the same case when the HWC refuses
to accept a surface for other hardware limitations -- e.g., a Dim layer usually
goes the GLES route regardless.  in our case, I'd expect that we might
transition to GLES composition for a few frames, though maybe only when
transitioning from alpha => no alpha.  in the other direction, the full screen
compositor view will be marked opaque.  The incoming one is entirely behind
(unfortunately!) the outgoing one, and won't participate at all in
SurfaceFlinger's composition since SF optimizes it out early.
 
> Also I think the piece of code I'm missing is what happens in the gpu stack,
ie
> with the onscreen compositor context, when surface changes. I would imagine it
> needs to be re-created? But if that actually is a delayed signal, maybe that
> could be improved, if we know for sure compositor needs to be start using a
new
> context for the next for, for example.

ah, yes, this is where things get interesting.  i'm not entirely familiar either
yet!  it does, in fact, get recreated.  it's also unfortunate, since adding an
alpha channel could be a much lighter operation than it is.  i'm not convinced
that we really need to destroy everything and recreate it, but that's how it
works now.  my attempts to make it lighter weight failed, unfortunately.  :(

it starts when it gets a surfaceDestroyed message here --
https://cs.chromium.org/chromium/src/chrome/browser/android/compositor/compos...
.

as an aside, we don't technically need to recreate the surface view -- we can
use EGL to push in buffers without an alpha channel.  this is equivalent to
setting eOpaque on the surface from a power perspective.  unfortunately, it
poses some issues for virtualized contexts, since it requires that the onscreen
context can operate without an alpha channel even though offscreen contexts
require it.  this would be the preferred, memory-neutral, approach, if we didn't
have to turn off virtual contexts.  sievers@ pointed me at an extension that
would allow us to go that route, but it doesn't exist yet.  if we could also
avoid recreating the whole compositor, then it would be super fast too!  but, i
think, those are very big changes.

[boliu, 2016-10-05 22:30:48]

So from the internal thread, I guess we won't bother with fixing the glitch on
low memory devices, right? Then that makes the peak memory regression easier to
take imo.

Let's move to the next step. What's the glitch? Do you have a video/description
of it? Theory of what causes the glitch in the existing behavior? You say the
glitch is caused by having to destroy and recreate the surface, but in your CL,
we still recreate the gpu context, so it's not obvious to me why this is better.


So khushal and I were guessing these thing happen (in some order) when going to
fullscreen:
* compositor surface marked translucent -> this involves destroying and
recreating the surface, and this is an asynchronous process (afaik?)
* media output redirected to its surface, media surface placed behind compositor
surface, media frames start to pushed to that
* compositor draws a hole (or only controls subtitles etc)


khushal pointed me to how the context gets destroyed:
CompositorView::SurfaceDestroyed ->
CompositorImpl::SetSurface(null) ->
CompositorImpl::SetVisible(false) ->
LayerTreeHost::ReleaseCompositorFrameSink
That will destroy the FrameSink (formerly OutputSurface), which owns the gpu
command buffer client.


You bring up EGL context and virtual context. Not directly related to this, but
I don't actually know how virtual context works in chrome. So for one window,
there is one onscreen context bound to the surface, and a whole bunch of
offscreen contexts used by renderer etc. With virtual context, is there only one
underlying EGLContext, the onscreen one, that's shared among all the contexts?
I'm guessing no, because otherwise, that mean tearing that down takes down all
the offscreen contexts as well. I probably should figure out this one for own
benefit..

[liberato (no reviews please), 2016-10-12 18:57:30]

On 2016/10/05 22:30:48, boliu wrote:
> So from the internal thread, I guess we won't bother with fixing the glitch on
> low memory devices, right? Then that makes the peak memory regression easier
to
> take imo.

It turns out that we can avoid the issue.  Since low memory devices use 565 EGL
surfaces when the compositor doesn't want an alpha channel, we can just keep the
pixel format set to translucent all the time.  SurfaceFlinger will get a 565
buffer, and optimize out alpha blending just like if we set the pixel format to
OPAQUE.  Yay!  It also opens the door to using this on hardware (like QC) that
has efficient alpha blending -- there's no power difference on my N5, for
example, if we just use TRANSLUCENT all the time.

> Let's move to the next step. What's the glitch? Do you have a
video/description
> of it? Theory of what causes the glitch in the existing behavior? You say the
> glitch is caused by having to destroy and recreate the surface, but in your
CL,
> we still recreate the gpu context, so it's not obvious to me why this is
better.

The glitch is a few frames of entirely black screen when one presses full screen
on a video element.  I don't have an easy way to capture it, unfortunately.

The black screen shows up when it enables alpha blending => sets the pixel
format on the CompositorView => android::SurfaceView destroys the underlying
surface to change the format => no layers in SurfaceFlinger to display for a few
frames.

The current CL avoids it by changing the format on a different SurfaceView, so
that the outgoing one is visible until the incoming one is ready.

> 
> So khushal and I were guessing these thing happen (in some order) when going
to
> fullscreen:
> * compositor surface marked translucent -> this involves destroying and
> recreating the surface, and this is an asynchronous process (afaik?)

yeah, it seems to be entirely unhinged from when blink's next frame comes along.
 :)

> * media output redirected to its surface, media surface placed behind
compositor
> surface, media frames start to pushed to that
> * compositor draws a hole (or only controls subtitles etc)

exactly.  plus, the compositor draw is optimized out unless some of the
non-video content changes.  this is where the power savings comes in -- the
compositor is shut off.  assuming that we're using hardware composition in
SurfaceFlinger, then the entire GPU can power down, I think.  it's 20-30%
savings on my N5.

> khushal pointed me to how the context gets destroyed:
> CompositorView::SurfaceDestroyed ->
> CompositorImpl::SetSurface(null) ->
> CompositorImpl::SetVisible(false) ->
> LayerTreeHost::ReleaseCompositorFrameSink
> That will destroy the FrameSink (formerly OutputSurface), which owns the gpu
> command buffer client.
> 
> 
> You bring up EGL context and virtual context. Not directly related to this,
but
> I don't actually know how virtual context works in chrome. So for one window,
> there is one onscreen context bound to the surface, and a whole bunch of
> offscreen contexts used by renderer etc. With virtual context, is there only
one
> underlying EGLContext, the onscreen one, that's shared among all the contexts?
> I'm guessing no, because otherwise, that mean tearing that down takes down all
> the offscreen contexts as well. I probably should figure out this one for own
> benefit..

when virtual contexts are enabled, i think that the off-screen ones are assumed
to be able to share the platform context.  the on-screen context tries to be
virtual too, but will fall back to a new platform context if it isn't
compatible.  see
https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub....
.

so, destroying the on-screen context won't affect offscreen ones.  it might just
switch between virtual and real, as needed.

[liberato (no reviews please), 2016-10-12 19:00:07]

On 2016/10/05 22:30:48, boliu wrote:
> So from the internal thread, I guess we won't bother with fixing the glitch on
> low memory devices, right? Then that makes the peak memory regression easier
to
> take imo.

It turns out that we can avoid the issue.  Since low memory devices use 565 EGL
surfaces when the compositor doesn't want an alpha channel, we can just keep the
pixel format set to translucent all the time.  SurfaceFlinger will get a 565
buffer, and optimize out alpha blending just like if we set the pixel format to
OPAQUE.  Yay!  It also opens the door to using this on hardware (like QC) that
has efficient alpha blending -- there's no power difference on my N5, for
example, if we just use TRANSLUCENT all the time.

> Let's move to the next step. What's the glitch? Do you have a
video/description
> of it? Theory of what causes the glitch in the existing behavior? You say the
> glitch is caused by having to destroy and recreate the surface, but in your
CL,
> we still recreate the gpu context, so it's not obvious to me why this is
better.

The glitch is a few frames of entirely black screen when one presses full screen
on a video element.  I don't have an easy way to capture it, unfortunately.

The black screen shows up when it enables alpha blending => sets the pixel
format on the CompositorView => android::SurfaceView destroys the underlying
surface to change the format => no layers in SurfaceFlinger to display for a few
frames.

The current CL avoids it by changing the format on a different SurfaceView, so
that the outgoing one is visible until the incoming one is ready.

> 
> So khushal and I were guessing these thing happen (in some order) when going
to
> fullscreen:
> * compositor surface marked translucent -> this involves destroying and
> recreating the surface, and this is an asynchronous process (afaik?)

yeah, it seems to be entirely unhinged from when blink's next frame comes along.
 :)

> * media output redirected to its surface, media surface placed behind
compositor
> surface, media frames start to pushed to that
> * compositor draws a hole (or only controls subtitles etc)

exactly.  plus, the compositor draw is optimized out unless some of the
non-video content changes.  this is where the power savings comes in -- the
compositor is shut off.  assuming that we're using hardware composition in
SurfaceFlinger, then the entire GPU can power down, I think.  it's 20-30%
savings on my N5.

> khushal pointed me to how the context gets destroyed:
> CompositorView::SurfaceDestroyed ->
> CompositorImpl::SetSurface(null) ->
> CompositorImpl::SetVisible(false) ->
> LayerTreeHost::ReleaseCompositorFrameSink
> That will destroy the FrameSink (formerly OutputSurface), which owns the gpu
> command buffer client.
> 
> 
> You bring up EGL context and virtual context. Not directly related to this,
but
> I don't actually know how virtual context works in chrome. So for one window,
> there is one onscreen context bound to the surface, and a whole bunch of
> offscreen contexts used by renderer etc. With virtual context, is there only
one
> underlying EGLContext, the onscreen one, that's shared among all the contexts?
> I'm guessing no, because otherwise, that mean tearing that down takes down all
> the offscreen contexts as well. I probably should figure out this one for own
> benefit..

when virtual contexts are enabled, i think that the off-screen ones are assumed
to be able to share the platform context.  the on-screen context tries to be
virtual too, but will fall back to a new platform context if it isn't
compatible.  see
https://cs.chromium.org/chromium/src/gpu/ipc/service/gpu_command_buffer_stub....
.

so, destroying the on-screen context won't affect offscreen ones.  it might just
switch between virtual and real, as needed.

at least, that's how i kinda understand it.

[liberato (no reviews please), 2016-10-12 19:00:40]

now with good transitions and no extra memory on low-end devices!

thanks
-fl

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:64:
return super.onTouchEvent(e);
On 2016/10/05 18:03:37, boliu wrote:
> so don't need this override?, in which case why is class needed?

that's a really good question.  i have no idea why this is here.  removed.

i think that i was having trouble with touch events, but it turned out to be one
of the flags that wasn't being forwarded to the SurfaceView properly.

thanks!

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:82:
private boolean mHideBackgroundPending = false;
On 2016/10/05 18:03:37, boliu wrote:
> drop "= false"
> 
> instance variables are default initialized to false/0 etc, but adding = false
> here actually generates more bytecode due to corner case subclass
interactions.
> There was an internal "PSA: Don't initialize default values in Java" thread on
> g/clank-team about this.

Done.

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:85:
private int mFramesUntilHide = 0;
On 2016/10/05 18:03:37, boliu wrote:
> ditto

Done.

https://codereview.chromium.org/2201483002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
private boolean mWillNotDraw = false;
On 2016/10/05 18:03:37, boliu wrote:
> ditto

Done.

[aelias_OOO_until_Jul13, 2016-10-12 19:12:34]

On 2016/10/12 at 19:00:07, liberato wrote:
> It turns out that we can avoid the issue.  Since low memory devices use 565
EGL surfaces when the compositor doesn't want an alpha channel, we can just keep
the pixel format set to translucent all the time.  SurfaceFlinger will get a 565
buffer, and optimize out alpha blending just like if we set the pixel format to
OPAQUE.  Yay!  It also opens the door to using this on hardware (like QC) that
has efficient alpha blending -- there's no power difference on my N5, for
example, if we just use TRANSLUCENT all the time.

It's interesting to hear there's no power difference on N5.  Have you measured a
device where translucency does cause a power regression?  (Our historical
measurements were pre-Battor so I don't trust them as much as any measurements
you make now.)  If you could prove that translucency never causes a power
regression after all (or, say, causes a tolerable level of regression on <10% of
market share), I wouldn't mind just turning it back on and avoiding the
complexity in this patch.

[liberato (no reviews please), 2016-10-12 19:59:23]

On 2016/10/12 19:12:34, aelias wrote:
> On 2016/10/12 at 19:00:07, liberato wrote:
> > It turns out that we can avoid the issue.  Since low memory devices use 565
> EGL surfaces when the compositor doesn't want an alpha channel, we can just
keep
> the pixel format set to translucent all the time.  SurfaceFlinger will get a
565
> buffer, and optimize out alpha blending just like if we set the pixel format
to
> OPAQUE.  Yay!  It also opens the door to using this on hardware (like QC) that
> has efficient alpha blending -- there's no power difference on my N5, for
> example, if we just use TRANSLUCENT all the time.
> 
> It's interesting to hear there's no power difference on N5.  Have you measured
a
> device where translucency does cause a power regression?  (Our historical
> measurements were pre-Battor so I don't trust them as much as any measurements
> you make now.)  If you could prove that translucency never causes a power
> regression after all (or, say, causes a tolerable level of regression on <10%
of
> market share), I wouldn't mind just turning it back on and avoiding the
> complexity in this patch.

unfortunately, i don't have an example of such a device.  i know that QC chips
are smart about it, so a lack of regression on an N5 is unsurprising.  it was a
monsoon measurement, not battor, though.

i'll see if telemetry has any devices instrumented for power on non-QC devices. 
otherwise, it might be a while before i can measure anything.

[boliu, 2016-10-12 21:02:12]

Still just replying to comments..

On 2016/10/12 18:57:30, liberato wrote:
> On 2016/10/05 22:30:48, boliu wrote:
> > Let's move to the next step. What's the glitch? Do you have a
> video/description
> > of it? Theory of what causes the glitch in the existing behavior? You say
the
> > glitch is caused by having to destroy and recreate the surface, but in your
> CL,
> > we still recreate the gpu context, so it's not obvious to me why this is
> better.
> 
> The glitch is a few frames of entirely black screen when one presses full
screen
> on a video element.  I don't have an easy way to capture it, unfortunately.
> 
> The black screen shows up when it enables alpha blending => sets the pixel
> format on the CompositorView => android::SurfaceView destroys the underlying
> surface to change the format => no layers in SurfaceFlinger to display for a
few
> frames.
> 
> The current CL avoids it by changing the format on a different SurfaceView, so
> that the outgoing one is visible until the incoming one is ready.

One more question.

When the compositor surface is destroyed, does it draw opaque black, or is it as
if it's "not there", so thus effectively transparent?

If it's transparent, then we could kinda rearrange the order of things to avoid
all black:
1) move video output to separate surface
2) change compositor surface format to enable alpha

Was this considered or tested? Or something obviously won't work here?

[boliu, 2016-10-12 21:03:24]

On 2016/10/12 21:02:12, boliu wrote:
> Still just replying to comments..
> 
> On 2016/10/12 18:57:30, liberato wrote:
> > On 2016/10/05 22:30:48, boliu wrote:
> > > Let's move to the next step. What's the glitch? Do you have a
> > video/description
> > > of it? Theory of what causes the glitch in the existing behavior? You say
> the
> > > glitch is caused by having to destroy and recreate the surface, but in
your
> > CL,
> > > we still recreate the gpu context, so it's not obvious to me why this is
> > better.
> > 
> > The glitch is a few frames of entirely black screen when one presses full
> screen
> > on a video element.  I don't have an easy way to capture it, unfortunately.
> > 
> > The black screen shows up when it enables alpha blending => sets the pixel
> > format on the CompositorView => android::SurfaceView destroys the underlying
> > surface to change the format => no layers in SurfaceFlinger to display for a
> few
> > frames.
> > 
> > The current CL avoids it by changing the format on a different SurfaceView,
so
> > that the outgoing one is visible until the incoming one is ready.
> 
> One more question.
> 
> When the compositor surface is destroyed, does it draw opaque black, or is it
as
> if it's "not there", so thus effectively transparent?
> 
> If it's transparent, then we could kinda rearrange the order of things to
avoid
> all black:
> 1) move video output to separate surface
> 2) change compositor surface format to enable alpha

Err, and during the transition, it would be displaying the video surface. So a
few frames of no controls rather than black?

> 
> Was this considered or tested? Or something obviously won't work here?

[liberato (no reviews please), 2016-10-12 21:34:51]

On 2016/10/12 21:03:24, boliu wrote:
> On 2016/10/12 21:02:12, boliu wrote:
> > Still just replying to comments..
> > 
> > On 2016/10/12 18:57:30, liberato wrote:
> > > On 2016/10/05 22:30:48, boliu wrote:
> > > > Let's move to the next step. What's the glitch? Do you have a
> > > video/description
> > > > of it? Theory of what causes the glitch in the existing behavior? You
say
> > the
> > > > glitch is caused by having to destroy and recreate the surface, but in
> your
> > > CL,
> > > > we still recreate the gpu context, so it's not obvious to me why this is
> > > better.
> > > 
> > > The glitch is a few frames of entirely black screen when one presses full
> > screen
> > > on a video element.  I don't have an easy way to capture it,
unfortunately.
> > > 
> > > The black screen shows up when it enables alpha blending => sets the pixel
> > > format on the CompositorView => android::SurfaceView destroys the
underlying
> > > surface to change the format => no layers in SurfaceFlinger to display for
a
> > few
> > > frames.
> > > 
> > > The current CL avoids it by changing the format on a different
SurfaceView,
> so
> > > that the outgoing one is visible until the incoming one is ready.
> > 
> > One more question.
> > 
> > When the compositor surface is destroyed, does it draw opaque black, or is
it
> as
> > if it's "not there", so thus effectively transparent?
> > 
> > If it's transparent, then we could kinda rearrange the order of things to
> avoid
> > all black:
> > 1) move video output to separate surface
> > 2) change compositor surface format to enable alpha
> 
> Err, and during the transition, it would be displaying the video surface. So a
> few frames of no controls rather than black?
> 
> > 
> > Was this considered or tested? Or something obviously won't work here?

it's an interesting idea.  i hadn't considered it.

i'm not sure how easy it would be in practice -- we'd have to make sure that
blink has completed re-layout before starting the alpha transition when entering
full screen, and before blink does any re-layout when leaving full screen.  it
seems like it'd have to be very tightly coupled together to make that work.  the
"exiting" part seems particularly difficult -- we can always just wait a little
on the "entering" case.

another concern is that it only works with full screen on the video element. i'd
be less comfortable that it would work with, for example, a video inside a
full-screened div element, since we really don't know that it's going to be a
full screen black background.

[boliu, 2016-10-12 22:09:42]

On 2016/10/12 21:34:51, liberato wrote:
> it's an interesting idea.  i hadn't considered it.
> 
> i'm not sure how easy it would be in practice -- we'd have to make sure that
> blink has completed re-layout before starting the alpha transition when
entering
> full screen, and before blink does any re-layout when leaving full screen.  it
> seems like it'd have to be very tightly coupled together to make that work. 
the
> "exiting" part seems particularly difficult -- we can always just wait a
little
> on the "entering" case.

Ok, yeah, exiting fullscreen doesn't really work I guess.

> 
> another concern is that it only works with full screen on the video element.
i'd
> be less comfortable that it would work with, for example, a video inside a
> full-screened div element, since we really don't know that it's going to be a
> full screen black background.

Does video go into a separate surface in that case? Even if it might not be full
screen?

[liberato (no reviews please), 2016-10-12 22:20:49]

On 2016/10/12 22:09:42, boliu wrote:
> On 2016/10/12 21:34:51, liberato wrote:
> > it's an interesting idea.  i hadn't considered it.
> > 
> > i'm not sure how easy it would be in practice -- we'd have to make sure that
> > blink has completed re-layout before starting the alpha transition when
> entering
> > full screen, and before blink does any re-layout when leaving full screen. 
it
> > seems like it'd have to be very tightly coupled together to make that work. 
> the
> > "exiting" part seems particularly difficult -- we can always just wait a
> little
> > on the "entering" case.
> 
> Ok, yeah, exiting fullscreen doesn't really work I guess.
> 
> > 
> > another concern is that it only works with full screen on the video element.
> i'd
> > be less comfortable that it would work with, for example, a video inside a
> > full-screened div element, since we really don't know that it's going to be
a
> > full screen black background.
> 
> Does video go into a separate surface in that case? Even if it might not be
full
> screen?

it doesn't currently.  that's a limitation that i'd like to fix with
DialogSurface, since it exposes positioning and sizing in a way that's more
compatible with chrome overlays.

of course, it still can't scroll in a frame-accurate way.  we have to be careful
about when we use it, but the normal 'full screen div' cases should be fairly
straightforward to detect and cover.

[boliu, 2016-10-12 23:33:35]

Approach is ok. Moving on to actual looking at the code. So I don't actually own
this code.. maybe all these refactor/style things don't matter to the owner.
Ted? :p

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:71:
// except that we own the compositor.
I'd prefer to refactor the code first, and avoid yet another layout.

So move basically move the native part of CompositorView to CompositorViewHolder
instead first, like you suggested.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:72:
private SurfaceView mForegroundSurfaceView;
suggestion: webview had a similar pattern for fullscreen (due to different
reasons)

there we have explicit non-fullscreen and fullscreen references to the "things",
and then a third "active" reference that always points to the active thing

it does require entering and exiting fullscreen actually be different calls, but
I think it makes reasoning through the case of exit before the enter sequence is
done saner.

Actually thinking about that, I *think* that sequence is broken, so breaking the
sequence into:
* setOverlayVideoMode
* onSurfaceCreated
* compositor swap
* compositor swap #2 to hide the non-active view

Let's say this happens:
* setOverlay(true)
* onSurfaceCreated
* setOverlay(false)

At this point there is not going to be an onSurfaceCreated again because both
surfaces are valid. Then I think.. compositor will be stuck drawing the the
wrong surface? I guess nothing will look wrong visually. But maybe the reverse,
ie rapid set(false) then set(true) will not have blending enabled?

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:284:
mVirtualPixelFormat = enabled ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
would it be easier to just keep a boolean mOverlayVideoModeEnabled?

virtual/actual pixel format has slightly higher friction in my head

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301:
nativeSurfaceDestroyed(mNativeCompositorView);
is Destroy/Created needed here? previous code didn't do that?

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:312:
requestLayout();
is invalidate by itself not enough?

also use postInvalidateOnAnimation instead of invalidate (android has this
heuristic that delays tasks until next frame frame if there's an invalidate, to
avoid jank)

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:324:
if (isBackgroundHolder(holder)) return;
suggestion: might be useful have an inner static class SurfaceHolder.Callback
implementation, and then have two callback instances, so don't need to keep
checking state like this.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:450:
if (mFramesUntilHide > 0) {
I'm not sure what the two frame delay is for. The first onSwapBuffersCompleted
should mean the new surface has some content, right? so then should be safe to
teardown the old surface? Or is android lacking synchronization here, and the
one frame is more of an arbitrary delay?

[Ted C, 2016-10-17 19:39:09]

On 2016/10/12 23:33:35, boliu wrote:
> Approach is ok. Moving on to actual looking at the code. So I don't actually
own
> this code.. maybe all these refactor/style things don't matter to the owner.
> Ted? :p

I'll defer to:
chrome/android/java/src/org/chromium/chrome/browser/compositor/OWNERS
here.

If the refactoring is bigger than a bread box, then it might be nice to get
a 1-pager doc that we could look over instead of having to do it in code.

But if it is mechanical moves, then maybe that is overkill.

> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
> (right):
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:71:
> // except that we own the compositor.
> I'd prefer to refactor the code first, and avoid yet another layout.
> 
> So move basically move the native part of CompositorView to
CompositorViewHolder
> instead first, like you suggested.
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:72:
> private SurfaceView mForegroundSurfaceView;
> suggestion: webview had a similar pattern for fullscreen (due to different
> reasons)
> 
> there we have explicit non-fullscreen and fullscreen references to the
"things",
> and then a third "active" reference that always points to the active thing
> 
> it does require entering and exiting fullscreen actually be different calls,
but
> I think it makes reasoning through the case of exit before the enter sequence
is
> done saner.
> 
> Actually thinking about that, I *think* that sequence is broken, so breaking
the
> sequence into:
> * setOverlayVideoMode
> * onSurfaceCreated
> * compositor swap
> * compositor swap #2 to hide the non-active view
> 
> Let's say this happens:
> * setOverlay(true)
> * onSurfaceCreated
> * setOverlay(false)
> 
> At this point there is not going to be an onSurfaceCreated again because both
> surfaces are valid. Then I think.. compositor will be stuck drawing the the
> wrong surface? I guess nothing will look wrong visually. But maybe the
reverse,
> ie rapid set(false) then set(true) will not have blending enabled?
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:284:
> mVirtualPixelFormat = enabled ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
> would it be easier to just keep a boolean mOverlayVideoModeEnabled?
> 
> virtual/actual pixel format has slightly higher friction in my head
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301:
> nativeSurfaceDestroyed(mNativeCompositorView);
> is Destroy/Created needed here? previous code didn't do that?
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:312:
> requestLayout();
> is invalidate by itself not enough?
> 
> also use postInvalidateOnAnimation instead of invalidate (android has this
> heuristic that delays tasks until next frame frame if there's an invalidate,
to
> avoid jank)
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:324:
> if (isBackgroundHolder(holder)) return;
> suggestion: might be useful have an inner static class SurfaceHolder.Callback
> implementation, and then have two callback instances, so don't need to keep
> checking state like this.
> 
>
https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:450:
> if (mFramesUntilHide > 0) {
> I'm not sure what the two frame delay is for. The first onSwapBuffersCompleted
> should mean the new surface has some content, right? so then should be safe to
> teardown the old surface? Or is android lacking synchronization here, and the
> one frame is more of an arbitrary delay?

[liberato (no reviews please), 2016-10-17 20:42:30]

On 2016/10/17 19:39:09, Ted C wrote:
> On 2016/10/12 23:33:35, boliu wrote:
> > Approach is ok. Moving on to actual looking at the code. So I don't actually
> own
> > this code.. maybe all these refactor/style things don't matter to the owner.
> > Ted? :p
> 
> I'll defer to:
> chrome/android/java/src/org/chromium/chrome/browser/compositor/OWNERS
> here.
> 
> If the refactoring is bigger than a bread box, then it might be nice to get
> a 1-pager doc that we could look over instead of having to do it in code.
> 
> But if it is mechanical moves, then maybe that is overkill.
> 

i don't believe that it's mechanical, and it would probably end up with a very
large (~1500 line) composite class.  so, i'm hoping to avoid it :)

however, before i embark on that either way, aelias@ raised a good question
about whether we could trust the original power measurements, made long before
me, that suggested that any of this is necessary.  i believe that i can measure,
somewhat reliably, the power drain on an exynos 5250 and tegra K1 (kepler) of
just turning on alpha blending all the time.

if those turn out to be okay, then maybe we can avoid this whole thing.

[liberato (no reviews please), 2016-11-14 22:12:35]

i factored out all of the new code from CompositorView, since it made it easier
to reason about.  it also addresses the race that boliu@ mentioned (thanks!).

also, i don't see any way to add tests for this.  is there such a way?

thanks
-fl

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:71:
// except that we own the compositor.
On 2016/10/12 23:33:34, boliu wrote:
> I'd prefer to refactor the code first, and avoid yet another layout.
> 
> So move basically move the native part of CompositorView to
CompositorViewHolder
> instead first, like you suggested.

i've refactored all of the new code out into a separate class.  it makes things
much clearer.

layout: yes, it would be nice to avoid, but i think that adds a lot of moving
parts to this CL.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:72:
private SurfaceView mForegroundSurfaceView;
On 2016/10/12 23:33:35, boliu wrote:
> suggestion: webview had a similar pattern for fullscreen (due to different
> reasons)
> 
> there we have explicit non-fullscreen and fullscreen references to the
"things",
> and then a third "active" reference that always points to the active thing
> 
> it does require entering and exiting fullscreen actually be different calls,
but
> I think it makes reasoning through the case of exit before the enter sequence
is
> done saner.
> 
> Actually thinking about that, I *think* that sequence is broken, so breaking
the
> sequence into:
> * setOverlayVideoMode
> * onSurfaceCreated
> * compositor swap
> * compositor swap #2 to hide the non-active view
> 
> Let's say this happens:
> * setOverlay(true)
> * onSurfaceCreated
> * setOverlay(false)
> 
> At this point there is not going to be an onSurfaceCreated again because both
> surfaces are valid. Then I think.. compositor will be stuck drawing the the
> wrong surface? I guess nothing will look wrong visually. But maybe the
reverse,
> ie rapid set(false) then set(true) will not have blending enabled?

yes, you're quite right.  good catch.

i've refactored all of this logic into a separate class.  it's much easier to
see what's going on, and it handles these cases.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:284:
mVirtualPixelFormat = enabled ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
On 2016/10/12 23:33:34, boliu wrote:
> would it be easier to just keep a boolean mOverlayVideoModeEnabled?
> 
> virtual/actual pixel format has slightly higher friction in my head

changed significantly due to the refactor.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:301:
nativeSurfaceDestroyed(mNativeCompositorView);
On 2016/10/12 23:33:34, boliu wrote:
> is Destroy/Created needed here? previous code didn't do that?

it did, via the real surfaceCreated / Destroyed caused by the setFormat.  we're
eliding the setFormat here, but these are still needed to get the EGL surface to
switch between 565 and 8888.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:312:
requestLayout();
On 2016/10/12 23:33:35, boliu wrote:
> is invalidate by itself not enough?
> 
> also use postInvalidateOnAnimation instead of invalidate (android has this
> heuristic that delays tasks until next frame frame if there's an invalidate,
to
> avoid jank)

Done.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:324:
if (isBackgroundHolder(holder)) return;
On 2016/10/12 23:33:34, boliu wrote:
> suggestion: might be useful have an inner static class SurfaceHolder.Callback
> implementation, and then have two callback instances, so don't need to keep
> checking state like this.

after refactoring, i'm not sure that this is still applicable.

https://codereview.chromium.org/2201483002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:450:
if (mFramesUntilHide > 0) {
On 2016/10/12 23:33:34, boliu wrote:
> I'm not sure what the two frame delay is for. The first onSwapBuffersCompleted
> should mean the new surface has some content, right? so then should be safe to
> teardown the old surface? Or is android lacking synchronization here, and the
> one frame is more of an arbitrary delay?

arbitrary delay, unfortunately.

[liberato (no reviews please), 2016-12-12 22:17:46]

does anybody have any comments on this?  methinks we may have deadlocked :)

thanks
-fl

[boliu, 2016-12-12 22:26:16]

boliu@chromium.org changed reviewers:
+ mdjones@chromium.org
- aelias@chromium.org

[boliu, 2016-12-12 22:26:17]

moving aelias and me to cc, +mdjones for chrome compositor owners

fwiw, aelias and I have vetted the approach, but we are not the owner here, and
I really do not want to own more code than I already do :p

[boliu, 2016-12-12 22:26:42]

boliu@chromium.org changed reviewers:
- boliu@chromium.org

[liberato (no reviews please), 2017-01-06 16:58:40]

On 2016/12/12 22:26:17, boliu wrote:
> moving aelias and me to cc, +mdjones for chrome compositor owners
> 
> fwiw, aelias and I have vetted the approach, but we are not the owner here,
and
> I really do not want to own more code than I already do :p

bo: thanks

mdjones: any comments on this?

thanks
-fl

[mdjones, 2017-01-09 18:41:46]

lgtm % suggestion

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:69:
private int mFramesUntilHide;
Will this be anything other than 1 and will anything other than surface creation
increment this? If no, can this be something like "boolean mSurfaceNeedsFrame"?

[Ted C, 2017-01-10 22:10:26]

SurfaceView is not something I'm in any way familiar with, so I'm commenting
more on structure here.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
2017! :-P

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:17:
* Manage multiple SurfaceViews for the compositor, so that transitions between
wrap to 100 for all comments in the file

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:25:
* Internally, we maintain two SurfaceViews, since calling setFormat() to change
is this something we should bring up with the Android team?  ideally, should we
be able to do this going forward without maintaining two SurfaceViews?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:42:
private class SurfaceState {
can this be static?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:43:
public SurfaceView mSurfaceView;
for public variables, you don't have the leading m prefix...just surfaceView

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:57:
public SurfaceHolder getHolder() {
I would name this something different to avoid clobbering with
CompositorSurfaceManager#getHolder (and to avoid incorrectly calling one by
mistake).

Maybe just .surfaceHolder() or something.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:98:
mTranslucent = new SurfaceState(parentView.getContext(),
PixelFormat.TRANSLUCENT, this);
Should we just lazily create these?

I feel that the constraint of "We do not guarantee that the surface will always
actually have that format," makes this class more complicated than it needs to
be.  If both of these could be null, then this class just deals with switching
out surfaces based on pixel format.

Basically I wonder if instead of having mAlwaysTranslucent here, should the
client know that it never needs to request the other surface?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:138:
if (mOwnedByClient != null)
mClient.surfaceDestroyed(mOwnedByClient.getHolder());
What would happen if mOwnedByClient == mRequestedByClient?  Is that expected?

I'm surprised that we are doing this here instead of relying on them calling
doneWithOwnedSurface.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:164:
public void doneWithUnownedSurface() {
I wonder if this would be clearer if it was destroySurface(int format);  again,
it makes this class "dumber" and requires the client to be very explicit about
what it wants to have done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:187:
if (mOwnedByClient != null) return mOwnedByClient.getHolder();
return mOwnedByClient != null ? mOwnedByClient.getHolder() : null;

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:193:
* by CompositorView.  otherwise, you probably don't want to call this.
This otherwise you probably don't want to call this.  Should we add an assert
that we are on JB to avoid confusion of the API?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:227:
if (state == null) return;
should we be asserting false for all these checks...do we expect this to happen?
 could it happen in timing races and it's not really an error?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:243:
if (state == mRequestedByClient) {
I think it would be clearer to do:

if (state != mRequestedByClient) {
    postSurfaceDestruction(state);
    return;
}

... expected code path ...

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:199:
// compositor_impl_android.cc will use 565 EGL surfaces if and only if
comment wrapping is done at 100 chars in java land

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:243:
public SurfaceHolder getHolder() {
does this need to be public or could it be package protected?

maybe add a simple javadoc:

/**
 * @see SurfaceView#getHolder
 */

[liberato (no reviews please), 2017-01-10 23:39:26]

thanks for the feedback!

tedc - there's a WDYT hidden in there somewhere, which i will wait on before
landing.

also, apologies that i rebased -- did that before noticing tedc's comments.

thanks
-fl

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:1:
// Copyright 2015 The Chromium Authors. All rights reserved.
On 2017/01/10 22:10:25, Ted C wrote:
> 2017! :-P

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:17:
* Manage multiple SurfaceViews for the compositor, so that transitions between
On 2017/01/10 22:10:25, Ted C wrote:
> wrap to 100 for all comments in the file

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:25:
* Internally, we maintain two SurfaceViews, since calling setFormat() to change
On 2017/01/10 22:10:25, Ted C wrote:
> is this something we should bring up with the Android team?  ideally, should
we
> be able to do this going forward without maintaining two SurfaceViews?

added crbug.com/679902, and referenced in the comments.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:42:
private class SurfaceState {
On 2017/01/10 22:10:25, Ted C wrote:
> can this be static?

yes, done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:43:
public SurfaceView mSurfaceView;
On 2017/01/10 22:10:25, Ted C wrote:
> for public variables, you don't have the leading m prefix...just surfaceView

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:57:
public SurfaceHolder getHolder() {
On 2017/01/10 22:10:25, Ted C wrote:
> I would name this something different to avoid clobbering with
> CompositorSurfaceManager#getHolder (and to avoid incorrectly calling one by
> mistake).
> 
> Maybe just .surfaceHolder() or something.

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:98:
mTranslucent = new SurfaceState(parentView.getContext(),
PixelFormat.TRANSLUCENT, this);
On 2017/01/10 22:10:25, Ted C wrote:
> Should we just lazily create these?
> 
> I feel that the constraint of "We do not guarantee that the surface will
always
> actually have that format," makes this class more complicated than it needs to
> be.  If both of these could be null, then this class just deals with switching
> out surfaces based on pixel format.
> 
> Basically I wonder if instead of having mAlwaysTranslucent here, should the
> client know that it never needs to request the other surface?

originally, all of that was in CompositorView, but it was really unwieldy.  so,
it got factored here.

i see your point, though.

my reasoning for grouping it this way is that the CompositorView pretty much
always has to destroy + recreate the compositor on any format change, regardless
of what the Android window does.  So, when we send "surface{created /
destroyed}", CompositorView hears "{create, destroy} the compositor".

in that sense, CompositorView is telling us the format that it wants the
*compositor* to use, and we pick the pixel format that matches what the
compositor will need.  there's no explicit coupling, either in this CL or in
existing code, between the pixel format that android gets asked for and the EGL
config that ends up being used in the compositor.

that's unfortunate, but also much harder to fix (it was my first choice, in
fact).  this class just encapsulates the assumptions of how to match.

maybe it would be clearer if we took 'bool will_compositor_be_opaque' rather
than PixelFormat, when requesting a surface.

WDYT?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:138:
if (mOwnedByClient != null)
mClient.surfaceDestroyed(mOwnedByClient.getHolder());
On 2017/01/10 22:10:25, Ted C wrote:
> What would happen if mOwnedByClient == mRequestedByClient?  Is that expected?
> 
> I'm surprised that we are doing this here instead of relying on them calling
> doneWithOwnedSurface.

this case can happen if one enters / leaves full screen quickly, before the new
surface is created.

the client might not have called back doneWithUnownedSurface (note: 'Un'owned)
yet.  it does that only when it's safe for us to hide the outgoing surface,
after it's drawn on the owned surface.

there's no notion of releasing the owned surface without getting a new one to
replace it.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:164:
public void doneWithUnownedSurface() {
On 2017/01/10 22:10:25, Ted C wrote:
> I wonder if this would be clearer if it was destroySurface(int format); 
again,
> it makes this class "dumber" and requires the client to be very explicit about
> what it wants to have done.

see above.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:187:
if (mOwnedByClient != null) return mOwnedByClient.getHolder();
On 2017/01/10 22:10:25, Ted C wrote:
> return mOwnedByClient != null ? mOwnedByClient.getHolder() : null;

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:193:
* by CompositorView.  otherwise, you probably don't want to call this.
On 2017/01/10 22:10:25, Ted C wrote:
> This otherwise you probably don't want to call this.  Should we add an assert
> that we are on JB to avoid confusion of the API?

Done, also added 'ForJellyBean' to the name.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:227:
if (state == null) return;
On 2017/01/10 22:10:25, Ted C wrote:
> should we be asserting false for all these checks...do we expect this to
happen?
>  could it happen in timing races and it's not really an error?

it shouldn't happen.  now assert().

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:243:
if (state == mRequestedByClient) {
On 2017/01/10 22:10:25, Ted C wrote:
> I think it would be clearer to do:
> 
> if (state != mRequestedByClient) {
>     postSurfaceDestruction(state);
>     return;
> }
> 
> ... expected code path ...

Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:69:
private int mFramesUntilHide;
On 2017/01/09 18:41:46, mdjones wrote:
> Will this be anything other than 1 and will anything other than surface
creation
> increment this? If no, can this be something like "boolean
mSurfaceNeedsFrame"?

good point.  Done.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:199:
// compositor_impl_android.cc will use 565 EGL surfaces if and only if
On 2017/01/10 22:10:25, Ted C wrote:
> comment wrapping is done at 100 chars in java land

Done, and elsewhere.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:243:
public SurfaceHolder getHolder() {
On 2017/01/10 22:10:25, Ted C wrote:
> does this need to be public or could it be package protected?
> 
> maybe add a simple javadoc:
> 
> /**
>  * @see SurfaceView#getHolder
>  */

Done, and package protected.

[Ted C, 2017-01-11 17:18:56]

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:50:
mSurfaceView = new SurfaceView(context);
does holding onto a surfaceview incur any memory cost?  If I watch a video and
don't watch one again before Chrome is killed, do we pay some cost there?  Or by
destroying the underlying surface does that do all the necessary cleanup?

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:98:
mTranslucent = new SurfaceState(parentView.getContext(),
PixelFormat.TRANSLUCENT, this);
On 2017/01/10 23:39:25, liberato wrote:
> On 2017/01/10 22:10:25, Ted C wrote:
> > Should we just lazily create these?
> > 
> > I feel that the constraint of "We do not guarantee that the surface will
> always
> > actually have that format," makes this class more complicated than it needs
to
> > be.  If both of these could be null, then this class just deals with
switching
> > out surfaces based on pixel format.
> > 
> > Basically I wonder if instead of having mAlwaysTranslucent here, should the
> > client know that it never needs to request the other surface?
> 
> originally, all of that was in CompositorView, but it was really unwieldy. 
so,
> it got factored here.
> 
> i see your point, though.
> 
> my reasoning for grouping it this way is that the CompositorView pretty much
> always has to destroy + recreate the compositor on any format change,
regardless
> of what the Android window does.  So, when we send "surface{created /
> destroyed}", CompositorView hears "{create, destroy} the compositor".
> 
> in that sense, CompositorView is telling us the format that it wants the
> *compositor* to use, and we pick the pixel format that matches what the
> compositor will need.  there's no explicit coupling, either in this CL or in
> existing code, between the pixel format that android gets asked for and the
EGL
> config that ends up being used in the compositor.
> 
> that's unfortunate, but also much harder to fix (it was my first choice, in
> fact).  this class just encapsulates the assumptions of how to match.
> 
> maybe it would be clearer if we took 'bool will_compositor_be_opaque' rather
> than PixelFormat, when requesting a surface.
> 
> WDYT?

I think I was going for something relatively light weight in CompositorView,
something along the lines of this:

private boolean mLowMemoryDevice;
private boolean mOverlayVideoMode;

public void setOverlayVideoMode(boolean overlayMode) {
    mOverlayVideoMode = overlayMode;
    ...
    mCompositorSurfaceManager.requestSurface(getSurfacePixelFormat());
}

private int getSurfacePixelFormat() {
    return mLowMemoryDevice || mOverlayVideoMode ?
            PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
}

To me, that makes it more predictable as an API as what you request always
matches the state here.  You can also have all of your comments about the 
power implications, low memory, etc... in one place instead of spread between
two places.

This class simply would handle switching surfaceviews with different
configurations for better UI transitions.

But to me requestSurface below doesn't handle that case as it would need to
do something like:

if (mRequestedByClient.isValid() && !mRequestedByClient.mDestroyPending) {
    // Handle the case where the client requested the same thing.
    if (mOwnedByClient == mRequestedByClient) return;

    // Existing logic..
}

[liberato (no reviews please), 2017-01-11 17:52:46]

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:50:
mSurfaceView = new SurfaceView(context);
On 2017/01/11 17:18:56, Ted C wrote:
> does holding onto a surfaceview incur any memory cost?  If I watch a video and
> don't watch one again before Chrome is killed, do we pay some cost there?  Or
by
> destroying the underlying surface does that do all the necessary cleanup?

destroying it cleans up everything of any appreciable size that i've been able
to find.  in particular, there are no outstanding gralloc buffers.

https://codereview.chromium.org/2201483002/diff/180001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:98:
mTranslucent = new SurfaceState(parentView.getContext(),
PixelFormat.TRANSLUCENT, this);
On 2017/01/11 17:18:56, Ted C wrote:
> On 2017/01/10 23:39:25, liberato wrote:
> > On 2017/01/10 22:10:25, Ted C wrote:
> > > Should we just lazily create these?
> > > 
> > > I feel that the constraint of "We do not guarantee that the surface will
> > always
> > > actually have that format," makes this class more complicated than it
needs
> to
> > > be.  If both of these could be null, then this class just deals with
> switching
> > > out surfaces based on pixel format.
> > > 
> > > Basically I wonder if instead of having mAlwaysTranslucent here, should
the
> > > client know that it never needs to request the other surface?
> > 
> > originally, all of that was in CompositorView, but it was really unwieldy. 
> so,
> > it got factored here.
> > 
> > i see your point, though.
> > 
> > my reasoning for grouping it this way is that the CompositorView pretty much
> > always has to destroy + recreate the compositor on any format change,
> regardless
> > of what the Android window does.  So, when we send "surface{created /
> > destroyed}", CompositorView hears "{create, destroy} the compositor".
> > 
> > in that sense, CompositorView is telling us the format that it wants the
> > *compositor* to use, and we pick the pixel format that matches what the
> > compositor will need.  there's no explicit coupling, either in this CL or in
> > existing code, between the pixel format that android gets asked for and the
> EGL
> > config that ends up being used in the compositor.
> > 
> > that's unfortunate, but also much harder to fix (it was my first choice, in
> > fact).  this class just encapsulates the assumptions of how to match.
> > 
> > maybe it would be clearer if we took 'bool will_compositor_be_opaque' rather
> > than PixelFormat, when requesting a surface.
> > 
> > WDYT?
> 
> I think I was going for something relatively light weight in CompositorView,
> something along the lines of this:
> 
> private boolean mLowMemoryDevice;
> private boolean mOverlayVideoMode;
> 
> public void setOverlayVideoMode(boolean overlayMode) {
>     mOverlayVideoMode = overlayMode;
>     ...
>     mCompositorSurfaceManager.requestSurface(getSurfacePixelFormat());
> }
> 
> private int getSurfacePixelFormat() {
>     return mLowMemoryDevice || mOverlayVideoMode ?
>             PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
> }
> 
> To me, that makes it more predictable as an API as what you request always
> matches the state here.  You can also have all of your comments about the 
> power implications, low memory, etc... in one place instead of spread between
> two places.
> 
> This class simply would handle switching surfaceviews with different
> configurations for better UI transitions.
> 
> But to me requestSurface below doesn't handle that case as it would need to
> do something like:
> 
> if (mRequestedByClient.isValid() && !mRequestedByClient.mDestroyPending) {
>     // Handle the case where the client requested the same thing.
>     if (mOwnedByClient == mRequestedByClient) return;
> 
>     // Existing logic..
> }

i'll give this a try.  i've got a feeling that it's going to make CompositorView
pretty subtle, because of the cases where multiple requests are in flight before
the SurfaceView is ready.  that's what spawned this weird api to begin with; it
lets CompositorView just worry about whatever the last thing it requested was,
even though android might fulfill a (now out-dated) request for the wrong pixel
format.

however, perhaps it'll work better now that some of the other logic is moved
here.

[liberato (no reviews please), 2017-01-11 21:38:50]

mdjones: i put back mFramesUntilHide, but removed two boolean flags in the
process. turns out that the two bools were just the high and low order bits of
the int anyway.

tedc: this PS has more of the 565 logic in CompositorView.  i haven't convinced
myself that there are no new race conditions in this version, though the simple
cases work.  i wanted to verify that you like this version better before
spending a day or two on the edge cases.

thanks
-fl

[aelias_OOO_until_Jul13, 2017-01-12 21:46:04]

aelias@chromium.org changed reviewers:
+ aelias@chromium.org

[aelias_OOO_until_Jul13, 2017-01-12 21:46:06]

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:18:
* Manage multiple SurfaceViews for the compositor, so that transitions between
Could you copy your design doc
https://docs.google.com/document/d/1uPiYdbW0XNSWhDXASXttTl3Jq253K3mHu_hiTqJ97...
onto @chromium.org (so that non-Google contributors can access it), update it
with your battery findings, and link it in your patch description?

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:210:
mCurrentPixelFormat = initialFormat + 1;
This seems weird, I'd rather we define an invalid format value like -1 instead.

[liberato (no reviews please), 2017-01-19 17:55:57]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

BUG=629130
==========

to

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130
==========

[liberato (no reviews please), 2017-01-19 18:12:34]

aelias: thanks for the feedback.

tedc: do you prefer the 565 logic moved around as it is in PS13/14, or the way
it was previously?

thanks
-fl

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:18:
* Manage multiple SurfaceViews for the compositor, so that transitions between
On 2017/01/12 21:46:05, aelias wrote:
> Could you copy your design doc
>
https://docs.google.com/document/d/1uPiYdbW0XNSWhDXASXttTl3Jq253K3mHu_hiTqJ97...
> onto @chromium.org (so that non-Google contributors can access it), update it
> with your battery findings, and link it in your patch description?

Done, and in this comment.

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/240001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:210:
mCurrentPixelFormat = initialFormat + 1;
On 2017/01/12 21:46:05, aelias wrote:
> This seems weird, I'd rather we define an invalid format value like -1
instead.

yeah -- no idea why i did that.  we have PixelFormat.UNKNOWN.

[David Trainor- moved to gerrit, 2017-01-19 22:08:21]

dtrainor@chromium.org changed reviewers:
+ dtrainor@chromium.org

[David Trainor- moved to gerrit, 2017-01-19 22:08:22]

drive by review :D!  Just had some questions on overlay mode and a few nits.

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:66:
private SurfaceState mTranslucent;
Should these two be final?

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:97:
if (mOpaque != null) mOpaque.surfaceHolder().removeCallback(this);
It looks like mOpaque always gets built now.  Could these null checks be
removed?

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:57:
private boolean mLowmMemoryDevice;
Looks like this is no longer used.

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:259:
nativeSurfaceCreated(mNativeCompositorView);
Hey sorry, could you clarify what this does for me?  Are we guaranteed to get a
surfaceChanged() call after this?

[liberato (no reviews please), 2017-01-20 17:50:46]

dtrainor: thanks for the comments!

-fl

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:66:
private SurfaceState mTranslucent;
On 2017/01/19 22:08:22, David Trainor wrote:
> Should these two be final?

Done.

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:97:
if (mOpaque != null) mOpaque.surfaceHolder().removeCallback(this);
On 2017/01/19 22:08:22, David Trainor wrote:
> It looks like mOpaque always gets built now.  Could these null checks be
> removed?

Done.

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:57:
private boolean mLowmMemoryDevice;
On 2017/01/19 22:08:22, David Trainor wrote:
> Looks like this is no longer used.

Done.

https://codereview.chromium.org/2201483002/diff/260001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:259:
nativeSurfaceCreated(mNativeCompositorView);
On 2017/01/19 22:08:22, David Trainor wrote:
> Hey sorry, could you clarify what this does for me?  Are we guaranteed to get
a
> surfaceChanged() call after this?

on low memory devices, there's no need to switch the surface format -- it's
always translucent.  this just lets the compositor switch the compositor's EGL
surface between 565 and 8888, based on whatever we provided to
nativeSetOverlayVideoMode.

surfaceChanged: we won't get one.  probably should call nativeSurfaceChanged
here, shouldn't we?

good catch.

[Ted C, 2017-01-20 21:59:01]

https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
state.destroyPending = true;
Should we set this in postSurfaceDestruction?  At that point, we have triggered
the runnable and we aren't canceling it, so to me that seems like the destroy is
in fact pending.

The fact that some places calls startSurfaceConstruction directly seems like it
could get into a race.

If you were to call postSurfaceDestruction on the opaque surface and then call
startSurfaceConstruction.  Start surface construction would complete (with the
view already being added to the view hierarchy) and now the delayed destruction
path comes along and removes the view.

I'm wondering if we should be doing the following checks:

private void startSurfaceConstruction(SurfaceState state) {
    state.destroyPending = false;
    ...
}

private void postSurfaceConstruction(SurfaceState state) {
    state.destroyPending = false;
    mParentView.post(new Runnable() {
        @Override
        public void run() {
            if (state.destroyPending) return;
            startSurfaceConstruction(state);
        }
    });
}

private void postSurfaceDestruction(final SurfaceState state) {
    state.destroyPending = true;
    mParentView.post(new Runnable() {
        @Override
        public void run() {
            if (!state.destroyPending) return;
            startSurfaceDestruction(state);
        }
    });
}

Thinking that in both delayed cases we'd verify that we are still in the
expected state before we start interacting with the view hierarchy.  Maybe this
isn't needed based on the other construction of the class?  But to me it seems
like if we mix the posting of destruction with the immediate creation that we
could get into cases that are bad.

[liberato (no reviews please), 2017-01-24 23:25:49]

the most recent PS aims to make the many different cases more explicit and
easier to reason about.  also fixes the bugs from tedc and dtrainor (thanks!).

I found an additional bug or two in the process, though i'm unsure if they could
happen in practice.  in any case, it seems to be robust in both low-mem and
high-mem mode, including with random surface tear downs.

as part of it, i had to revert the semantics of requestSurface a bit, so that
requesting the same format than once will send a synthetic destroyed / created /
changed message.  turns out that surfaceChanged has to be handled here anyway. 
the 565 logic is still moved into CompositorView, but it now depends on the
destroyed / created messages even if the format doesn't change.

as always, comments appreciated.

thanks
-fl

https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
state.destroyPending = true;
On 2017/01/20 21:59:01, Ted C wrote:
> Should we set this in postSurfaceDestruction?  At that point, we have
triggered
> the runnable and we aren't canceling it, so to me that seems like the destroy
is
> in fact pending.
> 
> The fact that some places calls startSurfaceConstruction directly seems like
it
> could get into a race.
> 
> If you were to call postSurfaceDestruction on the opaque surface and then call
> startSurfaceConstruction.  Start surface construction would complete (with the
> view already being added to the view hierarchy) and now the delayed
destruction
> path comes along and removes the view.
> 
> I'm wondering if we should be doing the following checks:
> 
> private void startSurfaceConstruction(SurfaceState state) {
>     state.destroyPending = false;
>     ...
> }
> 
> private void postSurfaceConstruction(SurfaceState state) {
>     state.destroyPending = false;
>     mParentView.post(new Runnable() {
>         @Override
>         public void run() {
>             if (state.destroyPending) return;
>             startSurfaceConstruction(state);
>         }
>     });
> }
> 
> private void postSurfaceDestruction(final SurfaceState state) {
>     state.destroyPending = true;
>     mParentView.post(new Runnable() {
>         @Override
>         public void run() {
>             if (!state.destroyPending) return;
>             startSurfaceDestruction(state);
>         }
>     });
> }
> 
> Thinking that in both delayed cases we'd verify that we are still in the
> expected state before we start interacting with the view hierarchy.  Maybe
this
> isn't needed based on the other construction of the class?  But to me it seems
> like if we mix the posting of destruction with the immediate creation that we
> could get into cases that are bad.

i think that you're right that it should set destroyPending.

to make it clearer, i've also added 'createPending', and generally made some of
the assumptions more explicit in the most recent PS.  i'm fairly sure that it
handles cases that can't actually occur, but that's okay.  it's easier to reason
about.

[David Trainor- moved to gerrit, 2017-01-25 18:18:39]

On 2017/01/24 23:25:49, liberato wrote:
> the most recent PS aims to make the many different cases more explicit and
> easier to reason about.  also fixes the bugs from tedc and dtrainor (thanks!).
> 
> I found an additional bug or two in the process, though i'm unsure if they
could
> happen in practice.  in any case, it seems to be robust in both low-mem and
> high-mem mode, including with random surface tear downs.
> 
> as part of it, i had to revert the semantics of requestSurface a bit, so that
> requesting the same format than once will send a synthetic destroyed / created
/
> changed message.  turns out that surfaceChanged has to be handled here anyway.

> the 565 logic is still moved into CompositorView, but it now depends on the
> destroyed / created messages even if the format doesn't change.
> 
> as always, comments appreciated.
> 
> thanks
> -fl
> 
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
> (right):
> 
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
> state.destroyPending = true;
> On 2017/01/20 21:59:01, Ted C wrote:
> > Should we set this in postSurfaceDestruction?  At that point, we have
> triggered
> > the runnable and we aren't canceling it, so to me that seems like the
destroy
> is
> > in fact pending.
> > 
> > The fact that some places calls startSurfaceConstruction directly seems like
> it
> > could get into a race.
> > 
> > If you were to call postSurfaceDestruction on the opaque surface and then
call
> > startSurfaceConstruction.  Start surface construction would complete (with
the
> > view already being added to the view hierarchy) and now the delayed
> destruction
> > path comes along and removes the view.
> > 
> > I'm wondering if we should be doing the following checks:
> > 
> > private void startSurfaceConstruction(SurfaceState state) {
> >     state.destroyPending = false;
> >     ...
> > }
> > 
> > private void postSurfaceConstruction(SurfaceState state) {
> >     state.destroyPending = false;
> >     mParentView.post(new Runnable() {
> >         @Override
> >         public void run() {
> >             if (state.destroyPending) return;
> >             startSurfaceConstruction(state);
> >         }
> >     });
> > }
> > 
> > private void postSurfaceDestruction(final SurfaceState state) {
> >     state.destroyPending = true;
> >     mParentView.post(new Runnable() {
> >         @Override
> >         public void run() {
> >             if (!state.destroyPending) return;
> >             startSurfaceDestruction(state);
> >         }
> >     });
> > }
> > 
> > Thinking that in both delayed cases we'd verify that we are still in the
> > expected state before we start interacting with the view hierarchy.  Maybe
> this
> > isn't needed based on the other construction of the class?  But to me it
seems
> > like if we mix the posting of destruction with the immediate creation that
we
> > could get into cases that are bad.
> 
> i think that you're right that it should set destroyPending.
> 
> to make it clearer, i've also added 'createPending', and generally made some
of
> the assumptions more explicit in the most recent PS.  i'm fairly sure that it
> handles cases that can't actually occur, but that's okay.  it's easier to
reason
> about.

Could we add some tests for CompositorSurfaceManager?  It looks like it would be
easy to add a bunch of unit tests to validate it :).

It'd be great to have tests for how it interacts with CompositorView as well,
but I understand that those kind of tests would be a lot harder to write (more
of large scale integration tests rather than unit tests).  I'd be fine with at
least unit tests on the new class for now.

[liberato (no reviews please), 2017-01-25 18:36:52]

On 2017/01/25 18:18:39, David Trainor wrote:
> On 2017/01/24 23:25:49, liberato wrote:
> > the most recent PS aims to make the many different cases more explicit and
> > easier to reason about.  also fixes the bugs from tedc and dtrainor
(thanks!).
> > 
> > I found an additional bug or two in the process, though i'm unsure if they
> could
> > happen in practice.  in any case, it seems to be robust in both low-mem and
> > high-mem mode, including with random surface tear downs.
> > 
> > as part of it, i had to revert the semantics of requestSurface a bit, so
that
> > requesting the same format than once will send a synthetic destroyed /
created
> /
> > changed message.  turns out that surfaceChanged has to be handled here
anyway.
> 
> > the 565 logic is still moved into CompositorView, but it now depends on the
> > destroyed / created messages even if the format doesn't change.
> > 
> > as always, comments appreciated.
> > 
> > thanks
> > -fl
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
> > state.destroyPending = true;
> > On 2017/01/20 21:59:01, Ted C wrote:
> > > Should we set this in postSurfaceDestruction?  At that point, we have
> > triggered
> > > the runnable and we aren't canceling it, so to me that seems like the
> destroy
> > is
> > > in fact pending.
> > > 
> > > The fact that some places calls startSurfaceConstruction directly seems
like
> > it
> > > could get into a race.
> > > 
> > > If you were to call postSurfaceDestruction on the opaque surface and then
> call
> > > startSurfaceConstruction.  Start surface construction would complete (with
> the
> > > view already being added to the view hierarchy) and now the delayed
> > destruction
> > > path comes along and removes the view.
> > > 
> > > I'm wondering if we should be doing the following checks:
> > > 
> > > private void startSurfaceConstruction(SurfaceState state) {
> > >     state.destroyPending = false;
> > >     ...
> > > }
> > > 
> > > private void postSurfaceConstruction(SurfaceState state) {
> > >     state.destroyPending = false;
> > >     mParentView.post(new Runnable() {
> > >         @Override
> > >         public void run() {
> > >             if (state.destroyPending) return;
> > >             startSurfaceConstruction(state);
> > >         }
> > >     });
> > > }
> > > 
> > > private void postSurfaceDestruction(final SurfaceState state) {
> > >     state.destroyPending = true;
> > >     mParentView.post(new Runnable() {
> > >         @Override
> > >         public void run() {
> > >             if (!state.destroyPending) return;
> > >             startSurfaceDestruction(state);
> > >         }
> > >     });
> > > }
> > > 
> > > Thinking that in both delayed cases we'd verify that we are still in the
> > > expected state before we start interacting with the view hierarchy.  Maybe
> > this
> > > isn't needed based on the other construction of the class?  But to me it
> seems
> > > like if we mix the posting of destruction with the immediate creation that
> we
> > > could get into cases that are bad.
> > 
> > i think that you're right that it should set destroyPending.
> > 
> > to make it clearer, i've also added 'createPending', and generally made some
> of
> > the assumptions more explicit in the most recent PS.  i'm fairly sure that
it
> > handles cases that can't actually occur, but that's okay.  it's easier to
> reason
> > about.
> 
> Could we add some tests for CompositorSurfaceManager?  It looks like it would
be
> easy to add a bunch of unit tests to validate it :).
> 
> It'd be great to have tests for how it interacts with CompositorView as well,
> but I understand that those kind of tests would be a lot harder to write (more
> of large scale integration tests rather than unit tests).  I'd be fine with at
> least unit tests on the new class for now.

in fact, i'd love to!  didn't realize that this code was unit testable.

is the right place chrome/android/javatests/.../compositor?

[Ted C, 2017-01-27 04:19:21]

On 2017/01/25 18:36:52, liberato wrote:
> On 2017/01/25 18:18:39, David Trainor wrote:
> > On 2017/01/24 23:25:49, liberato wrote:
> > > the most recent PS aims to make the many different cases more explicit and
> > > easier to reason about.  also fixes the bugs from tedc and dtrainor
> (thanks!).
> > > 
> > > I found an additional bug or two in the process, though i'm unsure if they
> > could
> > > happen in practice.  in any case, it seems to be robust in both low-mem
and
> > > high-mem mode, including with random surface tear downs.
> > > 
> > > as part of it, i had to revert the semantics of requestSurface a bit, so
> that
> > > requesting the same format than once will send a synthetic destroyed /
> created
> > /
> > > changed message.  turns out that surfaceChanged has to be handled here
> anyway.
> > 
> > > the 565 logic is still moved into CompositorView, but it now depends on
the
> > > destroyed / created messages even if the format doesn't change.
> > > 
> > > as always, comments appreciated.
> > > 
> > > thanks
> > > -fl
> > > 
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > > File
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
> > > state.destroyPending = true;
> > > On 2017/01/20 21:59:01, Ted C wrote:
> > > > Should we set this in postSurfaceDestruction?  At that point, we have
> > > triggered
> > > > the runnable and we aren't canceling it, so to me that seems like the
> > destroy
> > > is
> > > > in fact pending.
> > > > 
> > > > The fact that some places calls startSurfaceConstruction directly seems
> like
> > > it
> > > > could get into a race.
> > > > 
> > > > If you were to call postSurfaceDestruction on the opaque surface and
then
> > call
> > > > startSurfaceConstruction.  Start surface construction would complete
(with
> > the
> > > > view already being added to the view hierarchy) and now the delayed
> > > destruction
> > > > path comes along and removes the view.
> > > > 
> > > > I'm wondering if we should be doing the following checks:
> > > > 
> > > > private void startSurfaceConstruction(SurfaceState state) {
> > > >     state.destroyPending = false;
> > > >     ...
> > > > }
> > > > 
> > > > private void postSurfaceConstruction(SurfaceState state) {
> > > >     state.destroyPending = false;
> > > >     mParentView.post(new Runnable() {
> > > >         @Override
> > > >         public void run() {
> > > >             if (state.destroyPending) return;
> > > >             startSurfaceConstruction(state);
> > > >         }
> > > >     });
> > > > }
> > > > 
> > > > private void postSurfaceDestruction(final SurfaceState state) {
> > > >     state.destroyPending = true;
> > > >     mParentView.post(new Runnable() {
> > > >         @Override
> > > >         public void run() {
> > > >             if (!state.destroyPending) return;
> > > >             startSurfaceDestruction(state);
> > > >         }
> > > >     });
> > > > }
> > > > 
> > > > Thinking that in both delayed cases we'd verify that we are still in the
> > > > expected state before we start interacting with the view hierarchy. 
Maybe
> > > this
> > > > isn't needed based on the other construction of the class?  But to me it
> > seems
> > > > like if we mix the posting of destruction with the immediate creation
that
> > we
> > > > could get into cases that are bad.
> > > 
> > > i think that you're right that it should set destroyPending.
> > > 
> > > to make it clearer, i've also added 'createPending', and generally made
some
> > of
> > > the assumptions more explicit in the most recent PS.  i'm fairly sure that
> it
> > > handles cases that can't actually occur, but that's okay.  it's easier to
> > reason
> > > about.
> > 
> > Could we add some tests for CompositorSurfaceManager?  It looks like it
would
> be
> > easy to add a bunch of unit tests to validate it :).
> > 
> > It'd be great to have tests for how it interacts with CompositorView as
well,
> > but I understand that those kind of tests would be a lot harder to write
(more
> > of large scale integration tests rather than unit tests).  I'd be fine with
at
> > least unit tests on the new class for now.
> 
> in fact, i'd love to!  didn't realize that this code was unit testable.
> 
> is the right place chrome/android/javatests/.../compositor?

chrome/android/javatests would be for instrumentation tests (you are running
on the target device, spinning up activities, processes, etc..).

For CompositorSurfaceManager, I'd be tempted to try and test it via
Robolectric tests in chrome/android/junit.  These run on the local machine
without needing to do anything on device.  Since this would be more to
test the logic of switching vs interactions with our framework or android's
it seems ideal.

We might still want some integration tests, which would go in
chrome/android/javatests,
but I would do that as a last resort.

[liberato (no reviews please), 2017-01-27 06:20:46]

On 2017/01/27 04:19:21, Ted C wrote:
> On 2017/01/25 18:36:52, liberato wrote:
> > On 2017/01/25 18:18:39, David Trainor wrote:
> > > On 2017/01/24 23:25:49, liberato wrote:
> > > > the most recent PS aims to make the many different cases more explicit
and
> > > > easier to reason about.  also fixes the bugs from tedc and dtrainor
> > (thanks!).
> > > > 
> > > > I found an additional bug or two in the process, though i'm unsure if
they
> > > could
> > > > happen in practice.  in any case, it seems to be robust in both low-mem
> and
> > > > high-mem mode, including with random surface tear downs.
> > > > 
> > > > as part of it, i had to revert the semantics of requestSurface a bit, so
> > that
> > > > requesting the same format than once will send a synthetic destroyed /
> > created
> > > /
> > > > changed message.  turns out that surfaceChanged has to be handled here
> > anyway.
> > > 
> > > > the 565 logic is still moved into CompositorView, but it now depends on
> the
> > > > destroyed / created messages even if the format doesn't change.
> > > > 
> > > > as always, comments appreciated.
> > > > 
> > > > thanks
> > > > -fl
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > > > File
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
> > > > state.destroyPending = true;
> > > > On 2017/01/20 21:59:01, Ted C wrote:
> > > > > Should we set this in postSurfaceDestruction?  At that point, we have
> > > > triggered
> > > > > the runnable and we aren't canceling it, so to me that seems like the
> > > destroy
> > > > is
> > > > > in fact pending.
> > > > > 
> > > > > The fact that some places calls startSurfaceConstruction directly
seems
> > like
> > > > it
> > > > > could get into a race.
> > > > > 
> > > > > If you were to call postSurfaceDestruction on the opaque surface and
> then
> > > call
> > > > > startSurfaceConstruction.  Start surface construction would complete
> (with
> > > the
> > > > > view already being added to the view hierarchy) and now the delayed
> > > > destruction
> > > > > path comes along and removes the view.
> > > > > 
> > > > > I'm wondering if we should be doing the following checks:
> > > > > 
> > > > > private void startSurfaceConstruction(SurfaceState state) {
> > > > >     state.destroyPending = false;
> > > > >     ...
> > > > > }
> > > > > 
> > > > > private void postSurfaceConstruction(SurfaceState state) {
> > > > >     state.destroyPending = false;
> > > > >     mParentView.post(new Runnable() {
> > > > >         @Override
> > > > >         public void run() {
> > > > >             if (state.destroyPending) return;
> > > > >             startSurfaceConstruction(state);
> > > > >         }
> > > > >     });
> > > > > }
> > > > > 
> > > > > private void postSurfaceDestruction(final SurfaceState state) {
> > > > >     state.destroyPending = true;
> > > > >     mParentView.post(new Runnable() {
> > > > >         @Override
> > > > >         public void run() {
> > > > >             if (!state.destroyPending) return;
> > > > >             startSurfaceDestruction(state);
> > > > >         }
> > > > >     });
> > > > > }
> > > > > 
> > > > > Thinking that in both delayed cases we'd verify that we are still in
the
> > > > > expected state before we start interacting with the view hierarchy. 
> Maybe
> > > > this
> > > > > isn't needed based on the other construction of the class?  But to me
it
> > > seems
> > > > > like if we mix the posting of destruction with the immediate creation
> that
> > > we
> > > > > could get into cases that are bad.
> > > > 
> > > > i think that you're right that it should set destroyPending.
> > > > 
> > > > to make it clearer, i've also added 'createPending', and generally made
> some
> > > of
> > > > the assumptions more explicit in the most recent PS.  i'm fairly sure
that
> > it
> > > > handles cases that can't actually occur, but that's okay.  it's easier
to
> > > reason
> > > > about.
> > > 
> > > Could we add some tests for CompositorSurfaceManager?  It looks like it
> would
> > be
> > > easy to add a bunch of unit tests to validate it :).
> > > 
> > > It'd be great to have tests for how it interacts with CompositorView as
> well,
> > > but I understand that those kind of tests would be a lot harder to write
> (more
> > > of large scale integration tests rather than unit tests).  I'd be fine
with
> at
> > > least unit tests on the new class for now.
> > 
> > in fact, i'd love to!  didn't realize that this code was unit testable.
> > 
> > is the right place chrome/android/javatests/.../compositor?
> 
> chrome/android/javatests would be for instrumentation tests (you are running
> on the target device, spinning up activities, processes, etc..).
> 
> For CompositorSurfaceManager, I'd be tempted to try and test it via
> Robolectric tests in chrome/android/junit.  These run on the local machine
> without needing to do anything on device.  Since this would be more to
> test the logic of switching vs interactions with our framework or android's
> it seems ideal.
> 
> We might still want some integration tests, which would go in
> chrome/android/javatests,
> but I would do that as a last resort.

thanks, i'll give robolectric a try.  the test i wrote doesn't really use
instrumentation, anyway, so it'll be a welcome change.  :)

[liberato (no reviews please), 2017-01-28 00:21:08]

On 2017/01/27 06:20:46, liberato wrote:
> On 2017/01/27 04:19:21, Ted C wrote:
> > On 2017/01/25 18:36:52, liberato wrote:
> > > On 2017/01/25 18:18:39, David Trainor wrote:
> > > > On 2017/01/24 23:25:49, liberato wrote:
> > > > > the most recent PS aims to make the many different cases more explicit
> and
> > > > > easier to reason about.  also fixes the bugs from tedc and dtrainor
> > > (thanks!).
> > > > > 
> > > > > I found an additional bug or two in the process, though i'm unsure if
> they
> > > > could
> > > > > happen in practice.  in any case, it seems to be robust in both
low-mem
> > and
> > > > > high-mem mode, including with random surface tear downs.
> > > > > 
> > > > > as part of it, i had to revert the semantics of requestSurface a bit,
so
> > > that
> > > > > requesting the same format than once will send a synthetic destroyed /
> > > created
> > > > /
> > > > > changed message.  turns out that surfaceChanged has to be handled here
> > > anyway.
> > > > 
> > > > > the 565 logic is still moved into CompositorView, but it now depends
on
> > the
> > > > > destroyed / created messages even if the format doesn't change.
> > > > > 
> > > > > as always, comments appreciated.
> > > > > 
> > > > > thanks
> > > > > -fl
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > > > > File
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
> > > > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2201483002/diff/300001/chrome/android/java/sr...
> > > > >
> > > >
> > >
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:306:
> > > > > state.destroyPending = true;
> > > > > On 2017/01/20 21:59:01, Ted C wrote:
> > > > > > Should we set this in postSurfaceDestruction?  At that point, we
have
> > > > > triggered
> > > > > > the runnable and we aren't canceling it, so to me that seems like
the
> > > > destroy
> > > > > is
> > > > > > in fact pending.
> > > > > > 
> > > > > > The fact that some places calls startSurfaceConstruction directly
> seems
> > > like
> > > > > it
> > > > > > could get into a race.
> > > > > > 
> > > > > > If you were to call postSurfaceDestruction on the opaque surface and
> > then
> > > > call
> > > > > > startSurfaceConstruction.  Start surface construction would complete
> > (with
> > > > the
> > > > > > view already being added to the view hierarchy) and now the delayed
> > > > > destruction
> > > > > > path comes along and removes the view.
> > > > > > 
> > > > > > I'm wondering if we should be doing the following checks:
> > > > > > 
> > > > > > private void startSurfaceConstruction(SurfaceState state) {
> > > > > >     state.destroyPending = false;
> > > > > >     ...
> > > > > > }
> > > > > > 
> > > > > > private void postSurfaceConstruction(SurfaceState state) {
> > > > > >     state.destroyPending = false;
> > > > > >     mParentView.post(new Runnable() {
> > > > > >         @Override
> > > > > >         public void run() {
> > > > > >             if (state.destroyPending) return;
> > > > > >             startSurfaceConstruction(state);
> > > > > >         }
> > > > > >     });
> > > > > > }
> > > > > > 
> > > > > > private void postSurfaceDestruction(final SurfaceState state) {
> > > > > >     state.destroyPending = true;
> > > > > >     mParentView.post(new Runnable() {
> > > > > >         @Override
> > > > > >         public void run() {
> > > > > >             if (!state.destroyPending) return;
> > > > > >             startSurfaceDestruction(state);
> > > > > >         }
> > > > > >     });
> > > > > > }
> > > > > > 
> > > > > > Thinking that in both delayed cases we'd verify that we are still in
> the
> > > > > > expected state before we start interacting with the view hierarchy. 
> > Maybe
> > > > > this
> > > > > > isn't needed based on the other construction of the class?  But to
me
> it
> > > > seems
> > > > > > like if we mix the posting of destruction with the immediate
creation
> > that
> > > > we
> > > > > > could get into cases that are bad.
> > > > > 
> > > > > i think that you're right that it should set destroyPending.
> > > > > 
> > > > > to make it clearer, i've also added 'createPending', and generally
made
> > some
> > > > of
> > > > > the assumptions more explicit in the most recent PS.  i'm fairly sure
> that
> > > it
> > > > > handles cases that can't actually occur, but that's okay.  it's easier
> to
> > > > reason
> > > > > about.
> > > > 
> > > > Could we add some tests for CompositorSurfaceManager?  It looks like it
> > would
> > > be
> > > > easy to add a bunch of unit tests to validate it :).
> > > > 
> > > > It'd be great to have tests for how it interacts with CompositorView as
> > well,
> > > > but I understand that those kind of tests would be a lot harder to write
> > (more
> > > > of large scale integration tests rather than unit tests).  I'd be fine
> with
> > at
> > > > least unit tests on the new class for now.
> > > 
> > > in fact, i'd love to!  didn't realize that this code was unit testable.
> > > 
> > > is the right place chrome/android/javatests/.../compositor?
> > 
> > chrome/android/javatests would be for instrumentation tests (you are running
> > on the target device, spinning up activities, processes, etc..).
> > 
> > For CompositorSurfaceManager, I'd be tempted to try and test it via
> > Robolectric tests in chrome/android/junit.  These run on the local machine
> > without needing to do anything on device.  Since this would be more to
> > test the logic of switching vs interactions with our framework or android's
> > it seems ideal.
> > 
> > We might still want some integration tests, which would go in
> > chrome/android/javatests,
> > but I would do that as a last resort.
> 
> thanks, i'll give robolectric a try.  the test i wrote doesn't really use
> instrumentation, anyway, so it'll be a welcome change.  :)

robolectric is kinda neat.  i had no idea that we had a testing framework like
that.

anyway, i added about eight unit tests.  there are a few other cases that i'll
add tests for, but this covers the common ones.  since i've never used
robolectric before, i wanted to make sure that there's not something terribly
broken about what i've done so far :)

thanks
-fl

[Ted C, 2017-01-31 23:17:51]

a few last questions and then I think this is ok for me

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:169:
// Send a notification about any owned surface.  Note that this can be
|mRequestedByClient|m
there seems to be a trailing m after the final |

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:171:
// start tear-down of the owned surface; the client notifies us via
donwWithUnownedSurface
s/donw/done

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:173:
disownClientSurface(mOwnedByClient);
Would it be better to post this block of code to make the client api more
predictable (by calling this method you're not sure if you'll get an immediate
callback or a posted one).  I don't know if imposing the same async behavior
across both call sites makes this more difficult though (where you then have to
track even more state).

If I understand this correctly, this is purely for the path where low end phones
doesn't actually change their surface format but we need to destroy the native
surface for the changes to take effect.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:417:

remove blank line

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
(right):

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:61:
private SurfaceView mRealSurfaceView;
Not familiar with RealObject, but this doesn't seem to be used anywhere

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:141:
assertTrue(callbacks.size() < 2);
I don't know if we have access to it, but it would be nice to use:
http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/number/OrderingComp...

assertThat(callbacks.size(), lessThan(2));

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:183:
private SurfaceView requestAndGet(int format) {
Hmm...it would be hard to understand the difference between requestSurface and
requestAndGet.

Maybe requestSurfaceAndNotifyCallback or "something"

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:254:
assertTrue(findSurface(PixelFormat.TRANSLUCENT) != null);
would assertNotNull work here?

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:358:
verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());
For the normal SurfaceView lifetime, should you always expect a single create
followed by a single destroy (potentially followed by the same over and over)? 
Wondering if having it receive 2 creates is not a standard behavior.  Or would
we have synthesized a destroy as well?

[liberato (no reviews please), 2017-02-01 19:19:48]

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
(right):

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:169:
// Send a notification about any owned surface.  Note that this can be
|mRequestedByClient|m
On 2017/01/31 23:17:51, Ted C wrote:
> there seems to be a trailing m after the final |

thanks.  vim column bar @100 is the same color as the syntax-highlighted comment
text.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:171:
// start tear-down of the owned surface; the client notifies us via
donwWithUnownedSurface
On 2017/01/31 23:17:50, Ted C wrote:
> s/donw/done

vim column bar @80 is... maybe i should change the color of those bars.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:173:
disownClientSurface(mOwnedByClient);
On 2017/01/31 23:17:50, Ted C wrote:
> Would it be better to post this block of code to make the client api more
> predictable (by calling this method you're not sure if you'll get an immediate
> callback or a posted one).  I don't know if imposing the same async behavior
> across both call sites makes this more difficult though (where you then have
to
> track even more state).
> 
> If I understand this correctly, this is purely for the path where low end
phones
> doesn't actually change their surface format but we need to destroy the native
> surface for the changes to take effect.

i think that there are other cases, though rare.  if one requests A, B, A
without sending doneWith... for A, then it'll also get here since the surface is
still ready.

i like the idea of a simpler api.  i need to think what happens if
mOwnedByClient is set to null while a posted synthetic surfaceDestroyed is in
flight.  i'd like to avoid cases where some surfaceCreated arrives first at the
client.

it might work as it is, since android sends these things async anyway.  i'm
afraid that we'd end up with 'disownInProgress' and 'ownInProgress' flags to
keep things sorted.

if that turns out to be true, then i'll leave it as is.  otherwise, i'll post
the reply.

update: can't convince myself.  pretty sure that multiple requests could get
confusing.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java:417:

On 2017/01/31 23:17:50, Ted C wrote:
> remove blank line

Done.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
File
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
(right):

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:61:
private SurfaceView mRealSurfaceView;
On 2017/01/31 23:17:51, Ted C wrote:
> Not familiar with RealObject, but this doesn't seem to be used anywhere

docs made me think that it was needed, but seems not.  Done.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:141:
assertTrue(callbacks.size() < 2);
On 2017/01/31 23:17:51, Ted C wrote:
> I don't know if we have access to it, but it would be nice to use:
>
http://hamcrest.org/JavaHamcrest/javadoc/1.3/org/hamcrest/number/OrderingComp...
> 
> assertThat(callbacks.size(), lessThan(2));

Done.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:183:
private SurfaceView requestAndGet(int format) {
On 2017/01/31 23:17:51, Ted C wrote:
> Hmm...it would be hard to understand the difference between requestSurface and
> requestAndGet.
> 
> Maybe requestSurfaceAndNotifyCallback or "something"

i switched this to requestThenCreateSurface, and updated the comments for both
this and requestSurface to make the difference clear.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:254:
assertTrue(findSurface(PixelFormat.TRANSLUCENT) != null);
On 2017/01/31 23:17:51, Ted C wrote:
> would assertNotNull work here?

done, and elsewhere.

https://codereview.chromium.org/2201483002/diff/380001/chrome/android/junit/s...
chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java:358:
verify(mCallback, times(2)).surfaceCreated(opaque.getHolder());
On 2017/01/31 23:17:51, Ted C wrote:
> For the normal SurfaceView lifetime, should you always expect a single create
> followed by a single destroy (potentially followed by the same over and over)?

> Wondering if having it receive 2 creates is not a standard behavior.  Or would
> we have synthesized a destroy as well?

we should get a synthetic destroyed too.  i've added a check for it.

[liberato (no reviews please), 2017-02-03 21:10:38]

dtrainor: any concerns about the latest PS?

thanks
-fl

[Ted C, 2017-02-06 18:44:17]

lgtm ... let's dtrainor@ give one final look though.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339:
mCompositorSurfaceManager.doneWithUnownedSurface();
Looking at the code in doneWithUnownedSurface, it looks like we do work even if
there isn't really an unowned surface.  If android destroys our surface but we
don't change our pixel format, will this do everything as expected?

Looking at the code, it looks like it might just do unneeded work, but one final
thing I wanted to clarify.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:397:
// SetBackgroundDrawable is depricated, and the semantics are the same I think.
depr[e]cated

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398:
super.setBackgroundDrawable(background);
should we actually set the drawable in both places?  That would result in double
drawing.  Here, I "don't" think we should actually call into super.  Maybe just
super.setBackgroundDrawable(null) or something?

[David Trainor- moved to gerrit, 2017-02-07 19:36:16]

lgtm thanks!

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
* native library is properly loaded.
Move back up?  100 columns for Java.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:187:
// compositor_impl_android.cc will use 565 EGL surfaces if and only if  we're
using a low
Remove extra space after "only if"

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:195:
// on non-low memory devices , if we are running on hardware that implements
efficient alpha
Remove extra space before ","

[liberato (no reviews please), 2017-02-14 17:22:25]

thanks for the feedback, everybody.  sorry for the delay in getting back to
this.

dtrainor: please LMK if you're okay with my reasoning on the background drawable
before i land this.

thanks
-fl

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
(right):

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
* native library is properly loaded.
On 2017/02/07 19:36:16, David Trainor wrote:
> Move back up?  100 columns for Java.

Done.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:187:
// compositor_impl_android.cc will use 565 EGL surfaces if and only if  we're
using a low
On 2017/02/07 19:36:16, David Trainor wrote:
> Remove extra space after "only if"

Done.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:195:
// on non-low memory devices , if we are running on hardware that implements
efficient alpha
On 2017/02/07 19:36:16, David Trainor wrote:
> Remove extra space before ","

Done.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339:
mCompositorSurfaceManager.doneWithUnownedSurface();
On 2017/02/06 18:44:17, Ted C wrote:
> Looking at the code in doneWithUnownedSurface, it looks like we do work even
if
> there isn't really an unowned surface.  If android destroys our surface but we
> don't change our pixel format, will this do everything as expected?
> 
> Looking at the code, it looks like it might just do unneeded work, but one
final
> thing I wanted to clarify.

it will doneWithUnowned -> detachSurfaceLater a few frames after android
re-creates the surface, but that will early-out without posting.  the unowned
surface isn't attached.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:397:
// SetBackgroundDrawable is depricated, and the semantics are the same I think.
On 2017/02/06 18:44:17, Ted C wrote:
> depr[e]cated

Done.

https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398:
super.setBackgroundDrawable(background);
On 2017/02/06 18:44:17, Ted C wrote:
> should we actually set the drawable in both places?  That would result in
double
> drawing.  Here, I "don't" think we should actually call into super.  Maybe
just
> super.setBackgroundDrawable(null) or something?

i think that we should.  i tried with / without both of these, along with some
other variations (setting compositor to 0 all the time, etc.), and we get
visible artifacts on startup.

the background drawable is set to 0 once we get some compositor frames anyway. 
so, i think that if there is double drawing, it's only for a frame or two during
startup.

so, if it's okay with you, i think that we should keep it.

[David Trainor- moved to gerrit, 2017-02-17 17:41:59]

On 2017/02/14 17:22:25, liberato wrote:
> thanks for the feedback, everybody.  sorry for the delay in getting back to
> this.
> 
> dtrainor: please LMK if you're okay with my reasoning on the background
drawable
> before i land this.
> 
> thanks
> -fl
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
> (right):
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
> * native library is properly loaded.
> On 2017/02/07 19:36:16, David Trainor wrote:
> > Move back up?  100 columns for Java.
> 
> Done.
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:187:
> // compositor_impl_android.cc will use 565 EGL surfaces if and only if  we're
> using a low
> On 2017/02/07 19:36:16, David Trainor wrote:
> > Remove extra space after "only if"
> 
> Done.
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:195:
> // on non-low memory devices , if we are running on hardware that implements
> efficient alpha
> On 2017/02/07 19:36:16, David Trainor wrote:
> > Remove extra space before ","
> 
> Done.
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339:
> mCompositorSurfaceManager.doneWithUnownedSurface();
> On 2017/02/06 18:44:17, Ted C wrote:
> > Looking at the code in doneWithUnownedSurface, it looks like we do work even
> if
> > there isn't really an unowned surface.  If android destroys our surface but
we
> > don't change our pixel format, will this do everything as expected?
> > 
> > Looking at the code, it looks like it might just do unneeded work, but one
> final
> > thing I wanted to clarify.
> 
> it will doneWithUnowned -> detachSurfaceLater a few frames after android
> re-creates the surface, but that will early-out without posting.  the unowned
> surface isn't attached.
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:397:
> // SetBackgroundDrawable is depricated, and the semantics are the same I
think.
> On 2017/02/06 18:44:17, Ted C wrote:
> > depr[e]cated
> 
> Done.
> 
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398:
> super.setBackgroundDrawable(background);
> On 2017/02/06 18:44:17, Ted C wrote:
> > should we actually set the drawable in both places?  That would result in
> double
> > drawing.  Here, I "don't" think we should actually call into super.  Maybe
> just
> > super.setBackgroundDrawable(null) or something?
> 
> i think that we should.  i tried with / without both of these, along with some
> other variations (setting compositor to 0 all the time, etc.), and we get
> visible artifacts on startup.
> 
> the background drawable is set to 0 once we get some compositor frames anyway.

> so, i think that if there is double drawing, it's only for a frame or two
during
> startup.
> 
> so, if it's okay with you, i think that we should keep it.

Sorry for the delayed reply!  Feel free to ping me if I take more than 24 hours.
 I wonder if this is because because of setWillNotDraw on the SurfaceView
children?

[David Trainor- moved to gerrit, 2017-02-17 17:42:37]

On 2017/02/17 17:41:59, David Trainor wrote:
> On 2017/02/14 17:22:25, liberato wrote:
> > thanks for the feedback, everybody.  sorry for the delay in getting back to
> > this.
> > 
> > dtrainor: please LMK if you're okay with my reasoning on the background
> drawable
> > before i land this.
> > 
> > thanks
> > -fl
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> > File
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:88:
> > * native library is properly loaded.
> > On 2017/02/07 19:36:16, David Trainor wrote:
> > > Move back up?  100 columns for Java.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:187:
> > // compositor_impl_android.cc will use 565 EGL surfaces if and only if 
we're
> > using a low
> > On 2017/02/07 19:36:16, David Trainor wrote:
> > > Remove extra space after "only if"
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:195:
> > // on non-low memory devices , if we are running on hardware that implements
> > efficient alpha
> > On 2017/02/07 19:36:16, David Trainor wrote:
> > > Remove extra space before ","
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:339:
> > mCompositorSurfaceManager.doneWithUnownedSurface();
> > On 2017/02/06 18:44:17, Ted C wrote:
> > > Looking at the code in doneWithUnownedSurface, it looks like we do work
even
> > if
> > > there isn't really an unowned surface.  If android destroys our surface
but
> we
> > > don't change our pixel format, will this do everything as expected?
> > > 
> > > Looking at the code, it looks like it might just do unneeded work, but one
> > final
> > > thing I wanted to clarify.
> > 
> > it will doneWithUnowned -> detachSurfaceLater a few frames after android
> > re-creates the surface, but that will early-out without posting.  the
unowned
> > surface isn't attached.
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:397:
> > // SetBackgroundDrawable is depricated, and the semantics are the same I
> think.
> > On 2017/02/06 18:44:17, Ted C wrote:
> > > depr[e]cated
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2201483002/diff/400001/chrome/android/java/sr...
> >
>
chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java:398:
> > super.setBackgroundDrawable(background);
> > On 2017/02/06 18:44:17, Ted C wrote:
> > > should we actually set the drawable in both places?  That would result in
> > double
> > > drawing.  Here, I "don't" think we should actually call into super.  Maybe
> > just
> > > super.setBackgroundDrawable(null) or something?
> > 
> > i think that we should.  i tried with / without both of these, along with
some
> > other variations (setting compositor to 0 all the time, etc.), and we get
> > visible artifacts on startup.
> > 
> > the background drawable is set to 0 once we get some compositor frames
anyway.
> 
> > so, i think that if there is double drawing, it's only for a frame or two
> during
> > startup.
> > 
> > so, if it's okay with you, i think that we should keep it.
> 
> Sorry for the delayed reply!  Feel free to ping me if I take more than 24
hours.
>  I wonder if this is because because of setWillNotDraw on the SurfaceView
> children?

As a follow up, I'm ok with this since only one thing is actually drawing, but
it would be good to understand why :).

[liberato (no reviews please), 2017-02-17 17:48:25]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-02-17 17:48:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org, dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2201483002/#ps420001
(title: "rebased")

[commit-bot: I haz the power, 2017-02-17 17:49:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 18:09:53]

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

[commit-bot: I haz the power, 2017-02-17 18:09:54]

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

[liberato (no reviews please), 2017-02-22 19:13:14]

aelias: PTAL @ compositor_impl_android.

thanks
-fl

[aelias_OOO_until_Jul13, 2017-02-22 22:29:36]

lgtm

[liberato (no reviews please), 2017-02-22 22:30:39]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-02-22 22:31:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-23 01:11:39]

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

[commit-bot: I haz the power, 2017-02-23 01:11:41]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[liberato (no reviews please), 2017-03-08 18:55:09]

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

[commit-bot: I haz the power, 2017-03-08 18:55:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 19:56:16]

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

[commit-bot: I haz the power, 2017-03-08 19:56:17]

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

[liberato (no reviews please), 2017-03-10 17:41:21]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-10 17:41:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org, aelias@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2201483002/#ps440001
(title: "fixed tests, rebase fixes.")

[commit-bot: I haz the power, 2017-03-10 17:41:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 18:31:51]

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

[commit-bot: I haz the power, 2017-03-10 18:31:53]

Failed to apply patch for chrome/android/java_sources.gni:
While running git apply --index -p1;
  error: patch failed: chrome/android/java_sources.gni:1558
  error: chrome/android/java_sources.gni: patch does not apply

Patch:       chrome/android/java_sources.gni
Index: chrome/android/java_sources.gni
diff --git a/chrome/android/java_sources.gni b/chrome/android/java_sources.gni
index
29dba4afcae2be902f52f37696302e3df4695497..8ff451b6291c7fa17471f7c4a50fd7a5883064ce
100644
--- a/chrome/android/java_sources.gni
+++ b/chrome/android/java_sources.gni
@@ -126,6 +126,7 @@ chrome_java_sources = [
  
"java/src/org/chromium/chrome/browser/childaccounts/ChildAccountFeedbackReporter.java",
  
"java/src/org/chromium/chrome/browser/childaccounts/ChildAccountService.java",
  
"java/src/org/chromium/chrome/browser/childaccounts/ExternalFeedbackReporter.java",
+ 
"java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java",
   "java/src/org/chromium/chrome/browser/compositor/CompositorView.java",
   "java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java",
   "java/src/org/chromium/chrome/browser/compositor/Invalidator.java",
@@ -1558,6 +1559,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/ShortcutHelperTest.java",
   "junit/src/org/chromium/chrome/browser/SSLClientCertificateRequestTest.java",
  
"junit/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperTest.java",
+ 
"junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java",
  
"junit/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionControllerTest.java",
   "junit/src/org/chromium/chrome/browser/cookies/CanonicalCookieTest.java",
  
"junit/src/org/chromium/chrome/browser/crash/LogcatExtractionCallableUnitTest.java",

[liberato (no reviews please), 2017-03-10 18:41:52]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-10 18:41:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org, aelias@chromium.org,
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/2201483002/#ps460001
(title: "rebased")

[commit-bot: I haz the power, 2017-03-10 18:42:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 20:00:48]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1489171312289260,
"parent_rev": "5fe29f761d6791d746bd7e058d6f3d79227cc117", "commit_rev":
"1e87636c63028418b779dadeaadbe4e5f487c76e"}

[commit-bot: I haz the power, 2017-03-10 20:01:29]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130
==========

to

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

[commit-bot: I haz the power, 2017-03-10 20:01:30]

Committed patchset #24 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...

[klausw, 2017-03-11 01:20:41]

On 2017/03/10 20:01:30, commit-bot: I haz the power wrote:
> Committed patchset #24 (id:460001) as
>
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...

This change unfortunately breaks WebVR and probably VrShell, I confirmed via
bisect that 5fe29f761d6791d746bd7e058d6f3d79227cc117 is the last working change
right before this. It's plausible since it touches code such as detachForVR and
related code, seems like they don't agree on which surfaces should be visible.

[klausw, 2017-03-11 01:26:32]

A revert of this CL (patchset #24 id:460001) has been created in
https://codereview.chromium.org/2742133002/ by klausw@chromium.org.

The reason for reverting is: This change unfortunately breaks WebVR and probably
VrShell, I confirmed via
bisect that 5fe29f761d6791d746bd7e058d6f3d79227cc117 is the last working change
right before this. It's plausible since it touches code such as detachForVR and
related code, seems like they don't agree on which surfaces should be visible..

[klausw, 2017-03-11 01:29:45]

On 2017/03/11 01:26:32, klausw wrote:
> A revert of this CL (patchset #24 id:460001) has been created in
> https://codereview.chromium.org/2742133002/ by mailto:klausw@chromium.org.
> 
> The reason for reverting is: This change unfortunately breaks WebVR and
probably
> VrShell, I confirmed via
> bisect that 5fe29f761d6791d746bd7e058d6f3d79227cc117 is the last working
change
> right before this. It's plausible since it touches code such as detachForVR
and
> related code, seems like they don't agree on which surfaces should be
visible..

Proposed revert in https://codereview.chromium.org/2742133002/ . For the record,
here's how to reproduce the failure:

- load https://webvr.info/samples/03-vr-presentation.html
- press "Enter VR" button

This should transition into VR viewing mode, but it shows a white layer with the
VR divider drawn on top of it, the content isn't visible.

[liberato (no reviews please), 2017-03-15 21:07:31]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

to

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

[liberato (no reviews please), 2017-03-15 21:19:37]

Hi folks

sorry to re-open this, but I broke VR with the version that i landed.

PS25 has the fix.  turns out that setting visibility to INVISIBLE on a frame
doesn't notify any child views.  so, the SurfaceViews don't find out when VR
hides the compositor, and thus don't destroy the surface.  VR depends on the
fact that the surface is gone.  solution is just to propagate setVisibility
downwards.

can at least one of mdjones / dtrainor / tedc PTAL at the updated changes in
chrome/?

klausw: thanks for reverting.  added you FYI.

steimel: FYI.

thanks
-fl

[liberato (no reviews please), 2017-03-20 21:49:58]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

to

==========
Improve transition between opaque and translucent compositor views.

[Re-land after revert]

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

[liberato (no reviews please), 2017-03-20 21:53:09]

On 2017/03/15 21:19:37, liberato wrote:
> Hi folks
> 
> sorry to re-open this, but I broke VR with the version that i landed.
> 
> PS25 has the fix.  turns out that setting visibility to INVISIBLE on a frame
> doesn't notify any child views.  so, the SurfaceViews don't find out when VR
> hides the compositor, and thus don't destroy the surface.  VR depends on the
> fact that the surface is gone.  solution is just to propagate setVisibility
> downwards.
> 
> can at least one of mdjones / dtrainor / tedc PTAL at the updated changes in
> chrome/?
> 
> klausw: thanks for reverting.  added you FYI.
> 
> steimel: FYI.
> 
> thanks
> -fl

friendly reminder -- a revert required a few changes in chrome/ that need owners
approval.

thanks
-fl

[David Trainor- moved to gerrit, 2017-03-21 16:30:36]

rubberstamp lgtm

[liberato (no reviews please), 2017-03-21 17:02:06]

The CQ bit was checked by liberato@chromium.org

[liberato (no reviews please), 2017-03-21 17:02:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
mdjones@chromium.org, tedchoc@chromium.org, aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2201483002/#ps480001
(title: "now without VR-breaking badness")

[liberato (no reviews please), 2017-03-21 17:02:20]

The CQ bit was unchecked by liberato@chromium.org

[liberato (no reviews please), 2017-03-21 17:05:45]

The CQ bit was checked by liberato@chromium.org

[commit-bot: I haz the power, 2017-03-21 17:06:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 18:38:25]

CQ is committing da patch.

Bot data: {"patchset_id": 480001, "attempt_start_ts": 1490115945095250,
"parent_rev": "d369e670383cdee6cb66a4469b36e5308ed00aa2", "commit_rev":
"3fbc8b7641ee4665f400dee3641e7362b581d09a"}

[commit-bot: I haz the power, 2017-03-21 18:39:17]

Description was changed from

==========
Improve transition between opaque and translucent compositor views.

[Re-land after revert]

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
==========

to

==========
Improve transition between opaque and translucent compositor views.

[Re-land after revert]

On Android, when we'd like to use a SurfaceView for video playback,
we must enable the alpha channel on the CompositorView.  Otherwise,
we cannot draw a hole to see the video's SurfaceView.  We can't keep
transparency enabled all the time since it can cause power
regressions on some hardware.

However, because SurfaceView destroys and recreates the surface when
changing the pixel format, there is a visible flash during the
transition.  This CL removes that.

It causes the CompositorView to have two SurfaceView child views,
rather than extending SurfaceView directly.  When a request is made
to switch the output format, it makes the change to the background
SurfaceView, and swaps it to the front.  It also keeps the outgoing
SurfaceView around until the new one is in use by the compositor.

There are a few interesting bits:

- SurfaceViews are invisible until the first buffer is posted.  So,
  we can continue to see the outgoing view until the compositor is
  ready to use the new one.
- All buffers are released by the outgoing SurfaceView when its
  visibility is set to Gone.  'dumpsys SurfaceFlinger' shows that the
  same amount of gralloc memory is used in steady state.
- No power regression was observed on a Nexus 5.
- Unfortunately, new SurfaceViews are z-ordered behind existing ones,
  which makes it critical that we guess when to delete the outgoing
  (topmost) SurfaceView.  We currently time one frame, and then hide
  it.  Empirically, this works fine.
- This might seem like a more natural extension to
  CompositorViewHolder, but the CompositorView currently owns the
  native compositor.  Since CompositorViewHolder is already fairly
  complicated, it wasn't clear that it would be a good idea to
  refactor that into CVH.

Another approach is to use EGL to change the buffer format to include
an alpha channel, or not.  This avoids the power regression, since
opaque surfaces and buffers without alpha channels are treated the
same by SurfaceFlinger.  However, it causes problems for virtualized
contexts.  In particular, the off-screen contexts will have an alpha
channel at all times, so they cannot be shared with the compositor
context without one.  For some hardware (e.g., QC) that requires the
use of virtualized GL contexts rather than share groups, this may
have a stability regression.  So, we don't try it.

Please see https://goo.gl/aAmQzR for the design doc.

BUG=629130

Review-Url: https://codereview.chromium.org/2201483002
Cr-Original-Commit-Position: refs/heads/master@{#456142}
Committed:
https://chromium.googlesource.com/chromium/src/+/1e87636c63028418b779dadeaadb...
Review-Url: https://codereview.chromium.org/2201483002
Cr-Commit-Position: refs/heads/master@{#458486}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fbc8b7641ee4665f400dee3641e...
==========

[commit-bot: I haz the power, 2017-03-21 18:39:19]

Committed patchset #25 (id:480001) as
https://chromium.googlesource.com/chromium/src/+/3fbc8b7641ee4665f400dee3641e...