fix(linux/pipewire): avoid memory leaks

[Kishi85]

Description

In #5346 a memory leak in pipewire-based capture methods (portalgrab, kwingrab) was identified.

This PR is a (first) step to fix potential causes in pipewire-based capture by:

• Changing dummy_img() to just return 0 without allocating memory as that is enough to mark a dummy image (see also kmsgrab/wlgrab)
• Override the destructor of the used egl::img_descriptor_t to free allocated memory for the pipewire specific alloc_image() override by using a new subclass (also like kmsgrab/wlgrab do).
• Explicitly release any memory resources from kwingrab by calling reset() on any created shared_ptr instances in screencast_t and clear() on used lists/maps in screencast_t during destruction.
• Cleanup pipewire_t destruction sequence and roll cleanup_stream in as is is never used outside of it.

Screenshot

Issues Fixed or Closed

Roadmap Issues

Type of Change

• feat: New feature (non-breaking change which adds functionality)
• fix: Bug fix (non-breaking change which fixes an issue)
• docs: Documentation only changes
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc.)
• refactor: Code change that neither fixes a bug nor adds a feature
• perf: Code change that improves performance
• test: Adding missing tests or correcting existing tests
• build: Changes that affect the build system or external dependencies
• ci: Changes to CI configuration files and scripts
• chore: Other changes that don't modify src or test files
• revert: Reverts a previous commit
• BREAKING CHANGE: Introduces a breaking change (can be combined with any type above)

Checklist

• Code follows the style guidelines of this project
• Code has been self-reviewed
• Code has been commented, particularly in hard-to-understand areas
• Code docstring/documentation-blocks for new or existing methods/components have been added or updated
• Unit tests have been added or updated for any new or modified functionality

AI Usage

• None: No AI tools were used in creating this PR
• Light: AI provided minor assistance (formatting, simple suggestions)
• Moderate: AI helped with code generation or debugging specific parts
• Heavy: AI generated most or all of the code changes

[Kishi85]

In theory this PR shouldn't make a difference as C++ should manage memory for those types (std::shared_ptr, std::map, std::vector) by itself correctly for how they are used in kwingrab.

@psyke83 maybe you can check if it makes a difference on your end as my tests are a bit inconclusive right now...

[Kishi85]

Doing more testing and also cleaning up pipewire's destructor a bit but so far no luck with the memory leak. It's almost a stable ~16MB increase per full capture re-init for both kwingrab and portalgrab so the culprit is likely somewhere in pipewire.cpp or related to the pipewire code used overall.

Almost forgot: If I can't find the source of the leak myself I'll change this PR to do some code maintenance (like rolling 'cleanup_stream()` into the destructor as it's not used anywhere else anymore und updating a few unnecessary things based on CLion/Sonar suggestions).

[Kishi85]

Doing more testing and also cleaning up pipewire's destructor a bit but so far no luck with the memory leak. It's almost a stable ~16MB increase per full capture re-init for both kwingrab and portalgrab so the culprit is likely somewhere in pipewire.cpp or related to the pipewire code used overall.

Here's another interesting finding: The leak is only ~8MB per full capture re-init for both kwingrab and portalgrab when using VAAPI compared to ~16MB when using Vulkan encoding.

[psyke83]

This is part of the puzzle:

diff --git a/src/platform/linux/graphics.h b/src/platform/linux/graphics.h
index 2a2a0642..60be723f 100644
--- a/src/platform/linux/graphics.h
+++ b/src/platform/linux/graphics.h
@@ -603,7 +603,11 @@ namespace egl {
   class img_descriptor_t: public cursor_t {
   public:
     ~img_descriptor_t() {
+      BOOST_LOG(error) << "[leak] reset img_descriptor_t";
       reset();
+
+      delete[] data;
+      data = nullptr;
     }
 
     /**

With your PR branch applied, this resolves leaking with vaapi combined with kwin or portal capture. Vulkan still leaks, and software rendering breaks with the change as-is (haven't looked too deeply yet).

Some capture methods employ an img_t override that does extra cleanup. Example:

Sunshine/src/platform/linux/wlgrab.cpp

Lines 25 to 33 in 3437a0b

	struct img_t: public platf::img_t {
	/**
	* @brief Destroy the Wayland capture image.
	*/
	~img_t() override {
	delete[] data;
	data = nullptr;
	}
	};

or

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 807 to 812 in 3437a0b

	struct kms_img_t: public img_t {
	~kms_img_t() override {
	delete[] data;
	data = nullptr;
	}
	};

In the case of Pipewire capture, alloc_img() is overridden to use the img_descriptor_t type, but we don't do any special cleanup via a destructor override, and the standard destructor in graphics.h only cleans up the sd fds.

[Kishi85]

This is part of the puzzle:

diff --git a/src/platform/linux/graphics.h b/src/platform/linux/graphics.h
index 2a2a0642..60be723f 100644
--- a/src/platform/linux/graphics.h
+++ b/src/platform/linux/graphics.h
@@ -603,7 +603,11 @@ namespace egl {
   class img_descriptor_t: public cursor_t {
   public:
     ~img_descriptor_t() {
+      BOOST_LOG(error) << "[leak] reset img_descriptor_t";
       reset();
+
+      delete[] data;
+      data = nullptr;
     }
 
     /**

With your PR branch applied, this resolves leaking with vaapi combined with kwin or portal capture. Vulkan still leaks, and software rendering breaks with the change as-is (haven't looked too deeply yet).

Some capture methods employ an img_t override that does extra cleanup. Example:

Sunshine/src/platform/linux/wlgrab.cpp

Lines 25 to 33 in 3437a0b

	struct img_t: public platf::img_t {
	/**
	* @brief Destroy the Wayland capture image.
	*/
	~img_t() override {
	delete[] data;
	data = nullptr;
	}
	};

or

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 807 to 812 in 3437a0b

	struct kms_img_t: public img_t {
	~kms_img_t() override {
	delete[] data;
	data = nullptr;
	}
	};

In the case of Pipewire capture, alloc_img() is overridden to use the img_descriptor_t type, but we don't do any special cleanup via a destructor override, and the standard destructor in graphics.h only cleans up the sd fds.

So we probably have to just add a subclassed img_descriptor_t with the extra cleanup for pipewire and use that in the override?

Also is there a specific reason for pipewire using egl::img_descriptor_t instead of platf::img_t (like kms/wlgrab)?

[psyke83]

I'm not sure why that was done, but more investigation actually points this specific leak issue to be isolated to dummy_img:

Sunshine/src/platform/linux/pipewire.cpp

Lines 1069 to 1077 in 3437a0b

	int dummy_img(platf::img_t *img) override {
	if (!img) {
	return -1;
	}
	img->data = new std::uint8_t[img->height * img->row_pitch];
	std::fill_n(img->data, img->height * img->row_pitch, 0);
	return 0;
	}

Reverting my previous changes and instead short-circuiting dummy_img (inserting return 0; at the start or just deleting the function contents) also resolves the memory leak with the vaapi encoder, and software encoding no longer crashes (and also doesn't leak). Only Vulkan leaks as-is, which I assume is a separate unrelated leak.

Edit: I won't be able to test thoroughly until later, but if it turns out that the destructor really does need to do cleanup, being able to distinguish the ownership between dummy_img()'s vs other img_descriptor_t allocations would probably resolve the double-free crashing. Something like the below snippet. But perhaps there's a more appropriate fix.

diff --git a/src/platform/linux/graphics.h b/src/platform/linux/graphics.h
index 2a2a0642..997ee0f2 100644
--- a/src/platform/linux/graphics.h
+++ b/src/platform/linux/graphics.h
@@ -617,6 +617,11 @@ namespace egl {
           sd.fds[x] = -1;
         }
       }
+      if (data && data_allocated) {
+        delete[] data;
+      }
+      data = nullptr;
+      data_allocated = false;
     }
 
     surface_descriptor_t sd;  ///< DMA-BUF surface descriptor for the captured image.
@@ -632,6 +637,8 @@ namespace egl {
     std::optional<uint64_t> seq;  ///< PipeWire frame sequence number.
     std::optional<bool> pw_damage;  ///< Whether PipeWire damage tracking should be used.
     std::optional<uint32_t> pw_flags;  ///< PipeWire frame flags reported with the buffer.
+
+    bool data_allocated {false};
   };
 
   /**
diff --git a/src/platform/linux/pipewire.cpp b/src/platform/linux/pipewire.cpp
index fe71ff3b..7f53d6a4 100644
--- a/src/platform/linux/pipewire.cpp
+++ b/src/platform/linux/pipewire.cpp
@@ -1055,6 +1055,7 @@ namespace pipewire {
 
       img->data = new std::uint8_t[img->height * img->row_pitch];
       std::fill_n(img->data, img->height * img->row_pitch, 0);
+      static_cast<egl::img_descriptor_t*>(img)->data_allocated = true;
       return 0;
     }

[Kishi85]

I'm not sure why that was done, but more investigation actually points this specific leak issue to be isolated to dummy_img:

Sunshine/src/platform/linux/pipewire.cpp

Lines 1069 to 1077 in 3437a0b

	int dummy_img(platf::img_t *img) override {
	if (!img) {
	return -1;
	}
	img->data = new std::uint8_t[img->height * img->row_pitch];
	std::fill_n(img->data, img->height * img->row_pitch, 0);
	return 0;
	}

Reverting my previous changes and instead short-circuiting dummy_img (inserting return 0; at the start or just deleting the function contents) also resolves the memory leak with the vaapi encoder, and software encoding no longer crashes (and also doesn't leak). Only Vulkan leaks as-is, which I assume is a separate unrelated leak.

Edit: I won't be able to test thoroughly until later, but if it turns out that the destructor really does need to do cleanup, being able to distinguish the ownership between dummy_img()'s vs other img_descriptor_t allocations would probably resolve the double-free crashing. Something like the below snippet. But perhaps there's a more appropriate fix.

diff --git a/src/platform/linux/graphics.h b/src/platform/linux/graphics.h
index 2a2a0642..997ee0f2 100644
--- a/src/platform/linux/graphics.h
+++ b/src/platform/linux/graphics.h
@@ -617,6 +617,11 @@ namespace egl {
           sd.fds[x] = -1;
         }
       }
+      if (data && data_allocated) {
+        delete[] data;
+      }
+      data = nullptr;
+      data_allocated = false;
     }
 
     surface_descriptor_t sd;  ///< DMA-BUF surface descriptor for the captured image.
@@ -632,6 +637,8 @@ namespace egl {
     std::optional<uint64_t> seq;  ///< PipeWire frame sequence number.
     std::optional<bool> pw_damage;  ///< Whether PipeWire damage tracking should be used.
     std::optional<uint32_t> pw_flags;  ///< PipeWire frame flags reported with the buffer.
+
+    bool data_allocated {false};
   };
 
   /**
diff --git a/src/platform/linux/pipewire.cpp b/src/platform/linux/pipewire.cpp
index fe71ff3b..7f53d6a4 100644
--- a/src/platform/linux/pipewire.cpp
+++ b/src/platform/linux/pipewire.cpp
@@ -1055,6 +1055,7 @@ namespace pipewire {
 
       img->data = new std::uint8_t[img->height * img->row_pitch];
       std::fill_n(img->data, img->height * img->row_pitch, 0);
+      static_cast<egl::img_descriptor_t*>(img)->data_allocated = true;
       return 0;
     }

Both kmsgrab and wlgrab implement dummy_img() by just returning 0:

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 1685 to 1692 in 3437a0b

	* @brief Populate a fallback image when real capture data is unavailable.
	*
	* @param img Image or frame object to read from or populate.
	* @return Capture status reported to the streaming pipeline.
	*/
	int dummy_img(platf::img_t *img) override {
	return 0;
	}

Sunshine/src/platform/linux/kmsgrab.cpp

Lines 1762 to 1771 in 3437a0b

	/**
	* @brief Populate a fallback image when real capture data is unavailable.
	*
	* @param img Image or frame object to read from or populate.
	* @return Capture status reported to the streaming pipeline.
	*/
	int dummy_img(platf::img_t *img) override {
	// Empty images are recognized as dummies by the zero sequence number
	return 0;
	}

Sunshine/src/platform/linux/wlgrab.cpp

Lines 141 to 149 in 3437a0b

	/**
	* @brief Populate a fallback image when real capture data is unavailable.
	*
	* @param img Image or frame object to read from or populate.
	* @return Capture status reported to the streaming pipeline.
	*/
	int dummy_img(platf::img_t *img) override {
	return 0;
	}

So I'd say that pipewire.cpp should just do the same thing to keep things similar between linux capture methods. There seems to be no functional disadvantage from doing it this way.

[Kishi85]

@psyke83 I think you've found the main issues here. To keep things simple and in line with kmsgrab/wlgrab I've just added a subclass for those specific img_descriptor_t's. Also updated dummy_img() to what kmsgrab/wlgrab do and with that I'm not seeing any obvious memory leaks anymore using vaapi.

There might still be a small leak occurring but that would be somewhere affecting all capture methods including kmsgrab/wlgrab and a separate issue.

I also can confirm the memory leak in vulkan encoding as that is still happening albeit at half the previous increase (which was somewhat to be expected due to given behaviour).

I'll update the PR and mark this as ready unless you have something to add.

[sonarqubecloud]

Quality Gate failed

Failed conditions
1 New issue
1 New Code Smells (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE