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 323781006ed24c7a9a8a9d55a05cfd984668e57a..b4d261e97f20311ec416df5a9e3807afe10a5d8f 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
@@ -1,4 +1,4 @@
-// Copyright 2015 The Chromium Authors. All rights reserved.
+// Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -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;
@@ -17,9 +18,9 @@ import android.view.SurfaceHolder;
 import android.view.SurfaceView;
 import android.view.View;
 import android.view.WindowManager;
+import android.widget.FrameLayout;
 
 import org.chromium.base.CommandLine;
-import org.chromium.base.Log;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
@@ -43,8 +44,7 @@ import org.chromium.ui.resources.ResourceManager;
  * The is the {@link View} displaying the ui compositor results; including webpages and tabswitcher.
  */
 @JNINamespace("android")
-public class CompositorView
-        extends SurfaceView implements SurfaceHolder.Callback {
+public class CompositorView extends FrameLayout implements SurfaceHolder.Callback {
     private static final String TAG = "CompositorView";
     private static final long NANOSECONDS_PER_MILLISECOND = 1000000;
 
@@ -52,6 +52,17 @@ public class CompositorView
     private final Rect mCacheAppRect = new Rect();
     private final int[] mCacheViewPosition = new int[2];
 
+    private CompositorSurfaceManager mCompositorSurfaceManager;
+    private boolean mOverlayVideoEnabled;
+    private boolean mAlwaysTranslucent;
+
+    // Are we waiting to hide the outgoing surface until the foreground has something to display?
+    // If == 0, then no.  If > 0, then yes.  We'll hide when it transitions from one to zero.
+    private int mFramesUntilHideBackground;
+
+    // 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;
@@ -70,16 +81,11 @@ public class CompositorView
     private TabContentManager mTabContentManager;
 
     private View mRootView;
-    private int mSurfaceWidth;
-    private int mSurfaceHeight;
     private boolean mPreloadedResources;
 
-    // The current SurfaceView pixel format. Defaults to OPAQUE.
-    private int mCurrentPixelFormat = PixelFormat.OPAQUE;
-
     /**
-     * Creates a {@link CompositorView}. This can be called only after the native library is
-     * properly loaded.
+     * Creates a {@link CompositorView}. This can be called only after the
+     * native library is properly loaded.

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

Move back up?  100 columns for Java.

[liberato (no reviews please), 2017/02/14 17:22:24]

Done.

      * @param c        The Context to create this {@link CompositorView} in.
      * @param host     The renderer host owning this view.
      */
@@ -87,8 +93,8 @@ public class CompositorView
         super(c);
         mRenderHost = host;
         resetFlags();
+
         setVisibility(View.INVISIBLE);
-        setZOrderMediaOverlay(true);
     }
 
     /**
@@ -157,7 +163,7 @@ public class CompositorView
      * Should be called for cleanup when the CompositorView instance is no longer used.
      */
     public void shutDown() {
-        getHolder().removeCallback(this);
+        mCompositorSurfaceManager.shutDown();
         if (mNativeCompositorView != 0) nativeDestroy(mNativeCompositorView);
         mNativeCompositorView = 0;
     }
@@ -178,9 +184,23 @@ public class CompositorView
         mNativeCompositorView = nativeInit(lowMemDevice,
                 windowAndroid.getNativePointer(), layerTitleCache, tabContentManager);
 
-        assert !getHolder().getSurface().isValid()
-            : "Surface created before native library loaded.";
-        getHolder().addCallback(this);
+        // compositor_impl_android.cc will use 565 EGL surfaces if and only if  we're using a low

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

Remove extra space after "only if"

[liberato (no reviews please), 2017/02/14 17:22:24]

Done.

+        // 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

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

Remove extra space before ","

[liberato (no reviews please), 2017/02/14 17:22:24]

Done.

+        // blending.
+        mAlwaysTranslucent = lowMemDevice;
+
+        mCompositorSurfaceManager = new CompositorSurfaceManager(this, this);
+        // In case the background drawable was set before now, update it.
+        mCompositorSurfaceManager.setBackgroundDrawable(getBackground());
+        mCompositorSurfaceManager.setWillNotDraw(mWillNotDraw);
+        mCompositorSurfaceManager.requestSurface(getSurfacePixelFormat());
 
         // Cover the black surface before it has valid content.
         setBackgroundColor(Color.WHITE);
@@ -208,34 +228,53 @@ public class CompositorView
     }
 
     /**
+     * @see SurfaceView#getHolder
+     */
+    SurfaceHolder getHolder() {
+        return mCompositorSurfaceManager.getHolder();
+    }
+
+    /**
      * Enables/disables overlay video mode. Affects alpha blending on this view.
      * @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);
+
+        mOverlayVideoEnabled = enabled;
+        // Request the new surface, even if it's the same as the old one.  We'll get a synthetic
+        // destroy / create / changed callback in that case, possibly before this returns.
+        mCompositorSurfaceManager.requestSurface(getSurfacePixelFormat());
+        // Note that we don't know if we'll get a surfaceCreated / surfaceDestoyed for this surface.
+        // We do know that if we do get one, then it will be for the surface that we just requested.
+    }
+
+    private int getSurfacePixelFormat() {
+        return (mOverlayVideoEnabled || mAlwaysTranslucent) ? PixelFormat.TRANSLUCENT
+                                                            : PixelFormat.OPAQUE;
     }
 
     @Override
     public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
         if (mNativeCompositorView == 0) return;
+
         nativeSurfaceChanged(mNativeCompositorView, format, width, height, holder.getSurface());
         mRenderHost.onPhysicalBackingSizeChanged(width, height);
-        mSurfaceWidth = width;
-        mSurfaceHeight = height;
     }
 
     @Override
     public void surfaceCreated(SurfaceHolder holder) {
         if (mNativeCompositorView == 0) return;
+
         nativeSurfaceCreated(mNativeCompositorView);
+        mFramesUntilHideBackground = 2;
         mRenderHost.onSurfaceCreated();
     }
 
     @Override
     public void surfaceDestroyed(SurfaceHolder holder) {
         if (mNativeCompositorView == 0) return;
+
         nativeSurfaceDestroyed(mNativeCompositorView);
     }
 
@@ -264,29 +303,7 @@ public class CompositorView
      */
     @CalledByNative
     private void onJellyBeanSurfaceDisconnectWorkaround(boolean inOverlayMode) {
-        // There is a bug in JellyBean because of which we will not be able to
-        // reconnect to the existing Surface after we launch a new GPU process.
-        // We simply trick the JB Android code to allocate a new Surface.
-        // It does a strict comparison between the current format and the requested
-        // 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) {
-            case PixelFormat.OPAQUE:
-                mCurrentPixelFormat = PixelFormat.RGBA_8888;
-                break;
-            case PixelFormat.RGBA_8888:
-                mCurrentPixelFormat = inOverlayMode
-                        ? PixelFormat.TRANSLUCENT : PixelFormat.OPAQUE;
-                break;
-            case PixelFormat.TRANSLUCENT:
-                mCurrentPixelFormat = PixelFormat.RGBA_8888;
-                break;
-            default:
-                assert false;
-                Log.e(TAG, "Unknown current pixel format.");
-        }
-        getHolder().setFormat(mCurrentPixelFormat);
+        mCompositorSurfaceManager.recreateSurfaceForJellyBean();
     }
 
     /**
@@ -308,6 +325,20 @@ public class CompositorView
             }, mFramePresentationDelay);
         }
 
+        // If we're in the middle of a surface swap, then see if we've received a new frame yet for
+        // the new surface before hiding the outgoing surface.
+        if (mFramesUntilHideBackground > 1) {
+            // We need at least one more frame before we hide the outgoing surface.  Make sure that
+            // there will be a frame.
+            mFramesUntilHideBackground--;
+            requestRender();
+        } else if (mFramesUntilHideBackground == 1) {
+            // We can hide the outgoing surface, since the incoming one has a frame.  It's okay if
+            // we've don't have an unowned surface.
+            mFramesUntilHideBackground = 0;
+            mCompositorSurfaceManager.doneWithUnownedSurface();

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

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.

[liberato (no reviews please), 2017/02/14 17:22:24]

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.

+        }
+
         mRenderHost.onSwapBuffersCompleted(pendingSwapBuffersCount);
     }
 
@@ -351,6 +382,25 @@ public class CompositorView
         TraceEvent.end("CompositorView:finalizeLayers");
     }
 
+    @Override
+    public void setWillNotDraw(boolean willNotDraw) {
+        mWillNotDraw = willNotDraw;
+        if (mCompositorSurfaceManager != null) {
+            mCompositorSurfaceManager.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.

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

depr[e]cated

[liberato (no reviews please), 2017/02/14 17:22:24]

Done.

+        super.setBackgroundDrawable(background);

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

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?

[liberato (no reviews please), 2017/02/14 17:22:24]

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.

+        if (mCompositorSurfaceManager != null) {
+            mCompositorSurfaceManager.setBackgroundDrawable(background);
+        }
+    }
+
     // Implemented in native
     private native long nativeInit(boolean lowMemDevice, long nativeWindowAndroid,
             LayerTitleCache layerTitleCache, TabContentManager tabContentManager);