Merge "Really make Surface thread-safe." into jb-mr2-dev

3 files changed, 108 insertions, 106 deletions

diff --git a/core/java/android/view/Surface.java b/core/java/android/view/Surface.java
index 4989c3a..e0786f7 100644
--- a/core/java/android/view/Surface.java
+++ b/core/java/android/view/Surface.java
@@ -34,19 +34,21 @@ public class Surface implements Parcelable {
 
     private static native int nativeCreateFromSurfaceTexture(SurfaceTexture surfaceTexture)
             throws OutOfResourcesException;
+    private static native int nativeCreateFromSurfaceControl(int surfaceControlNativeObject);
 
-    private native Canvas nativeLockCanvas(int nativeObject, Rect dirty);
-    private native void nativeUnlockCanvasAndPost(int nativeObject, Canvas canvas);
+    private static native void nativeLockCanvas(int nativeObject, Canvas canvas, Rect dirty)
+            throws OutOfResourcesException;
+    private static native void nativeUnlockCanvasAndPost(int nativeObject, Canvas canvas);
 
     private static native void nativeRelease(int nativeObject);
     private static native boolean nativeIsValid(int nativeObject);
     private static native boolean nativeIsConsumerRunningBehind(int nativeObject);
-    private static native int nativeCopyFrom(int nativeObject, int surfaceControlNativeObject);
     private static native int nativeReadFromParcel(int nativeObject, Parcel source);
     private static native void nativeWriteToParcel(int nativeObject, Parcel dest);
 
     public static final Parcelable.Creator<Surface> CREATOR =
             new Parcelable.Creator<Surface>() {
+        @Override
         public Surface createFromParcel(Parcel source) {
             try {
                 Surface s = new Surface();
@@ -57,26 +59,20 @@ public class Surface implements Parcelable {
                 return null;
             }
         }
+
+        @Override
         public Surface[] newArray(int size) {
             return new Surface[size];
         }
     };
 
     private final CloseGuard mCloseGuard = CloseGuard.get();
-    private String mName;
 
-    // Note: These fields are accessed by native code.
-    // The mSurfaceControl will only be present for Surfaces used by the window
-    // server or system processes. When this class is parceled we defer to the
-    // mSurfaceControl to do the parceling. Otherwise we parcel the
-    // mNativeSurface.
+    // Guarded state.
+    final Object mLock = new Object(); // protects the native state
+    private String mName;
     int mNativeObject; // package scope only for SurfaceControl access
-
-    // protects the native state
-    private final Object mNativeObjectLock = new Object();
-
-    private int mGenerationId; // incremented each time mNativeSurface changes
-    @SuppressWarnings("UnusedDeclaration")
+    private int mGenerationId; // incremented each time mNativeObject changes
     private final Canvas mCanvas = new CompatibleCanvas();
 
     // A matrix to scale the matrix set by application. This is set to null for
@@ -125,21 +121,22 @@ public class Surface implements Parcelable {
             throw new IllegalArgumentException("surfaceTexture must not be null");
         }
 
-        mName = surfaceTexture.toString();
-        try {
-            mNativeObject = nativeCreateFromSurfaceTexture(surfaceTexture);
-        } catch (OutOfResourcesException ex) {
-            // We can't throw OutOfResourcesException because it would be an API change.
-            throw new RuntimeException(ex);
+        synchronized (mLock) {
+            mName = surfaceTexture.toString();
+            try {
+                setNativeObjectLocked(nativeCreateFromSurfaceTexture(surfaceTexture));
+            } catch (OutOfResourcesException ex) {
+                // We can't throw OutOfResourcesException because it would be an API change.
+                throw new RuntimeException(ex);
+            }
         }
-
-        mCloseGuard.open("release");
     }
 
     /* called from android_view_Surface_createFromIGraphicBufferProducer() */
     private Surface(int nativeObject) {
-        mNativeObject = nativeObject;
-        mCloseGuard.open("release");
+        synchronized (mLock) {
+            setNativeObjectLocked(nativeObject);
+        }
     }
 
     @Override
@@ -160,13 +157,11 @@ public class Surface implements Parcelable {
      * This will make the surface invalid.
      */
     public void release() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             if (mNativeObject != 0) {
                 nativeRelease(mNativeObject);
-                mNativeObject = 0;
-                mGenerationId++;
+                setNativeObjectLocked(0);
             }
-            mCloseGuard.close();
         }
     }
 
@@ -187,7 +182,7 @@ public class Surface implements Parcelable {
      * Otherwise returns false.
      */
     public boolean isValid() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             if (mNativeObject == 0) return false;
             return nativeIsValid(mNativeObject);
         }
@@ -201,7 +196,9 @@ public class Surface implements Parcelable {
      * @hide
      */
     public int getGenerationId() {
-        return mGenerationId;
+        synchronized (mLock) {
+            return mGenerationId;
+        }
     }
 
     /**
@@ -211,7 +208,7 @@ public class Surface implements Parcelable {
      * @hide
      */
     public boolean isConsumerRunningBehind() {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             checkNotReleasedLocked();
             return nativeIsConsumerRunningBehind(mNativeObject);
         }
@@ -234,9 +231,10 @@ public class Surface implements Parcelable {
      */
     public Canvas lockCanvas(Rect inOutDirty)
             throws OutOfResourcesException, IllegalArgumentException {
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             checkNotReleasedLocked();
-            return nativeLockCanvas(mNativeObject, inOutDirty);
+            nativeLockCanvas(mNativeObject, mCanvas, inOutDirty);
+            return mCanvas;
         }
     }
 
@@ -247,7 +245,12 @@ public class Surface implements Parcelable {
      * @param canvas The canvas previously obtained from {@link #lockCanvas}.
      */
     public void unlockCanvasAndPost(Canvas canvas) {
-        synchronized (mNativeObjectLock) {
+        if (canvas != mCanvas) {
+            throw new IllegalArgumentException("canvas object must be the same instance that "
+                    + "was previously returned by lockCanvas");
+        }
+
+        synchronized (mLock) {
             checkNotReleasedLocked();
             nativeUnlockCanvasAndPost(mNativeObject, canvas);
         }
@@ -273,7 +276,6 @@ public class Surface implements Parcelable {
         }
     }
 
-
     /**
      * Copy another surface to this one.  This surface now holds a reference
      * to the same data as the original surface, and is -not- the owner.
@@ -287,22 +289,24 @@ public class Surface implements Parcelable {
         if (other == null) {
             throw new IllegalArgumentException("other must not be null");
         }
-        if (other.mNativeObject == 0) {
+
+        int surfaceControlPtr = other.mNativeObject;
+        if (surfaceControlPtr == 0) {
             throw new NullPointerException(
                     "SurfaceControl native object is null. Are you using a released SurfaceControl?");
         }
-        synchronized (mNativeObjectLock) {
-            mNativeObject = nativeCopyFrom(mNativeObject, other.mNativeObject);
-            if (mNativeObject == 0) {
-                // nativeCopyFrom released our reference
-                mCloseGuard.close();
+        int newNativeObject = nativeCreateFromSurfaceControl(surfaceControlPtr);
+
+        synchronized (mLock) {
+            if (mNativeObject != 0) {
+                nativeRelease(mNativeObject);
             }
-            mGenerationId++;
+            setNativeObjectLocked(newNativeObject);
         }
     }
 
     /**
-     * This is intended to be used by {@link SurfaceView.updateWindow} only.
+     * This is intended to be used by {@link SurfaceView#updateWindow} only.
      * @param other access is not thread safe
      * @hide
      * @deprecated
@@ -313,21 +317,18 @@ public class Surface implements Parcelable {
             throw new IllegalArgumentException("other must not be null");
         }
         if (other != this) {
-            synchronized (mNativeObjectLock) {
+            final int newPtr;
+            synchronized (other.mLock) {
+                newPtr = other.mNativeObject;
+                other.setNativeObjectLocked(0);
+            }
+
+            synchronized (mLock) {
                 if (mNativeObject != 0) {
-                    // release our reference to our native object
                     nativeRelease(mNativeObject);
                 }
-                // transfer the reference from other to us
-                if (other.mNativeObject != 0 && mNativeObject == 0) {
-                    mCloseGuard.open("release");
-                }
-                mNativeObject = other.mNativeObject;
-                mGenerationId++;
+                setNativeObjectLocked(newPtr);
             }
-            other.mNativeObject = 0;
-            other.mGenerationId++;
-            other.mCloseGuard.close();
         }
     }
 
@@ -340,14 +341,10 @@ public class Surface implements Parcelable {
         if (source == null) {
             throw new IllegalArgumentException("source must not be null");
         }
-        synchronized (mNativeObjectLock) {
+
+        synchronized (mLock) {
             mName = source.readString();
-            int nativeObject = nativeReadFromParcel(mNativeObject, source);
-            if (nativeObject !=0 && mNativeObject == 0) {
-                mCloseGuard.open("release");
-            }
-            mNativeObject = nativeObject;
-            mGenerationId++;
+            setNativeObjectLocked(nativeReadFromParcel(mNativeObject, source));
         }
     }
 
@@ -356,7 +353,7 @@ public class Surface implements Parcelable {
         if (dest == null) {
             throw new IllegalArgumentException("dest must not be null");
         }
-        synchronized (mNativeObjectLock) {
+        synchronized (mLock) {
             dest.writeString(mName);
             nativeWriteToParcel(mNativeObject, dest);
         }
@@ -367,7 +364,27 @@ public class Surface implements Parcelable {
 
     @Override
     public String toString() {
-        return "Surface(name=" + mName + ")";
+        synchronized (mLock) {
+            return "Surface(name=" + mName + ")";
+        }
+    }
+
+    private void setNativeObjectLocked(int ptr) {
+        if (mNativeObject != ptr) {
+            if (mNativeObject == 0 && ptr != 0) {
+                mCloseGuard.open("release");
+            } else if (mNativeObject != 0 && ptr == 0) {
+                mCloseGuard.close();
+            }
+            mNativeObject = ptr;
+            mGenerationId += 1;
+        }
+    }
+
+    private void checkNotReleasedLocked() {
+        if (mNativeObject == 0) {
+            throw new IllegalStateException("Surface has already been released.");
+        }
     }
 
     /**
@@ -451,9 +468,4 @@ public class Surface implements Parcelable {
             mOrigMatrix.set(m);
         }
     }
-
-    private void checkNotReleasedLocked() {
-        if (mNativeObject == 0) throw new NullPointerException(
-                "mNativeObject is null. Have you called release() already?");
-    }
 }
diff --git a/core/java/android/view/SurfaceControl.java b/core/java/android/view/SurfaceControl.java
index e869d09..6b530ef 100644
--- a/core/java/android/view/SurfaceControl.java
+++ b/core/java/android/view/SurfaceControl.java
@@ -496,8 +496,14 @@ public class SurfaceControl {
         if (displayToken == null) {
             throw new IllegalArgumentException("displayToken must not be null");
         }
-        int nativeSurface = surface != null ? surface.mNativeObject : 0;
-        nativeSetDisplaySurface(displayToken, nativeSurface);
+
+        if (surface != null) {
+            synchronized (surface.mLock) {
+                nativeSetDisplaySurface(displayToken, surface.mNativeObject);
+            }
+        } else {
+            nativeSetDisplaySurface(displayToken, 0);
+        }
     }
 
     public static IBinder createDisplay(String name, boolean secure) {
diff --git a/core/jni/android_view_Surface.cpp b/core/jni/android_view_Surface.cpp
index a41a389..9a19ce5 100644
--- a/core/jni/android_view_Surface.cpp
+++ b/core/jni/android_view_Surface.cpp
@@ -55,8 +55,7 @@ static const char* const OutOfResourcesException =
 static struct {
     jclass clazz;
     jfieldID mNativeObject;
-    jfieldID mNativeObjectLock;
-    jfieldID mCanvas;
+    jfieldID mLock;
     jmethodID ctor;
 } gSurfaceClassInfo;
 
@@ -93,7 +92,7 @@ sp<ANativeWindow> android_view_Surface_getNativeWindow(JNIEnv* env, jobject surf
 sp<Surface> android_view_Surface_getSurface(JNIEnv* env, jobject surfaceObj) {
     sp<Surface> sur;
     jobject lock = env->GetObjectField(surfaceObj,
-            gSurfaceClassInfo.mNativeObjectLock);
+            gSurfaceClassInfo.mLock);
     if (env->MonitorEnter(lock) == JNI_OK) {
         sur = reinterpret_cast<Surface *>(
                 env->GetIntField(surfaceObj, gSurfaceClassInfo.mNativeObject));
@@ -200,12 +199,13 @@ static inline void swapCanvasPtr(JNIEnv* env, jobject canvasObj, SkCanvas* newCa
   SkSafeUnref(previousCanvas);
 }
 
-static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObject, jobject dirtyRectObj) {
+static void nativeLockCanvas(JNIEnv* env, jclass clazz,
+        jint nativeObject, jobject canvasObj, jobject dirtyRectObj) {
     sp<Surface> surface(reinterpret_cast<Surface *>(nativeObject));
 
     if (!isSurfaceValid(surface)) {
         doThrowIAE(env);
-        return NULL;
+        return;
     }
 
     // get dirty region
@@ -232,11 +232,10 @@ static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObje
                 OutOfResourcesException :
                 "java/lang/IllegalArgumentException";
         jniThrowException(env, exception, NULL);
-        return NULL;
+        return;
     }
 
     // Associate a SkCanvas object to this surface
-    jobject canvasObj = env->GetObjectField(surfaceObj, gSurfaceClassInfo.mCanvas);
     env->SetIntField(canvasObj, gCanvasClassInfo.mSurfaceFormat, outBuffer.format);
 
     SkBitmap bitmap;
@@ -277,17 +276,10 @@ static jobject nativeLockCanvas(JNIEnv* env, jobject surfaceObj, jint nativeObje
         env->SetIntField(dirtyRectObj, gRectClassInfo.right, bounds.right);
         env->SetIntField(dirtyRectObj, gRectClassInfo.bottom, bounds.bottom);
     }
-
-    return canvasObj;
 }
 
-static void nativeUnlockCanvasAndPost(JNIEnv* env, jobject surfaceObj, jint nativeObject, jobject canvasObj) {
-    jobject ownCanvasObj = env->GetObjectField(surfaceObj, gSurfaceClassInfo.mCanvas);
-    if (!env->IsSameObject(ownCanvasObj, canvasObj)) {
-        doThrowIAE(env);
-        return;
-    }
-
+static void nativeUnlockCanvasAndPost(JNIEnv* env, jclass clazz,
+        jint nativeObject, jobject canvasObj) {
     sp<Surface> surface(reinterpret_cast<Surface *>(nativeObject));
     if (!isSurfaceValid(surface)) {
         return;
@@ -306,8 +298,8 @@ static void nativeUnlockCanvasAndPost(JNIEnv* env, jobject surfaceObj, jint nati
 
 // ----------------------------------------------------------------------------
 
-static jint nativeCopyFrom(JNIEnv* env, jclass clazz,
-        jint nativeObject, jint surfaceControlNativeObj) {
+static jint nativeCreateFromSurfaceControl(JNIEnv* env, jclass clazz,
+        jint surfaceControlNativeObj) {
     /*
      * This is used by the WindowManagerService just after constructing
      * a Surface and is necessary for returning the Surface reference to
@@ -315,17 +307,11 @@ static jint nativeCopyFrom(JNIEnv* env, jclass clazz,
      */
 
     sp<SurfaceControl> ctrl(reinterpret_cast<SurfaceControl *>(surfaceControlNativeObj));
-    sp<Surface> other(ctrl->getSurface());
-    if (other != NULL) {
-        other->incStrong(&sRefBaseOwner);
-    }
-
-    sp<Surface> sur(reinterpret_cast<Surface *>(nativeObject));
-    if (sur != NULL) {
-        sur->decStrong(&sRefBaseOwner);
+    sp<Surface> surface(ctrl->getSurface());
+    if (surface != NULL) {
+        surface->incStrong(&sRefBaseOwner);
     }
-
-    return int(other.get());
+    return reinterpret_cast<jint>(surface.get());
 }
 
 static jint nativeReadFromParcel(JNIEnv* env, jclass clazz,
@@ -386,12 +372,12 @@ static JNINativeMethod gSurfaceMethods[] = {
             (void*)nativeIsValid },
     {"nativeIsConsumerRunningBehind", "(I)Z",
             (void*)nativeIsConsumerRunningBehind },
-    {"nativeLockCanvas", "(ILandroid/graphics/Rect;)Landroid/graphics/Canvas;",
+    {"nativeLockCanvas", "(ILandroid/graphics/Canvas;Landroid/graphics/Rect;)V",
             (void*)nativeLockCanvas },
     {"nativeUnlockCanvasAndPost", "(ILandroid/graphics/Canvas;)V",
             (void*)nativeUnlockCanvasAndPost },
-    {"nativeCopyFrom", "(II)I",
-            (void*)nativeCopyFrom },
+    {"nativeCreateFromSurfaceControl", "(I)I",
+            (void*)nativeCreateFromSurfaceControl },
     {"nativeReadFromParcel", "(ILandroid/os/Parcel;)I",
             (void*)nativeReadFromParcel },
     {"nativeWriteToParcel", "(ILandroid/os/Parcel;)V",
@@ -407,10 +393,8 @@ int register_android_view_Surface(JNIEnv* env)
     gSurfaceClassInfo.clazz = jclass(env->NewGlobalRef(clazz));
     gSurfaceClassInfo.mNativeObject =
             env->GetFieldID(gSurfaceClassInfo.clazz, "mNativeObject", "I");
-    gSurfaceClassInfo.mNativeObjectLock =
-            env->GetFieldID(gSurfaceClassInfo.clazz, "mNativeObjectLock", "Ljava/lang/Object;");
-    gSurfaceClassInfo.mCanvas =
-            env->GetFieldID(gSurfaceClassInfo.clazz, "mCanvas", "Landroid/graphics/Canvas;");
+    gSurfaceClassInfo.mLock =
+            env->GetFieldID(gSurfaceClassInfo.clazz, "mLock", "Ljava/lang/Object;");
     gSurfaceClassInfo.ctor = env->GetMethodID(gSurfaceClassInfo.clazz, "<init>", "(I)V");
 
     clazz = env->FindClass("android/graphics/Canvas");