Skip to content
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

Open
1 task done
low-batt opened this issue Feb 16, 2024 · 48 comments · May be fixed by #4830
Open
1 task done

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

low-batt opened this issue Feb 16, 2024 · 48 comments · May be fixed by #4830

Comments

@low-batt
Copy link
Contributor

low-batt commented Feb 16, 2024

System and IINA version:

  • macOS 13.6.3
  • IINA 1.3.4

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:

[  34.592][d][cplayer] Run command: screenshot, flags=73, args=[flags="window", legacy="unused"]
[  34.630][e][cplayer] Taking screenshot failed.

Steps to reproduce:

  • Configure IINA key bindings to use mpv Default
  • Start playing a video
  • Press ⌃s
  • MPV does not have this problem.

How often does this happen?
Every time.

@low-batt
Copy link
Contributor Author

From the entry for the screenshot command in the mpv manual:

screenshot <flags>
Take a screenshot.

Multiple flags are available (some can be combined with +):

<window>
Save the contents of the mpv window. Typically scaled, with OSD and subtitles. The exact behavior depends on the selected video output.

The expectation is that this form of the screenshot command is somehow tied to the mpv UI and thus can not be used by a libmpv client. That needs to be verified with the mpv project.

For now the ⌃s key binding should be removed from the mpv Default key bindings.

If the window flag is indeed not usable by a libmpv client then IINA may be able to implement this feature itself.

@svobs
Copy link
Contributor

svobs commented Feb 16, 2024

I wonder if screenshot window should really be allowed as a libmpv command.

Side comment / rant. IINA could replicate this by taking a snapshot of its own player window. I have working code which calls CGWindowListCreateImage to create a CGImage from the window's contentView. But Apple has created a lot of uncertainty here.

CGWindowListCreateImage is, as best I can tell, the standard way to convert a view's contents to picture data. But this API was deprecated in Sonoma with a note stating that ScreenCaptureKit should be used instead. I wonder if whoever at Apple decided to deprecate the original API actually realizes how drag & drop previews have been generated on a Mac.

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 CGWindowListCreateImage is not used, I don't see another way for IINA to create images from its own views without asking for permission to record the whole screen and access the microphone.

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 ~/Downloads (which is why I now don't use that folder at all, and instead use ~/ActualDownloads for all my downloads).

@low-batt
Copy link
Contributor Author

Yes, this is why I think the current solution for now should be to remove the key binding to the screenshot window command. There are more important issues to address. I created this issue so it wouldn't be totally forgotten.

The first place to start would be to explore what mpv is doing and understand why it is failing. If indeed the solution requires IINA to implement then yes it seemed like that would require using CGWindowListCreateImage and ScreenCaptureKit.

I found people discussing the security prompts. Apparently that was added for CGWindowListCreateImage as well in a macOS Ventura Beta. Maybe Apple backed off that due to negative feedback? I agree with ScreenCaptureKit requiring permission be granted this seems like inconsistent security practices.

If you do want to address this now, that is fine with me.

@svobs
Copy link
Contributor

svobs commented Feb 18, 2024

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 ⇧⌘5.

I found people discussing the security prompts. Apparently that was added for CGWindowListCreateImage as well in a macOS Ventura Beta. Maybe Apple backed off that due to negative feedback?

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 CGWindowListCreateImage. They could have changed the legacy API so it returns all black pixels until the user grants permissions. Possibly some apps would still break. It is a curiosity and maybe an insight into Apple's priorities.

@saagarjha
Copy link
Member

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?

@low-batt
Copy link
Contributor Author

I entered this issue to "kick the can down the road" as it involves work to first understand exactly what this mpv command does and why it is failing. I'm thinking this is a very low priority issue and we should not work on it at this time, but I thought we should at least have a record of the problem.

This came up due to a regression with the mpv screenshot command. In fixing that we took a close look at IINA's behavior and found some other existing problems that we are fixing.

I just played with mpv and the screenshot command with the various flags. One difference is that the resulting screenshot with the window flag is scaled, whereas the other flags use the original resolution.

The IINA mpv Defaults key bindings correctly reflects mpv and binds ⌃s to screenshot window, which is failing as discussed above. So my proposal is what we remove this binding for now and take another look at this in the far future.

I see we have a priority: low label. I will apply that.

@krackers
Copy link

krackers commented Feb 23, 2024

Window screenshot works fine with libmpv clients as well. I think the issue is that you don't have MPV_RENDER_PARAM_ADVANCED_CONTROL set which is necessary for gpu screenshots. The distinction is a bit annoying, it used to be the case in some ancient mpv version that screenshots were software rendered while GPU screenshots were simply grabbed from the framebuffer (and hence would be taken after scaling, with osd, etc.). Later this limitation was reduced, and now non-window screenshots can go through the gpu rendering path as well, so I think the only difference with window screenshots is that they explicitly rendered onto an unscaled fbo, without osd.

https://github.com/mpv-player/mpv/blob/master/video/out/gpu/video.c#L3437-L3458

But with libmpv clients setting MPV_RENDER_PARAM_ADVANCED_CONTROL is necessary for gpu screenshots. So if it's disabled then only sw scaled non-window screenshots will work, while window screenshots will not.

Btw blame shows e0dc0bf. Don't know why was originally disabled, there's no harm in having it enabled.

@low-batt
Copy link
Contributor Author

@krackers Thank you very much for pointing this out!

At one point I investigated that commented out code that sets MPV_RENDER_PARAM_ADVANCED_CONTROL. I too don't know why it was disabled, but I've always assumed it was to enable support for non-fatal timeouts to ensure the mpv core does not deadlock forever as per this comment in render.h:

Basically, the situation that your render thread waits for a "not safe" libmpv API function to return must not happen. If you ignore this requirement, deadlocks can happen, which are made non-fatal with timeouts; then playback quality will be degraded, and the message
mpv_render_context_render() not being called or stuck.
is logged. If you set MPV_RENDER_PARAM_ADVANCED_CONTROL, you promise that this won't happen, and must absolutely guarantee it, or a real deadlock will freeze the mpv core thread forever.

I probably read this comment:

Rendering screenshots with the GPU API if supported by the backend (instead of using a suboptimal software fallback via libswscale).

But that makes it sound like mpv will fall back to software, not fail.

What else is IINA missing out on by not enabling MPV_RENDER_PARAM_ADVANCED_CONTROL?

Quite a long time ago I tried running with MPV_RENDER_PARAM_ADVANCED_CONTROL enabled to check for deadlocks and did not manage to trigger one. Of course that was not a deterministic test.

IINA will sometimes trigger that warning. I have seen it during shutdown.

I ran a quick test with MPV_RENDER_PARAM_ADVANCED_CONTROL enabled and screenshot window worked correctly.

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.

@krackers
Copy link

But that makes it sound like mpv will fall back to software, not fail.

That's only possible for non-window screenshot though, where it passes the mp_image through libswscale

it was to enable support for non-fatal timeouts to ensure the mpv core does not

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.

enabled to check for deadlocks and did not manage to trigger one.

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.

IINA will sometimes trigger that warning. I have seen it during shutdown.

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).

@krackers
Copy link

krackers commented Feb 23, 2024

Actually in your case maybe this is the issue

     videoLayer.suspend()
      player.mpv.mpvUninitRendering()

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 videoLayer.suspend().

low-batt added a commit that referenced this issue Feb 23, 2024
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.
@low-batt low-batt linked a pull request Feb 23, 2024 that will close this issue
2 tasks
@low-batt low-batt self-assigned this Feb 23, 2024
@low-batt low-batt linked a pull request Feb 23, 2024 that will close this issue
2 tasks
@low-batt
Copy link
Contributor Author

@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!

svobs pushed a commit to svobs/iina-advance that referenced this issue Feb 28, 2024
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.
@svobs
Copy link
Contributor

svobs commented Feb 28, 2024

Looks like this is a duplicate of #4505.

@low-batt
Copy link
Contributor Author

Good catch! I totally had forgotten about #4505.

@low-batt
Copy link
Contributor Author

low-batt commented Mar 9, 2024

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 mpv render API methods. I will need to investigate some more.

@krackers
Copy link

krackers commented Mar 13, 2024

@low-batt Thanks for digging that up. I'm a bit confused though:

The situation where using advanced_control would deadlock seems to be something like the following:

  • You call mpv_get_property from the render thread, for a property like vo-passes
  • The vo-passes property requires a blocking voctrl down to libmpv, which schedules a task on the internal libmpv dispatch queue (note that when I use the term "libmpv dispatch queue" I mean mpv's own implementation of a task queue, NOT GCD dispatch queue), which is to be processed by the render thread during the next call to render update, and the caller blocks until this is processed
  • But that render update call never comes (because render thread is itself blocked on our get_property call). Deadlock.

This is what wm4 meant by

Note how enabling advanced rendering control disallows the API user from calling certain "blocking" functions from the same thread as the rendering functions.

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 mpv_render_* calls, and in fact there's no com.colliderli.iina.mpvgl GCD queue present in the stack trace at all!

Instead, the situation is actually more subtle: we can see in the process dump (thank you to author for including that)

    2439 Thread_16655846: mpv/mpv core
    + 2439 get_buffer2_direct  (in libmpv.dylib) + 694  [0x1094aa346]
    +   2439 dr_helper_get_image  (in libmpv.dylib) + 53  [0x1094b9c15]
    +     2439 mp_dispatch_run  (in libmpv.dylib) + 91  [0x10944e72b]
    +       2439 _pthread_cond_wait  (in libsystem_pthread.dylib) + 724  [0x7fff71e135cb]
    +         2439 __psynch_cvwait  (in libsystem_kernel.dylib) + 10  [0x7fff71d5a1b2]

The core goes to decode a frame (in vd_lavc, calls into libavcodec, which in turns calls back into the registered directRendering callback to get a GPU buffer. (Unrelated: on osx OpenGL is old and does not support AZDO functions, so this always returns null. But nonetheless the callback still happens every frame when software decoding. I mistakenly thought that Direct Rendering callback would never be called if it was unsupported, but it seems it still is, and it fails later on.)

This is where the issue lies: because IINA is not calling mpv_render_update at all, the core remains blocked. So naturally all other calls into the core block as well.

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:

For some reason it only happens when the window is offscreen.

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 render_update inside canDraw of the layer. Assuming that the queue itself is still active (not suspended) at the time of the deadlock then the only thing I can think of is osx trying to be clever and skipping such calls entirely when window is occluded.

@svobs
Copy link
Contributor

svobs commented Mar 20, 2024

Assuming that the queue itself is still active (not suspended) at the time of the deadlock then the only thing I can think of is osx trying to be clever and skipping such calls entirely when window is occluded.

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.

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).

Thank you for this! The need for suspend() had long been confusing me. The code calls this in 4 places: while entering full screen, exiting full screen, when IINA initiates shutdown, and when mpv initiates shutdown and MPV_EVENT_SHUTDOWN was received.

@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 suspend() when changing tracks, but if the user had the pref fullScreenWhenOpen enabled then the track change may have triggered a call to enter full screen.

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, suspend() was only called for the full screen toggles (i.e. not during shutdown) so those must have been added after the bug report.

The calls to suspend() during shutdown were the only remaining invocations of them in my fork of IINA. I removed them and found no adverse impact. I'd recommend doing the same in this project if there is any potential danger.

As for the calls to suspend() at full screen toggle...sounds like they are unsafe as well? The toggle will suspend for the duration of the animation (0.5 sec). There are some potential tasks which may call get_property from the main queue during that time - the most immediate which comes to my mind is the Inspector window, which can query almost any set of properties and does so once per second while it's open.

@low-batt
Copy link
Contributor Author

@krackers @svobs This discussion is very helpful.

Issue #3997 shows a mpv_get_property related deadlock. A full process sample is available. I believe the problem is that this requirement is being violated:

Basically if VO is active, IINA needs to process the dispatch queue

The mpv queue is processed byMPVController using the com.colliderli.iina.controller queue. In #3997 MPVController is blocked trying to draw in response to a MPV_EVENT_PLAYBACK_RESTART event.

PR #4819 changes MPVController to asynchronously dispatch most work to the main queue. I am proposing this change to fix numerous data races by only accessing the data using the main thread, rather than resorting to locking. The requirement to continue to process mpv events provides another reason to simplify the work performed using the com.colliderli.iina.controller queue.

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.

@krackers
Copy link

krackers commented Mar 21, 2024

@svobs

unpublicized feature called App Nap

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).

The need for suspend() had long been confusing me.

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.

As for the calls to suspend() at full screen toggle...sounds like they are unsafe as well

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).

@low-batt

I'm not too familiar with IINA internals, so sorry for noob question but in your linked issue #3997 why are both com.colliderli.iina.controller and com.colliderli.iina.mpvgl trying to call into mpv render? I thought all rendering was done through the view layer would go through com.colliderli.iina.mpvgl .

@low-batt
Copy link
Contributor Author

Questions are always fine. Exchanging information is how we get to the root cause of some of these harder problems. I'm always happy to answer questions. Trouble is I am still somewhat a noob and do not know the reasons behind why the code is the way it is.

I can confirm that drawing is not always done by the thread associated with the com.colliderli.iina.mpvgl queue. One example in MainWindowController can be seen here where the main thread is used:

// force rerender a frame
videoView.videoLayer.mpvGLQueue.async {
  DispatchQueue.main.sync {
    self.videoView.videoLayer.draw()
  }
}

I'm not sure why that code is using both the mpvGLQueue and the main queue. There are two other places in MainWindowController that also draw using the main thread.

As long as there isn't some reason why so much work is done directly by the com.colliderli.iina.controller queue and the changes in PR #4819 are approved by the other developers, that would at least remove drawing by that queue.

@svobs
Copy link
Contributor

svobs commented Mar 21, 2024

@krackers:

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).

Well. I took some peeks at the App Nap status of my various apps over the past day to see if I could see some patterns. It looks like it is only ever enabled for "traditional" apps with windows (i.e. not daemons, menu bar apps, etc.). Among Apple's stock apps, Xcode, Notes, and Preview take naps, but looks like [i]Messages does not, oddly. But I've also been running Spotify paused in the background and haven't touched it since yesterday, but it is not napping. This suggest there must be a way for apps to disable it or reject it.

I know that some time ago, @low-batt did a bunch of work getting idle usage down to 0%, so IINA doesn't even need this feature. I'll try to find time to dig up more info.

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.

See this comment, but the gist is that it was done to hide a flickering effect when transitioning into legacy full screen because the window style changes couldn't be applied cleanly while doing a zoom animation. Though it looks like Apple has been making significant improvements to this area over the last few releases, there are a lot of other problematic things (problems even?) with the full screen transition code such that the observed behavior may be unpredictable for different MacOS versions. IMHO it needs a big refactor to do properly. Which I did on my fork. But big changes are not being approved right now.

I thought all rendering was done through the view layer would go through com.colliderli.iina.mpvgl .

That is the normal way to render while playing, yes. But there are also "forced" renders which exist as a workaround when the video is paused. When paused, applying certain changes to the enclosing window such as minimizing + unminimizing or changing the window style for legacy full screen (as mentioned above) will cause the video to go black. It's as though (I'm speculating)CAOpenGLLayer has the only reference to the image and does not preserve it when it loses visibility. But the behavior is very consistent and predictable, and adding a redundant render call brings it back before the user can even see it's gone.

I've mused about finding another way to cache or capture the paused image so that the "forced" renders wouldn't be needed. But never got around to it.

@krackers
Copy link

krackers commented Mar 21, 2024

@svobs

way for apps to disable it

Yeah I think there's a way for apps to opt out of it.

transitioning into legacy full screen because the window style changes couldn't be applied cleanly while doing a zoom animation

Curious, is it IINA itself that creates the zoom-animation here? If you normally change the style mask on a window the changes take effect relatively instantly.

has the only reference to the image and does not preserve it when it loses visibility

Yeah that makes sense, probably things like switching style mask cause the window backing store to be flushed and recreated entirely. But why can't we just queue these redundant renders on the existing gl queue, instead of needing to do it from another queue?

@svobs
Copy link
Contributor

svobs commented Mar 21, 2024

Curious, is it IINA itself that creates the zoom-animation here?

Correct. Not sure what the default one would look like, to be honest. I suspect it wouldn't be as pretty as the current one.

If you normally change the style mask on a window the changes take effect relatively instantly.

For most style bits you are correct, but adding or removing the titled style seems to be a special case which has many side effects. For example, when it's removed, Zoom and Minimize menu items are disabled (+ others), and the window loses its entry in the Window menu and Dock. But as recently as Big Sur it also was monstrously slow to toggle this style - perhaps 0.25 sec on its own.

With Sonoma it's effectively instantaneous in some cases, but in others I still can't avoid a small flicker wherein the window seems to momentarily disappear. Compounding the problem is the way the Cocoa does animations. Depending on what else is going on in the window or in other apps, or the CPU is under load, they can sometimes act laggy or show some tearing and it can be really hard to nail everything down. And when using legacy full screen, the menu bar and Dock visibility need to be toggled via a separate call from the zoom and from the style change, and all this is supposed to be look simultaneous.

But why can't we just queue these redundant renders on the existing gl queue, instead of needing to do it from another queue?

Not sure :) They definitely need to be done quickly to avoid a visible flash of black. On the other hand, the queue should be empty and ready for work. I'll try playing around with it and see if it can work.

@low-batt
Copy link
Contributor Author

From Inform the System About Lengthy User-Initiated Activities:

Denoting user-initiated work prevents the system from deferring the operations or putting your app in App Nap

If we are sure App Nap is causing issues we can add code to call NSProcessInfo.beginActivityWithOptions passing NSActivityUserInitiated. Something like that.

I can enter an issue for this, but it would be nice to first confirm that App Nap is causing problems.

Testing seemed to show that macOS is using heristics to decide if App Nap should be prevented or not. Playing a small movie that can use hardware decoding shows napping being prevented some of the time:
issue-4822-prevented

But playing a 4K 60fps AV1 encoded video does not prevent App Nap:
issue-4822-allowed

@low-batt
Copy link
Contributor Author

I tried quick test by adding this code to AppDelegate.applicationDidFinishLaunching:

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 Yes for App Nap.

I even tried [.idleSystemSleepDisabled, .latencyCritical, .userInteractive]. App Nap was not prevented.

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?

@svobs
Copy link
Contributor

svobs commented Mar 22, 2024

It looks like you need to save the variable it returns somewhere. Don't let it get garbage collected.

I tried this and it seemed to work:

  var activity: NSObjectProtocol?

  func applicationDidFinishLaunching(_ aNotification: Notification) {
    Logger.log("App launched")
    activity = ProcessInfo().beginActivity(options: ProcessInfo.ActivityOptions.userInitiated, reason: "Playing video, allegedly")

The only way I've been able to reliably activate App Nap was to pause my video and use ⌘H to hide the window, then wait a couple of minutes. It didn't always activate if I only occluded it. And only tried one case of playing a video in the background, which didn't activate. So I used IINA > Hide IINA and waited, which seemed definitive as a test.

The graph below is from one of my tests. Strange that "App Nap Prevention" was actually turned off while playing the video. Though I did play while fully occluded for several minutes and App Nap did not turn on.

SCR-20240321-uerz

So it looks promising to begin an "activity" like this when entering the play state and ending it when pausing/stopping. My only concern is about "Idle Prevention". I'm unfamiliar with this. Might it interfere with machine sleep?

@low-batt
Copy link
Contributor Author

The Energy report makes no sense to me.

The row label is App Nap Prevention. But underneath there is a Napping label, but it fails to show an associated color? Under that is a section that seems to be intended to describe the rows except two of the labels do not match up, App Nap, missing "Prevention" and the words are out of order in High CPU Utilization. And the order of the rows of descriptions do not match the table, as High CPU Utilization should be first. Very sloppy design..

Looking at the pictures here. It seems the Napping label is supposed to have a green square in front of it. I do see the green bar when Activity Monitor reports IINA is napping.

IINA does have a SleepPreventor, but that does not seem to be what Idle Prevention is reporting. I think this is reflecting IINA's use of timers?

@svobs
Copy link
Contributor

svobs commented Mar 22, 2024

The Energy report makes no sense to me.

Ahh, good observations. Yeah, it looks like it was thrown together and then forgotten.

Looking at the pictures here.

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:

SCR-20240322-kuwc

To me it looks like the App Nap Prevention and possibly Idle Prevention indicators are broken and shouldn't be relied upon. It makes no sense for App Nap and App Nap Prevention to both be active at the same time.

Moreover, this suggests that there is a solution in ProcessInfo:

SCR-20240322-lamq

This page looks useful. It looks like a "power assertion" is a synonym for the "activity" started frombeginActivityWithOptions, though I could be wrong.

I think it's reasonable to assume Xcode is broken. But there are other tools. The Preventing Sleep column in Activity Monitor could be useful, along with pmset -g assertions. But manual testing is always a reliable fallback...

@low-batt
Copy link
Contributor Author

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 OpenGL, so according to that documentation it shouldn't be a candidate for App Nap.

Power assertions are separate from ProcessInfo assertions. IINA's SleepPreventor class calls IOPMAssertionCreateWithName to establish a power assertion that prevents the display from sleeping. Issue #4354 shows pmset -g assertions being used to investigate a problem involving power assertions.

There is also an App Nap column in Activity Monitor. I see that turning to Yes when I stop playback and minimize IINA to the dock.

@svobs
Copy link
Contributor

svobs commented Mar 22, 2024

What Xcode version? I'm using Xcode 15.2.

I'm using 15.3.

I'm thinking some of this information in Apple's documentation archive is out of date. IINA is using OpenGL, so according to that documentation it shouldn't be a candidate for App Nap.

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?

Power assertions are separate from ProcessInfo assertions. IINA's SleepPreventor class calls IOPMAssertionCreateWithName to establish a power assertion that prevents the display from sleeping. Issue #4354 shows pmset -g assertions being used to investigate a problem involving power assertions.

Ahh, thank you. And I see no mention of App Nap in IOKit.pwr_mgt. Conway's Law in action. So there's probably not much useful in Power Management for this problem.

My best bet remains to do testing of processInfo.beginActivity to see if it is strong enough to prevent the app from napping but weak enough not to prevent the machine from sleeping.

@krackers
Copy link

@svobs

but adding or removing the titled style seems to be a special case which has many side effects.

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.

@svobs
Copy link
Contributor

svobs commented Mar 23, 2024

@krackers:

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.

You're forgetting the rounded corners. There seems to be no way to make them square again except by removing the titled style. I also considered a kludge to work around it: expanding the window off screen in every direction by the corner radius. But the window manager really does not like it when the title bar goes off screen, and IINA's current window layout would require huge changes to avoid clipping the video.

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.

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 insidedisplay(). I don't know for sure that's where it is. It could be farther up. I'd need to find a video which will trigger App Nap while playing. I have witnessed that repeat timers continue to fire during App Nap but I'm still unsure how dispatch queues are treated, much less mpv's internal queues. But I can confirm that during App Nap, canDraw(inCGLContext:, ...) and draw(inCGLContext:, ...) are not being called, and thus neither are mpv_render_context_update and mpv_render_context_render.

@krackers
Copy link

You're forgetting the rounded corners. There seems to be no way to make them square again except by removing the titled style

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 NSThemeFrame and undo the rounded corners (and this could be done dynamically, since it's just called as part of the frame view's drawRect). I'm fairly certain it should still be possible to undo rounded corners this way but it might require digging into some AppKit internals. Not sure if you want to go down that route... (might make for a day of fun spelunking though).

But I can confirm that during App Nap, canDraw(inCGLContext:, ...) and draw(inCGLContext:, ...) are not being called, and thus neither are mpv_render_context_update and mpv_render_context_render.

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 CAOpenGLLayer's [draw] method exiting early, then can we move mpv_render_context_render out of canDraw and just run it on the dispatch queue directly? It still needs a valid gl context, so you'll have to surround it with cgllock yourself, but it's safe to call this outside the draw method because it doesn't rely on there being an active framebuffer. And likewise it should be possible to detect the case when draw exited early by having some sort of bit that's set there. In such a case we could call mpv_context_render ourselves and pass the skip flag to basically unblock the render loop but not actually draw anything.

@svobs
Copy link
Contributor

svobs commented Mar 23, 2024

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 test

Since 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 Hide IINA to hide it. I saw that App Nap was activated even though the timer, the dispatch queue, and display() were all being called! But that did not mean that things were being drawn...

2. CAOpenGLLayer's draw callbacks test

I 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 Hide IINA to hide it.

But now I noticed that as soon as Hide IINA was used, canDraw(inCGLContext:, ...) and draw(inCGLContext:, ...) both stopped getting called. It wasn't that canDraw returned false - it never got a chance. This is something higher up which has already decided that drawing will not happen.

Thoughts

  1. What does App Nap actually do when it is on? I wasn't able to get it to prevent rendering or cause any problems, so I really haven't seen anything it's actually preventing and thus saving CPU from. I remain nervous about what it might do...although a lot less so.
  2. It looks like the original concern is still a problem which still needs to be addressed, even if the conditions for triggering it are different that originally thought. Namely, we need to get mpv_render_context_update and mpv_render_context_render out of canDraw and draw(*GL*), respectively.

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 canDraw and draw and paste them into display (matching their logic), and hope that mpv drives the timing correctly. But:

If it's just a matter of CAOpenGLLayer's [draw] method exiting early

Not sure what you mean by this? When could this happen? And I've never understood the what the skip flag does, really...

@svobs
Copy link
Contributor

svobs commented Mar 23, 2024

@krackers also, am I correct in thinking it is necessary for the sake of mpv to call both mpv_render_context_update and mpv_render_context_render (if the first indicated that it should), even if there is no drawing needed? Or can *update be called alone and save CPU?

@krackers
Copy link

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 need to get mpv_render_context_update and mpv_render_context_render out of canDraw and draw(GL), respectively.

We can get mpv_render_context_update out of canDraw, but we can't get mpv_render_context_render out of draw. The curse of CAOpenGLLayer is that you are only given a valid FBO target for the duration of the drawGL call. Outside of it, you have no fbo connected to the display (you can use offscreen fbos of course, but those won't do you any good if you want to actually see your output).

What you need to do is have a fallback path for when draw is bypassed entirely. In such a case you still need to call mpv_render_context_render but you should use the skip rendering flag when calling libmpv, so it knows to not try to draw anything to the screen. This will keep the a/v sync code happy.

Not sure what you mean by this? When could this happen

I probaby worded it badly, I meant exactly what you observed where canDraw and draw are just never called at all. You can detect such a case by e.g. incrementing a counter from within draw and checking its value before/after the call to [super display].

mpv_render_context_update and mpv_render_context_render (if the first indicated that it should), even if there is no drawing needed? Or can *update be called alone and save CPU?

Update can be called alone. You only need to call into render if the return value update indicates you shoould.

@svobs
Copy link
Contributor

svobs commented Mar 26, 2024

We can get mpv_render_context_update out of canDraw, but we can't get mpv_render_context_render out of draw. The curse of CAOpenGLLayer is that you are only given a valid FBO target for the duration of the drawGL call. Outside of it, you have no fbo connected to the display (you can use offscreen fbos of course, but those won't do you any good if you want to actually see your output).

Ah, that makes a lot of sense and helped some pieces click into place which had long been loose in my mental model.

What you need to do is have a fallback path for when draw is bypassed entirely. In such a case you still need to call mpv_render_context_render but you should use the skip rendering flag when calling libmpv, so it knows to not try to draw anything to the screen [...]

[...] where canDraw and draw are just never called at all. You can detect such a case by e.g. incrementing a counter from within draw and checking its value before/after the call to [super display].

And this! I think I finally understand the code in draw(forced:). So the variable needsMPVRender is doing that job. It's set to true before calling display(). It will be set to false in draw(inCGLContext:) if that ends up getting called. After display() returns, needsMPVRender is checked to see if it is still true, and if so, does a skip render. That's what this comment meant (just needs to be past tense, and should be a few lines up):
// draw(inCGLContext:) is not called, needs a skip render

So it looks like IINA's code is already doing the correct thing here by doing the skip render. Though it is bypassing canDraw() and thus any call to mpv's update. So am correct in thinking it is not critical to do so, but it could be more efficient if the skip render is only used if update says that a render is needed?

Also, I now see another reason draw(forced:) should always be called from the same serial dispatch queue. The use of a lock above and below the display() call won't guarantee that needsMPVRender will always be read & written in correct order if there are concurrent calls from different threads.

@krackers
Copy link

krackers commented Mar 26, 2024

but it could be more efficient if the skip render is only used if update says that a render is needed?

Yes, if render_context_update did not request a render, then you don't need to call render at all.

call won't guarantee that needsMPVRender will always be read & written in correct order if there are concurrent calls from different threads.

Possibly, it needs a thorough examination of whether it's safe to treat needsMPVRender as a relaxed atomic in this case (well swift doesn't have atomics in the C11 sense at all, but we can basically assume that int is atomic all/nothing on every platform osx runs on, and it's only a question of memory ordering). The fact that locks above/below exist may well mean that those locks provide acq/rel semantics on visibility of changes to needsMPVRender. The general principle is that it's always safe to call context_render() more than requested, but we should never call it less than requested. But it's not worth doing this unless you like the intellectual curiosity, it's definitely safer to use serial dispatch and/or surround with mutex as needed

@low-batt
Copy link
Contributor Author

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 ViewLayer properties needsMPVRender and forceRender. Rather than applying IINA's @Atomic property wrapper to these properties (which adds locks to a property), they are always accessed while holding a lock on the VideoView property isUninited:

@Atomic var isUninited = false

This is documented in the draw method:

  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.

@krackers
Copy link

krackers commented Mar 26, 2024

At least that is the idea. Keep an eye out for anything I got wrong.

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 isUninited lock surrounding draw(inCGLContext from what I can see, so things still race? The situation we want to avoid is the following ordering:

gl layer draw (called by system as async) calls mpv_render_context_render
draw called, sets needsRender to true
gl layer draw sets needsRender to false

In such a case we have a "lost draw". My gut feeling (but I am still a noob on memory ordering) is that treating needsMPVRender as a relaxed atomic should be fine. If we are not inside a live resize then everything will go through the single dispatch queue so there is no issue. If we are inside a live resize, then we know that canDraw + draw will be called periodically by the system. Events occurring on a given thread have an implicit ordering, so as long as needsRender is only set to true (never set to false) from another thread, this is a similar situation to shutdown signaling which I believe is a known OK pattern for relaxed atomics (acq/rel is not needed because we don't have any additional state invariant other than the signal itself). I thought more and my original feeling was wrong, because it's written to, not just read, from both threads this differs from the one-wya communication of a shutdown signal, so the pattern is not the same. In particular because we effectively have this 2-way communication, we need to consider the visibility to the other thread: and because it is relaxed, if we don't make any assumptions on the internals of mpv_render() then the store needsRender = false could be globally visible after mpv_render(). In which case we could precisely have the problematic ordering mentioned above. Hence I think you either need seq_cst on the store, use atomic rmw with acq_rel ordering, or wrap it in a mutex.

Where did that end up?

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.

Would be nice to support dynamic display timing for ProMotion displays

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 glFlush()/swapBuffers(). So all you would need to do is just have mpv use display-sync=audio mode and you should already have judder-free playback. Well at least in theory, in practice with vrr things are now actually sensitive to the CPU timing (e.g. the presentation interval is implicitly the cpu-time between our glFlush() so the results can be more jittery than fixed-vsync. See also the mpv issue "An implementation of display-resample that works in tandem with VRR displays?" for more discussion on this. Ideally you'd have a reference clock (e.g. cvdisplaylink) at the highest fps the vrr display supports and use that to constrain the present() so as to avoid the jitter. (No idea how cvdisplaylink works on a vrr display, worst case you could always use a realtime mach thread). There's also the LFC issue where if your content is below the VRR range you have to take care of frame-doubling your present() as needed. I don't know if osx handles LFC for you or not.

feature is about changing the screen refresh rate to match the video when in full screen mode

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.

@low-batt
Copy link
Contributor Author

What about the case where async=true is set on the layer? As we found out in that PR discussion

Which PR was that? Yes, if that happens then the current code is no good.

Is this basically the same as VRR/gsync/freesync?

I believe so. The WWDC video I linked has a good explanation of it.

Is the disruption still present with M1+ macs, those have seamless and instant mode switches.

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:

  • IINA Use legacy full screen
  • macOS Reduce motion

Likely most users have not enabled Reduce motion or IINA's Use legacy full screen and experience the full animated visual transition effects slowly showing the app going into full screen mode. Adding changing the display refresh rate causes audio to be disrupted during the transition:

[ 264.748][w][cplayer] Audio device underrun detected.

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.

@svobs
Copy link
Contributor

svobs commented Mar 28, 2024

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 completionHandler feature when calling NSAnimationContext.runAnimationGroup because without it there is no guarantee that animations will be atomic.

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...

@svobs
Copy link
Contributor

svobs commented Mar 28, 2024

Which PR was that? Yes, if that happens then the current code is no good.

Issue #4667 links to two different PRs, neither of is a satisfactory solution. In my fork, I ultimately created a method, enterAsynchronousMode which sets isAsynchronous = true and then sets a 2sec timer to set it false (similar to what @krackers was originally arguing for); then called it from an override of setFrame and a few other places where the window will resize. This eliminates the video wobble. But the window resize itself looks less smooth when playing as it is while paused. I haven't been sure if it was a Cocoa limitation, or the priority of something needs to be increased somewhere somehow...

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 needsMPVRender race. Maybe replace that variable with an incrementing sequential identifier....which [again!] sounds like something @krackers was suggesting.

@low-batt
Copy link
Contributor Author

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:

14:47:40.392 [iina][w] mpv log: [cplayer] warn: 
14:47:40.392 [iina][w] mpv log: [cplayer] warn: Audio/Video desynchronisation detected! Possible reasons include too slow
14:47:40.392 [iina][w] mpv log: [cplayer] warn: hardware, temporary CPU spikes, broken drivers, and broken files. Audio
14:47:40.392 [iina][w] mpv log: [cplayer] warn: position will not match to the video (see A-V status field).
14:47:40.392 [iina][w] mpv log: [cplayer] warn: 

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 mpv project has finally added the missing support. There are quite a few other subtitle issues that need addressing as well.

I haven't had time to look into issue #4667. I will try and find the time to do so.

@svobs
Copy link
Contributor

svobs commented Mar 29, 2024

BUT… Do we need to dig into this now?

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 Reduce motion setting...by any chance do you normally have it enabled? I remember that you normally use a mouse so you wouldn't encounter the issue via pinch-to-zoom. For me the wobble is very annoying and makes IINA seem "junky", like driving a car with a broken window.

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).

@low-batt
Copy link
Contributor Author

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 mpv does not have this problem. Do we know how mpv avoids it?

I nearly always have Reduce motion enabled along with Use legacy full screen. I don't like to wait for computers. I want the application to go in and out of full screen mode as fast as it possibly can. If it can be instantaneous that would be ideal.

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 mpv adding the functionality. As mpv has now added the functionality I'm thinking it is a good time to add the functionality to IINA and fix some of the other subtitle issues. I already created PR #4235 (that you helped with) that adds support for subtitle visibility. There are a couple of other PRs with subtitles fixes. And it is likely the implementation for downloading from Open Subtitles will be switched to a plugin. So I'm thinking it is a good time to focus on fixing and enhancing subtitles.

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 mpv does. Fixing that would be appreciated by users.

I am worried that we already have too much work to do for one feature release.

@svobs
Copy link
Contributor

svobs commented May 2, 2024

I nearly always have Reduce motion enabled along with Use legacy full screen. I don't like to wait for computers. I want the application to go in and out of full screen mode as fast as it possibly can. If it can be instantaneous that would be ideal.

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 Reduce motion off but I'll never use full screen modes, Stage Manager, etc, for regular apps. It's just too sluggish. I really wish we could be allowed to speed up animations or turn them off individually.

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...

There are a good number of issues regarding IINA not handling the geometry of the window and the video like mpv does. Fixing that would be appreciated by users.

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!

@low-batt
Copy link
Contributor Author

low-batt commented May 3, 2024

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 mpv does. Users would definitely appreciate such a change.

Is the crop work going to make use of the new mpv crop feature mentioned here?

@svobs
Copy link
Contributor

svobs commented May 14, 2024

Is the crop work going to make use of the new mpv crop feature mentioned #4842 (comment)?

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 videoView but the mpv render API often sends nonsense data when it is between states and its exact state is too hard to infer. But as a result of all the extra effort I am getting much finer-grained control over many aspects of the video rendering. I'm actually pretty close to having IINA able do the crop (also arbitrary zoom) by manipulating the layer's geometry. It would be much more versatile / easier to fine-tune than working through libmpv APIs. But pulling on that thread will cause screenshots to unravel, etc...

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 iina_crop filter which contains a crop rectangle whose dimensions are derived from the current video. So a "4:3" crop which is applied to a 720p video will immediately break if the user skips to the next media in playlist which is 1080p. Would be better to preserve the intended "4:3" crop by recomputing the necessary filter when the next video starts. And would be great if IINA could recognize when another filter applied a crop and disable the iina_crop filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants