Improve transition between opaque and translucent compositor views.

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

+// Copyright 2017 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.content.Context;
+import android.graphics.PixelFormat;
+import android.graphics.drawable.Drawable;
+import android.os.Build;
+import android.view.SurfaceHolder;
+import android.view.SurfaceView;
+import android.view.View;
+import android.view.ViewGroup;
+import android.widget.FrameLayout;
+
+/**
+ * Manage multiple SurfaceViews for the compositor, so that transitions between
+ * surfaces with and without an alpha channel can be visually smooth.
+ *
+ * This class allows a client to request a 'translucent' or 'opaque' surface, and we will signal via
+ * SurfaceHolder.Callback when it's ready.  We guarantee that the client will receive surfaceCreated
+ * / surfaceDestroyed only for a surface that represents the most recently requested PixelFormat.
+ *
+ * Internally, we maintain two SurfaceViews, since calling setFormat() to change the PixelFormat
+ * results in a visual glitch as the surface is torn down.  crbug.com/679902
+ *
+ * The client has the responsibility to call doneWithUnownedSurface() at some point between when we
+ * call back its surfaceCreated, when it is safe for us to hide the SurfaceView with the wrong
+ * format.
+ *
+ * We do not guarantee that the surface will always actually have that format, though, if we're set
+ * up to always provide a translucent surface.  This helps low-memory devices, but not requiring two
+ * SurfaceViews.  However, it does require that the compositor use the EGL config to turn off alpha
+ * blending.  For low memory devices, this already happens, since it selects between 565 and 8888
+ * EGL configs.  In this case, we will still provide synthetic surfaceCreated / surfaceDestroyed
+ * messages to the client, even though the underlying surface isn't changing.
+ *
+ * The full design doc is at https://goo.gl/aAmQzR .
+ */
+class CompositorSurfaceManager implements SurfaceHolder.Callback {
+    private static class SurfaceState {
+        public SurfaceView surfaceView;
+
+        // Have we started destroying |surfaceView|, but haven't been notified of it yet?
+        public boolean destroyPending;
+
+        public SurfaceState(Context context, int format, SurfaceHolder.Callback callback) {
+            surfaceView = new SurfaceView(context);
+            surfaceView.setZOrderMediaOverlay(true);
+            surfaceView.setVisibility(View.VISIBLE);
+            surfaceHolder().setFormat(format);
+            surfaceHolder().addCallback(callback);
+        }
+
+        public SurfaceHolder surfaceHolder() {
+            return surfaceView.getHolder();
+        }
+
+        public boolean isValid() {
+            return surfaceHolder().getSurface().isValid();
+        }
+    }
+
+    // SurfaceView with a translucent PixelFormat.
+    private final SurfaceState mTranslucent;
+
+    // SurfaceView with an opaque PixelFormat.
+    private final SurfaceState mOpaque;
+
+    // Surface that we last gave to the client with surfaceCreated.  Cleared when we call
+    // surfaceDestroyed.
+    private SurfaceState mOwnedByClient;
+
+    // Surface that was most recently requested by the client.
+    private SurfaceState mRequestedByClient;
+
+    // Client that we notify about surface change events.
+    private SurfaceHolder.Callback mClient;
+
+    // View to which we'll attach the SurfaceView.
+    private final ViewGroup mParentView;
+
+    public CompositorSurfaceManager(ViewGroup parentView, SurfaceHolder.Callback client) {
+        mParentView = parentView;
+        mClient = client;
+
+        mTranslucent = new SurfaceState(parentView.getContext(), PixelFormat.TRANSLUCENT, this);
+        mOpaque = new SurfaceState(mParentView.getContext(), PixelFormat.OPAQUE, this);
+    }
+
+    /**
+     * Turn off everything.
+     */
+    public void shutDown() {
+        mTranslucent.surfaceHolder().removeCallback(this);
+        mOpaque.surfaceHolder().removeCallback(this);
+    }
+
+    /**
+     * Called by the client to request a surface.  Once called, we guarantee that the next call to
+     * onSurfaceAvailable will match the most recent value of |format|.  If the surface is already
+     * available for use, then we'll elide the created callback.  Note that |format| must be either
+     * OPAQUE or TRANSLUCENT.
+     */
+    public void requestSurface(int format) {
+        mRequestedByClient = (format == PixelFormat.TRANSLUCENT) ? mTranslucent : mOpaque;
+
+        // Is the requested surface ready?  "Ready" means that it's valid, and that we don't have a
+        // destroy in-flight for it.
+        if (mRequestedByClient.isValid() && !mRequestedByClient.destroyPending) {
+            // The client has requested a surface that's already valid and isn't being torn down.
+            // It might be requesting the same surface it already has, or it might have toggled
+            // back to one it had before without notifying doneWithUnownedSurface.  In either case,
+            // we elide the surface created / destroyed callbacks.  We might want to return whether
+            // we've elided them, but CompositorView doesn't care.
+            mOwnedByClient = mRequestedByClient;
+            return;
+        }
+
+        // Surface is not valid, or it'll be destroyed soon.  If it's not valid now, then start
+        // construction.  Once it's ready, we'll signal the client if it still wants this surface.
+        // If destruction is pending, then we must wait for it to complete.  When we're notified
+        // that it is destroyed, we'll re-start construction if the client still wants this surface.
+        // Note that we could send a surfaceDestroyed for the owned surface, if there is one, but we
+        // defer it until later so that the compositor can still use it.
+        if (!mRequestedByClient.isValid()) startSurfaceConstruction(mRequestedByClient);
+    }
+
+    /**
+     * Called to notify us that the client no longer needs the surface that it doesn't own.  This
+     * tells us that we may destroy it.  Note that it's okay if it never had an unowned surface.
+     */
+    public void doneWithUnownedSurface() {
+        if (mOwnedByClient == null) return;
+
+        SurfaceState unowned = (mOwnedByClient == mTranslucent) ? mOpaque : mTranslucent;
+
+        if (mRequestedByClient == unowned) {
+            // Client is giving us back a surface that it's since requested but hasn't gotten yet.
+            // Do nothing.  It will be notified when the new surface is ready, and it can call us
+            // again.
+            return;
+        }
+
+        // Start destruction of this surface.
+        postSurfaceDestruction(unowned);
+    }
+
+    /**
+     * Return the currently owned SurfaceHolder, if any.
+     */
+    public SurfaceHolder getHolder() {
+        return mOwnedByClient != null ? mOwnedByClient.surfaceHolder() : null;
+    }
+
+    /**
+     * Destroy and re-create the surface.  Useful for a JB workaround needed by CompositorView.
+     */
+    public void recreateSurfaceForJellyBean() {
+        assert Build.VERSION.SDK_INT <= Build.VERSION_CODES.JELLY_BEAN_MR2;
+
+        // If they don't have a surface, then they'll get a new one anyway.
+        if (mOwnedByClient == null) return;
+
+        // Notify the client that it no longer owns this surface, then destroy it.  When destruction
+        // completes, we will recreate it automatically, since it will look like the client since
+        // re-requested it.  That's why we send surfaceDestroyed here rather than letting our
+        // surfaceDestroyed do it when destruction completes.  If we just started destruction while
+        // the client still owns the surface, then our surfaceDestroyed would assume that android
+        // initiated the destruction since |mSurfaceDestroyed| would imply that the client didn't.
+
+        mParentView.post(new Runnable() {
+            @Override
+            public void run() {
+                if (mOwnedByClient == null) return;
+                SurfaceState owned = mOwnedByClient;
+                mClient.surfaceDestroyed(mOwnedByClient.surfaceHolder());
+                mOwnedByClient = null;
+                startSurfaceDestruction(owned);
+            }
+        });
+    }
+
+    @Override
+    public void surfaceChanged(SurfaceHolder holder, int format, int width, int height) {
+        SurfaceState state = getStateForHolder(holder);
+        assert state != null;
+
+        // If this is the surface that the client currently cares about, then notify the client.
+        // Note that surfaceChanged is guaranteed to come only after surfaceCreated.  Also, if the
+        // client has requested a different surface but hasn't gotten it yet, then skip this.
+        if (state == mOwnedByClient && state == mRequestedByClient) {
+            mClient.surfaceChanged(holder, format, width, height);
+        }
+    }
+
+    @Override
+    public void surfaceCreated(SurfaceHolder holder) {
+        SurfaceState state = getStateForHolder(holder);
+        assert state != null;
+
+        if (state != mRequestedByClient) {
+            // Surface is created, but it's not the one that's been requested most recently.  Just
+            // destroy it again.
+            postSurfaceDestruction(state);
+            return;
+        }
+
+        // The client requested a surface, and it's now available.  If the client owns a
+        // surface, then notify it that it doesn't.  Note that the client can't own |state| at
+        // this point, since we would have removed ownership when we got surfaceDestroyed.
+        if (mOwnedByClient != null) mClient.surfaceDestroyed(mOwnedByClient.surfaceHolder());
+
+        mOwnedByClient = mRequestedByClient;
+        mClient.surfaceCreated(mOwnedByClient.surfaceHolder());
+    }
+
+    @Override
+    public void surfaceDestroyed(SurfaceHolder holder) {
+        SurfaceState state = getStateForHolder(holder);
+        assert state != null;
+
+        // We might not have requested destruction, but clear the flag in case we did.
+        state.destroyPending = false;
+
+        // If the client owns this surface, then notify it synchronously that it no longer does.
+        // This can happen if Android destroys the surface on its own.
+        if (state == mOwnedByClient) {
+            mClient.surfaceDestroyed(holder);
+            mOwnedByClient = null;
+
+            // Do not re-request the surface here.  If android gives the surface back, then we'll
+            // re-signal the client about construction.
+            return;
+        }
+
+        // The client doesn't own this surface, but might want it.
+        // If the client has requested this surface, then start construction on it.  The client will
+        // be notified when it completes.  This can happen if the client re-requests a surface after
+        // we start destruction on it from a previous request, for example.  We post this for later,
+        // since we might be called while removing |state| from the view tree. In general, posting
+        // from here is good.
+        if (state == mRequestedByClient) {
+            postSurfaceConstruction(state);
+        } else {
+            // This isn't the requested surface.  If android destroyed it, then also unhook it so
+            // that it isn't recreated later.  If we did, then it's already not attached, and this
+            // will do nothing.
+            postSurfaceDestruction(state);
+        }
+    }
+
+    /**
+     * Update the background drawable on all surfaces.
+     */
+    public void setBackgroundDrawable(Drawable background) {
+        mTranslucent.surfaceView.setBackgroundDrawable(background);
+        mOpaque.surfaceView.setBackgroundDrawable(background);
+    }
+
+    /**
+     * Set |willNotDraw| on all surfaces.
+     */
+    public void setWillNotDraw(boolean willNotDraw) {
+        mTranslucent.surfaceView.setWillNotDraw(willNotDraw);
+        mOpaque.surfaceView.setWillNotDraw(willNotDraw);
+    }
+
+    /**
+     * Return the SurfaceState for |holder|, or null if it isn't either.
+     */
+    private SurfaceState getStateForHolder(SurfaceHolder holder) {
+        if (mTranslucent.surfaceHolder() == holder) return mTranslucent;
+
+        if (mOpaque.surfaceHolder() == holder) return mOpaque;
+
+        return null;
+    }
+
+    private void startSurfaceConstruction(SurfaceState state) {
+        if (state.surfaceView.getParent() != null) return;
+
+        FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams(
+                ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.MATCH_PARENT);
+        mParentView.addView(state.surfaceView, lp);
+        mParentView.bringChildToFront(state.surfaceView);
+        mParentView.postInvalidateOnAnimation();
+    }
+
+    private void postSurfaceConstruction(final SurfaceState state) {
+        mParentView.post(new Runnable() {
+            @Override
+            public void run() {
+                startSurfaceConstruction(state);
+            }
+        });
+    }
+
+    private void startSurfaceDestruction(SurfaceState state) {
+        // If we're called while we're not attached, then do nothing.  This makes it easier for the
+        // client, since it doesn't have to keep track of whether the outgoing surface has been
+        // destroyed or not.
+        if (state.surfaceView.getParent() == null) return;
+
+        state.destroyPending = true;

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

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]

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.

+        mParentView.removeView(state.surfaceView);
+    }
+
+    private void postSurfaceDestruction(final SurfaceState state) {
+        mParentView.post(new Runnable() {
+            @Override
+            public void run() {
+                startSurfaceDestruction(state);
+            }
+        });
+    }
+}