Fix data races, #3829, #3830, #3936, #3937 #4819

low-batt · 2024-02-14T20:59:25Z

This commit will:

Change MPVController to perform most event processing using the main thread
Remove dispatching to main queue from PlayerCore methods now being called by MPVController using the main thread
Add the Atomic property wrapper to the playlistTotalLength and playlistTotalLengthIsReady PlaylistViewController properties
Add the Atomic property wrapper to the following PlayerInfo properties:
- aid
- secondSid
- sid
- subTracks
- thumbnails
- thumbnailsProgress
- thumbnailsReady
- vid
Change references to subTracks to lock the array
Change references to thumbnails to lock the array
Add the Atomic property wrapper to the hooks MPVController property
Change references to hooks to lock the array
Change PlayerCore.playbackRestarted to re-enable the OSD by dispatching to the main thread using async with a delay instead of using a timer
Add the method logPropertyValueError to MPVController to report when property values can not be converted to the expected type
Replace MPVController method reEnableOSDAfterFileLoading with inline code

These changes will correct data races between the main thread and the thread for the MPVController com.colliderli.iina.controller queue by changing MPVController to dispatch work to the main queue. This will simplify the code as in some cases work was being done on both the controller queue and the main queue as some processing required calling AppKit methods that must be run on the main thread.

These changes will correct the data races between the main thread and PlayerCore backgroundQueue used for loading subtitles by adding the Atomic property wrapper to properties shared between these threads and using the withLock method. Similar changes correct the data races between the main thread and PlayerCore thumbnailQueue.

These changes will also fix data races with the playlistTotalLengthIsReady and playlistTotalLength properties.

Description:

low-batt · 2024-02-14T21:13:57Z

Been waiting for a release that includes beta testing to make these significant changes. Lots here for reviewers to criticize.

Before the changes in this PR MPVController performed many operations using its com.colliderli.iina.controller queue. Some operations were partially performed on that queue and then dispatched using DispatchQueue.main.sync due to needing to call AppKit methods that can only be called using the main thread. In the documentation for DispatchQueue Apple says:

Important
Attempting to synchronously execute a work item on the main queue results in deadlock.

So we should be careful about using DispatchQueue.main.sync . I'm assuming sync was chosen in an attempt to keep the data modified while running in the controller queue consistent with the data modified while running on the main thread?

Rather than add locks around lots of properties this PR changes MPVController to dispatch most work using DispatchQueue.main.async so that only one thread is accessing the data and locks are not needed. For other situations where we really do need processing to occur in the background locks are used.

Does anyone know why the re-enabling of the OSD which is disabled until after file loading is not re-enabled in the fileLoaded method and instead is done in playbackRestarted and involves a 0.2 second delay?

As mpv is forcing us to raise the minimum supported macOS version to 10.15, another possibility would be to put data shared between threads into Actors. As we already have too much work to do for 1.4.0, I thought it would be better solve the data races using our current techniques and consider making use of Swift Concurrency in a future release.

svobs · 2024-02-15T11:35:02Z

So we should be careful about using DispatchQueue.main.sync . I'm assuming sync was chosen in an attempt to keep the data modified while running in the controller queue consistent with the data modified while running on the main thread?

I'm a lot more doubtful about the need for DispatchQueue.main.sync. On my fork I did a bunch of refactoring to ensure that com.colliderli.iina.controller was used for all libmpv API calls, and just replaced a few of the syncs with async. No major hiccups to report.

Are you familiar with dispatchPrecondition(condition: .onQueue(DispatchQueue.main)) for indicating / enforcing which execution queue a given method is running from? Would highly recommend for reducing bugs / increasing sanity.

Does anyone know why the re-enabling of the OSD which is disabled until after file loading is not re-enabled in the fileLoaded method and instead is done in playbackRestarted and involves a 0.2 second delay?

The playbackRestarted event (despite being called more often in general) is fired later in the file loading sequence. There is a bunch of stuff that happens after fileLoaded like playlist refresh and (if I recall correctly) tracklist and current track may not be finished loading yet. A lot of these things generate events which trigger display of OSD messages.

But MPV_EVENT_PLAYBACK_RESTART is almost always accompanied by an MPV_EVENT_SEEK message, and often a pause property change, and IINA normally generates an OSD message for each whenever it receives one. It's hard to distinguish them from user behavior. Also, the order in which they are received is not deterministic. So the 0.2 sec delay seems to be enough to block these redundant messages.

low-batt · 2024-02-16T03:15:16Z

Good to hear that switching from sync to async did not exhibit problems. Of course the trouble with thread related changes is that it may behave differently on other Macs. I fixed some other thread related problems that I never could reproduce on my Macs, but the timing was right on some user's Macs such that they would experience the problem. That is why I held off on these changes for a release that uses beta releases.

Using dispatchPrecondition is a good tip. I will think about where that could be used.

I was expecting the odd 0.2 second delay and the code not being in the fileLoaded method had something to do with suppressing OSD messages for events after the MPV_EVENT_FILE_LOADED. What disturbs me is that there aren't any comments in the code about this. The name of the flag (disableOSDForFileLoading) makes you expect the OSD would be re-enabled upon receiving the MPV_EVENT_FILE_LOADED event. Code that relies upon delays like this should have comments discussing why a nondeterministic solution is required. Anyway, this PR preserves the current behavior. It just replaces a timer with asyncAfter.

low-batt · 2024-05-27T00:09:18Z

Rebased with develop and fixed merge conflicts.

low-batt · 2024-06-21T19:16:37Z

Rebased with develop again and fixed merge conflicts.

lhc70000

Good job! Generally looks good to me.

Another concern is that now lots of functions in PlayerCore are meant to be called on the main thread. We should at least add some comments to alert future developers.

lhc70000 · 2024-06-21T23:12:53Z

iina/MPVController.swift

-          mpv_hook_continue(self.mpv, hookID)
-        }
+      let hook = $hooks.withLock { $0[userData] }
+      guard let hook = hook else { break }


We can write

guard let hook = $hooks.withLock({ $0[userData] }) else { break }

lhc70000 · 2024-06-21T23:22:38Z

iina/MPVController.swift

-      player.refreshEdrMode()
+      if #available(macOS 10.15, *) {
+        DispatchQueue.main.async { self.player.refreshEdrMode() }
+      }


We are now targeting 10.15. Maybe you added it back accidentally during rebase?

Yes, I probably missed that during the rebase. Will fix.

lhc70000 · 2024-06-21T23:27:49Z

iina/MPVController.swift

-      player.syncUI(.chapterList)
-      player.postNotification(.iinaMediaTitleChanged)
+      DispatchQueue.main.async { [self] in
+        player.info.chapter = Int(getInt(MPVProperty.chapter))


This getInt() needs to be moved outside

I'm confused. Why can't getInt be called on the main thread?

Because it's mpv_get_property(), and we want it to be on the com.colliderli.iina.controller thread. If it's on the main thread, then for example, mpv_destroy() is on the com.colliderli.iina.controller thread, and it will cause another potential crash due to race condition when closing the window.

iina/iina/MPVController.swift

Lines 970 to 975 in 1ebe0f4

} else {

RunLoop.main.perform(#selector(self.terminateApplication), target: self,

argument: nil, order: Int.min, modes: [.common])

}

} else {

mpv_destroy(mpv)

Will fix, but we do call methods like getInt outside of MPVController from code I'm sure is running on the main thread. I will think on how to better coordinate this. I have seen mpv emit some events during shutdown that it really shouldn't be emitting. I've been worried that we let q through to mpv causing mpv to shutdown "behind" IINA's back although I've not been able to trigger a crash in testing that. The gets are a problem because they do not return optional values. If they did it would be easy to detect shutdown and return nil.

Yes, but these ones are perhaps much easier to trigger because it's in handleEvent. We can't say exactly that "mpv is emitting events during shutdown", because this block is queued on the main thread and executed later than the event, after some unknown delay. If mpv shuts down right after emitting the event, there will be a crash.

The gets are a problem because they do not return optional values. If they did it would be easy to detect shutdown and return nil.

If we are going to add the check, I think we can just return 0. If mpv is already terminated then the value makes little sense.

Brain wasn't remembering this PR last night, it has been a while since I coded it. I remember thinking having the get methods return some random value seemed a bit wrong. So I decided it would be better to coordinate shutdown at a higher level. I have been slowly refactoring MPVController to call "listener" methods in PlayerCore. Those methods have the ability to check if shutdown is in progress and if so, drop the event. I need to refactor the code you pointed out into additional "listener" methods in PlayerCore that check the state and avoid calling getInt.

I'm used to a layered design where the bottom layer classes know very little about the higher level code. I was expecting we'd have a class that was focused solely on providing a Swift interface for the mpv client. Since we did not have such a class I did not consider implementing the interesting suggestion from @CarterLi in PR #4367 of using mpv to display previews of JPEG XL screenshots. In macOS Sonoma AppKit renders the preview of JPEG XL screenshots, so going forward mpv is not needed for that. However we still at some point need support for HDR screenshots. One way to do that would be to move away from our proprietary thumbnail file format and store the thumbnails as a MP4 and use mpv to display them. A discussion for another day. Anyway, just an example of how a more isolated mpv client could have been used.

I think I have addressed the comments. I added a comment about the main thread requirement. I moved the code that was calling back into mpv to PlayerCore with checks for shutting down.

lhc70000 · 2024-06-21T23:33:12Z

iina/MPVController.swift

-        DispatchQueue.main.async(execute: self.player.mainWindow.toggleWindowFullScreen)
+      DispatchQueue.main.async { [self] in
+        guard player.mainWindow.loaded else { return }
+        let fs = getFlag(MPVOption.Window.fullscreen)


getFlag should be moved out

lhc70000 · 2024-06-21T23:33:31Z

iina/MPVController.swift

-          self.player.mainWindow.setWindowFloatingOnTop(ontop)
+      DispatchQueue.main.async { [self] in
+        guard player.mainWindow.loaded else { return }
+        let ontop = getFlag(MPVOption.Window.ontop)


same as above

lhc70000 · 2024-06-21T23:33:42Z

iina/MPVController.swift

-          self.player.mainWindow.setWindowScale(windowScale)
+      DispatchQueue.main.async { [self] in
+        guard player.mainWindow.loaded else { return }
+        let windowScale = getDouble(MPVOption.Window.windowScale)


same as above

This commit will: - Change MPVController to perform most event processing using the main thread - Remove dispatching to main queue from PlayerCore methods now being called by MPVController using the main thread - Add the Atomic property wrapper to the playlistTotalLength and playlistTotalLengthIsReady PlaylistViewController properties - Add the Atomic property wrapper to the following PlayerInfo properties: - aid - secondSid - sid - subTracks - thumbnails - thumbnailsProgress - thumbnailsReady - vid - Change references to subTracks to lock the array - Change references to thumbnails to lock the array - Add the Atomic property wrapper to the hooks MPVController property - Change references to hooks to lock the array - Add the Atomic property wrapper to the playlistTotalLengthIsReady and playlistTotalLength PlaylistViewController properties - Change PlayerCore.playbackRestarted to re-enable the OSD by dispatching to the main thread using async with a delay instead of using a timer - Add the method logPropertyValueError to MPVController to report when property values can not be converted to the expected type - Replace MPVController method reEnableOSDAfterFileLoading with inline code These changes will correct data races between the main thread and the thread for the MPVController com.colliderli.iina.controller queue by changing MPVController to dispatch work to the main queue. This will simplify the code as in some cases work was being done on both the controller queue and the main queue as some processing required calling AppKit methods that must be run on the main thread. These changes will also correct the data races between the main thread and PlayerCore backgroundQueue used for loading subtitles by adding the Atomic property wrapper to properties shared between these threads and using the withLock method. Similar changes correct the data races between the main thread and PlayerCore thumbnailQueue. These changes will also fix data races with the playlistTotalLengthIsReady and playlistTotalLength properties.

low-batt mentioned this pull request Mar 7, 2024

OSD Progress bar very late to update #4779

Open

1 task

low-batt mentioned this pull request Mar 20, 2024

Ctrl-s (screenshot window) does not work #4822

Closed

1 task

low-batt force-pushed the tsan branch from dbfed32 to d3f0441 Compare May 27, 2024 00:08

low-batt mentioned this pull request May 31, 2024

Fix ctrl-s (screenshot window) does not work, #4822 #4830

Merged

2 tasks

low-batt requested a review from uiryuu June 1, 2024 01:07

uiryuu assigned lhc70000 Jun 2, 2024

low-batt force-pushed the tsan branch from d3f0441 to 1ebe0f4 Compare June 21, 2024 19:15

low-batt requested a review from lhc70000 June 21, 2024 19:17

lhc70000 requested changes Jun 22, 2024

View reviewed changes

low-batt added 3 commits June 22, 2024 17:31

low-batt force-pushed the tsan branch from 5e9ff7c to 61c1986 Compare June 22, 2024 22:05

lhc70000 approved these changes Jun 25, 2024

View reviewed changes

lhc70000 merged commit ee10496 into develop Jun 25, 2024
1 check passed

lhc70000 deleted the tsan branch June 25, 2024 03:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix data races, #3829, #3830, #3936, #3937 #4819

Fix data races, #3829, #3830, #3936, #3937 #4819

low-batt commented Feb 14, 2024

low-batt commented Feb 14, 2024

svobs commented Feb 15, 2024 •

edited

Loading

low-batt commented Feb 16, 2024

low-batt commented May 27, 2024

low-batt commented Jun 21, 2024

lhc70000 left a comment

lhc70000 Jun 21, 2024

lhc70000 Jun 21, 2024

low-batt Jun 22, 2024

lhc70000 Jun 21, 2024

low-batt Jun 22, 2024

lhc70000 Jun 22, 2024

low-batt Jun 22, 2024

lhc70000 Jun 22, 2024

low-batt Jun 22, 2024

low-batt Jun 22, 2024

lhc70000 Jun 21, 2024

lhc70000 Jun 21, 2024

lhc70000 Jun 21, 2024

	} else {
	RunLoop.main.perform(#selector(self.terminateApplication), target: self,
	argument: nil, order: Int.min, modes: [.common])
	}
	} else {
	mpv_destroy(mpv)

Fix data races, #3829, #3830, #3936, #3937 #4819

Fix data races, #3829, #3830, #3936, #3937 #4819

Conversation

low-batt commented Feb 14, 2024

low-batt commented Feb 14, 2024

svobs commented Feb 15, 2024 • edited Loading

low-batt commented Feb 16, 2024

low-batt commented May 27, 2024

low-batt commented Jun 21, 2024

lhc70000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svobs commented Feb 15, 2024 •

edited

Loading