-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OSD Progress bar very late to update #4779
Comments
No, that is addressing the case where the user only does a single action to display the OSD, and the OSD is configured to stay up for more than a second, but despite the video continuing to play (playback time is increasing), the time displayed in the OSD would not change. If this issue is referring to the tendency of the OSD to stop updating when the keyboard repeat is used, I filed a separate issue for that a couple of years ago - although I can't seem to find it now. I may have closed it. I did notice that the severity of that problem can change dramatically with each OS release + even minor code changes. I thought at one point that it had gone away for good, but I did recently notice that it was back in Sonoma. |
I had been testing under Ventura. I ran more tests today under Sonoma. I still can't reproduce this one. However if I change the → key binding to step by frame the OSD is showing pause/resume, which is supposed to be a hidden internal operation in that case, so that needs to be fixed. |
Forgot to follow up with this. I investigated this problem much more deeply. It only happens when the key binding being repeated is bound a menu item. Bindings which are not attached to menu items do not have this problem. I found that the problem is not actually caused by a race condition or the mpv queue. I don't have 100% visibility but I'm very certain it's a consequence of Cocoa's event loop handling. I think something is happening like this on the main thread (pseudocode):
In other words, the whole main thread seems to be getting blocked if menu item key equivalents come in. I believe this is done to give higher priority to menu items over any other events, and this an effect which is at the discretion of Apple's developers. As I see it, there are two different ways to fix this. One is to override |
For what it's woth the problem at some point resolved itself. I played around using the mpv usd and when I changed back it was fixed. |
@asgeirtj All evidence is helpful. Thanks for reporting the latest developments. I can not explain why switching to the @svobs Some thoughts… Redrawing upon receiving a key event seems really wrong. That is before a seek has finished, so the times may not have changed. This would only work in the case of repeated seeks as it would update the OSD to reflect the previous key press and seek. Or am I confused? In any case an application should not need to do this. I've not been able to reproduce this on either of my machines. Feels like there is some contributing factor that has not been identified. |
Doing so would not change the timing of any existing redraws. It would only be adding more redraws. Currently, the redraw process can get starved because each redraw is enqueued in a new DispatchQueue task separate from the key event processing task, and the key event processing must return in a timely manner for the redraws to be displayed.
It is still occurring for me on MacOS 14.3.1. Make sure your keyboard repeat rate is set to the highest value, and you are using a key binding which is associated with a menu item. For example, with |
Felt like I could add more to the explanation. Say the
Steps 3 and 6 are symbolic, trying to highlight the dependency on the each queue due to being asynchronous. The problem occurs because Step 6 doesn't complete until the key events slow down and the previous work items are allowed to finish. At this point all of the previously enqueued "seek" OSD messages are executed in quick succession, which isn't as wasteful as it sounds because AppKit seems to have optimizations which cause excessive redraws to get dropped, but it isn't ideal. On my fork I used a solution which uses a new, simple OSD queue (not a |
This was the contributing factor I was missing:
In macOS I confirmed your finding that this does not happen if the key is not a keyboard shortcut for a menu item. And yes, if in IINA's settings on the I'm going to ask @saagarjha for guidance on this one. @asgeirtj What setting are you using for |
key repeat rate is in fastest slot. delay until repeat is shortest slot fwiw. I also disabled the hold aplhanumerical key for diactirics thing. |
Need to look into this more but the gist of it is that the speculation is incorrect, |
It's not that it's not getting called. It's being blocked. Seems to be due to previous You can unblock the |
I instrumented IINA to show the queue behavior reported by @svobs. This first set of log messages shows holding the → key which goes through the menu. It shows the seek command being submitted but receiving the event and synchronizing the UI stalls until I lift the key. Going Through Menu:
This does not happen when I assign a key binding to seek that does not go through the menu. Without Triggering Menu:
On That has been changed in PR #4819 to fix data races. If somebody can think of a reason why Running on the PR and holding the → key now shows the Going Through Menu with Async Changes:
This is some of the code I used to generate those messages. MethodTracer::class MethodTracer {
struct record {
var time: Date = Date()
var method: String
}
private static let baseTime: Date = Date()
@Atomic private static var records: [record] = []
static func recordCall(_ method: String = #function) {
$records.withLock { $0.append(record(method: method)) }
}
static func log() {
$records.withLock {
var lines: String = ""
for entry in $0 {
if !lines.isEmpty {
lines += "\n"
}
lines.append("\(Int(entry.time.timeIntervalSinceReferenceDate * 1000000)) \(entry.method)")
}
Logger.log("Method call trace:\n" + lines)
}
}
} |
Starting with the Xcode application template I added a menu item with → as a shortcut key along with the following custom class Window: NSWindow {
private var counter = 0
private func test(_ invoker: String) {
counter += 1
let count = counter
print("\(count) \(invoker)")
DispatchQueue.main.async {
print("\(count) Task Run")
}
}
override func keyDown(with event: NSEvent) { test("Key") }
@IBAction func menuItem(_ sender: AnyObject) { test("Menu") }
} This test program reproduces the behavior seen. When the macOS Xcode Console::
If I reduce the The full test program: Starving.tar.gz |
I took the test program to a MacBookAir8,2 running macOS Catalina 10.15.7 and the problem reproduced on that Mac. This confirms this task scheduling behavior is not a recent change. |
Ok so as expected |
Really not good to be sleeping while tasks are waiting. If possible an improvement would be to delay the event by running a task from the dispatch queue, only sleeping if there are no tasks to run. Or allow the application to opt out of this event rate throttling. Did you want to send Apple feedback, or do you want me to do it? I tried this code as an experiment: NSEvent.addLocalMonitorForEvents(matching: .keyDown) { [self] event in
guard event.keyCode == 124, event.isARepeat else {
lastInvocation = event.timestamp
return event
}
let timeSinceLast = event.timestamp - lastInvocation
guard timeSinceLast < minTimeBetween else {
lastInvocation = event.timestamp
return event
}
print(String(format: "Time since last key event (%.1f ms) is too short, dropping event", timeSinceLast * 1000))
return nil
} I found I had to drop an event if the time between events was less than 70 ms. Dropping events allowed the main queue tasks to run. What should IINA do about this?
I'm thinking maybe nothing as users can add key bindings that are not tied to the menu. If you are really interested in high speed scrubbing, a key that is not a menu shortcut will perform better. Thoughts? |
Would definitely be great if Apple would improve this. Supposedly in Sonoma the menu system was "completely rewritten to take advantage of Cocoa" - what was it before? Some ancient NeXTStep-Apple Frankenstein stuff from 30 years ago? Its functionality has barely changed since Mac OS 10.0. Personally I've barely noticed much difference in menu functionality at all (erm, other than those Just wanted to point out the other "option" to work around this. You can subclass Seems like such a solution has been the standard way to build games and other "nonstandard" Mac apps in the past. But once you start messing with event handling priorities you open the door to plenty of possible side effects and corner cases. It's useful to ask just how useful it would be to repeat key events more than... every 25ms? Which ≈ 40 events per second. Seems like that should be pretty good for most everything? Unless someone comes up with a Lua script which can play Doom 😉 |
Filed as FB13685409 |
Hopefully Apple will reply to FB13685409. I'm still waiting for Apple to reply to two I entered back in January 2023. I'm hoping we will quickly hear from Apple as to what the offending code is trying to accomplish. Looking like Apple did not fully fix the So IINAApplication is merely an example, that branch does not contain a fix for this issue, yes? |
I saw that. Not sure it's exactly the same bug, but definitely seems like there are a few loose bolts still. It's too bad Apple didn't take the opportunity to start replacing AppKit with something more modern. They can't keep deferring it much longer. It's taken me the last 2 years to figure out how to build basic UI features for Cocoa. The same kinds of things I've been able to learn in weeks using modern webapp libraries. And it's still challenging to work with because it's fundamentally really old tech which existed before anyone knew how to organize UI, and thus has poor separation of concerns and requires perfect developer discipline. I imagine Apple would be having trouble retaining people on the Mac teams. Most developers wouldn't want to keep working with this stuff.
It does not, unfortunately. If I remember right, once I started going down that path there were many corner cases which popped up. For example, you'd need to detect when open file dialogs and editors in text fields have focus. Because the built-in responder chain does provides the logic to figure out which controls should receive key events, so any code which preempts that mechanism has to assume that responsibility. Which is just a little bit too much of a headache... I still think the best approach is just to force OSD/OSC redraws from the menu item handler. It doesn't appear that Apple's sleeps are actually limiting video playback (empirically...not sure how to write a test to quantify this) since OpenGL does not use the main DQ. So having ~40 redraws per sec of these components via explicit draws should be fine. |
System and IINA version:
Expected behavior:
That the OSD progress bar is consistent with actual progress
Actual behavior:
It takes a few seconds to catch up but I use seek 60 seconds it doesn't catch up at all
mpv log:
Steps to reproduce:
seek many seconds at once
How often does this happen?
![CleanShot 2024-01-17 at 07 03 35@2x](https://private-user-images.githubusercontent.com/27446620/297288587-8ef808f6-c98f-4579-8327-2986925cd610.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MTg5MzAxNDAsIm5iZiI6MTcxODkyOTg0MCwicGF0aCI6Ii8yNzQ0NjYyMC8yOTcyODg1ODctOGVmODA4ZjYtYzk4Zi00NTc5LTgzMjctMjk4NjkyNWNkNjEwLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA2MjElMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNjIxVDAwMzA0MFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcxODczMjIzMzdjNjJhMTdiOTc3ZTc3MWFhMTQ0Y2MwOGZlOWU0NmMyNGM1NGI0YTE4ZmY2NTEwYmVlNjJiNGYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.TbDsy6OtoNGCRtijDd1Z2V8hBMzx_As58lTjzZnXUTI)
Every time I use IIna
The text was updated successfully, but these errors were encountered: