From eb925e42c587903e7805a3bfe9a7ec86b000045f Mon Sep 17 00:00:00 2001 From: cobalt-github-releaser-bot <95661244+cobalt-github-releaser-bot@users.noreply.github.com> Date: Wed, 20 Sep 2023 10:30:27 -0700 Subject: [PATCH] Cherry pick PR #1453: [Android] Don't release the lock to clear the video surface. (#1460) Refer to the original PR: https://github.com/youtube/cobalt/pull/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 --- .../dev/cobalt/media/VideoSurfaceView.java | 33 +++++++++++---- starboard/android/shared/video_window.cc | 41 +++++++++++-------- 2 files changed, 50 insertions(+), 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 index 339e36365453b..816c6495af14c 100644 --- 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 needResetSurfaceList = new HashSet<>(); @@ -71,7 +72,8 @@ 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 @@ -79,20 +81,37 @@ private void initialize(Context context) { } 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 index 15237ec6164d3..a47cd54c18549 100644 --- 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");