Fix random failure to resume playback position, #3939 #3961
Conversation
|
This proposed fix is the same fix in closed PR #3952 from a high level perspective. But the new code is event driven eliminating the background thread that was being used by |
|
|
||
| // Establish an observer for a player core shutting down. | ||
| observer = center.addObserver(forName: .iinaPlayerShutdown, object: nil, queue: .main) { _ in | ||
| guard !timedOut else { |
There was a problem hiding this comment.
I don't see the meaning of this guard and the timeOut variable. Soon after timeOut is set to true, the application is terminated forcibly, no?
There was a problem hiding this comment.
Maybe I should rephrase that "Forcing application termination" log message. The timeout tells Cocoa to proceed with termination. This does not happen immediately. Tasks on the main queue are still run after this. This guard statement insures IINA does not reply again to Cocoa telling it a second time to proceed with termination. Was worried about what Cocoa would do if IINA replied a second time. This also avoids outputing the log message that indicates a normal termination.
Eventually after IINA tells Cocoa to proceed with termination AppKit will call back to IINA, executing applicationWillTerminate. At that point IINA closes the log file. Even after that the main queue may run other tasks before the application is terminated. We know that happens because it was causing crashes when a task tried to log a message after the log file was closed.
Maybe the log message should be: "Timeout. Proceeding with application termination"?
There was a problem hiding this comment.
Okay, got that. I also think is reasonable to change the log message.
| // Initiate application termination. AppKit requires this be done from the main thread, | ||
| // however the main dispatch queue must not be used to avoid blocking the queue as per | ||
| // instructions from Apple. | ||
| if #available(macOS 10.12, *) { |
There was a problem hiding this comment.
Terminating one mpv instance should not cause the whole app to be terminated. I know this was the behavior before, but I think it's a good time to correct that behavior.
This commit will: - Add isStopped and isShutdown properties to PlayerCore - Rename PlayerCore property isMpvTerminated to isShuttingDown - Add playbackStopped and mpvHasShutdown methods to PlayerCore - Add savePlayerState method to PlayerCore - Add iinaPlayerStopped and iinaPlayerShutdown notifications - Change PlayerCore to post the new notifications - Change savePlaybackPosition to not send a command to write the watch later file if playback has been stopped - Move code in MainWindowController method windowWillClose related to stopping playback to the PlayerCore stop method - Change MPVController to inform PlayerCore when the stop command finishes and when mpv shuts down - Change MPVController to remove IINA preference observers before sending a quit command to mpv - Change MPVController to not use the main dispatch queue when calling NSApp.terminate to avoid blocking the queue - Add an isDisabled property to MenuController - Add disableAllMenus and disableAllMenuItems methods to MenuController - Change menuWillOpen to not update menus if menus are disabled - Add a disableAllCommands method to RemoteCommandController - Add isTerminated property to AppDelegate - Add a removeAllMenuItems method to AppDelegate - Change AppDelegate method applicationShouldHandleReopen to not permit reopening during termination - Change AppDelegate method applicationShouldTerminate to return terminateLater to Cocoa to delay termination until mpv has finished shutting down - Change applicationShouldTerminate to shutdown further user input and coordinate application shutdown using observers for the new iinaPlayerStopped and iinaPlayerShutdown notifications From a high level perspective these changes: - Stop further input from the user once application termination starts - Disallow reopening during termination - Wait for the asynchronous mpv stop command to complete before sending the mpv quit command - Wait for the asynchronous quit command to complete before allowing Cocoa to proceed with termination These changes are needed because mpv may decide to write to the watch later file and then fail to store the playback position if the quit command is sent to mpv while a stop command is still executing. This breaks the "Resume last playback position" IINA feature. To correct this IINA must move to a controlled clean shutdown.
This commit will: - Add isStopped and isShutdown properties to PlayerCore - Rename PlayerCore property isMpvTerminated to isShuttingDown - Add playbackStopped and mpvHasShutdown methods to PlayerCore - Add savePlayerState method to PlayerCore - Add iinaPlayerStopped and iinaPlayerShutdown notifications - Change PlayerCore to post the new notifications - Change savePlaybackPosition to not send a command to write the watch later file if playback has been stopped - Move code in MainWindowController method windowWillClose related to stopping playback to the PlayerCore stop method - Change MPVController to inform PlayerCore when the stop command finishes and when mpv shuts down - Change MPVController to remove IINA preference observers before sending a quit command to mpv - Change MPVController to not use the main dispatch queue when calling NSApp.terminate to avoid blocking the queue - Add an isDisabled property to MenuController - Add disableAllMenus and disableAllMenuItems methods to MenuController - Change menuWillOpen to not update menus if menus are disabled - Add a disableAllCommands method to RemoteCommandController - Add isTerminated property to AppDelegate - Add a removeAllMenuItems method to AppDelegate - Change AppDelegate method applicationShouldHandleReopen to not permit reopening during termination - Change AppDelegate method applicationShouldTerminate to return terminateLater to Cocoa to delay termination until mpv has finished shutting down - Change applicationShouldTerminate to shutdown further user input and coordinate application shutdown using observers for the new iinaPlayerStopped and iinaPlayerShutdown notifications From a high level perspective these changes: - Stop further input from the user once application termination starts - Disallow reopening during termination - Wait for the asynchronous mpv stop command to complete before sending the mpv quit command - Wait for the asynchronous quit command to complete before allowing Cocoa to proceed with termination These changes are needed because mpv may decide to write to the watch later file and then fail to store the playback position if the quit command is sent to mpv while a stop command is still executing. This breaks the "Resume last playback position" IINA feature. To correct this IINA must move to a controlled clean shutdown.
This commit will:
From a high level perspective these changes:
These changes are needed because mpv may decide to write to the watch later file and then fail to store the playback position if the quit command is sent to mpv while a stop command is still executing. This breaks the "Resume last playback position" IINA feature. To correct this IINA must move to a controlled clean shutdown.
Description:
This proposed fix replaces the original thread based fix with one that is event driven.