From f286061255017eb2e4010e942a2ca58043e79e97 Mon Sep 17 00:00:00 2001 From: low-batt <86170219+low-batt@users.noreply.github.com> Date: Thu, 30 Dec 2021 17:43:42 -0500 Subject: [PATCH] Fix crash in mpv_set_property during termination, #11 This was also reported against IINA in issue iina#3596 The commit in the pull request will: - Rename the PlayerCore property isMpvTerminated to isMpvTerminating to reflect that termination has been initiated and being processed asynchronously - Change existing references to isMpvTerminated to use new name - Change PlayerCore.terminateMPV to set isMpvTerminating to true right before starting the termination process - Change PlayerCore.syncUI to do nothing if mpv is terminating - Change MPVController.mpvQuit to remove observers MPVController registered before sending the quit command to mpv - Change many MPVController methods to check to see if mpv is being terminated --- iina/MPVController.swift | 34 +++++++++++++++++++++++++-- iina/MainWindowController.swift | 2 +- iina/MiniPlayerWindowController.swift | 2 +- iina/PlayerCore.swift | 10 ++++---- 4 files changed, 40 insertions(+), 8 deletions(-) diff --git a/iina/MPVController.swift b/iina/MPVController.swift index 55e0add273..a3f47360cf 100644 --- a/iina/MPVController.swift +++ b/iina/MPVController.swift @@ -418,8 +418,27 @@ class MPVController: NSObject { return flags & UInt64(MPV_RENDER_UPDATE_FRAME.rawValue) > 0 } + /// Remove registered observers for mpv properties and IINA preferences. + /// + /// This method is used when terminating mpv. Once the controller has initiated the shutdown of the mpv context we don't want to + /// continue using the context while mpv is asynchronously shutting it down. + private func removeObservers() { + // Remove observers for mpv properties. Because 0 was passed for reply_userdata when registering + // mpv property observers all observers can be removed in one call. + mpv_unobserve_property(mpv, 0) + // Remove observers for IINA preferences. + ObjcUtils.silenced { + self.optionObservers.forEach { (k, _) in + UserDefaults.standard.removeObserver(self, forKeyPath: k) + } + } + } + // Basically send quit to mpv func mpvQuit() { + removeObservers() + // The quit command executes asynchronously. A MPV_EVENT_SHUTDOWN event is emitted when the quit + // command finishes. command(.quit) } @@ -476,36 +495,43 @@ class MPVController: NSObject { // Set property func setFlag(_ name: String, _ flag: Bool) { + guard !player.isMpvTerminating else { return } var data: Int = flag ? 1 : 0 mpv_set_property(mpv, name, MPV_FORMAT_FLAG, &data) } func setInt(_ name: String, _ value: Int) { + guard !player.isMpvTerminating else { return } var data = Int64(value) mpv_set_property(mpv, name, MPV_FORMAT_INT64, &data) } func setDouble(_ name: String, _ value: Double) { + guard !player.isMpvTerminating else { return } var data = value mpv_set_property(mpv, name, MPV_FORMAT_DOUBLE, &data) } func setFlagAsync(_ name: String, _ flag: Bool) { + guard !player.isMpvTerminating else { return } var data: Int = flag ? 1 : 0 mpv_set_property_async(mpv, 0, name, MPV_FORMAT_FLAG, &data) } func setIntAsync(_ name: String, _ value: Int) { + guard !player.isMpvTerminating else { return } var data = Int64(value) mpv_set_property_async(mpv, 0, name, MPV_FORMAT_INT64, &data) } func setDoubleAsync(_ name: String, _ value: Double) { + guard !player.isMpvTerminating else { return } var data = value mpv_set_property_async(mpv, 0, name, MPV_FORMAT_DOUBLE, &data) } func setString(_ name: String, _ value: String) { + guard !player.isMpvTerminating else { return } mpv_set_property_string(mpv, name, value) } @@ -724,7 +750,7 @@ class MPVController: NSObject { switch eventId { case MPV_EVENT_SHUTDOWN: - let quitByMPV = !player.isMpvTerminated + let quitByMPV = !player.isMpvTerminating if quitByMPV { DispatchQueue.main.sync { NSApp.terminate(nil) @@ -865,7 +891,7 @@ class MPVController: NSObject { private func onVideoReconfig() { // If loading file, video reconfig can return 0 width and height - if player.info.fileLoading { + if player.info.fileLoading, player.isMpvTerminating { return } var dwidth = getInt(MPVProperty.dwidth) @@ -888,6 +914,10 @@ class MPVController: NSObject { // MARK: - Property listeners private func handlePropertyChange(_ name: String, _ property: mpv_event_property) { + // Once mpv starts terminating we no longer care about property changes and especially do not + // want to process changes that could trigger calls to mpv. Observers are being deregistered so + // there is only a brief window where this might trigger. + guard !player.isMpvTerminating else { return } var needReloadQuickSettingsView = false diff --git a/iina/MainWindowController.swift b/iina/MainWindowController.swift index 84f3799ee3..536ce0c7cc 100644 --- a/iina/MainWindowController.swift +++ b/iina/MainWindowController.swift @@ -1141,7 +1141,7 @@ class MainWindowController: PlayerWindowController { } } // stop playing - if !player.isMpvTerminated { + if !player.isMpvTerminating { if case .fullscreen(legacy: true, priorWindowedFrame: _) = fsState { restoreDockSettings() } diff --git a/iina/MiniPlayerWindowController.swift b/iina/MiniPlayerWindowController.swift index 281e72f639..02126865e8 100644 --- a/iina/MiniPlayerWindowController.swift +++ b/iina/MiniPlayerWindowController.swift @@ -186,7 +186,7 @@ class MiniPlayerWindowController: PlayerWindowController, NSPopoverDelegate { func windowWillClose(_ notification: Notification) { player.switchedToMiniPlayerManually = false player.switchedBackFromMiniPlayerManually = false - if !player.isMpvTerminated { + if !player.isMpvTerminating { // not needed if called when terminating the whole app player.switchBackFromMiniPlayer(automatically: true, showMainWindow: false) } diff --git a/iina/PlayerCore.swift b/iina/PlayerCore.swift index a24cbd0748..60841c8325 100644 --- a/iina/PlayerCore.swift +++ b/iina/PlayerCore.swift @@ -131,7 +131,7 @@ class PlayerCore: NSObject { var displayOSD: Bool = true - var isMpvTerminated: Bool = false + var isMpvTerminating: Bool = false var isInMiniPlayer = false var switchedToMiniPlayerManually = false @@ -377,14 +377,14 @@ class PlayerCore: NSObject { // Terminate mpv func terminateMPV(sendQuit: Bool = true) { - guard !isMpvTerminated else { return } + guard !isMpvTerminating else { return } + isMpvTerminating = true savePlaybackPosition() invalidateTimer() uninitVideo() if sendQuit { mpv.mpvQuit() } - isMpvTerminated = true } /// Wait until this player core has been terminated. @@ -1491,7 +1491,9 @@ class PlayerCore: NSObject { func syncUI(_ option: SyncUIOption) { // if window not loaded, ignore - guard mainWindow.loaded else { return } + // No need for updating the UI if currently terminating mpv. Requesting property values from mpv + // can cause crashes depending upon how far into the asynchronous termination sequence mpv is. + guard mainWindow.loaded, !isMpvTerminating else { return } Logger.log("Syncing UI \(option)", level: .verbose, subsystem: subsystem) switch option {