Ensure MediaPlayer has dedicated thread owner that is not the main thread by JZDesign · Pull Request #3148 · RevenueCat/purchases-android

JZDesign · 2026-02-26T19:53:35Z

Important

I need to physically test this well… slow going

The media player is essentially bound to a single thread. If we don't ensure that it performs tasks on a background thread, the interactions from the UI layer make it run on the main thread. When we want to deallocate one, we should stop and release the resources to free up RAM on the device. Doing this on the main thread (like on paywall dismissal) can cause certain devices to hang from a main thread block. This is happening and we have reports of ANRs, and it's apparently one of the problems solved by ExoPlayer—which we decided not to use for transient dependency management reasons IIRC.

This PR wraps the media player into a thread management object to ensure we don't block the main thread.

Resolves #3142

Note

Medium Risk
Touches video playback lifecycle and threading (surface attach/detach, prepare/start/pause/release), which can introduce race conditions or device-specific playback issues despite added safeguards and tests.

Overview
Refactors TextureVideoView to route all MediaPlayer operations through a new MediaPlayerThreadOwner, ensuring prepare/start/pause/seek/surface attach/detach/release run on a dedicated HandlerThread instead of the main thread (reducing ANR risk during teardown).

Adds bounded blocking surface-detach (clearSurfaceBlocking) to preserve ordering when SurfaceTexture is destroyed, introduces request-id guarding to ignore stale prepare callbacks, and includes Robolectric tests validating thread serialization, non-blocking release, surface-release ordering/timeout behavior, and immediate isPlaying state reporting.

^{Written by Cursor Bugbot for commit 637add7. This will update automatically on new commits. Configure here.}

• Updated TextureVideoView.release() in ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt to: • set released = true immediately, • hide + clear controller, • detach surface with player.setSurface(null), • directly player.release() inside safely. • Removed player.stop() and player.reset() from teardown (the blocking path shown in the stack trace). This keeps teardown one-shot and avoids the reset()->MediaHTTPConnection.disconnect() main-thread stall during detach. Validation run:

Introduce MediaPlayerFacade and AndroidMediaPlayerFacade to abstract Android MediaPlayer usage and improve testability. Move MediaPlayerThreadOwner out of VideoView into its own file and update it to use the facade with an injectable playerFactory, worker thread handling, and robust state caching. Make safely internal for broader use. Add unit tests (MediaPlayerThreadOwnerTests) and a FakeMediaPlayerFacade to verify non-blocking release and that all player operations are serialized on a single worker thread.

Remove MediaPlayerFacade and AndroidMediaPlayerFacade and refactor MediaPlayerThreadOwner to depend on android.media.MediaPlayer directly. Introduce PlaybackSnapshot to centralize and atomically update playback state (prepared, duration, position, isPlaying, audioSessionId) instead of multiple volatile cached fields. Update position ticker, player lifecycle, listeners, and ensurePlayer to work with MediaPlayer. Add safe snapshot update helpers and adjust threading/handler logic. VideoView now properly releases the attached Surface via a releaseAttachedSurface() helper to avoid leaks. Tests replaced the FakeMediaPlayerFacade with a createMockMediaPlayer helper using mockk, plus supporting utilities to capture listeners and record thread usage.

cursor

Cursor Bugbot has reviewed your changes and found 2 potential issues.

Autofix Details

Bugbot Autofix prepared fixes for both issues found in the latest run.

✅ Fixed: MediaPlayer accessed on main thread in onPrepared callback
- Captured video width and height on the worker-thread onPrepared callback and passed those immutable values to the main-thread callback.
✅ Fixed: Non-atomic snapshot update from calling thread races with worker
- Moved the release snapshot mutation into the workerHandler cleanup block so all snapshot writes are serialized on the worker thread.

Or push these changes by commenting:

@cursor push 970ee3e688

Preview (970ee3e688)

diff --git a/ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt b/ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt
--- a/ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt
+++ b/ui/revenuecatui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt
@@ -126,6 +126,8 @@
                 mediaPlayer.setOnPreparedListener { preparedPlayer ->
                     if (released) return@setOnPreparedListener
                     val activePlayer = preparedPlayer ?: return@setOnPreparedListener
+                    val videoWidth = getPlayerValue(activePlayer, 0) { currentPlayer -> currentPlayer.videoWidth }
+                    val videoHeight = getPlayerValue(activePlayer, 0) { currentPlayer -> currentPlayer.videoHeight }
                     updatePlaybackSnapshot {
                         it.copy(
                             prepared = true,
@@ -140,7 +142,7 @@
                     }
                     mainHandler.post {
                         if (!released) {
-                            onPrepared(activePlayer.videoWidth, activePlayer.videoHeight)
+                            onPrepared(videoWidth, videoHeight)
                         }
                     }
                 }
@@ -236,14 +238,14 @@
     fun release() {
         if (released) return
         released = true
-        updatePlaybackSnapshot {
-            it.copy(
-                prepared = false,
-                isPlaying = false,
-            )
-        }
         workerHandler.removeCallbacksAndMessages(null)
         workerHandler.post {
+            updatePlaybackSnapshot {
+                it.copy(
+                    prepared = false,
+                    isPlaying = false,
+                )
+            }
             val mediaPlayer = player
             player = null
             currentSurface = null

_{This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.}

tonidero

Mostly @vegaro 's comments. I left a couple of my own, but less important.

Add defensive checks and logging to MediaPlayerThreadOwner to avoid crashes when operations occur before the player or surface are ready: cache video dimensions, warn when looping is set before initialization, handle null surface during prepare, and move playback snapshot clearing into the worker thread on release. Introduce a safely(...) helper to centralize try/catch logging. Adjust TextureVideoView.onSurfaceTextureDestroyed to avoid releasing resources if already released and ensure the player surface is cleared only when appropriate. Also add .cursor/.gitignore to ignore plans/.

Add clearSurfaceBlocking to MediaPlayerThreadOwner to synchronously detach a Surface from the player on the worker thread and wait for that detach to finish (using CountDownLatch). This preserves the ordering invariant that MediaPlayer.setSurface(null) completes before the caller can release the Surface, avoiding native rendering races. The method executes inline if called from the worker thread to avoid deadlocks. Factor out setSurfaceInternal and update TextureVideoView to use clearSurfaceBlocking when detaching surfaces. Add a unit test that verifies the blocking behavior and ordering.

codecov · 2026-03-11T21:37:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.40%. Comparing base (0700176) to head (637add7).
⚠️ Report is 16 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3148   +/-   ##
=======================================
  Coverage   79.40%   79.40%           
=======================================
  Files         356      356           
  Lines       14345    14345           
  Branches     1959     1959           
=======================================
  Hits        11390    11390           
  Misses       2151     2151           
  Partials      804      804

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

… into pw-996/thread-owner

JZDesign · 2026-03-16T19:00:51Z

I've tested it a bunch on my Pixel 7, doing more now on my Samsung, if I can't get an ANR, I think it's probably good to go

MonikaMateska · 2026-03-17T11:09:38Z

The code looks good (from what I can understand), but I've noticed there is some inconsistency with the playback controls. Is there something we still need to update on the main thread?
https://github.com/user-attachments/assets/082562ec-278c-4468-8c83-37882a768704

JZDesign · 2026-03-17T12:05:43Z

@MonikaMateska

The code looks good (from what I can understand), but I've noticed there is some inconsistency with the playback controls. Is there something we still need to update on the main thread?

I tested on my Pixel 7 and my Galaxy fold and I'm not experiencing anything odd. The video doesn't exactly show what the inconsistency is that you're referencing. Would you be a bit more specific?

MonikaMateska · 2026-03-17T13:16:50Z

I tested on my Pixel 7 and my Galaxy fold and I'm not experiencing anything odd. The video doesn't exactly show what the inconsistency is that you're referencing. Would you be a bit more specific?

Yes, sorry for my vague explanation! When there is a video component with the playback controls being shown, the initial UI of the controls is correct (presenting the ⏸️ pause icon when the video is playing).
When you select the pause button, the video pauses (desired behavior), but the ⏸️ pause icon still stays there instead of changing to ▶️ play.
Once you select the same button, the video resumes from where it was paused (desired behavior), but the icon is changed to ▶️ play instead of ⏸️ pause.

One thing to mention is that I've tested this on an emulator, as I don't have a physical device that runs Android.

JZDesign · 2026-03-17T13:20:45Z

Ah, I tested it again and paid close attention to the symbols on the buttons, and I can replicate that. I will check to see if that is new from this PR or not and if I can dispatch to main to correct it. Great feedback. Thank you.

Introduce a PendingPlaybackState and playbackCommandId to record intended playing state immediately when start()/pause() are called, so isPlaying() reflects the requested state even while the worker thread command is blocked. start() and pause() now mark a pending state (and early-return if released/unprepared), clear it on success or failure, and ensure pending state is cleared on prepare/release/stop paths. Added helper methods markPendingPlaybackState/clearPendingPlaybackState and adjusted isPlaying() to prefer the pending state. Tests added to verify isPlaying() returns the expected value immediately after start/pause when the worker command is blocked, plus a waitForCondition helper used by tests.

cursor

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

Remove the private 'looping' property from TextureVideoView and the redundant assignment in setLooping. The looping state is delegated to playerOwner.setLooping, so the field was unused; this is a small cleanup with no behavior change.

Replace eager releaseAttachedSurface() with takeAttachedSurface() to hand off the existing Surface to MediaPlayerThreadOwner instead of releasing it directly. TextureVideoView now defensively clears any stale surface on texture available, captures the attached surface before detaching, and forwards that Surface to playerOwner.clearSurfaceBlocking(...) and playerOwner.release(...). This avoids double-releasing the Surface and lets the player owner perform asynchronous or timed releases. Tests updated/added to mock a handed-off Surface, assert it is released exactly once, and verify clearSurfaceBlocking's timeout behavior and asynchronous release semantics. Also added an AtomicInteger import and adjusted expected event ordering in tests.

@matteinn

**This is an automatic release.** ## RevenueCat SDK ### 🐞 Bugfixes * [EXTERNAL] fix: ensure activity is attached before showing in-app messages (#3274) contributed by @matteinn (#3275) via Toni Rico (@tonidero) * Ensure MediaPlayer has dedicated thread owner that is not the main thread (#3148) via Jacob Rakidzich (@JZDesign) * Fix heartbeat monitor and Slack notifications for nightly integration tests (#3259) via Rick (@rickvdl) ## RevenueCatUI SDK ### Paywallv2 #### ✨ New Features * Feat: Restore gating in paywalls UI (#3171) via Jacob Rakidzich (@JZDesign) ### 🔄 Other Changes * security: pin GitHub Actions to SHA hashes (#3272) via Alfonso Embid-Desmet (@alfondotnet) * Bump activesupport from 8.0.2 to 8.0.4.1 (#3270) via dependabot[bot] (@dependabot[bot]) * Merge release PR after deploy (#3269) via Antonio Pallares (@ajpallares) * Require PR approval before release tagging (#3268) via Antonio Pallares (@ajpallares) * Bump json from 2.18.1 to 2.19.2 (#3261) via dependabot[bot] (@dependabot[bot]) * feat(ads): update admob sample app (#3264) via Peter Porfy (@peterporfy) * feat(ads): add vanilla-ad-tracker-sample (#3263) via Peter Porfy (@peterporfy) * [Purchase Tester]: Persist appUserId on login screen across app launches (#3266) via Will Taylor (@fire-at-will)  --- > [!NOTE] > **Medium Risk** > Mostly release automation and versioning changes, but it modifies CI/orb references and deploy/merge automation, which could affect the release pipeline if misconfigured. > > **Overview** > Cuts the `9.28.0` release by removing `-SNAPSHOT` across version sources (`.version`, `gradle.properties`, `Config.frameworkVersion`) and updating sample/test-app dependency pins to `9.28.0`. > > Updates release documentation: publishes Dokka docs to the `9.28.0` S3 path, updates `docs/index.html` redirect to `9.28.0`, and rolls `CHANGELOG.latest.md` into a new `9.28.0` section in `CHANGELOG.md`. > > Tweaks release tooling/CI: pins `fastlane-plugin-revenuecat_internal` to a specific git ref (and bumps a few Ruby deps), switches the CircleCI `revenuecat/sdks-common-config` orb to a dev commit, and adds a temporary `test_merge_queue` workflow to exercise `revenuecat/merge-release-pr` with `use_merge_queue: true`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5050888. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup>

JZDesign added 3 commits February 26, 2026 11:12

Update VideoView.kt

5b6a383

JZDesign mentioned this pull request Feb 26, 2026

FIX ANR - Removed player.stop() + player.reset() from teardown. #3145

Closed

2 tasks

JZDesign changed the title ~~Pw 996/thread owner~~ Ensure MediaPlayer has dedicated thread owner that is not the main thread Feb 26, 2026

JZDesign added the pr:fix A bug fix label Mar 2, 2026

JZDesign changed the base branch from pw-996/video-crash to main March 2, 2026 19:14

JZDesign added 2 commits March 2, 2026 13:14

Merge branch 'main' into pw-996/thread-owner

04709f8

Merge branch 'main' into pw-996/thread-owner

aaaec7f

vegaro reviewed Mar 9, 2026

View reviewed changes

JZDesign requested a review from vegaro March 9, 2026 19:29

JZDesign marked this pull request as ready for review March 9, 2026 19:29

JZDesign requested a review from a team as a code owner March 9, 2026 19:29

cursor Bot reviewed Mar 9, 2026

View reviewed changes

Comment thread ...n/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt

Comment thread ...n/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt

vegaro reviewed Mar 10, 2026

View reviewed changes

Comment thread ...n/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt

vegaro reviewed Mar 10, 2026

View reviewed changes

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt Outdated

vegaro reviewed Mar 10, 2026

View reviewed changes

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt

vegaro reviewed Mar 10, 2026

View reviewed changes

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt Outdated

tonidero reviewed Mar 11, 2026

View reviewed changes

cursor Bot reviewed Mar 11, 2026

View reviewed changes

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt Outdated

JZDesign marked this pull request as draft March 11, 2026 16:19

JZDesign added 3 commits March 11, 2026 15:35

remove dead code

fc33365

detekt

b60d2a4

Merge branch 'main' of https://github.com//RevenueCat/purchases-android…

d8a33d5

… into pw-996/thread-owner

JZDesign marked this pull request as ready for review March 16, 2026 19:00

JZDesign requested review from tonidero and vegaro March 18, 2026 14:56

cursor Bot reviewed Mar 18, 2026

View reviewed changes

Comment thread ...n/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/MediaPlayerThreadOwner.kt Outdated

Comment thread ...catui/src/main/kotlin/com/revenuecat/purchases/ui/revenuecatui/components/video/VideoView.kt

MonikaMateska approved these changes Mar 18, 2026

View reviewed changes

JZDesign added 2 commits March 19, 2026 10:03

Remove unused looping field

1668002

Remove the private 'looping' property from TextureVideoView and the redundant assignment in setLooping. The looping state is delegated to playerOwner.setLooping, so the field was unused; this is a small cleanup with no behavior change.

JZDesign added this pull request to the merge queue Mar 19, 2026

Merged via the queue into main with commit b6b6349 Mar 19, 2026
34 checks passed

JZDesign deleted the pw-996/thread-owner branch March 19, 2026 16:19

RCGitBot mentioned this pull request Mar 25, 2026

[AUTOMATIC] Release/9.28.0 #3276

Merged

Conversation

JZDesign commented Feb 26, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cursor Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonidero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JZDesign commented Mar 16, 2026

Uh oh!

MonikaMateska commented Mar 17, 2026

Uh oh!

JZDesign commented Mar 17, 2026

Uh oh!

MonikaMateska commented Mar 17, 2026

Uh oh!

JZDesign commented Mar 17, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JZDesign commented Feb 26, 2026 •

edited by cursor Bot

Loading

cursor Bot left a comment •

edited

Loading

codecov Bot commented Mar 11, 2026 •

edited

Loading