Cherry pick PR #1453: [Android] Don't release the lock to clear the v…

…ideo surface. (#1460) Refer to the original PR: youtube/cobalt#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 Co-authored-by: Jelle Foks <[email protected]>
youtube · Sep 20, 2023 · eb925e4 · eb925e4
1 parent 4b858e8
commit eb925e4
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 24 deletions.
diff --git a/starboard/android/apk/app/src/main/java/dev/cobalt/media/VideoSurfaceView.java b/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 {

diff --git a/starboard/android/shared/video_window.cc b/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");