[Android] Don't release the lock to clear the video surface. (#1453)

To ensure that resetting the surface doesn't race with other accesses
to the surface, keep the lock the entire time. During this, replace the
SurfaceHolderCallback with explicit and synchronous equivalent behavior.

b/298213425
b/297264187

starboard/android/apk/app/src/main/java/dev/cobalt/media/VideoSurfaceView.java

@@ -37,6 +37,7 @@
 public class VideoSurfaceView extends SurfaceView {
 
   private static Surface currentSurface = null;
+  private SurfaceHolder.Callback mSurfaceHolderCallback = null;
 
   private static final Set<String> needResetSurfaceList = new HashSet<>();
 
@@ -71,28 +72,46 @@ public VideoSurfaceView(Context context, AttributeSet attrs, int defStyleAttr, i
 
   private void initialize(Context context) {
     setBackgroundColor(Color.TRANSPARENT);
-    getHolder().addCallback(new SurfaceHolderCallback());
+    mSurfaceHolderCallback = new SurfaceHolderCallback();
+    getHolder().addCallback(mSurfaceHolderCallback);
 
     // TODO: Avoid recreating the surface when the player bounds change.
     // Recreating the surface is time-consuming and complicates synchronizing
     // punch-out video when the position / size is animated.
   }
 
   public void clearSurface() {
-    if (getHolder().getSurface().isValid()) {
-      Canvas canvas = getHolder().lockCanvas();
+    SurfaceHolder holder = getHolder();
+    if (holder == null) {
+      return;
+    }
+    Surface surface = holder.getSurface();
+    if ((surface != null) && surface.isValid()) {
+      Canvas canvas = holder.lockCanvas();
       if (canvas != null) {
         canvas.drawColor(Color.BLACK, PorterDuff.Mode.CLEAR);
-        getHolder().unlockCanvasAndPost(canvas);
+        holder.unlockCanvasAndPost(canvas);
       }
-      // Trigger a surface changed event to prevent 'already connected'.
-      getHolder().setFormat(PixelFormat.TRANSPARENT);
-      getHolder().setFormat(PixelFormat.OPAQUE);
+    }
+    // Trigger a surface changed event to prevent 'already connected'.
+    // But disable the callback to prevent it from making calls to the locking
+    // nativeOnVideoSurfaceChanged because we already are holding the same lock.
+    if (mSurfaceHolderCallback != null) {
+      holder.removeCallback(mSurfaceHolderCallback);
+    }
+    holder.setFormat(PixelFormat.TRANSPARENT);
+    holder.setFormat(PixelFormat.OPAQUE);
+    currentSurface = holder.getSurface();
+    nativeOnVideoSurfaceChangedLocked(currentSurface);
+    if (mSurfaceHolderCallback != null) {
+      holder.addCallback(mSurfaceHolderCallback);
     }
   }
 
   private static native void nativeOnVideoSurfaceChanged(Surface surface);
 
+  private static native void nativeOnVideoSurfaceChangedLocked(Surface surface);
+
   private static native void nativeSetNeedResetSurface();
 
   private class SurfaceHolderCallback implements SurfaceHolder.Callback {

starboard/android/shared/video_window.cc

@@ -47,11 +47,10 @@ bool g_reset_surface_on_clear_window = false;
 }  // namespace
 
 extern "C" SB_EXPORT_PLATFORM void
-Java_dev_cobalt_media_VideoSurfaceView_nativeOnVideoSurfaceChanged(
+Java_dev_cobalt_media_VideoSurfaceView_nativeOnVideoSurfaceChangedLocked(
     JNIEnv* env,
     jobject unused_this,
     jobject surface) {
-  ScopedLock lock(*GetViewSurfaceMutex());
   if (g_video_surface_holder) {
     g_video_surface_holder->OnSurfaceDestroyed();
     g_video_surface_holder = NULL;
@@ -70,6 +69,16 @@ Java_dev_cobalt_media_VideoSurfaceView_nativeOnVideoSurfaceChanged(
   }
 }
 
+extern "C" SB_EXPORT_PLATFORM void
+Java_dev_cobalt_media_VideoSurfaceView_nativeOnVideoSurfaceChanged(
+    JNIEnv* env,
+    jobject j_this,
+    jobject surface) {
+  ScopedLock lock(*GetViewSurfaceMutex());
+  Java_dev_cobalt_media_VideoSurfaceView_nativeOnVideoSurfaceChangedLocked(
+      env, j_this, surface);
+}
+
 extern "C" SB_EXPORT_PLATFORM void
 Java_dev_cobalt_media_VideoSurfaceView_nativeSetNeedResetSurface(
     JNIEnv* env,
@@ -119,26 +128,24 @@ bool VideoSurfaceHolder::GetVideoWindowSize(int* width, int* height) {
 void VideoSurfaceHolder::ClearVideoWindow(bool force_reset_surface) {
   // Lock *GetViewSurfaceMutex() here, to avoid releasing g_native_video_window
   // during painting.
-  {
-    ScopedLock lock(*GetViewSurfaceMutex());
+  ScopedLock lock(*GetViewSurfaceMutex());
 
-    if (!g_native_video_window) {
-      SB_LOG(INFO) << "Tried to clear video window when it was null.";
-      return;
-    }
+  if (!g_native_video_window) {
+    SB_LOG(INFO) << "Tried to clear video window when it was null.";
+    return;
+  }
 
-    if (force_reset_surface) {
+  if (force_reset_surface) {
+    JniEnvExt::Get()->CallStarboardVoidMethodOrAbort("resetVideoSurface",
+                                                     "()V");
+    return;
+  } else if (g_reset_surface_on_clear_window) {
+    int width = ANativeWindow_getWidth(g_native_video_window);
+    int height = ANativeWindow_getHeight(g_native_video_window);
+    if (width <= height) {
       JniEnvExt::Get()->CallStarboardVoidMethodOrAbort("resetVideoSurface",
                                                        "()V");
       return;
-    } else if (g_reset_surface_on_clear_window) {
-      int width = ANativeWindow_getWidth(g_native_video_window);
-      int height = ANativeWindow_getHeight(g_native_video_window);
-      if (width <= height) {
-        JniEnvExt::Get()->CallStarboardVoidMethodOrAbort("resetVideoSurface",
-                                                         "()V");
-        return;
-      }
     }
   }
   JniEnvExt::Get()->CallStarboardVoidMethodOrAbort("clearVideoSurface", "()V");