-
Notifications
You must be signed in to change notification settings - Fork 3k
refactor: don't use nitro dispose #4802
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the video player cleanup mechanism to avoid calling Nitro's dispose() method, which was causing crashes when player instances were accessed from events after disposal. The new approach introduces a release() method that cleans up native resources while allowing the hybrid object to be garbage collected naturally, with a 5-second grace period for pending events.
Key changes:
- Replaced
dispose()calls with a newrelease()method that only releases native resources - Added a 5-second timeout before nullifying the player reference to allow late events to complete
- Improved thread safety in Android's event emitter with synchronized blocks
- Added
AutoCloseableinterface to Android'sHybridVideoPlayerfor better resource management
Reviewed changes
Copilot reviewed 7 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
packages/react-native-video/src/spec/nitro/VideoPlayer.nitro.ts |
Added release() method to VideoPlayer interface and documented source replacement behavior |
packages/react-native-video/src/core/VideoPlayer.ts |
Implemented delayed cleanup with getter-based access control and 5-second timeout for event processing |
packages/react-native-video/nitrogen/generated/shared/c++/HybridVideoPlayerSpec.hpp |
Generated C++ header with new release() method declaration |
packages/react-native-video/nitrogen/generated/shared/c++/HybridVideoPlayerSpec.cpp |
Generated C++ implementation registering release() as hybrid method |
packages/react-native-video/nitrogen/generated/ios/swift/HybridVideoPlayerSpec_cxx.swift |
Generated Swift-C++ bridge implementation for release() method |
packages/react-native-video/nitrogen/generated/ios/swift/HybridVideoPlayerSpec.swift |
Generated Swift protocol with release() method signature |
packages/react-native-video/nitrogen/generated/ios/c++/HybridVideoPlayerSpecSwift.hpp |
Generated iOS C++ wrapper implementing release() method |
packages/react-native-video/nitrogen/generated/android/kotlin/com/margelo/nitro/video/HybridVideoPlayerSpec.kt |
Generated Kotlin spec with abstract release() method |
packages/react-native-video/nitrogen/generated/android/c++/JHybridVideoPlayerSpec.hpp |
Generated Android JNI header with release() override declaration |
packages/react-native-video/nitrogen/generated/android/c++/JHybridVideoPlayerSpec.cpp |
Generated Android JNI implementation calling Java release() method |
packages/react-native-video/ios/hybrids/VideoPlayer/HybridVideoPlayer.swift |
Implemented release() to clean up iOS player resources and clear event listeners |
packages/react-native-video/ios/hybrids/VideoPlayer/HybridVideoPlayer+Events.swift |
Modified playback status logic to handle non-waiting states differently |
packages/react-native-video/android/src/main/java/com/twg/video/hybrids/videoplayereventemitter/HybridVideoPlayerEventEmitter.kt |
Added thread safety with synchronized blocks for listener management and event emission |
packages/react-native-video/android/src/main/java/com/twg/video/hybrids/videoplayer/HybridVideoPlayer.kt |
Made release() public, added AutoCloseable interface, and updated memory size calculation |
packages/react-native-video/android/src/main/java/com/twg/video/core/VideoManager.kt |
Enhanced player unregistration to null out player references in associated views |
packages/drm-plugin/nitrogen/generated/ios/swift/HybridPluginManagerSpec_cxx.swift |
Auto-generated update from Nitro code generator (Bool conversion and toString) |
packages/drm-plugin/nitrogen/generated/ios/swift/HybridPluginManagerSpec.swift |
Auto-generated default toString() implementation |
packages/drm-plugin/nitrogen/generated/ios/c++/HybridPluginManagerSpecSwift.hpp |
Auto-generated toString() C++ wrapper |
packages/drm-plugin/nitrogen/generated/ios/ReactNativeVideoDrm-Swift-Cxx-Bridge.cpp |
Auto-generated include for NitroDefines header |
packages/drm-plugin/nitrogen/generated/android/kotlin/com/margelo/nitro/videodrm/HybridPluginManagerSpec.kt |
Auto-generated default toString() implementation and import cleanup |
packages/drm-plugin/nitrogen/generated/android/c++/JHybridPluginManagerSpec.hpp |
Auto-generated toString() declaration |
packages/drm-plugin/nitrogen/generated/android/c++/JHybridPluginManagerSpec.cpp |
Auto-generated toString() JNI implementation |
example/ios/Podfile.lock |
Version bump from 7.0.0-beta.0 to 7.0.0-beta.1 |
bun.lock |
Version bump across packages from 7.0.0-beta.0 to 7.0.0-beta.1 |
Comments suppressed due to low confidence (2)
packages/react-native-video/android/src/main/java/com/twg/video/hybrids/videoplayer/HybridVideoPlayer.kt:374
- The
release()method is not synchronized or protected against concurrent calls. Ifrelease()is called multiple times concurrently, it could lead to race conditions where listeners are removed multiple times or the player is released while still being accessed. Consider adding synchronization or a flag to ensurerelease()only executes once, similar to the guard in the TypeScript implementation at line 49.
override fun release() {
if (playInBackground || showNotificationControls) {
VideoPlaybackService.stopService(this, videoPlaybackServiceConnection)
}
runOnMainThread {
VideoManager.unregisterPlayer(this)
stopProgressUpdates()
loadedWithSource = false
eventEmitter.clearAllListeners()
player.removeListener(playerListener)
player.removeAnalyticsListener(analyticsListener)
player.release() // Release player
// Clean Listeners
audioFocusChangedListener.removeEventEmitter()
audioBecomingNoisyReceiver.removeEventEmitter()
// Update status
status = VideoPlayerStatus.IDLE
}
}
packages/react-native-video/ios/hybrids/VideoPlayer/HybridVideoPlayer.swift:224
- The
release()method is not protected against concurrent calls. Ifrelease()is called multiple times concurrently or while other operations are in progress, it could lead to race conditions or attempting to access already-released resources. Consider adding a flag to track whether release has already been called and guard against multiple executions, similar to the TypeScript implementation's guard at line 49.
func release() {
sourceLoader.cancelSync()
NowPlayingInfoCenterManager.shared.removePlayer(player: player)
try? _eventEmitter?.clearAllListeners()
self.player.replaceCurrentItem(with: nil)
self.playerItem = nil
if let source = self.source as? HybridVideoPlayerSource {
source.releaseAsset()
}
// Clear player observer
self.playerObserver = nil
status = .idle
VideoManager.shared.unregister(player: self)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ct-native-video/android/src/main/java/com/twg/video/hybrids/videoplayer/HybridVideoPlayer.kt
Outdated
Show resolved
Hide resolved
packages/react-native-video/ios/hybrids/VideoPlayer/HybridVideoPlayer+Events.swift
Show resolved
Hide resolved
packages/react-native-video/src/spec/nitro/VideoPlayer.nitro.ts
Outdated
Show resolved
Hide resolved
…o/hybrids/videoplayer/HybridVideoPlayer.kt Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Hi, I had a quick question about your PR and version 7 in general. The point of Perhaps I misunderstood the code, and it's specific to Android, I don't know. But I find it more intuitive to create/replace with: Can you explain its purpose/use to me, as I may have misunderstood it? |
Summary
Until now, when player was released we call
dispose()to clear native object, but oncedispose()is called we cannot access any properties.This cause crashes when player is used with events:
sequenceDiagram autonumber participant RN as React Native participant App as App Logic participant Player as HybridVideoPlayer Player->>RN: Event is called (scheduled by RN) App->>Player: Release player (dispose()) Note right of Player: Resources freed and references invalid App-->>RN: JS callback scheduled/executed RN->>App: Run JS callback App->>Player: Access player reference Player-->>App: Crash due to disposed instance Note over App,Player: Access after dispose() triggers crashThis PR changes this behaviour. Now we don't call
dispose(), instead we set hybrid player to undefined and we let GC to clean it when needed. We throw "specific" error when, player being accessed after release. This fixes uncontrolled crashes