-
-
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
Ctrl-s (screenshot window) does not work #4822
Comments
From the entry for the screenshot command in the
The expectation is that this form of the For now the ⌃s key binding should be removed from the If the |
I wonder if Side comment / rant. IINA could replicate this by taking a snapshot of its own player window. I have working code which calls
But ok, I played with ScreenCaptureKit. I discovered there is no way to take a screenshot (or any of its APIs actually) without requiring the user to go to System Settings and allow IINA "to record this computer's screen and audio". This means that if Not just that, but they are presently allowing both APIs to live side by side, and only one of them needs permission to work. This is fake security. I wonder if this idea came from the same people who decided to require a special permission for access to |
Yes, this is why I think the current solution for now should be to remove the key binding to the The first place to start would be to explore what I found people discussing the security prompts. Apparently that was added for If you do want to address this now, that is fine with me. |
No, I agree that this is not worth bothering with right now. It's just an annoyance. If people really want to take a screenshot of the window, they can just use
Could be. If they wanted to modify its behavior to match ScreenCaptureKit's, they'd have to make it into an async API, because all of its APIs need to wait for the user to respond to the permissions prompt. This would be infeasible to retrofit for |
I generally think that taking a screenshot of the window is an unusual operation. Is this actually what is desired here? Or is this a feature request for the current video frame, which I am sure there is better API to grab an image for? |
I entered this issue to "kick the can down the road" as it involves work to first understand exactly what this This came up due to a regression with the I just played with The IINA I see we have a |
Window screenshot works fine with libmpv clients as well. I think the issue is that you don't have https://github.com/mpv-player/mpv/blob/master/video/out/gpu/video.c#L3437-L3458 But with libmpv clients setting Btw blame shows e0dc0bf. Don't know why was originally disabled, there's no harm in having it enabled. |
@krackers Thank you very much for pointing this out! At one point I investigated that commented out code that sets
I probably read this comment:
But that makes it sound like mpv will fall back to software, not fail. What else is IINA missing out on by not enabling Quite a long time ago I tried running with IINA will sometimes trigger that warning. I have seen it during shutdown. I ran a quick test with I'm thinking if IINA were to consider enabling this we should wait for the beta release as the next release is intended to fix regressions. |
That's only possible for non-window screenshot though, where it passes the mp_image through libswscale
Haven't looked too much at the DR codepaths but my understanding is that the two features that setting this flag enables for libmpv clients are direct rendering and gpu screenshots. Direct decoding (zero copy decoding) doesn't matter on mac because it's version of opengl does not support it anyway, best you can do is opengl-pbo. And for most people it also doesn't matter since they're using hwdec.
Deadlocks should not really be an issue even with ADVANCED_CONTROL enabled, if you're not doing direct rendering. The only addition case it adds is gpu path for screenshots, but that's exactly the same as just a normal render. So as long as you do it in the same dispatch queue as you normally render, then there won't be an issue.
Yes, it can happen during shutdown if you don't coordinate sequence of events carefully. What happens is that mpv sends shutdown message to VO, but VO can only process that shutdown message after it exists the flip loop. But the flip loop is itself blocked until the vsync is received from the display link. So you cannot shut down display link until the libmpv uninit is fully finished. (Actually this is still a bit racy, the better solution is to modify libmpv so that as soon as you shutdown it no longer waits for the usual render loop). |
Actually in your case maybe this is the issue
You are suspend the render dispatch queue before uninit. But this means that if VO thread is blocked on waiting for the update, it can only service the shutdown after the timeout. I think you should remove |
This commit will change the MPVController.mpvInitRendering method to enable advanced control when creating the mpv renderer context. Advanced control must be enabled for this variation of the screenshot command to work.
@krackers Just noticed your additional replies. As you can see I was off preparing a PR that enabled advanced control. I will create a separate issue for the shutdown sequence problem that is sometimes triggering the mpv_render_context_render not being called warning and prepare a fix for that as well. Thanks for pointing out the root cause of that warning! |
This commit will change the MPVController.mpvInitRendering method to enable advanced control when creating the mpv renderer context. Advanced control must be enabled for this variation of the screenshot command to work.
Looks like this is a duplicate of #4505. |
Good catch! I totally had forgotten about #4505. |
I asked the other developers why advanced control had been disabled. The answer is that it is the fix for issue #2149, a hang. Reading through that issue I'm worried there are still problems with how IINA is using the |
@low-batt Thanks for digging that up. I'm a bit confused though: The situation where using
This is what wm4 meant by
If you look at the implementation there are only 3 things that can cause a task to be scheduled on the internal libmpv dispatch queue: screenshots, perf stats, and direct-rendering (the last of which is actually unsupported on osx). So the set of things that can cause a deadlock is fairly bounded (because conceptually the only difference in internal state between advanced_rendering enabled/disabled is whether something is scheduled and someone is synchronously waiting on the libmpv dispatch queue). But this is actually not the issue that IINA faced in the bug you linked. If you look there, you don't even see any Instead, the situation is actually more subtle: we can see in the process dump (thank you to author for including that)
The core goes to decode a frame (in This is where the issue lies: because IINA is not calling Basically if VO is active, IINA needs to process the dispatch queue, which for some reason it doesn't seem to be doing in this edge-case (I have not looked into IINA code, I remember there was some calls to suspend() or something which needs to be investigated. The fact that it happens when one file ends and another file begins is suspicious). Edit:
Maybe this is part of it, does IINA have some logic to skip calls if the window is offscreen? Or perhaps it's osx being smart enough to skip the layer draw entirely if it's off screen? Skimmed IINA codebase and indeed you are sticking the call to |
Yes - my past testing has indicated that this is what happens. Eventually I discovered this is an unpublicized feature called App Nap and you can check its state by showing the App Nap column in Activity Monitor.
Thank you for this! The need for @krackers I think we've talked about the full screen cases; quick rehash: they can be removed but there are other bugs in the full screen transition code which must be fixed too to actually get a clean animation. And unless I'm missing something, the other two cases can only happen while quitting the app. There's no direct call to I noticed the affected issue is over 4 years old, so I dug up the code from the time of the report. I found that at the time, The calls to As for the calls to |
@krackers @svobs This discussion is very helpful. Issue #3997 shows a
The PR #4819 changes The next release of IINA will be focused on fixing simple issues. After that I believe we will be starting on the long planned feature release. That will include beta releases, which is a good time to make more risky changes such has fixing thread issues. That is why I had been delaying posting the changes in PR #4819 until recently when I started to prepare PRs for the feature release. There are some other unexplained hangs. I will look for them and continue to think about this issue. |
Well app nap itself is fairly well known, but I'm surprised the app is scheduled for naptime when audio was playing just a few milliseconds ago. But yeah that could definitely cause it, I also wouldn't be surprised if osx had some other optimizations when windows are off screen (for instance I've observed that when a window is occluded with mpv paused, cvdisplaylink callbacks become less frequent. But if video is playing when occluded this doesn't happen, so something takes into account if there was recent video activity when deciding to deprioritize the app).
I'm of the opinion that all uses of suspending the render GCD queue should be investigated and removed, because it's too risky to use correctly with libmpv. For shutdown the issues with current iina implementation are already documented on another issue. I'm not too familiar with the full-screen issue though (I vaguely remember something about it in one of the PRs which ended up taking a tangent into a discussion the nuances of caopengllayer on mac...), could you remind me again why IINA has to suspend it during the FS animation? What happens if you just keep rendering to the layer while the fs animation is in progress? I'd think (or hope) that apple would have made it do something sensible.
It's "safe" in the sense that it won't cause a deadlock assuming you can guarantee that it's unsuspend in a bounded time, but it can block the entire core for 0.5 seconds which is probably bad for other reasons (it can e.g. lead to audio artifacts if the audio buffer runs dry during that time). I'm not too familiar with IINA internals, so sorry for noob question but in your linked issue #3997 why are both |
I tried quick test by adding this code to ProcessInfo.processInfo.beginActivity(options: [.userInitiated], reason: "IINA is playing video") And Xcode did not show App Nap being prevented. I also tried userInteractive, which was added for macOS 13. Again, no effect on App Nap according to Xcode. I stopped playback and minimized IINA to the dock. After a while Activity Monitor showed I even tried Possibly a lot of the documentation is out of date and Apple decided macOS "knows better" and apps are no longer able to disable App Nap? |
The Energy report makes no sense to me. The row label is Looking at the pictures here. It seems the IINA does have a |
Ahh, good observations. Yeah, it looks like it was thrown together and then forgotten.
This is a lot more helpful. The green square disappearing is a red flag (pardon the clashing color references). And when I compare their graphs to what I actually see in Xcode when the app is in the App Nap state: To me it looks like the Moreover, this suggests that there is a solution in ![]() This page looks useful. It looks like a "power assertion" is a synonym for the "activity" started from I think it's reasonable to assume Xcode is broken. But there are other tools. The |
The Energy report in that screenshot sure looks defective to me. What Xcode version? I'm using Xcode 15.2. I'm thinking some of this information in Apple's documentation archive is out of date. IINA is using Power assertions are separate from There is also an |
I'm using 15.3.
In my experience, if a doc falls out of date Apple usually just deletes it. Notice they start with the word "Generally". It weakens the usefulness of the bullet points considerably. Probably they are changing the heuristics slightly with each release. You mentioned you saw App Nap engage with a video playing in the background. I wonder if it was muted or had very little audio output?
Ahh, thank you. And I see no mention of App Nap in My best bet remains to do testing of |
Is removing titled strictly necessary to achieve the "pseudo full screen" effect? Setting fullSizeContentView + titlebarAppearsTransparent + then manually hiding/showing the titlebar as needed (via setting opacity?) seems like it might be able to achieve the same. Anyway, are we sure that it's actually AppNap causing issues here? The suspending of GL draws may well be something CAOpenGLLayer specific and not necessarily app nap related. |
You're forgetting the rounded corners. There seems to be no way to make them square again except by removing the
That is a fair question... Ordinarily, when video is playing, the call sequence looks roughly like this: mpvUpdateCallback():
layer.mpvGLQueue.async():
draw(forced: false):
display():
canDraw(inCGLContext:, ...):
mpv_render_context_update()
draw(inCGLContext:, ...)
mpv_render_context_render() I believe App Nap is stopping work somewhere inside |
Hm right... I have not checked modern osx, but on ye olden days of lion (which iirc introduced the rounded bottom corners) it used to be possible to swizzle something in
Mpv's internal queues are really just wrappers around pthread signals, so unless the scheduler decides not to schedule things at all, those should still work. And I'm hoping that a GCD with userInitiated or higher priority still gets serviced otherwise we're pretty much hosed. If it's just a matter of |
OK, after further testing, I have to backtrack a little bit, because I may have been confusing two separate behaviors... Note that I still have been unable to get App Nap to activate while a video is playing under any circumstances. I tried different videos and settings. But I discovered some other things. You can take a look or try running my test code if you want - it's set up for the second test, with the first test disabled. 1. App Nap testSince it's documented that timers are not affected by App Nap, I created a timer which fired every 1 sec and ran the following code: @objc func doTest() {
videoView.videoLayer.mpvGLQueue.async { [self] in
videoView.videoLayer.draw(forced: false)
}
} Then I launched, opened a video, paused it, and used 2. CAOpenGLLayer's draw callbacks testI disabled the code for the first test, and then just added a bunch of logging to all the draw functions. Same as the first test, I launched, opened a video, paused it, and used But now I noticed that as soon as Thoughts
On that second point, @krackers you sound you are talking about render timing again, which ups my anxiety. My naive approach would be to cut the bodies of
Not sure what you mean by this? When could this happen? And I've never understood the what the skip flag does, really... |
@krackers also, am I correct in thinking it is necessary for the sake of mpv to call both |
I can answer the second one at least (as for the first, I doubt anyone but apple knows what exactly app nap does. There might be some hints in the kernel code if it does go so far as to reduce scheduling priority or coalesce timers even more aggressively, but I really doubt app nap would activate if audio was playing. Otherwise pretty much every music playing app would run into it.)
We can get What you need to do is have a fallback path for when
I probaby worded it badly, I meant exactly what you observed where
Update can be called alone. You only need to call into render if the return value |
Ah, that makes a lot of sense and helped some pieces click into place which had long been loose in my mental model.
And this! I think I finally understand the code in So it looks like IINA's code is already doing the correct thing here by doing the skip render. Though it is bypassing Also, I now see another reason |
Yes, if render_context_update did not request a render, then you don't need to call render at all.
Possibly, it needs a thorough examination of whether it's safe to treat |
There were many threading issues in IINA especially during application termination. I have been fixing them, but many remain. One of the fixes I attempted introduced a regression, so I have been holding off until the planed feature release which includes beta testing. PR #4819 I mentioned above is one of the fixes that is waiting for the beta release. Issue #3827 reports data races with the @Atomic var isUninited = false This is documented in the func draw(forced: Bool = false) {
videoView.$isUninited.withLock() { isUninited in
// The properties forceRender and needsMPVRender are always accessed while holding isUninited's
// lock. This avoids the need for separate locks to avoid data races with these flags. No need
// to check isUninited at this point.
needsMPVRender = true
if forced { forceRender = true }
} The locking provides a memory barrier enforcing memory ordering and CPU core cache-consistency. At least that is the idea. Keep an eye out for anything I got wrong. @krackers Since we are talking about rendering, you were working on fixes for the frame dropping reported in discussion #4652 and shown by the YouTube 60fps Tester. Where did that end up? I'm thinking we want to get the current code working correctly first before adding functionality, but since we are taking a close look at rendering there are two other related future functionalities to be aware of… Would be nice to support dynamic display timing for ProMotion displays as discussed in the WWDC21 video Optimize for variable refresh rate displays. The other feature is about changing the screen refresh rate to match the video when in full screen mode, issue #3414. There is a PR for that issue, but it needs some work. I mention this feature because it significantly disrupts the transition in to and out of full screen mode. |
What about the case where async=true is set on the layer? As we found out in that PR discussion, then the system directly calls CAOpenGLLayer's canDraw + draw atTime method. But then there's no
In such a case we have a "lost draw".
I heavily modified libmpv internals to support presentation feedback and coordinate the double-buffering properly, so I don't know how useful it is for iina which tries to stick close to upstream libmpv. There are also a lot of messy assumptions regarding timing of the CVDisplayLink callbacks: after a bunch of measuring across different os versions I'm convinced that there's some method to the system's madness of having CVDisplaylink callbacks occur halfway into the vsync interval. It must somehow be related to compositor timings. The reason is that traditional opengl double-buffering (back when it worked at least) unblocks at the start of the vsync interval. But on older osx versions this situation is reversed, opengl buffers unblock halfway and displaylink callbacks fire at the start of the interval. My working hypothesis is that displaylink callbacks are tied to the timing of WindowServer compositing process, and the halfway offset is to try and minimize the compositing latency after an opengl swapBuffers. Every time I investigate it I go slightly more insane though, so for now I've just settled on accepting that these are magic numbers which empirically give good results. I find it surprising all of this is undiscussed on the internet, without this knowledge it is impossible to get frame-perfect presentation with render loop controlled by the app. Also fwiw I realized that it actually is possible to detect the case where gpu processing-time takes longer than the next vsync by using an opengl fence to ensure that processing has finished upon receiving a vsync signal before continuing. I haven't added that yet though because I'm lazy.
Is this basically the same as VRR/gsync/freesync? Do you have to do anything special to enable it? I thought the whole point of VRR was that vsync happens on-demand whenever you
Oh that's cool, I always wondered about writing something like the xrandr-mpv plugin that use the CoreGraphics apis, but never bothered to do it. Is the disruption still present with M1+ macs, those have seamless and instant mode switches. |
Which PR was that? Yes, if that happens then the current code is no good.
I believe so. The WWDC video I linked has a good explanation of it.
Not seamless and instant on my MacBook Pro M1 machine running macOS Ventura. There are two settings that affect the behavior with respects to matching the display refresh rate when transitioning to full screen mode:
Likely most users have not enabled
Enabling those settings in order to quickly transition to full screen mode without the needless animations the transition into full screen mode gives you nearly an instant switching in to and out of full screen mode. But when you add in changing the display refresh rate the screen goes to black when transitioning. It is definitely not seamless and instant, although not too bad. And the audio is not disrupted. |
I didn't see #3414 before, and now have only managed to get through the first kiloscroll of comments. It sounds like the transition is very fast for M series Macbooks' built-in displays, but could be much slower for older displays? And that discussion is a couple years old, and I'll witnessed huge improvements in MacOS's handling of this over that time (to Apple's credit - I wonder why I haven't seen much written about it) so that situation may have changed. It's unclear whether some monitors might be limited by hardware or lousy drivers. I used to own a couple of chonky CRTs which never took less than a full second to change refresh rate, even using the built-in OSD. It's probably asking too much for the Cocoa compositor to change the display settings in the middle of an animation. Like I've mentioned above, it had a hard time hiding the menu bar while resizing a window. Why not just change it as soon as the animation is done (or before, when leaving full screen)? The IINA animation code needs to make a lot more use of the Odd that audio would stop. I wonder if it's hung up when the rendering falls too far behind? I wonder if doing skip renders during the transition might be more graceful... |
Issue #4667 links to two different PRs, neither of is a satisfactory solution. In my fork, I ultimately created a method, Or maybe there's a race condition which is resulting in fewer draws? Looks like I need to come up with a fix for the possible |
Possibly macOS has improved the speed at which the display refresh rate can be changed, but the audio disruption I reported above was from a test I performed yesterday. Today I did some more testing and:
BUT… Do we need to dig into this now? Issue #3414 is on the schedule for the upcoming feature release, however it is not the priority for that release. There is a lot ahead of it. I had planned to discuss this with the senior developers. I only brought it up in this issue as it will affect the transition to and from full screen mode. The main priority for the feature release is to officially release the plugin system. I'm thinking the second priority should be on subtitle support. For a long time users have been asking why secondary subtitles do not support the controls available for the primary subtitles. There are multiple issues concerning this. The I haven't had time to look into issue #4667. I will try and find the time to do so. |
Not really. #3414 is an enhancement, agreed. There are still too many bugs to fix. But I am a bit surprised that #4667 doesn't seem to be a priority for the core devs... And @low-batt, I remember you've done a lot of work to add full support for MacOS's It does seem like there is a lot of work to do in the subtitles area. But TBH they aren't a feature I use heavily. And I suspect once I dive into that area I might get annoyed at the limitations in mpv handles them and be tempted to dive into a rabbit hole of extra work. Are there really a lot of people who use secondary subtitles? I still see a lot of fixes & improvements I'd like to make for rendering & geometry (though won't rule out unrelated stuff completely). |
News to me that #4667 is not a priority for the core devs. It is my expectation that they will want to fix problems with animated transition effects. I've not been following this issue. I see I nearly always have I'm not sure how many users use the ability to show secondary subtitles, but there are a number of issues asking for the ability to control their rendering like is available for the primary subtitles. All of these have been pended on This does not preclude other enhancements and fixes. As the feature release will use betas it is definitely a good time address problems that require a risky fix. And we really need to reduce the number of outstanding pull requests. There are a good number of issues regarding IINA not handling the geometry of the window and the video like I am worried that we already have too much work to do for one feature release. |
Can't argue with that. I personally like to have smooth animations when I'm doing "fun" things like watching media or playing games, but loathe them when I'm trying to be productive. For that reason I leave Getting IINA animations to work has turned out to be extremely hard and a massive time sink, but I'm not giving up on it yet so it will remain my focus for some time...
Already fixed in IINA Advance. Will see about back-porting the fix at some point. I've also been working on shoring up custom crop, which has tons of edge cases which need to be addressed. I'd really like to integrate autocrop into the GUI, and/or figure out a way to allow autocrop to play nicely with IINA's existing crop. Lots of stuff! |
There used to be many undocumented macOS properties that let you adjust the speed of various animations. Another one would stop working with each new macOS release. The really useful ones no longer work. 😞 There have been many requests for handling geometry like Is the crop work going to make use of the new |
Thanks for bringing that to my attention. Sounds like it should be trivial to migrate to. Will add it to the list. I must confess I've been hesitant to try testing newer versions of mpv yet. Many of the things I've been doing have been de facto replicating mpv logic in IINA Swift code, because I need to apply crop, aspect, and scale calculations just to determine the dimensions for But yes, I've been trying to smooth out the existing crop features. For example, the Video Settings sidebar has crop buttons which look like aspect ratios...but when one is clicked it actually creates an |
This commit will change the MPVController.mpvInitRendering method to enable advanced control when creating the mpv renderer context. Advanced control must be enabled for this variation of the screenshot command to work.
This commit will change the MPVController.mpvInitRendering method to enable advanced control when creating the mpv renderer context. Advanced control must be enabled for this variation of the screenshot command to work.
System and IINA version:
Expected behavior:
Using the
mpv Default
key bindings and pressing the ⌃s key takes a screenshot of IINA's window.Actual behavior:
Nothing happens when the ⌃s key is pressed. No screenshot is generated.
Checking the
mpv
log shows the screenshot window command is failing:Steps to reproduce:
mpv Default
How often does this happen?
Every time.
The text was updated successfully, but these errors were encountered: