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

platform/windows: release frame early during mouse movement #286

Closed
wants to merge 2 commits into from

Conversation

psyke83
Copy link
Contributor

@psyke83 psyke83 commented Dec 7, 2021

Resolves visual stutter during mouse movement in certain applications.

Proposed resolution for: #227

It seems that waiting for the frame to be released between consecutive snapshot functions calls introduces visual stutter (only in certain applications & during mouse movement - see above linked issue). This may not be the ideal way to implement the fix, so feedback is appreciated.

@psyke83 psyke83 marked this pull request as draft December 7, 2021 18:27
@psyke83 psyke83 marked this pull request as ready for review December 7, 2021 21:07
@psyke83 psyke83 force-pushed the frame_release branch 2 times, most recently from 78aa7ff to 87aa253 Compare December 10, 2021 02:33
@psyke83 psyke83 changed the title platform/windows: release frame before waiting for next snapshot cycle platform/windows: release frame early during mouse movement Dec 10, 2021
@psyke83
Copy link
Contributor Author

psyke83 commented Dec 10, 2021

I updated the patch to only release the frame early when mouse movement occurred. I'm still not confident this is the best way to fix the issue in the code, but perhaps someone can test and point out a better solution.

@psyke83 psyke83 force-pushed the frame_release branch 3 times, most recently from d709328 to e784dc3 Compare December 10, 2021 23:25
@AetherCollective
Copy link

I've been wondering about this issue. Any ideas when it might get merged?

@psyke83
Copy link
Contributor Author

psyke83 commented Dec 14, 2021

This PR dramatically improves the problem to the extent that I'm using it on my machine, but I'm not convinced this is the ideal way to fix the issue.

Without this PR, any mouse movement during capture of certain content would cause frame_info.AccumulatedFrames to increase to 2, meaning that a screen update already occurred since the capture was taken. The patch set greatly reduces the problem so that AccumulatedFrames remains at 0/1 (when mouse/screen updates occur, respectively), but with mouse movement I still see occasional stutters (i.e. AccumulatedFrames can still increase to 2).

Microsoft's documentation states:

For performance reasons, we recommend that you release the frame just before you call the IDXGIOutputDuplication::AcquireNextFrame method to acquire the next frame.

Sunshine already does this, but it holds the frame for the delay interval between frames (std::chrono::nanoseconds { 1s } / framerate, i.e. 16.6ms for 60fps). I'm not certain if Microsoft's guidance includes the assumption that you aren't continuously capturing without delay, then waiting the requisite interval before picking frames in a separate thread.

@resech
Copy link

resech commented Dec 26, 2021

I'm getting similar errors to what the failed appveyor build shows, are there any extra pre-requisites/commands that need to be run to build this?

@psyke83
Copy link
Contributor Author

psyke83 commented Dec 26, 2021

@resech

It looks like the mingw headers were updated recently, so this PR itself is not the cause. #296 should fix it.

@TheElixZammuto
Copy link
Contributor

Thank you for the heads up @psyke83, I was also having the same problem that I couldn't reproduce on my local build but only on GitHub actions

Resolves visual stutter during mouse movement in certain applications.
* add DXGI_ERROR_INVALID_CALL: assume that the application already
  released the frame.
* DXGI_ERROR_ACCESS_LOST: reinit without assuming that the frame
  was released.

See: https://docs.microsoft.com/en-us/windows/win32/api/dxgi1_2/nf-dxgi1_2-idxgioutputduplication-releaseframe

Fixes intermittant stream disconnection due to frames not being
released correctly during mode (fullscreen <-> windowed) changes,
which is exacerbated if frames are not released immediately before AcquireNextFrame.
@psyke83
Copy link
Contributor Author

psyke83 commented Dec 31, 2021

After some more research, I found this: https://docs.microsoft.com/en-us/windows/win32/direct3ddxgi/desktop-dup-api

When AccumulatedFrames is >1, AcquireNextFrame does not return a desktop image, but instead provides update regions (dirty/move rectangles). Since sunshine doesn't yet support these updates, it considers these frames as empty, which results in visible stutters.

This PR was only helping to mask the issue by giving AcquireNextFrame less opportunity to construct dirty/move regions by placing ReleaseFrame() in a less optimal position. As I suspected, this PR is not the proper solution, so I'll convert this a draft until I've had time to investigate the real fix.

@psyke83 psyke83 marked this pull request as draft December 31, 2021 11:13
@psyke83
Copy link
Contributor Author

psyke83 commented Jan 6, 2022

Unfortunately, processing dirty rects* doesn't seem to affect slight stutter effect noticeable when AccumulatedFrames is >1. I'm struggling to think of another way to eliminate the remaining stutters (which I'll admit are quite rare, and mostly only occur when dealing with the desktop or a game running in windowed mode). I'll leave this in draft status, but I'm not sure what else I can do to alleviate these remaining rare stutters.

*If anyone is interested, here's a test branch: master...psyke83:dirty_rects
I'm not certain if this realistically improves performance, as each frame received by the encoder is still the complete image.

@hksdpc255
Copy link

I recommend to always send frames to the client even though the screen is not changed. Only for Windows.

As far as I know, a new screen capture mechanism was introduced in 7ddf8bb, while the old one works fine on Windows.

See: #253

It seems that current screen capture method is introduced in this commit:

7ddf8bb

@psyke83
Copy link
Contributor Author

psyke83 commented Jan 21, 2022

@hksdpc250

I've tested older builds without the capture callback change, but it has its own issues compared to master. The positive difference is that there isn't any lag in simple desktop applications such as Notepad (which is closer related to your bug report than this, I imagine). Unfortunately, performance for 3D applications, especially in windowed mode, is very choppy. Calling ReleaseFrame() doesn't help as much with that problem compared to latest master with the callback changes.

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

Successfully merging this pull request may close these issues.

5 participants