See comment in AppDelegate. Shuffling is happening after the first file has started playing, so the first file played in a directory is always the same.

I'm working on a solution, but this is turning out to be a tricky fix. It's going to involve manipulating the file opening process so that the user doesn't see the initial load before playlist shuffle. But the loading process has a few asynchronous stages, and I need to make sure I have a very good understanding of how they all work in order to feel confident about the fix.

Getting minor surgery on my wrist & fingers later today. Will be mostly away from keyboards for a couple of weeks. If a fix is urgently needed it may be better to just back out the changes from #4439 until this is ready.

@svobs Still busy, or have time to work on this one?

We are currently actively reviewing and merging fixes for regressions, so this one is a high priority to fix.

The problem with the current fix is that mpv starts playing a video before the playlist is shuffled. So each time you play a directory the same video is played first. I don't think we tried commanding mpv to play the first video in the playlist after it is shuffled. That works, but I am concerned mpv starts playing the wrong file. To reduce the problems that might cause we can avoid sending the commands in AppDelegate and instead use a mpv hook. I did a quick test and it seemed to work.

I added a flag to PlayerCore:

var shufflePending = false

And a new listener:

  func onLoad() {
    guard shufflePending else { return }
    Logger.log("Shuffling playlist")
    mpv.command(.playlistShuffle)
    mpv.command(.playlistPlayIndex, args: ["0"])
    shufflePending = false
  }

I then added this to MPVHook:

  static let onBeforeStartFile = MPVHook("on_before_start_file")

And then added this to MPVController.mpvInit right before property observers are setup:

    addHook(MPVHook.onBeforeStartFile, hook: MPVHookValue(withBlock: { next in
      self.player.onLoad()
      next()
    }))

And changed the code in AppDelegate that was sending the shuffle command to just set the shufflePending flag to true.

This uses the on_before_start_file hook:

on_before_start_file
Run before a start-file event is sent. (If any client changes the current playlist entry, or sends a quit command to the player, the corresponding event will not actually happen after the hook returns.) Useful to drain property changes before a new file is loaded.

More testing is needed.

@svobs Still busy, or have time to work on this one?
We are currently actively reviewing and merging fixes for regressions, so this one is a high priority to fix.

Less busy now, but still going to be a busy month.

@low-batt as mentioned before, it'd be easier to just revert. Adding support for CLI shuffle looks tricky relative to the amount of reward, and if the goal is just to have better compatibility with mpv I'm sure there are lower-hanging fruit to pick. But I'm guessing it's a feature you use regularly? ;) Well, you did somehow get the fix for "exact" seek pushed through and I'm grateful for that, so I'll work on this a bit more.

Never used mpv hooks until now, but will try that. Hopefully it can do a shuffle before the file is marked as being "open"...

We can't just revert as that would reintroduce issue #4434.

Using mpv to open the directory fixes #4434 but causes issue #4548. To correct that mpv indicates a script should be used. That sounds messy enough that I suggest we go back (revert) to your original fix and try and fix the problem we found with that fix. That original fix sent the comment to shuffle the playlist. That almost worked except the same file was always played first. As you can see above in the onload hook it shuffles and then sends a command to play the first file in the playlist.

A hook is used to make this happen as soon as possible. I think this works, but more testing is needed. It looks like mpv opens the file, but before it starts playing the hook runs and that mpv never starts playing the file.

If you need any help adding the hook, I can add that code to sources and post them here.

Thanks very much for the exact fix. Really good to get that one merged.

@low-batt I implemented the changes as you outlined, but found that on_before_start_file didn't end up shuffling all the files in my test case of 10 files. It usually shuffled only the first 5-6, and the remainder were still in order. I haven't dug into the relevant mpv code, but just going off the docs, it's not clear when the playlist is guaranteed to finish populating, so I can speculate that there is a race condition if doing it this early.

So I changed it to use the on_load hook instead, which seems to be farther down the pipeline, but is defined as being before the first file is loaded, which is good because it still allows us time to call playlist-shuffle before MPV_EVENT_FILE_LOADED is triggered. Since the latter event calls fileLoaded(), which adds the first file in the playlist to the Recent Documents list, we don't want this to be triggered before we do the shuffle.

And it looks like this works. I confirmed in my testing that all items in my test playlist were shuffled and then the first item in the shuffled playlist was loaded, and that IINA's playback history and Recent Documents lists both correctly indicate that only that one file was added to each.

I see the PR has already been updated. Some of my comments have already been addressed. One minor one remains. Not critical to fix

Made another commit with some cleanup.

@low-batt just saw your review comments - sorry!

I did some further testing, and this time dug in to the relevant mpv code. I can now be confident saying that the on_load hook is the best place to put this logic. I tried adding it to the fileStarted event callback, but discovered that doing the shuffle there will not prevent the initial file from being loaded prior to the shuffle - it looks like there is no logic to do that in the mpv code for the "file started". And putting it in "file loaded" wastes CPU and may result in watch_later getting deleted for the initial file. So I think this is a good solution. Let me know if you find anything else.

Agreed, we have to use the hook and on_load seems to be the right one to use.

One last change I missed in my review. The declaration of the shufflePending property in PlayerCore needs to be:

@Atomic var shufflePending = false

The flag is set by the main thread and then read by the thread processing mpv events. Use of flags by multiple threads needs to be coordinated.

Sorry I missed this.

The flag is set by the main thread and then read by the thread processing mpv events. Use of flags by multiple threads needs to be coordinated.

A lock isn't necessary here due to the context in which it's used. Remember that shufflePending will only be set to true at most once per player, and that will happen at startup, because it was activated from the command line. Of the two possible problem points:

1. When shufflePending is set to true, in getNewPlayerCore inside AppDelegate, that is guaranteed to occur before the player starts loading the file because loading won't occur until openURL() (or equivalent) is called, which happens later on the same thread that sets the flag.
2. When shufflePending is again set to false, it is done right before calling any mpv commands which will cause a new load sequence to occur, so there will be no race there. And having examined the internals of the relevant mpv code it's clear that the hook is called only once per load sequence, and won't be called by a different thread or anything weird.

This is the race condition:

1. shufflePending is set to true in AppDelegate by the main thread
2. shufflePending is read by fileWillLoad in PlayerCore by the com.colliderli.iina.controller thread
3. The value read by the com.colliderli.iina.controller thread is false and the playlist is not shuffled
4. The CPU core running the main thread flushes its cache to main memory making the assignment of shufflePending to true visible to other cores

From Wikipedia Multithreaded programming and memory visibility:

Synchronization primitives such as mutexes and semaphores are provided to synchronize access to resources from parallel threads of execution. These primitives are usually implemented with the memory barriers required to provide the expected memory visibility semantics. In such environments explicit use of memory barriers is not generally necessary.

The lock provides the memory fence that forces the write by the main thread out of the CPU core's cache and into main memory. Due to the simplistic use in this case a memory barrior could be used instead of a lock. Pretty sure IINA does not have any examples of that.

In this case it is likely the write will always have been flushed to main memory. But it is good practice to lock everything that is shared between threads rather than trying to identify cases where it is safe to run naked.

How is the playlist being loaded? Is IINA doing that in a background thread? Is that properly coordinated? Caught me about to look at that.

In PlayerCore.openURLs I see this:

    // open the first file
    open(playableFiles[0])
    // add the remaining to playlist
    playableFiles[1..<count].forEach { url in
      addToPlaylist(url.isFileURL ? url.path : url.absoluteString)
    }

Seems like it is IINA that is causing the first file to start playing. Maybe the hook can be avoided by changing this code to check shufflePending and fill the playlist, shuffle and then start the first entry playing. If that can be done it would greatly simplify the changes.

The "race condition" you described sounds more like the possibility of reading stale data due to thread-local caching. I know in Java this is the reason for the existence of the volatile keyword, and with C/C++ the compiler settings determine whether it's a problem. I suppose this is possible with Swift too, but I'm unable to find any actual documentation suggesting this (although I did find people making unfounded speculations on message boards). And would the use of a lock actually solve this problem? It's a pretty similar setup to the use of isClosing, isShuttingDown, etc, which are also just straight booleans.

On the other hand, the playlist loading is a problem which looks like a real race condition. I had forgotten that it was being assembled in IINA now and not just submitting to mpv...

Seems like it is IINA that is causing the first file to start playing. Maybe the hook can be avoided by changing this code to check shufflePending and fill the playlist, shuffle and then start the first entry playing. If that can be done it would greatly simplify the changes.

Unfortunately no, because the call to open() must be done first. A bunch of init code is tangled up in it, including the mpv init and the various rendering inits, and they get very fussy (crashy) if there is a deviation from their current order. I haven't had much luck untangling them. I suspect the best we can do will be similar to what this PR is doing now, which is to start loading the initial file and then cancel it before anyone notices. I'll try to play around a bit and see if there's a way to block the loading sequence at some point to guarantee that all the playlist items have been loaded.

I was worried that changing this code might prove to be "interesting"…

It was the description of the problem that caused the switch to on_load that got me to look into this some more after sleeping on it. Sorry I didn't pick up on this right away.

On the "atomic" issue (which may not be applicable to this PR anymore)…

These days there are multiple issues with respects to sharing data between threads. The compiler may reorder operations. The hardware also may reorder operations. And there is the issue of on CPU core caches and when values are written to and read from main memory. Using a lock addresses these issues.

On the existing flags. There are lots of threading problems. If you enable TSAN you will see problems detected. As the planned fix is disruptive I am holding off until the 1.4.0 Beta. But since this is new code I didn't want us to introduce any new problems.

I didn't read the whole thread but Swift uses C++'s memory model if that helps

Actually doing a quick test using Thread.sleep() as a proxy for processing time, I find that the hook is not called until all playlist items are added. I found it was blocking, although I couldn't immediately find where. I did narrow it down to right after the media-title property change event. Best guess is that it needed the main thread for something to continue the load process, or...

  // This is called on the main thread
  func openURLs(_ urls: [URL], shouldAutoLoad autoLoad: Bool = true) -> Int? {
    // ...

    // open the first file
    open(playableFiles[0])
    // add the remaining to playlist
    var index: Int = 1
    playableFiles[1..<count].forEach { url in
      Logger.log("Adding URL \(index) of \(count)")
      addToPlaylist(url.isFileURL ? url.path : url.absoluteString)
      index += 1
      Thread.sleep(forTimeInterval: 1.0)
    }
    Logger.log("Done adding URLs to playlist")
    // ...

Then I realized that the hook itself has a blocking mechanism built-in! It will wait until the hook sends mpv_hook_continue to continue the load sequence.

So I made a simple change, so that the hook puts sends work to the main queue. This way, shufflePending will always be read & written to on the same thread, so TSan won't complain. And it will be a real guarantee that the playlist will finish loading before shuffle.

    addHook(MPVHook.onLoad, hook: MPVHookValue(withBlock: { [self] next in
      DispatchQueue.main.async { [self] in
        Logger.log("Processing mpv 'on_load' hook", level: .verbose, subsystem: player.subsystem)
        player.fileWillLoad()
        next() // <-- calls mpv_hook_continue()
      }
    }))

Yes, a hook is like a synchronous event that blocks the player.

I played with not opening first. I didn't encounter any crashes. But the playlist was not populated. Not sure what was up with that. So I didn't find an alternative to using the hook.

The primary goal of the next release is to fix regressions. So we are trying to merge changes that are safe and won't introduce any new problems. This fix is making me nervous. The latest change brings a 3rd thread into the picture. If the main thread attempts to access mpv before running the hook will that deadlock due to mpv waiting for the hook to run? I played around and was not able to trigger a deadlock.

If we wanted to be more conservative we could move the check of shufflePending into MPVController right before the call to DispatchQueue.main.async. That would be back to requiring @Atomic. This would isolate any deadlock to use of the --shuffle option. Not as clean as what we have now. I'm not sure this change is worth doing, but I thought I'd bounce it off you.

As we now know it was the thread race condition that caused the problem should we go back to using the on_before_start_file hook?

As we now know it was the thread race condition that caused the problem should we go back to using the on_before_start_file hook?

You're right...that is technically a better place to do it. I've updated the code accordingly.

The latest change brings a 3rd thread into the picture.

Not sure what you mean. Are you talking about: the main thread, the MPVController queue, and libmpv's runtime thread? Those were in active use already. This doesn't change things in that regard.

If the main thread attempts to access mpv before running the hook will that deadlock due to mpv waiting for the hook to run?

It's already accessing mpv and not deadlocking. When the main thread calls addToPlaylist inside openURLs(), it is interacting with the libmpv API and getting responses. It would seem that it's not using the same thread for those as it is for the file start / load. Although to be fair, I've never seen mpv_command block at all. I've seen it fail immediately if it's not ready, but that is another situation.

Also recall that to deadlock, you need two threads depending on each other at the same time; you're safe if you always block in the same direction. We have a few places during startup, such as PlayerCore.aidChanged(), where mpv.queue blocks on main by calling DispatchQueue.main.sync(), but I don't see any cases where main is waiting on some result from mpv.queue. But doing so would be terrible design and bad UX anyway - you don't want to block the main thread.

If we wanted to be more conservative we could move the check of shufflePending into MPVController right before the call to DispatchQueue.main.async. That would be back to requiring @atomic. This would isolate any deadlock to use of the --shuffle option. Not as clean as what we have now. I'm not sure this change is worth doing, but I thought I'd bounce it off you.

I personally like the cleaner, simpler design of the way it is now, and feels like the the more "Apple" way. If it causes any increased delay in the loading process due to the main queue being too burdened/blocked with other tasks, the problem needing to be fixed would be to get some of those tasks to stop abusing the main queue.

I should have explained more. We know mpv is suspending processing while a hook runs. The worry is that some mpv operations might pend waiting for the hook to finish.

Previously the hook ran on the com.colliderli.iina.controller queue's thread. It executed two mpv commands, so we know those commands are safe and will not pend. That makes us feel confident that other mpv operations will not pend, but we really don't know if that is true.

With the change to queue the work to the main thread there is a tiny window where other operations could run on the main thread before the hook task. What could that be? Would something run an mpv operation that blocks? If that happens it would cause a deadlock.

Do I think that is a high possibility? No. But the point is that we moved from a very deterministic code path where we could say exactly what would happen to one that opened the possibility that other operations might run in the middle of this critical operation.

To test this I added a 5 second delay before the queuing to the main thread and then tried clicking on menu items. IINA did not deadlock. It terminated with a fatal error due to mpv returning an error code. The code returned was -12. That is described as:

General error when running a command with mpv_command and similar.

Thanks to the use of a generic error we don't know what this is about. My guess is that this is an existing problem that is normally not encountered due to the small window of vulnerability during the initial loading of a video. But that is just a guess. I'm not sure how I can test that on a version of IINA without these changes.

Anyway, moving the flag before queuing to the main thread would mean the risk of such a problem is restricted to use of the --shuffle flag.

Do I think we need to do that? I'm thinking no. I'm pretty confident we won't have a problem. But then I didn't expect IINA would fail with a fatal error during the test I ran.

To test this I added a 5 second delay before the queuing to the main thread and then tried clicking on menu items. IINA did not deadlock. It terminated with a fatal error due to mpv returning an error code. The code returned was -12.

Can you share your test code? I'm very curious about how this happened. I have on rare occasion seen a -12 error from mpv when I was resuming playback. Although likely that your scenario is unrelated, I'm like to get more insight into how that could happen.

I do understand the spirit of your objection though, and though I'm not yet sold about the need for the lock around the boolean, I went ahead and added it. I also went a little bit further and refactored the code so that it doesn't add a hook at all unless it needs to, which should keep things quite pristine for those who never use CLI shuffle.

I didn't save my test code because it is trivial to create it. I merely added this line:

Thread.sleep(forTimeInterval: 5.0)

Before the call in MPVController to DispatchQueue.main.async that queues the call to perform the shuffle.

I also changed the default for shufflePending to be true so I did not need to use the option.

The delay means I have 5 seconds to invoke some other operation while the hook is running. I was trying various stuff under the Playback menu, like making it seek forward and that triggered the command failure.

Changes look good.

On booleans that are shared between threads…

If this was C++ we'd be using volatile. But Swift wanted the avoid the bad problems that can occur if low level concurrency operations are not coded properly, so there isn't a direct equivalent to that keyword. Originally Swift's answer was to use dispatch queues. That works, but adds a lot of overhead. Eventually Swift added high level concurrency features.

The correct way to code this today is to put properties that are shared between threads into an Actor. However Swift concurrency features use runtimes added in macOS 10.15. So IINA can not use these language features.

Another choice would be to use swift-atomics. I asked the team about this, but they did not want to establish a dependency on this package.

That is why at this time we are using our own lock code.