Use AVPlayerLayer for no-controls; fix KVO crash#6987
Conversation
Add PlayerLayerBackedView and VideoPlayerLayerView to render looping/auto-playing videos via AVPlayerLayer for the no-controls path, avoiding AVKit's internal AVPlayerController KVO observers. Update VideoPlayerView to choose between AVPlayerViewController when controls are shown and the new AVPlayerLayer path when hidden. Refactor VideoPlayerUIView to move player lifecycle into its Coordinator, implement manual looping for the AVPlayerViewController path (to avoid AVPlayerLooper-driven KVO issues), disable video frame analysis on iOS 16+, manage/restore audio session category, and deterministically tear down players and observers during dismantle/deinit.
|
Nice fix, this is basically the same shape I landed on independently. One ask before merge: there's no automated coverage, and the core invariant here is cheap to pin so it can't silently regress. Drafted the tests in #6988 (stacked on this branch, verified 5/5 passing locally). The key one, Happy to fold them straight into this PR instead of stacking if you prefer. |
That's a great call, I'm fine with either option. Feel free to just merge into this branch if you like, otherwise we can easily stack. |
Add deterministic teardown for layer-backed players and simplify the controls path. - Added VideoPlayerLayerUIView to the Xcode project and call coordinator.tearDown() from dismantleUIView so playback (audio and rendering) stops promptly (e.g. on carousel navigation). Coordinator.tearDown() now pauses the AVPlayer. - Simplified the AVPlayerViewController-backed UIView path: removed the showControls flag and always enable playback controls; keep user interaction enabled when controls are shown. Added doc note explaining AVPlayerViewController is only used for the controls path to avoid looper-related KVO issues in the layer-backed path. - Updated project.pbxproj to include the new source file in the target and group.
Add PlayerLayerBackedView and VideoPlayerLayerView to render looping/auto-playing videos via AVPlayerLayer for the no-controls path, avoiding AVKit's internal AVPlayerController KVO observers. Update VideoPlayerView to choose between AVPlayerViewController when controls are shown and the new AVPlayerLayer path when hidden. Refactor VideoPlayerUIView to move player lifecycle into its Coordinator, implement manual looping for the AVPlayerViewController path (to avoid AVPlayerLooper-driven KVO issues), disable video frame analysis on iOS 16+, manage/restore audio session category, and deterministically tear down players and observers during dismantle/deinit.
Add deterministic teardown for layer-backed players and simplify the controls path. - Added VideoPlayerLayerUIView to the Xcode project and call coordinator.tearDown() from dismantleUIView so playback (audio and rendering) stops promptly (e.g. on carousel navigation). Coordinator.tearDown() now pauses the AVPlayer. - Simplified the AVPlayerViewController-backed UIView path: removed the showControls flag and always enable playback controls; keep user interaction enabled when controls are shown. Added doc note explaining AVPlayerViewController is only used for the controls path to avoid looper-related KVO issues in the layer-backed path. - Updated project.pbxproj to include the new source file in the target and group.
870b3e1 to
b4575c1
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 870b3e1. Configure here.
Add invalidate() to VideoAutoplayHandler to stop observing lifecycle events (clear cancellables and reset wasPlayingBeforeBackground) so a torn-down player can't be resumed later. Call invalidate() from VideoPlayerLayerView.tearDown and VideoPlayerUIView.tearDown; add isTornDown guard in VideoPlayerUIView's loop notification handler to avoid resurrecting playback. Add a unit test (testDoesNotResumeAfterInvalidate) to verify playback does not resume after invalidation.
…RevenueCat/purchases-ios into alex/fix-avfoundation-kvo-crash
- controls path (AVPlayerViewController) never builds an AVQueuePlayer, so an AVPlayerLooper-driven queue player is never handed to AVKit's internal AVPlayerController (the #6985 teardown crash) - no-controls path uses an AVQueuePlayer via AVPlayerLayer, safe because there is no AVPlayerController - PlayerLayerBackedView is backed by AVPlayerLayer Tests assert through makeCoordinator(), covering the player-ownership seam. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Pushed the tests straight onto this branch (58ba665) and closed #6988. Adjusted for your |
facumenzella
left a comment
There was a problem hiding this comment.
I didn't test it manually, but looks good. Great one @alexrepty !
I tested manually using Paywalls Tester on device, both looping and non-looping videos. |
Checklist
purchases-androidand hybrids - n/a, this is an iOS bugMotivation
Resolves PWENG-112
Resolves #6985
Paywalls V2 looping video backgrounds crash in production (
NSInternalInconsistencyExceptiononcurrentItem.status, ~3.9k events / ~3.5k users). The crash is the well-known AVFoundation/AVKit bug: anAVPlayerLooper-drivenAVQueuePlayerswapscurrentItemwithout KVO notifications, soAVPlayerViewController's internalAVPlayerControllercrashes when removing those observers on teardown.Description
The robust fix is to never hand an
AVPlayerLooper-drivenAVQueuePlayertoAVPlayerViewController:AVPlayerLayer-backed view, so AVKit'sAVPlayerController(and itscurrentItem.*observers) never exists. This mirrors the existing macOSAVPlayerLayerapproach.AVPlayerViewControllerfor the playback UI, but loops manually viadidPlayToEndTimeNotification+seek(to:.zero)instead ofAVPlayerLooper, plusallowsVideoFrameAnalysis = false(iOS 16+) and deterministic teardown as defense-in-depth.Player lifecycle, audio-session save/restore, and lifecycle pause/resume now live on each representable's
Coordinator. iOS-only internal change —VideoComponentView, public API, and Obj-C surface are untouched.allowsVideoFrameAnalysis = falsealone is not sufficient: the production stack trace is the media-selection KVO variant (reloadCurrentMediaSelectionsAsynchronously→currentItem.status), not video-frame-analysis.Verified by reproducing teardown on a video-backed paywall (rapid dismiss / background-foreground) on iOS 16+; the manual-loop controls path and looping background both play correctly.
Note
Medium Risk
Touches AVFoundation/AVKit lifecycle on a high-traffic paywall path, but changes are iOS-internal to RevenueCatUI with targeted tests and no public API changes.
Overview
Fixes production crashes when looping no-controls paywall video backgrounds tear down (
currentItem.status/AVPlayerLooper+AVPlayerViewControllerKVO issue #6985).On iOS,
VideoPlayerViewnow branches: controls still useVideoPlayerUIView(AVPlayerViewController); backgrounds use newVideoPlayerLayerView(AVPlayerLayer), so AVKit’s internal player controller never observes a looper-driven queue player. The layer path can keepAVPlayerLooper; the controls path loops withdidPlayToEndTime+ seek on a singleAVPlayer, setsallowsVideoFrameAnalysis = false(iOS 16+), and tears down deterministically indismantleUIViewController.Player setup, audio session restore, and lifecycle pause/resume move into each representable’s
Coordinator.VideoAutoplayHandler.invalidate()stops foreground resume after teardown; unit tests cover invalidate and which player type each path uses.Reviewed by Cursor Bugbot for commit 58ba665. Bugbot is set up for automated code reviews on this repo. Configure here.