Improve transition between opaque and translucent compositor views.

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

 // Copyright 2015 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.
 
 package org.chromium.chrome.browser.compositor;
 
 import android.app.Activity;
 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;
 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;
 import org.chromium.base.TraceEvent;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.compositor.layouts.Layout;
 import org.chromium.chrome.browser.compositor.layouts.LayoutProvider;
@@ skipping 10 matching lines @@
 import org.chromium.ui.base.DeviceFormFactor;
 import org.chromium.ui.base.WindowAndroid;
 import org.chromium.ui.resources.AndroidResourceType;
 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 ContentOffsetProvider, SurfaceHolder.Callback {
+        extends FrameLayout implements ContentOffsetProvider, SurfaceHolder.Callback {
     private static final String TAG = "CompositorView";
     private static final long NANOSECONDS_PER_MILLISECOND = 1000000;
 
     // Cache objects that should not be created every frame
     private final Rect mCacheViewport = new Rect();
     private final Rect mCacheAppRect = new Rect();
     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;
     private int mPreviousWindowTop = -1;
 
     private int mLastLayerCount;
 
     // A conservative estimate of when a frame is guaranteed to be presented after being submitted.
     private long mFramePresentationDelay;
 
     // Resource Management
     private ResourceManager mResourceManager;
 
     // Lazily populated as it is needed.
     private View mRootActivityView;
     private WindowAndroid mWindowAndroid;
     private LayerTitleCache mLayerTitleCache;
     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;
+    // 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
      * properly loaded.
      * @param c        The Context to create this {@link CompositorView} in.
      * @param host     The renderer host owning this view.
      */
     public CompositorView(Context c, LayoutRenderHost host) {
         super(c);
         mRenderHost = host;
         resetFlags();
+
+        mForegroundSurfaceView = createSurfaceView();
+        attachSurfaceView(mForegroundSurfaceView);
+
         setVisibility(View.INVISIBLE);
-        setZOrderMediaOverlay(true);
+        bringChildToFront(mForegroundSurfaceView);
     }
 
     /**
      * @param view The root view of the hierarchy.
      */
     public void setRootView(View view) {
         mRootView = view;
     }
 
     /**
@@ skipping 48 matching lines @@
      * @return The ResourceManager.
      */
     public ResourceManager getResourceManager() {
         return mResourceManager;
     }
 
     /**
      * 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;
     }
 
     /**
      * Initializes the {@link CompositorView}'s native parts (e.g. the rendering parts).
      * @param lowMemDevice         If this is a low memory device.
      * @param windowAndroid        A {@link WindowAndroid} instance.
      * @param layerTitleCache      A {@link LayerTitleCache} instance.
      * @param tabContentManager    A {@link TabContentManager} instance.
      */
     public void initNativeCompositor(boolean lowMemDevice, WindowAndroid windowAndroid,
             LayerTitleCache layerTitleCache, TabContentManager tabContentManager) {
         mWindowAndroid = windowAndroid;
         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) {
             Display display =
                     ((WindowManager) getContext().getSystemService(Context.WINDOW_SERVICE))
                     .getDefaultDisplay();
             long presentationDeadline = display.getPresentationDeadlineNanos()
                     / NANOSECONDS_PER_MILLISECOND;
             long vsyncPeriod = mWindowAndroid.getVsyncPeriodInMillis();
             mFramePresentationDelay = Math.min(3 * vsyncPeriod,
                     ((presentationDeadline + vsyncPeriod - 1) / vsyncPeriod) * vsyncPeriod);
         }
 
         // Grab the Resource Manager
         mResourceManager = nativeGetResourceManager(mNativeCompositorView);
     }
 
     @Override
     public boolean onTouchEvent(MotionEvent e) {
         return super.onTouchEvent(e);
     }
 
     /**
      * 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);
+        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;
         mSurfaceHeight = height;
     }
 
     @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();
     }
 
     @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);
     }
 
     @Override
     public void onWindowVisibilityChanged(int visibility) {
         super.onWindowVisibilityChanged(visibility);
         if (mWindowAndroid == null) return;
         if (visibility == View.GONE) {
             mWindowAndroid.onVisibilityChanged(false);
         } else if (visibility == View.VISIBLE) {
@@ skipping 15 matching lines @@
      */
     @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) {
+        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);
     }
 
     /**
      * Request compositor view to render a frame.
      */
     public void requestRender() {
         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.
         if (getBackground() != null) {
             postDelayed(new Runnable() {
                 @Override
                 public void run() {
                     setBackgroundResource(0);
                 }
             }, 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);
     }
 
     /**
      * Converts the layout into compositor layers. This is to be called on every frame the layout
      * is changing.
      * @param provider               Provides the layout to be rendered.
      * @param forRotation            Whether or not this is a special draw during a rotation.
      */
     public void finalizeLayers(final LayoutProvider provider, boolean forRotation,
@@ skipping 50 matching lines @@
         return mLastLayerCount;
     }
 
     @Override
     public int getOverlayTranslateY() {
         return mRenderHost.areTopControlsPermanentlyHidden()
                 ? mRenderHost.getTopControlsHeightPixels()
                 : 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);
     private native void nativeDestroy(long nativeCompositorView);
     private native ResourceManager nativeGetResourceManager(long nativeCompositorView);
     private native void nativeSurfaceCreated(long nativeCompositorView);
     private native void nativeSurfaceDestroyed(long nativeCompositorView);
     private native void nativeSurfaceChanged(
             long nativeCompositorView, int format, int width, int height, Surface surface);
     private native void nativeFinalizeLayers(long nativeCompositorView);
     private native void nativeSetNeedsComposite(long nativeCompositorView);
     private native void nativeSetLayoutViewport(long nativeCompositorView, float x, float y,
             float width, float height, float visibleXOffset, float visibleYOffset,
             float dpToPixel);
     private native void nativeSetOverlayVideoMode(long nativeCompositorView, boolean enabled);
     private native void nativeSetSceneLayer(long nativeCompositorView, SceneLayer sceneLayer);
 }