Reusable buffers#10
Closed
jholveck wants to merge 13 commits into
Closed
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR introduces zero-copy reusable buffers on Python 3.12+ GNU/Linux by implementing a finalizing-buffer mechanism that invokes caller-supplied cleanup when buffers are released, refactoring XShmGetImage to pool shared-memory slots instead of using a single long-lived buffer, and updating the backend interface contract to enable buffer-like return types. ChangesBuffer reuse with zero-copy finalization
Sequence DiagramsequenceDiagram
participant Client
participant XShmGetImage
participant SHM_Pool
participant finalizing_buffer
participant Desktop
Client->>XShmGetImage: grab(monitor)
XShmGetImage->>SHM_Pool: _acquire_shm_slot(size)
SHM_Pool-->>XShmGetImage: ShmSlot allocated or from free list
XShmGetImage->>Desktop: xcb.shm_get_image(slot)
Desktop-->>XShmGetImage: image pixels in slot mmap
XShmGetImage->>finalizing_buffer: finalizing_buffer(memoryview, release_fn)
finalizing_buffer-->>XShmGetImage: memoryview with finalizer attached
XShmGetImage-->>Client: memoryview
Note over Client: Client reads/uses buffer
Client-->>finalizing_buffer: Release memoryview<br/>(on Python 3.12+ via __release_buffer__)
finalizing_buffer->>SHM_Pool: _release_shm_slot(slot)
SHM_Pool-->>finalizing_buffer: Slot returned to free list
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR introduces reusable/zero-copy buffer support (primarily for the GNU/Linux xshmgetimage backend) by returning buffer-protocol objects whose underlying SHM resources are released/recycled via Python 3.12+ buffer finalization.
Changes:
- Add
mss.buffer.finalizing_buffer()to provide finalizer-backed buffers on Python 3.12+ and a copy-based fallback on older versions. - Rework the Linux
xshmgetimagebackend to use a pool of SHM slots and returnmemoryviewbuffers instead of always copying into abytearray. - Update typing/tests/docs/packaging to reflect the new buffer module and
Buffer-basedgrab()API.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/mss/buffer.py |
Adds finalizer-backed buffer helper to enable safe reusable/zero-copy buffers on Python 3.12+. |
src/mss/linux/xshmgetimage.py |
Implements SHM slot pool + integrates finalizing_buffer to avoid per-grab copies and enable buffer reuse. |
src/mss/base.py |
Updates MSSImplementation.grab() return typing from bytearray to Buffer. |
src/mss/linux/base.py |
Adds commentary about a possible future zero-copy approach for XGetImage (no functional change). |
src/tests/test_gnu_linux.py |
Updates tests for finalizing-buffer behavior and adds a dynamic SHM pool growth failure test (3.12+). |
src/tests/test_setup.py |
Ensures mss/buffer.py is included in sdist and wheel contents. |
docs/source/usage.rst |
Documents buffer reuse/zero-copy behavior and expected performance impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+37
to
+41
| # Quick note (this should go in the commit log, or something, not source, but I had to record it somewhere): | ||
| # On my home box, at 4k resolution, doing 1000 grabs + NumPy sums of the pixel data, with/without reusable buffers: | ||
| # * Enabled: 18.6 ms / each | ||
| # * Disabled: 22.6 ms / each | ||
| # * Delta: 17.9% faster |
| mm: mmap | None = None | ||
| try: | ||
| try: | ||
| memfd = os.memfd_create("mss-shm-buf", flags=os.MFD_CLOEXEC) # type:ignore[attr-defined] |
| raise ScreenShotError(msg) from exc | ||
|
|
||
| try: | ||
| mm = mmap(memfd, size, prot=PROT_READ | PROT_WRITE) # type:ignore[call-arg] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tests/test_gnu_linux.py`:
- Around line 325-336: The test currently opens mss.MSS(...) inside the
pytest.raises context so __exit__ runs during exception unwinding and prevents
asserting the SHM free-list; change the test to create the MSS instance (mss.MSS
with backend="xshmgetimage") outside the pytest.raises block, monkeypatch
xshmgetimage.finalizing_buffer to raise, call sct.grab(monitor) inside
pytest.raises, then after the raised error assert that sct._free_shm_slots (or
call the helper that reports free slots) equals the pre-grab size to ensure
_release_shm_slot was called; also ensure the test uses a
pyvirtualdisplay.Display(...) like the other SHM tests (or otherwise set up
$DISPLAY) so an SHM-capable X server is available in CI.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b69e7251-b8ff-4c5d-a3e6-17a9b6cd911d
📒 Files selected for processing (7)
docs/source/usage.rstsrc/mss/base.pysrc/mss/buffer.pysrc/mss/linux/base.pysrc/mss/linux/xshmgetimage.pysrc/tests/test_gnu_linux.pysrc/tests/test_setup.py
Comment on lines
325
to
336
| def test_exception_while_wrapping_finalizing_buffer(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """Verify that wrapping failures still release the in-use SHM slot.""" | ||
|
|
||
| def boom(_data: memoryview, _finalizer: Any) -> memoryview: | ||
| msg = "Boom!" | ||
| raise RuntimeError(msg) | ||
|
|
||
| The particular point is the window when the XShmGetImage's mmapped | ||
| buffer has a memoryview still outstanding, and the pixel data is | ||
| being copied into a bytearray. This can take a few milliseconds. | ||
| """ | ||
| # Force an exception during bytearray(img_mv) | ||
| real_bytearray = builtins.bytearray | ||
|
|
||
| def boom(*args: list, **kwargs: dict[str, Any]) -> bytearray: | ||
| # Only explode when called with the memoryview (the code path we care about). | ||
| if len(args) > 0 and isinstance(args[0], memoryview): | ||
| # We still need to eliminate args from the stack frame, just like the fix. | ||
| del args, kwargs | ||
| msg = "Boom!" | ||
| raise RuntimeError(msg) | ||
| return real_bytearray(*args, **kwargs) | ||
|
|
||
| # We have to be careful about the order in which we catch things. If we were to catch and discard the exception | ||
| # before the MSS object closes, it won't trigger the bug. That's why we have the pytest.raises outside the | ||
| # mss.MSS block. In addition, we do as much as we can before patching bytearray, to limit its scope. | ||
| with pytest.raises(RuntimeError, match="Boom!"), mss.MSS(backend="xshmgetimage") as sct: # noqa: PT012 | ||
| monitor = sct.monitors[0] | ||
| with monkeypatch.context() as m: | ||
| m.setattr(builtins, "bytearray", boom) | ||
| m.setattr(mss.linux.xshmgetimage, "finalizing_buffer", boom) | ||
| sct.grab(monitor) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for any global/session/autouse display or xvfb setup that these tests depend on.
fd -t f 'conftest.py'
rg -nP -C3 '(autouse|DISPLAY|pyvirtualdisplay|xvfb|new_display_var)' $(fd -t f 'conftest.py') 2>/dev/null
# Compare how SHM tests obtain a display: sibling tests vs the two new tests.
rg -nP -C3 'backend="xshmgetimage"' --type=pyRepository: jholveck/python-mss
Length of output: 4692
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the SHM implementation: where _release_shm_slot and grab error handling live
fd -t f 'xshmgetimage.py' . | head -n 20 | cat
FILE="$(fd -t f 'xshmgetimage.py' . | head -n 1)"
echo "Using $FILE"
rg -n "def _release_shm_slot|_free_shm_slots|finalizing_buffer|def grab|try:|finally:" "$FILE" -n
echo "----"
# Print a small surrounding region for the key pieces
python3 - <<'PY'
import os, re, subprocess, textwrap, sys
file = subprocess.check_output(["bash","-lc","fd -t f 'xshmgetimage.py' . | head -n 1"], text=True).strip()
# Get candidate line numbers for key identifiers
keys = ["def _release_shm_slot", "_free_shm_slots", "finalizing_buffer", "def grab"]
out = subprocess.check_output(["bash","-lc", "rg -n \"("+"|".join(map(re.escape, keys))+")\" "+re.escape(file)], text=True)
print(out.strip())
PY
# Dump relevant sections with line numbers (avoid huge output)
sed -n '1,200p' "$FILE" | head -n 200 >/dev/null
# Heuristically print around occurrences
for ln in $(rg -n "def _release_shm_slot|_free_shm_slots|finalizing_buffer|def grab" "$FILE" | cut -d: -f1 | sort -u); do
start=$((ln-60)); end=$((ln+120));
echo "---- $FILE:$start-$end ----"
sed -n "${start},${end}p" "$FILE"
done
# 2) Confirm all uses of pyvirtualdisplay/xvfb/DISPLAY setup across tests
rg -n --hidden --no-ignore-vcs "(pyvirtualdisplay|xvfb|new_display_var|DISPLAY\s*=|os\.environ\['DISPLAY'\]|getenv\(\"DISPLAY\"\)|x11|Xvfb)" src/tests -S
# 3) Inspect the specific test(s) around the snippet for context and patterns
rg -n "test_exception_while_wrapping_finalizing_buffer|test_dynamic_shm_growth_allocation_failure_raises|test_shm_available|test_shm_fallback" src/tests/test_gnu_linux.py
sed -n '240,380p' src/tests/test_gnu_linux.py
# 4) Inspect conftest fully enough to see if any virtual display is created globally
sed -n '1,220p' src/tests/conftest.pyRepository: jholveck/python-mss
Length of output: 1920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print relevant sections from xshmgetimage.py (grab + release logic)
FILE="$(fd -t f 'xshmgetimage.py' src/mss/linux | head -n 1)"
echo "Using: $FILE"
echo "---- _release_shm_slot and _free_shm_slots ----"
rg -n -F "def _release_shm_slot" "$FILE"
sed -n '170,240p' "$FILE"
echo "---- grab() error handling + any finally ----"
rg -n -F "def grab" "$FILE"
rg -n -F "_release_shm_slot" "$FILE"
sed -n '280,380p' "$FILE"
# Display setup in tests
echo "---- test_gnu_linux.py virtual display usage ----"
sed -n '250,370p' src/tests/test_gnu_linux.py
rg -n -F "pyvirtualdisplay.Display" src/tests/test_gnu_linux.py
echo "---- display-related fixtures in conftest.py ----"
sed -n '1,220p' src/tests/conftest.py
sed -n '220,520p' src/tests/conftest.py
rg -n -F "pyvirtualdisplay|DISPLAY|xvfb|new_display_var" src/tests/conftest.pyRepository: jholveck/python-mss
Length of output: 16418
Fix test to actually assert the SHM free-list post-condition (and align DISPLAY setup)
- The test’s name/docstring promise a “release the in-use SHM slot” post-condition, but it currently only asserts
RuntimeErrorpropagation; whensct.grab(...)raises insidewith pytest.raises(...), mss.MSS(...) as sct,mss.MSS.__exit__runs during exception unwinding and performs SHM cleanup, so_free_shm_slotscan’t be meaningfully checked afterward. Sincexshmgetimage.grabcatches exceptions and calls_release_shm_slot(slot), move themss.MSS(...)context outsidepytest.raisesand assert_free_shm_slotsreturns to its pre-grab size after the failed grab. - Separately,
test_shm_available/test_shm_fallbackwrap inpyvirtualdisplay.Display(...), buttest_exception_while_wrapping_finalizing_buffer(andtest_dynamic_shm_growth_allocation_failure_raises) instantiatemss.MSS(backend="xshmgetimage")without a virtual display; these tests rely on ambient$DISPLAY(andconftest.pydoesn’t start a virtual display). Align them with the other SHM tests or ensure CI always provides an SHM-capable X server via$DISPLAY.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/tests/test_gnu_linux.py` around lines 325 - 336, The test currently opens
mss.MSS(...) inside the pytest.raises context so __exit__ runs during exception
unwinding and prevents asserting the SHM free-list; change the test to create
the MSS instance (mss.MSS with backend="xshmgetimage") outside the pytest.raises
block, monkeypatch xshmgetimage.finalizing_buffer to raise, call
sct.grab(monitor) inside pytest.raises, then after the raised error assert that
sct._free_shm_slots (or call the helper that reports free slots) equals the
pre-grab size to ensure _release_shm_slot was called; also ensure the test uses
a pyvirtualdisplay.Display(...) like the other SHM tests (or otherwise set up
$DISPLAY) so an SHM-capable X server is available in CI.
These are straight from the AI, but I want to commit them to start testing in other versions of Python.
Adds a test for the new buffer module. The test is pretty basic, but it at least verifies that the buffer module is working and that the buffers it creates are usable by outside modules. This requires adding NumPy to the [tests] optional-dependencies group for all versions of Python, since that's the buffer consumer we use in the tests.
In my test cleanup and expansion, I accidentally included an assumption that's only valid on 3.12+.
We already had tests for the finalizing_buffer behavior, but this makes sure it survives at the higher level MSS API.
The buffer finalizers can run at any time, from any thread. This could even happen while the XCB connection is being closed. While it's hard to imagine, it could happen from the same thread that's closing the XCB connection, given some crazy weak references or something. Make sure that we don't call shm_detach from a finalizer at the same time as we're closing the XCB connection. If we're about to close the connection, even from a different thread than the one that is running the finalizer, there's no need to call shm_detach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this PR
Fixes #
(...)
./check.shpassedSummary by CodeRabbit
New Features
Documentation
Tests