Improve transition between opaque and translucent compositor views.

chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java

Index: chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java b/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
index caa0d341b67dd20f4e2372cf95c52bb6d59a2359..e8fa932b235ac5562d77989b1588e30d8ae7474e 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/compositor/CompositorView.java
@@ -9,6 +9,7 @@ import android.content.Context;
 import android.graphics.Color;
 import android.graphics.PixelFormat;
 import android.graphics.Rect;
+import android.graphics.drawable.Drawable;
 import android.os.Build;
 import android.view.Display;
 import android.view.MotionEvent;
@@ -16,7 +17,9 @@ import android.view.Surface;
 import android.view.SurfaceHolder;
 import android.view.SurfaceView;
 import android.view.View;
+import android.view.ViewGroup;
 import android.view.WindowManager;
+import android.widget.FrameLayout;
 
 import org.chromium.base.CommandLine;
 import org.chromium.base.Log;
@@ -47,7 +50,7 @@ import org.chromium.ui.resources.ResourceManager;
  */
 @JNINamespace("android")
 public class CompositorView
-        extends SurfaceView implements ContentOffsetProvider, SurfaceHolder.Callback {
+        extends FrameLayout implements ContentOffsetProvider, SurfaceHolder.Callback {
     private static final String TAG = "CompositorView";
     private static final long NANOSECONDS_PER_MILLISECOND = 1000000;
 
@@ -57,6 +60,31 @@ public class CompositorView
     private final Rect mCacheVisibleViewport = new Rect();
     private final int[] mCacheViewPosition = new int[2];
 
+    // If true, then we'll use one surface, and always make it translucent.
+    // This can have power implications unless the compositor is using a format
+    // that does not have an alpha channel for opaque rendering (e.g., 565), or
+    // if the hardware is optimized for it (e.g., some QC GPUs).
+    private boolean mAlwaysTranslucent;
+
+    // Set of SurfaceViews that we maintain.
+    // CompositorViewHolder might be a more natural place to maintain these,
+    // except that we own the compositor.

[boliu, 2016/10/12 23:33:34]

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.

[liberato (no reviews please), 2016/11/14 22:12:34]

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.

+    private SurfaceView mForegroundSurfaceView;

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

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?

[liberato (no reviews please), 2016/11/14 22:12:34]

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.

+
+    // This will be null if |mAlwaysTranslucent|, since we never switch the
+    // pixel format in that case.
+    private SurfaceView mBackgroundSurfaceView;
+
+    // Are we waiting to hide mBackgroundSurfaceView until the foreground SV
+    // has something to display?
+    private boolean mHideBackgroundPending;
+
+    // How many frames remain until we hide the newly-swapped background?
+    private int mFramesUntilHide;
+
+    // Cache of setWillNotDraw, which we keep up to date if we switch SVs.
+    private boolean mWillNotDraw;
+
     private long mNativeCompositorView;
     private final LayoutRenderHost mRenderHost;
     private boolean mEnableTabletTabStack;
@@ -81,8 +109,12 @@ public class CompositorView
     private int mSurfaceHeight;
     private boolean mPreloadedResources;
 
-    // The current SurfaceView pixel format. Defaults to OPAQUE.
-    private int mCurrentPixelFormat = PixelFormat.OPAQUE;
+    // The pixel format that we've told the compositor about.
+    private int mVirtualPixelFormat = PixelFormat.OPAQUE;
+
+    // The current SurfaceView pixel format.  Defaults to OPAQUE.  This will be
+    // the same as mVirtualPixelFormat unless we're mAlwaysTranslucent.
+    private int mActualPixelFormat = PixelFormat.OPAQUE;
 
     /**
      * Creates a {@link CompositorView}. This can be called only after the native library is
@@ -94,8 +126,12 @@ public class CompositorView
         super(c);
         mRenderHost = host;
         resetFlags();
+
+        mForegroundSurfaceView = createSurfaceView();
+        attachSurfaceView(mForegroundSurfaceView);
+
         setVisibility(View.INVISIBLE);
-        setZOrderMediaOverlay(true);
+        bringChildToFront(mForegroundSurfaceView);
     }
 
     /**
@@ -164,7 +200,8 @@ public class CompositorView
      * Should be called for cleanup when the CompositorView instance is no longer used.
      */
     public void shutDown() {
-        getHolder().removeCallback(this);
+        mForegroundSurfaceView.getHolder().removeCallback(this);
+        if (mBackgroundSurfaceView != null) mBackgroundSurfaceView.getHolder().removeCallback(this);
         if (mNativeCompositorView != 0) nativeDestroy(mNativeCompositorView);
         mNativeCompositorView = 0;
     }
@@ -182,16 +219,40 @@ public class CompositorView
         mLayerTitleCache = layerTitleCache;
         mTabContentManager = tabContentManager;
 
+        // compositor_impl_android.cc will use 565 EGL surfaces if and only if
+        // we're using a low memory device, and no alpha channel is desired.
+        // Otherwise, it will use 8888.  Since SurfaceFlinger doesn't need
+        // the eOpaque flag to optimize out alpha blending during composition if
+        // the buffer has no alpha channel, we can avoid using the extra
+        // background surface (and the memory it requires) in the low memory
+        // case.  The output buffer will either have an alpha channel or not,
+        // depending on whether the compositor needs it.
+        // We can keep the surface translucent all the times without worrying
+        // about the impact on power usage during SurfaceFlinger composition.
+        // We might also want to set |mAlwaysTranslucent| on non-low memory
+        // devices , if we are running on hardware that implements efficient
+        // alpha blending.
+        mAlwaysTranslucent = lowMemDevice;
+        if (mAlwaysTranslucent) {
+            mActualPixelFormat = PixelFormat.TRANSLUCENT;
+            mForegroundSurfaceView.getHolder().setFormat(mActualPixelFormat);
+        } else {
+            // We will switch between background and foreground SurfaceViews.
+            mBackgroundSurfaceView = createSurfaceView();
+        }
+
         mNativeCompositorView = nativeInit(lowMemDevice,
                 windowAndroid.getNativePointer(), layerTitleCache, tabContentManager);
 
         assert !getHolder().getSurface().isValid()
             : "Surface created before native library loaded.";
-        getHolder().addCallback(this);
+        mForegroundSurfaceView.getHolder().addCallback(this);
+        if (mBackgroundSurfaceView != null) mBackgroundSurfaceView.getHolder().addCallback(this);
 
         // Cover the black surface before it has valid content.
         setBackgroundColor(Color.WHITE);
         setVisibility(View.VISIBLE);
+        mForegroundSurfaceView.setVisibility(View.VISIBLE);
 
         mFramePresentationDelay = 0;
         if (Build.VERSION.SDK_INT > Build.VERSION_CODES.M) {
@@ -219,14 +280,49 @@ public class CompositorView
      * @param enabled Whether to enter or leave overlay video mode.
      */
     public void setOverlayVideoMode(boolean enabled) {
-        mCurrentPixelFormat = enabled ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
-        getHolder().setFormat(mCurrentPixelFormat);
-        nativeSetOverlayVideoMode(mNativeCompositorView, enabled);
+        int oldFormat = mVirtualPixelFormat;
+        mVirtualPixelFormat = enabled ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;

[boliu, 2016/10/12 23:33:34]

would it be easier to just keep a boolean mOverlayVideoModeEnabled?

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

[liberato (no reviews please), 2016/11/14 22:12:34]

changed significantly due to the refactor.

+        if (mVirtualPixelFormat == oldFormat) return;
+
+        // Figure out what we want to tell the surface.
+        mActualPixelFormat = mAlwaysTranslucent ? PixelFormat.TRANSLUCENT : mVirtualPixelFormat;
+
+        if (!mForegroundSurfaceView.getHolder().getSurface().isValid()) {
+            // No current view -- just set the format on it.  The compositor
+            // will get a surfaceCreated message later.
+            getHolder().setFormat(mActualPixelFormat);
+            nativeSetOverlayVideoMode(mNativeCompositorView, enabled);
+            return;
+        }
+
+        if (mAlwaysTranslucent) {
+            // Update the compositor by pretending that the surface changed.  We
+            // don't need to update the actual pixel format, though.
+            nativeSurfaceDestroyed(mNativeCompositorView);

[boliu, 2016/10/12 23:33:34]

is Destroy/Created needed here? previous code didn't do that?

[liberato (no reviews please), 2016/11/14 22:12:34]

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.

+            nativeSetOverlayVideoMode(mNativeCompositorView, enabled);
+            nativeSurfaceCreated(mNativeCompositorView);
+            return;
+        }
+
+        // Update the actual pixel format by switching to the other SurfaceView.
+        // The rest of the transition will happen when the surface is ready.
+        mBackgroundSurfaceView.getHolder().setFormat(mActualPixelFormat);
+        attachSurfaceView(mBackgroundSurfaceView);
+        mBackgroundSurfaceView.setVisibility(View.VISIBLE);
+        requestLayout();

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

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)

[liberato (no reviews please), 2016/11/14 22:12:34]

Done.

+        invalidate();
+    }
+
+    public SurfaceHolder getHolder() {
+        return mForegroundSurfaceView.getHolder();
     }
 
     @Override
     public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
         if (mNativeCompositorView == 0) return;
+
+        if (isBackgroundHolder(holder)) return;

[boliu, 2016/10/12 23:33:34]

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.

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

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

+
         nativeSurfaceChanged(mNativeCompositorView, format, width, height, holder.getSurface());
         mRenderHost.onPhysicalBackingSizeChanged(width, height);
         mSurfaceWidth = width;
@@ -236,6 +332,32 @@ public class CompositorView
     @Override
     public void surfaceCreated(SurfaceHolder holder) {
         if (mNativeCompositorView == 0) return;
+
+        if (isBackgroundHolder(holder)) {
+            // We requested that background view as part of a surface swap.
+            // Switch the compositor.
+            nativeSurfaceDestroyed(mNativeCompositorView);
+            nativeSetOverlayVideoMode(
+                    mNativeCompositorView, mVirtualPixelFormat == PixelFormat.TRANSLUCENT);
+
+            // Switch the primary and secondary surfaces.
+            SurfaceView tmp = mForegroundSurfaceView;
+            mForegroundSurfaceView = mBackgroundSurfaceView;
+            mBackgroundSurfaceView = tmp;
+
+            // We will wait to actually hide the new background surface until
+            // the foreground surface has something to show.  Note that,
+            // unfortunately, the new foreground surface is actually z-ordered
+            // below the new background, so mBackgroundSurfaceView will prevent
+            // new compositor updates from showing until we hide it.
+            mHideBackgroundPending = true;
+            mFramesUntilHide = 1;
+
+            // Refresh any properties that we proxy for the SurfaceView.
+            mForegroundSurfaceView.setWillNotDraw(mWillNotDraw);
+            mForegroundSurfaceView.setBackground(getBackground());
+        }
+
         nativeSurfaceCreated(mNativeCompositorView);
         mRenderHost.onSurfaceCreated();
     }
@@ -243,6 +365,10 @@ public class CompositorView
     @Override
     public void surfaceDestroyed(SurfaceHolder holder) {
         if (mNativeCompositorView == 0) return;
+
+        // If the non-primary surface has been destroyed, then ignore it.
+        if (isBackgroundHolder(holder)) return;
+
         nativeSurfaceDestroyed(mNativeCompositorView);
     }
 
@@ -278,22 +404,21 @@ public class CompositorView
         // one, even if they are the same in practice. Furthermore, the format
         // does not matter here since the producer-side EGL config overwrites it
         // (but transparency might matter).
-        switch (mCurrentPixelFormat) {
+        switch (mActualPixelFormat) {
             case PixelFormat.OPAQUE:
-                mCurrentPixelFormat = PixelFormat.RGBA_8888;
+                mActualPixelFormat = PixelFormat.RGBA_8888;
                 break;
             case PixelFormat.RGBA_8888:
-                mCurrentPixelFormat = inOverlayMode
-                        ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
+                mActualPixelFormat = inOverlayMode ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
                 break;
             case PixelFormat.TRANSLUCENT:
-                mCurrentPixelFormat = PixelFormat.RGBA_8888;
+                mActualPixelFormat = PixelFormat.RGBA_8888;
                 break;
             default:
                 assert false;
                 Log.e(TAG, "Unknown current pixel format.");
         }
-        getHolder().setFormat(mCurrentPixelFormat);
+        getHolder().setFormat(mActualPixelFormat);
     }
 
     /**
@@ -303,6 +428,10 @@ public class CompositorView
         if (mNativeCompositorView != 0) nativeSetNeedsComposite(mNativeCompositorView);
     }
 
+    private boolean isBackgroundHolder(SurfaceHolder holder) {
+        return mBackgroundSurfaceView != null && holder == mBackgroundSurfaceView.getHolder();
+    }
+
     @CalledByNative
     private void onSwapBuffersCompleted(int pendingSwapBuffersCount) {
         // Clear the color used to cover the uninitialized surface.
@@ -315,6 +444,21 @@ public class CompositorView
             }, mFramePresentationDelay);
         }
 
+        // If we're in the middle of a surface swap, then see if we've received
+        // enough frames to hide the outgoing surface.
+        if (mHideBackgroundPending) {
+            if (mFramesUntilHide > 0) {

[boliu, 2016/10/12 23:33:34]

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/11/14 22:12:34]

arbitrary delay, unfortunately.

+                mFramesUntilHide--;
+                // Make sure that there actually will be a frame.
+                requestRender();
+            } else {
+                // Remove the background so that we're sure that the underlying
+                // Surface is destroyed.
+                removeView(mBackgroundSurfaceView);
+                mHideBackgroundPending = false;
+            }
+        }
+
         mRenderHost.onSwapBuffersCompleted(pendingSwapBuffersCount);
     }
 
@@ -385,6 +529,38 @@ public class CompositorView
                 : mRenderHost.getVisibleViewport(mCacheVisibleViewport).top;
     }
 
+    @Override
+    public void setWillNotDraw(boolean willNotDraw) {
+        mWillNotDraw = willNotDraw;
+        mForegroundSurfaceView.setWillNotDraw(mWillNotDraw);
+    }
+
+    @Override
+    public void setBackgroundDrawable(Drawable background) {
+        // We override setBackgroundDrawable since that's the common entry point
+        // from all the setBackground* calls in View.  We still call to
+        // setBackground on the SurfaceView because SetBackgroundDrawable is
+        // depricated, and the semantics are the same I think.
+        super.setBackgroundDrawable(background);
+        mForegroundSurfaceView.setBackground(background);
+    }
+
+    /**
+     * Helper to create and attach a SurfaceView
+     */
+    private SurfaceView createSurfaceView() {
+        SurfaceView surfaceView = new SurfaceView(getContext());
+        surfaceView.setZOrderMediaOverlay(true);
+        surfaceView.setVisibility(View.INVISIBLE);
+        return surfaceView;
+    }
+
+    private void attachSurfaceView(SurfaceView surfaceView) {
+        FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams(
+                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);
+        addView(surfaceView, lp);
+    }
+
     // Implemented in native
     private native long nativeInit(boolean lowMemDevice, long nativeWindowAndroid,
             LayerTitleCache layerTitleCache, TabContentManager tabContentManager);