Fix crash while quitting in AutoFileMatcher, #5056 #5102
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
uiryuu
requested changes
Aug 5, 2024
iina/PlayerState.swift
Outdated
| @@ -58,7 +58,7 @@ enum PlayerState: Int { | |||
| /// | |||
| /// These are the states in which the player normally interacts with the mpv core. The mpv core **must not** be accessed when the | |||
| /// player is in the `shuttingDown` or `shutDown` states. Accessing the core in these states can trigger a crash. | |||
| @inlinable var active: Bool { self.rawValue <= PlayerState.stopping.rawValue } | |||
| @inlinable var active: Bool { self.rawValue < PlayerState.stopping.rawValue } | |||
There was a problem hiding this comment.
Please revert this line since it's already fixed in dev.
There was a problem hiding this comment.
I will rebase with dev.
There was a problem hiding this comment.
Rebased with develop so this change is no longer in the commit. I amended the commit comment and removed the statement about changing active.
| @@ -1024,18 +1028,23 @@ class MPVController: NSObject { | |||
| private func readEvents() { | |||
| queue.async { | |||
| while ((self.mpv) != nil) { | |||
| let event = mpv_wait_event(self.mpv, 0) | |||
| let event = mpv_wait_event(self.mpv, 0)! | |||
There was a problem hiding this comment.
The change here is that the code was treating the return as option in readEvents, but then passed it to handleEvent which declared the type as UnsafePointer<mpv_event>!. So readEvents went to the trouble of treating it as optional only for it to be forced when handleEvent was called. Thus the forcing just got moved to an earlier time. I don't think there is a change as far as risking a crash due to force unwrapping.
If this is ever nil then mpv is seriously broken as the doc for mpv_wait_event says:
The return value is never NULL.
| guard currentTicket == self.backgroundQueueTicket else { return } | ||
| self.loadExternalSubFile(sub) | ||
| backgroundTaskInUse = true | ||
| backgroundQueue.async { [self] in |
There was a problem hiding this comment.
Consider to use [unowned self] here?
There was a problem hiding this comment.
We never free players until shutdown so I don't see an advantage to the background task not having a strong reference.
| log("Background task stopping due to error \(err.localizedDescription)", level: .error) | ||
| } | ||
| // This code must be queued to the main thread to avoid thread data races. | ||
| DispatchQueue.main.async { [self] in |
This commit will: - Change the PlaybackInfo.state computed property to prevent additional inappropriate state transitions - Change PlayerCore.sendOSD to not show messages unless the player is active to prevent an unreported crash - Change MPVController.readEvents to stop once a MPV_EVENT_SHUTDOWN has been processed - Change the fileLoaded and fileStarted PlayerCore methods to do nothing if the player state is not active - Change the logger subsystem for AutoFileMatcher to include the player identifier - Change the names of the PlayerCore dispatch queues to include the player identifier - Move TicketExpiredError from AutoFileMatcher to PlayerCore - Add a checkTicket method to PlayerCore - Change more AutoFileMatcher methods to check the ticket and throw errors - Change AutoFileMatcher.startMatching to rethrow errors - Change PlayerCore.fileStarted to check the background queue ticket and catch errors - Change PlayerCore.fileStarted to have the background task queue a main thread task that will continue the process of stopping the player if it was waiting for the background task to finish - Change PlayerCore.findIdlePlayerCore to not choose an idle core if it's background task is still running These changes cause the player to not send a stop command to mpv until the background task has stopped. A mpv core will stop on its own when the end of a video is reached. For that reason these changes also cause the player to not send a quit command to mpv until the background task has stopped. These changes correct crashes that happens when the background task is still running when the mpv core is quitting. This can occur for example, when a user starts playing the wrong video and immediately quits IINA before the background task has a chance to finish.
0f18254 to
f72d224
Compare
MouJieQin
pushed a commit
to MouJieQin/iina-for-varchive
that referenced
this pull request
Oct 21, 2024
This commit will: - Change the PlaybackInfo.state computed property to prevent additional inappropriate state transitions - Change PlayerCore.sendOSD to not show messages unless the player is active to prevent an unreported crash - Change MPVController.readEvents to stop once a MPV_EVENT_SHUTDOWN has been processed - Change the fileLoaded and fileStarted PlayerCore methods to do nothing if the player state is not active - Change the logger subsystem for AutoFileMatcher to include the player identifier - Change the names of the PlayerCore dispatch queues to include the player identifier - Move TicketExpiredError from AutoFileMatcher to PlayerCore - Add a checkTicket method to PlayerCore - Change more AutoFileMatcher methods to check the ticket and throw errors - Change AutoFileMatcher.startMatching to rethrow errors - Change PlayerCore.fileStarted to check the background queue ticket and catch errors - Change PlayerCore.fileStarted to have the background task queue a main thread task that will continue the process of stopping the player if it was waiting for the background task to finish - Change PlayerCore.findIdlePlayerCore to not choose an idle core if it's background task is still running These changes cause the player to not send a stop command to mpv until the background task has stopped. A mpv core will stop on its own when the end of a video is reached. For that reason these changes also cause the player to not send a quit command to mpv until the background task has stopped. These changes correct crashes that happens when the background task is still running when the mpv core is quitting. This can occur for example, when a user starts playing the wrong video and immediately quits IINA before the background task has a chance to finish.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit will:
activecomputed property inPlayerStateto not consider the stopping state as activePlaybackInfo.statecomputed property to prevent additional inappropriate state transitionsPlayerCore.sendOSDto not show messages unless the player is active to prevent an unreported crashMPVController.readEventsto stop once aMPV_EVENT_SHUTDOWNhas been processedfileLoadedandfileStartedPlayerCoremethods to do nothing if the player state is not activeAutoFileMatcherto include the player identifierPlayerCoredispatch queues to include the player identifierTicketExpiredErrorfromAutoFileMatchertoPlayerCorecheckTicketmethod toPlayerCoreAutoFileMatchermethods to check the ticket and throw errorsAutoFileMatcher.startMatchingto rethrow errorsPlayerCore.fileStartedto check the background queue ticket and catch errorsPlayerCore.fileStartedto have the background task queue a main thread task that will continue the process of stopping the player if it was waiting for the background task to finishPlayerCore.findIdlePlayerCoreto not choose an idle core if it's background task is still runningThese changes cause the player to not send a stop command to mpv until the background task has stopped. A mpv core will stop on its own when the end of a video is reached. For that reason these changes also cause the player to not send a quit command to mpv until the background task has stopped. These changes correct crashes that happens when the background task is still running when the mpv core is quitting. This can occur for example, when a user starts playing the wrong video and immediately quits IINA before the background task has a chance to finish.
Description: