close
The Wayback Machine - https://web.archive.org/web/20220323175656/https://github.com/obsproject/obs-studio/pull/6091
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

linux-capture: Rewrite xcomposite #6091

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kkartaltepe
Copy link
Collaborator

@kkartaltepe kkartaltepe commented Mar 5, 2022

Description

Generally moves all the plugin code into xcomposite-input.cpp with a
mostly C style but keep some heavily c++ helpers xcomposite-helpers.cpp

Migrate as much as possible to xcb from Xlib to enable us to handle
errors and attribute them to the correct callers. This caused many other
knock on issues such as wrongly attributed errors and cleanup code
working incorrectly.

That allows us to use the xcursor-xcb implementation and delete the pure
Xlib implementation. We also add the missing functionality from the Xlib
implementation to the xcb implementation.

Capture glXCreatePixmap errors which occur most commonly on
nvidia+gnome due to nvidia's driver being unable to allocate more than 1
pixmap per window and gnome being the only compositor to read window
data via glx pixmaps.

Fix cleanup after failed glXCreatePixmap that might have leaked pixmaps
and prevented later captures on nvidia drivers for the same reason.

Migrate the x11 window event watcher to process events on its own
thread. This fixes a render pipeline stall in obs and compositors that
caused i3 and obs to reach ~10fps or gnome and obs to reach ~30fps.

Motivation and Context

This is a much more drastic version of #5998 but removes lots of the
abstraction and deadly sharp edges of Xlib to hopefully avoid us reintroducing
even more bugs. Also fixes bugs identified in earlier PR and some additional
perf improvement.

How Has This Been Tested?

Tested on i3/gnome/nvidia and i3/gnome/intel. Once taken out of draft there should be
no regressions from master.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@WizardCM WizardCM added Bug Fix Code Cleanup Enhancement Linux labels Mar 5, 2022
@kkartaltepe kkartaltepe added the Seeking Testers label Mar 5, 2022
@kkartaltepe
Copy link
Collaborator Author

@kkartaltepe kkartaltepe commented Mar 5, 2022

If anyone on AMD hardware can take this for a spin and let me know if anything behaves differently/worse than master that would be great.

Note if you do: Consider building master without the "window updated" blog so you can see existing gs_copy_texture failures on master. They appear more prominently in this PR because the window update message no longer spams the log when the window is resized (which is the same point where the pixmap texture might be invalid and print a gs_copy_texture failure).

@Chiitoo
Copy link
Contributor

@Chiitoo Chiitoo commented Mar 6, 2022

What does without the "window updated" blog mean?

So far haven't noticed anything too weird with the actual capturing using AMD (navi10, LLVM 13.0.1 / Mesa 22.0.0-rc2), but there is something odd happening, that probably isn't specific to AMD, possibly in

https://github.com/obsproject/obs-studio/pull/6091/files#diff-3219ad45ddd4eafa31d9cdeaa31fea7f980a3e7b2f5ec28dba6fafba21aec821R84

which results into some of the window names having extra characters suffixed into their names (for the most part, they don't show up in the terminal output info messages, but for some they do).

One example that seems to easily affect things here is the Falkon browser.

The characters appearing can be parts of other window names, box characters, numbers, or other characters such as the exclamation mark. Need to play around more with it to find out more.

@kkartaltepe
Copy link
Collaborator Author

@kkartaltepe kkartaltepe commented Mar 6, 2022

The characters appearing can be parts of other window names, box characters, numbers, or other characters such as the exclamation mark. Need to play around more with it to find out more.

Sorry i didnt mention why this was draft but window names have garbage in them right now, xcb doesnt implement proper string parsing so I havnt put that code back in yet, probably will finish that up today.

What does without the "window updated" blog mean?

Previously if you resized a window the info: [window-capture: 'Window Capture (Xcomposite) 4'] update settings: block was pushed to your log every frame. This was instead changed to only be output when you update your settings.

@ogmkp
Copy link

@ogmkp ogmkp commented Mar 7, 2022

If I test this one, should I drop #5998?

@kkartaltepe
Copy link
Collaborator Author

@kkartaltepe kkartaltepe commented Mar 7, 2022

If I test this one, should I drop #5998?

Yes, these are two different approaches.

Generally moves all the plugin code into xcomposite-input.cpp with a
mostly C style but keep some heavily c++ helpers xcomposite-helpers.cpp

Migrate as much as possible to xcb from Xlib to enable us to handle
errors and attribute them to the correct callers. This caused many other
knock on issues such as wrongly attributed errors and cleanup code
working incorrectly.

That allows us to use the xcursor-xcb implementation and delete the pure
Xlib implementation. We also add the missing functionality from the Xlib
implementation to the xcb implementation.

Capture glXCreatePixmap errors which occur most commonly on
nvidia+gnome due to nvidia's driver being unable to allocate more than 1
pixmap per window and gnome being the only compositor to read window
data via glx pixmaps.

Fix cleanup after failed glXCreatePixmap that might have leaked pixmaps
and prevented later captures on nvidia drivers for the same reason.

Migrate the x11 window event watcher to its process events on its own
thread. This fixes a stall render pipeline of obs and compositors that
caused i3 and obs to reach ~10fps or gnome and obs to reach ~30fps.
@kkartaltepe kkartaltepe force-pushed the xcompcap-smol branch 2 times, most recently from b03afb6 to 9834869 Compare Mar 13, 2022
@kkartaltepe
Copy link
Collaborator Author

@kkartaltepe kkartaltepe commented Mar 13, 2022

This should be equivalent to master now. There is one annoyance to cleanup which is that windows that fail to capture will repeatedly log the failing glxCreatePixmap error.

Known errors on this branch and master (thus not regressions): gs_copy_texture failures due to invalid pixmap textures being captured, displaying a stale frame sometimes when multiple windows resize at once (such as in tiling window managers).

One regression is that errors can be leaked out to the default XErrorHandler producing two errors in the log. Preferably we can avoid this but so far even moving error handler to include the last glx call still allows the error to slip through.

@kkartaltepe kkartaltepe force-pushed the xcompcap-smol branch 2 times, most recently from 151f34f to 5bc6deb Compare Mar 13, 2022
Redo the watcher change notification to just modify the source directly,
this is fine since make sure the source is unregistered before it is
deleted. Also remove some of the X11 calls during (un)registration.

Try to fix glXCreatePixmap error capture. Syncing on the xcb connection
doesnt appear to sync the queued xlib messages, so use XSync. Also
tighten the bounds of the error handler. There are still glx errors
leaking into the previous error handler sometimes so that sucks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Fix Code Cleanup Enhancement Linux Seeking Testers
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants