Crash due to PlayerCore.active being called outside the main thread #4251
Comments
|
Want me to take this one? It is on my list to refactor code in A lot of the work being done is small. I don't see a reason to be doing this work on the background thread other than a little bit of overhead to queue it to the main thread. Performing work on this background thread is causing data races accessing the information stored in Quite a few places in I've fixed some problems in |
@low-batt yes that would be great! The
This makes a lot of sense. It might be a nice clean line of separation if the mpv objects were parsed into IINA objects before being sent to some handler on the main thread. It might be great to move a lot of that logic out of One more thought on the subject of |
|
I've added this crash to my list. Will get to it soon. Not certain which playback hiccups you are referring to. If it is a IINA 1.3.1 regression it is likely to due to the regression the fix in PR #4055 addresses. The attempt in 1.3.1 to fix a different data race without using locking did not work. Not sure of all the ways that might manifest because with my Macs things worked, except for the issue with switching from music to video. The usual problem with fixes to threading. An incorrect fix may just happen to work in the developer's environment. I'm used to independent classes and dependency injection. There is an awful lot of tight coupling and exposed internals in the current code. However, I'm not in favor of trying to refactor code to address that at the moment, due to priorities and not wanting to trigger merge conflicts with the many outstanding PRs. I want to first get to a point where IINA passes the checks that can be run with developer tools. I tracked down and fixed a bunch of memory leaks already. I've not checked lately to see where we are at with regards to leaks. I am working on fixing data races identified by TSan. I'm very glad you are tracking down and fixing the UI constraint issues. |
I took a look at that but it is completely outside my expertise 🤧 Regarding playback hiccups, I don't keep a spreadsheet anymore but I took a look at the last few pages of seemingly related defects.... Issue #4153 sounds like a hardware driver is to blame(?). Issues #4154 and #4176 are likely due to confusion caused by When I see code that looks structurally unsafe, rather than painfully troubleshooting the existing code and all its corner cases (and in doing so, becoming more mentally invested in the existing code), I'd rather just refactor it right away, and eliminate the hazards via improved design, and then see if the bugs are still present. I admit this decision has never been met with enthusiasm when I presented it 😎 and for obvious reasons; there is definite risk in any new solution. Sorry - started to go a bit of a tangent there. There isn't a pressing need to refactor |
|
I'm actually working on issue #4153 right now. There are a number of things that effect seeking performance, but the regression is likely due to my change that tried to avoid wasting CPU while paused. Turns out the display link needs to be running when seeking or mpv is unhappy. I'm not sure what #4154 and #4176 are reporting, so I've not looked into those. Issue #4212 has a fix pending. A regression caused by trying to not waste CPU when the OSC is hidden. If the Mac has a Touch Bar then the OSD is always visible. I just responded to issue #4159. That is likely an upstream issue. Not liking the sounds of this:
I'm a bit worried that is due to the display link thread being shutdown. Keep an eye out for that one. We need to get these outstanding PRs merged and then do a lot of testing of the On the refactoring question. At work I too have been the one pushing for refactoring and getting frowny faces. There is good reason for that. Hidden in the messy code is usually odd code that fixes a problem. It is easy to loose such fixes during a refactoring. And if threading is not correct even a proper refactoring may just happen to change the timing and introduce a problem. At the moment I think IINA first needs to get out a release that addresses the regressions, before doing much refactoring. |
This commit will: - Change PlayerCore.playbackRestarted to use the main thread to call NowPlayingInfoManager.updateInfo - Remove locking from NowPlayingInfoManager.updateInfo since there is no longer a need to coordinate access from multiple threads - Add comments pointing out the requirement to use the main thread
|
The problem was very reproducible for me by dragging and dropping a video file to the welcome window when running under Xcode. |
This commit will: - Change PlayerCore.playbackRestarted to use the main thread to call NowPlayingInfoManager.updateInfo - Remove locking from NowPlayingInfoManager.updateInfo since there is no longer a need to coordinate access from multiple threads - Add comments pointing out the requirement to use the main thread
|
The fix for this issue has been merged into the IINA Should you wish to confirm the fix is working and do not have the ability to build IINA from the sources in the |
|
IINA 1.3.2 contains the fix for this issue. |
System and IINA version:
Expected behavior:
Continued operation
Actual behavior:
IINA crashes. See crash report.
Crash report:
Steps to reproduce:
I was testing thumbnail generation/display, but in this case I had launched the
pr-custom-thumbnail-widthbranch from Xcode, then dragged & dropped a 9:16 video I had generated onto the Welcome Window, and as soon as it opened I tried to hover over the play slider in an attempt to display a thumbnail. (Probably unrelated, but it was stuttering at the start of the video, and I didn't wait for it to calm down. I've noticed that it does that for me occasionally. Probably should track that down at some point).How often does this happen?
It's the only time it ever happened, but I think I see the problem because I've stumbled onto in before.
The static properties in PlayerCore (
PlayerCore.lastActive,PlayerCore.active, etc) are a bit dangerous because it's not obvious that they can access UI code and thus trigger this exception (especially since it doesn't seem to always get triggered). It's probably related to another nasty trap in the code which I've noticed: just by accessing theNSWindowobject forMainWindowController, you run the risk of actually opening the window. I haven't dug in to how exactly this happens, but it can result in a black window being opened named "Window". I'm just speculating that this is the UI code which tried to run, but I think it's clear that these staticPlayerCoremethods should be shored up so that they aren't accessed outside the main thread.The text was updated successfully, but these errors were encountered: