Issue 629018  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1e87636c63028418b779dadeaadbe4e5f487c76e

commit 1e87636c63028418b779dadeaadbe4e5f487c76e
Author: liberato <liberato@chromium.org>
Date: Fri Mar 10 20:00:43 2017

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}

[modify] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[add] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
[modify] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/java_sources.gni
[add] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
[modify] https://crrev.com/1e87636c63028418b779dadeaadbe4e5f487c76e/content/browser/renderer_host/compositor_impl_android.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e31bd807912a2540769e990c9f053d209bf383b

commit 2e31bd807912a2540769e990c9f053d209bf383b
Author: klausw <klausw@chromium.org>
Date: Sat Mar 11 01:34:18 2017

Revert of Improve transition between opaque and translucent compositor views. (patchset #24 id:460001 of https://codereview.chromium.org/2201483002/ )

Reason for revert:
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.

Original issue's description:
> 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/+/1e87636c63028418b779dadeaadbe4e5f487c76e

TBR=aelias@chromium.org,dtrainor@chromium.org,mdjones@chromium.org,tedchoc@chromium.org,watk@chromium.org,liberato@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 629130 

Review-Url: https://codereview.chromium.org/2742133002
Cr-Commit-Position: refs/heads/master@{#456245}

[modify] https://crrev.com/2e31bd807912a2540769e990c9f053d209bf383b/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[delete] https://crrev.com/dd3c8c6e5537c766c0db2a05a1cb744f4b93bf19/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
[modify] https://crrev.com/2e31bd807912a2540769e990c9f053d209bf383b/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/2e31bd807912a2540769e990c9f053d209bf383b/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/2e31bd807912a2540769e990c9f053d209bf383b/chrome/android/java_sources.gni
[delete] https://crrev.com/dd3c8c6e5537c766c0db2a05a1cb744f4b93bf19/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
[modify] https://crrev.com/2e31bd807912a2540769e990c9f053d209bf383b/content/browser/renderer_host/compositor_impl_android.cc

Sorry about the rollback. This is probably easy to fix, please contact me on Mon if you want to collaborate on getting this sorted out.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3fbc8b7641ee4665f400dee3641e7362b581d09a

commit 3fbc8b7641ee4665f400dee3641e7362b581d09a
Author: liberato <liberato@chromium.org>
Date: Tue Mar 21 18:38:18 2017

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}

[modify] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
[add] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManager.java
[modify] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
[modify] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorViewHolder.java
[modify] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/java_sources.gni
[add] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/chrome/android/junit/src/org/chromium/chrome/browser/compositor/CompositorSurfaceManagerTest.java
[modify] https://crrev.com/3fbc8b7641ee4665f400dee3641e7362b581d09a/content/browser/renderer_host/compositor_impl_android.cc

Since this fix is only for Chrome, "visible flash during the fullscreen transition" still reproduces in content_shell. Is there any plan to fix it in the content level?

at the moment, i don't have any plans to do so.

the method is a fairly complicated hack, so i didn't want to force it on all embedders.  it's somewhat nice that ContentViewRenderView is simple.

if we did want to make this available in content, i'd suggest something like:

 - move CompositorSurfaceManager + Impl into content
 - modify CVRV to use a CompositorSurfaceManager
 - create a new, simple CompositorSurfaceManager impl that uses only one SV in
   the same way that CVRV does now.
 - allow the embedder to provide the CSM implementation that CVRV uses.

i don't have the bandwidth to pick that up right now.  i'm also not a content owner, so it's unclear that there would be agreement that this is a good idea.  :)