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

Resolve memory leak during Wayland recording using Xwayland #4247 #5977

Merged
merged 1 commit into from Feb 22, 2024

Conversation

tkna91
Copy link
Contributor

@tkna91 tkna91 commented Feb 17, 2024

Add GDK_BACKEND=x11 to Exec env

Resolves: #4247

Add GDK_BACKEND=x11 to Exec env
Resolve memory leak during Wayland recording using Xwayland

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

@LWinterberg
Copy link
Member

Do we know that the x11 environment/xwayland is always available? If systems without exist, I believe this change would break Audacity on them.

I see in the docs that there'd potentially an easy fix to that:

This environment variable can contain a comma-separated list of backend names, which are tried in order. The list may also contain a *, which means: try all remaining backends.

so GDK_BACKEND=x11,* would probably make this safe everywhere.

@tkna91
Copy link
Contributor Author

tkna91 commented Feb 20, 2024

Sorry, I was including a link to the gtk3 documentation.
If the next Audacity will use gtk4, it would be better to include a link to the following documentation
https://docs.gtk.org/gtk4/running.html#gdk_backend

@LWinterberg
Copy link
Member

ah, actually, it doesn't work. Audacity uses wxwidgets, and for compatibility reasons we build with GTK2. I see the * thing goes back at least 10 years, but that's not enough to reach GTK2 apparently. Please do an interactive rebase and drop the commit with my suggestion, then force-push.

Anyway, this means we actually have to answer the question: Do we know that the x11 environment/xwayland is always available for anything that uses the .desktop file? I will research that.

@LWinterberg
Copy link
Member

Okay, so:

  • RedHat is dropping the xorg server, but explicitly keeping the x11 environment (xwayland) around.
  • BSD seems to still run xorg most of the time.
  • Online forks of Audacity may or may not use broadway, but given that online services only need the maintainer to update this (and not every individual end user), it'd be fine.
  • Gnome 40+ has an xwayland-on-demand thing which would launch xwayland only when an app requests it (which we'd do here)
  • Outside of a few arch users, I have not seen any attempts to ditch xwayland by anyone.

Given that (and given that Mac and Windows don't care if you break the .desktop file), I think it's safe to assume that x11 is present in some capacity.

@imciner2
Copy link
Contributor

Fedora has already been carrying a patch adding this for at least the last 3 or so years: https://src.fedoraproject.org/rpms/audacity/blob/rawhide/f/gdk_x11_backend.patch

@tkna91
Copy link
Contributor Author

tkna91 commented Feb 21, 2024

@LWinterberg That is, GDK_BACKEND=x11,* is not good and GDK_BACKEND=x11 is good. right?
But I am having a hard time understanding why.
Are you saying that the x11,* values are not supported in GTK 3.0-3.9.x and may cause malfunctions?

@tkna91
Copy link
Contributor Author

tkna91 commented Feb 21, 2024

@LWinterberg No, no. You simply want me to organize my commits in the form GDK_BACKEND=x11,*?

@LWinterberg
Copy link
Member

@LWinterberg That is, GDK_BACKEND=x11,* is not good and GDK_BACKEND=x11 is good. right?

correct. Simply drop my suggested change, you had it right the first time.

But I am having a hard time understanding why.

It seems like old GTK does an exact string comparison, while new GTK does a substring comparison.

Fedora has already been carrying a patch adding this for at least the last 3 or so years

That's good to know. I imagine you're not aware of anyone running into problems because x11 is missing either then, correct? @imciner2

@imciner2
Copy link
Contributor

That's good to know. I imagine you're not aware of anyone running into problems because x11 is missing either then, correct? @imciner2

No, we've never had any problems reported by any users when using this. We first introduced this to fix #459, actually.

@LWinterberg LWinterberg merged commit 7f2b47b into audacity:master Feb 22, 2024
22 checks passed
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.

Memory leaks while recording on Wayland
3 participants