Add Sound.copy and Sound.__copy__ #3556
Conversation
oddbookworm
added
New API
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds Sound.copy() and Sound.copy to pygame.mixer.Sound with a C implementation that clones a Sound from its raw buffer (preserving volume), updates type stubs and docs, adjusts mixer cleanup semantics, and adds tests verifying copy parity, independence, playback, and subclass preservation. Changes
Sequence Diagram(s)sequenceDiagram
participant Py as Python
participant Sound as Sound (self)
participant C as mixer.c (snd_copy)
participant Raw as raw buffer
participant New as New Sound
Py->>Sound: call copy() / __copy__()
Sound->>C: invoke snd_copy(self)
C->>Sound: snd_get_raw(self)
Sound-->>Raw: return buffer/bytes
C->>New: allocate new Sound (tp_new)
C->>New: initialize from buffer via sound_init
C->>New: set volume to source volume
C-->>Py: return New
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
buildconfig/stubs/pygame/mixer.pyi (1)
75-76: Use Self as the return type to preserve subclass typing for copy/copyAcross pygame stubs, copy() methods usually return Self (see Surface, Rect, etc.). For consistency and better type inference when subclassing Sound, prefer Self here. If you keep returning Sound, copying a subclass will be typed as base Sound.
Apply this diff:
-from typing_extensions import ( - Buffer, # collections.abc 3.12 - deprecated, # added in 3.13 -) +from typing_extensions import ( + Buffer, # collections.abc 3.12 + deprecated, # added in 3.13 + Self, +) @@ - def copy(self) -> Sound: ... - def __copy__(self) -> Sound: ... + def copy(self) -> Self: ... + def __copy__(self) -> Self: ...docs/reST/ref/mixer.rst (1)
507-523: Clarify subclass behavior and add copy/deepcopy note if applicableIf we keep or change the implementation to preserve subclass type on copy (recommended), consider adding a sentence: “The returned object has the same class as the original.” If implementing deepcopy to delegate to copy(), mirror the Surface docs and add that note here as well.
Proposed doc tweak (only if subclass type is preserved):
- Return a new Sound object that is a deep copy of this Sound. The new Sound - will be just as if you loaded it from the same file on disk as you did the - original Sound. If the copy fails, a ``TypeError`` or :meth:`pygame.error` - exception will be raised. + Return a new Sound object that is a deep copy of this Sound. The returned + object will have the same class as the original and be just as if you + loaded it from the same file on disk. If the copy fails, a ``TypeError`` + or :meth:`pygame.error` exception will be raised.src_c/mixer.c (1)
895-897: Consider supporting deepcopy for parity with Surface and Python’s copy protocolOptional but nice-to-have: implement deepcopy to delegate to copy(), like Surface does, so copy.copy() and copy.deepcopy() both work intuitively.
Add this wrapper function (outside current hunk):
// Add near snd_copy static PyObject * snd_deepcopy(PyObject *self, PyObject *memo) { // ignore memo, perform a deep copy identical to copy() return snd_copy(self, NULL); }Then add this entry to sound_methods:
{"copy", snd_copy, METH_NOARGS, DOC_MIXER_SOUND_COPY}, {"__copy__", snd_copy, METH_NOARGS, DOC_MIXER_SOUND_COPY}, + {"__deepcopy__", snd_deepcopy, METH_O, DOC_MIXER_SOUND_COPY},And update the stubs and docs accordingly, if adopted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildconfig/stubs/pygame/mixer.pyi(1 hunks)docs/reST/ref/mixer.rst(2 hunks)src_c/doc/mixer_doc.h(1 hunks)src_c/mixer.c(3 hunks)test/mixer_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
buildconfig/stubs/pygame/mixer.pyi (7)
buildconfig/stubs/pygame/sprite.pyi (1)
copy(142-142)buildconfig/stubs/pygame/surface.pyi (1)
copy(335-346)buildconfig/stubs/pygame/math.pyi (1)
copy(62-62)buildconfig/stubs/pygame/mask.pyi (1)
copy(21-21)buildconfig/stubs/pygame/cursors.pyi (1)
copy(83-83)buildconfig/stubs/pygame/geometry.pyi (2)
copy(138-138)copy(175-175)buildconfig/stubs/pygame/rect.pyi (1)
copy(152-152)
test/mixer_test.py (3)
src_c/mixer.c (3)
mixer(2088-2179)get_num_channels(1455-1460)get_busy(1509-1517)buildconfig/stubs/pygame/mixer.pyi (20)
init(16-23)get_sdl_mixer_version(46-46)Sound(48-76)copy(75-75)get_length(73-73)get_num_channels(40-40)get_num_channels(72-72)get_volume(71-71)get_volume(99-99)get_raw(74-74)set_volume(70-70)set_volume(96-96)set_volume(98-98)play(53-58)play(82-88)get_busy(45-45)get_busy(100-100)stop(35-35)stop(68-68)stop(89-89)test/test_utils/__init__.py (1)
example_path(36-37)
🪛 Flake8 (7.2.0)
test/mixer_test.py
[error] 1350-1350: local variable 'e' is assigned to but never used
(F841)
🔇 Additional comments (4)
src_c/doc/mixer_doc.h (1)
29-29: Docs addition looks good and matches the new APIThe macro clearly documents both Sound.copy() and copy.copy(original_sound), aligned with the implementation.
test/mixer_test.py (1)
1333-1372: Good coverage of Sound.copy parity and independenceThe test exercises core invariants: length, channels, volume, raw bytes parity; volume independence; basic playback on the copy; and conditional MP3 coverage. Looks solid.
docs/reST/ref/mixer.rst (1)
372-375: Nice clarification about bytes(Sound) and buffer interfaceThis note preempts confusion about bytes and explains the existing buffer interface well.
src_c/mixer.c (1)
92-94: Forward declaration is fineAdding the forward declaration fixes ordering and avoids compiler warnings when referencing the getter in sound_getset.
|
@coderabbitai please review the changes now |
|
Tip For best results, initiate chat on the files or code changes. @oddbookworm I'll provide a comprehensive review of your Sound.copy implementation. This is a well-structured PR that adds copy functionality to pygame.mixer.Sound objects. 🎯 Overall AssessmentExcellent implementation! The PR demonstrates solid software engineering practices with comprehensive coverage across type stubs, documentation, C implementation, and tests. 📋 Detailed Review✅ Type Stubs (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src_c/mixer.c (1)
298-305: Refcount leak in endsound_callback: missing DECREF of current soundWhen a channel finishes or dequeues to the next sound, channeldata[channel].sound is reassigned or set to NULL without a corresponding Py_XDECREF, leaking a reference per playback.
Apply this diff:
if (channeldata[channel].queue) { PyGILState_STATE gstate = PyGILState_Ensure(); int channelnum; Mix_Chunk *sound = pgSound_AsChunk(channeldata[channel].queue); + Py_XDECREF(channeldata[channel].sound); channeldata[channel].sound = channeldata[channel].queue; channeldata[channel].queue = NULL; PyGILState_Release(gstate); channelnum = Mix_PlayChannelTimed(channel, sound, 0, -1); if (channelnum != -1) { Mix_GroupChannel(channelnum, (int)(intptr_t)sound); } } else { PyGILState_STATE gstate = PyGILState_Ensure(); + Py_XDECREF(channeldata[channel].sound); channeldata[channel].sound = NULL; PyGILState_Release(gstate); Mix_GroupChannel(channel, -1); }Also applies to: 311-315
♻️ Duplicate comments (2)
src_c/mixer.c (2)
810-812: Fix subclass-unsafe tp_new call: pass Py_TYPE(self) to tp_new, and NULL-check the allocationUsing the subclass’s tp_new with the base type argument forces base-type allocation and can drop subclass type. Also, check allocation failure.
Apply this diff:
- pgSoundObject *newSound = - (pgSoundObject *)Py_TYPE(self)->tp_new(&pgSound_Type, NULL, NULL); + pgSoundObject *newSound = + (pgSoundObject *)Py_TYPE(self)->tp_new(Py_TYPE(self), NULL, NULL); + if (newSound == NULL) { + // tp_new has already set an exception + return NULL; + }
813-836: Handle allocation failures, simplify kwargs insertion, and fix newSound leak on error paths
- kwargs is not checked for NULL (OOM).
- Creating a separate key object is unnecessary; use PyDict_SetItemString.
- On errors (bytes == NULL or dict insertion failure), newSound is leaked; DECREF it.
Apply this diff:
- PyObject *kwargs = PyDict_New(); - PyObject *key = PyUnicode_FromString("buffer"); - PyObject *bytes = snd_get_raw(self, NULL); + PyObject *kwargs = PyDict_New(); + if (kwargs == NULL) { + Py_DECREF(newSound); + return NULL; + } + PyObject *bytes = snd_get_raw(self, NULL); if (bytes == NULL) { - // exception set already by PyBytes_FromStringAndSize + // exception set already by snd_get_raw / PyBytes_FromStringAndSize PG_SAVE_EXCEPTION - Py_DECREF(key); Py_DECREF(kwargs); + Py_DECREF(newSound); PG_UNSAVE_EXCEPTION return NULL; } - if (PyDict_SetItem(kwargs, key, bytes) < 0) { + if (PyDict_SetItemString(kwargs, "buffer", bytes) < 0) { // exception set already PG_SAVE_EXCEPTION - Py_DECREF(key); Py_DECREF(bytes); Py_DECREF(kwargs); + Py_DECREF(newSound); PG_UNSAVE_EXCEPTION return NULL; } - Py_DECREF(key); Py_DECREF(bytes);
🧹 Nitpick comments (1)
src_c/mixer.c (1)
907-909: Method wiring LGTM; consider adding a subclass retention testcopy and copy entries are correctly exposed and share the same implementation/doc. Given the tp_new fix above, please add a regression test ensuring that copying a subclass of Sound returns an instance of the same subclass.
Example test sketch:
- Define a trivial subclass of pygame.mixer.Sound
- Construct from a small in-memory buffer
- Assert type(copy()) is the subclass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src_c/mixer.c(3 hunks)test/mixer_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/mixer_test.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/mixer.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: AMD64
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: x86_64
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: aarch64
- GitHub Check: i686
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: dev-check
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: build (macos-14)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: build (windows-latest)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (1)
src_c/mixer.c (1)
92-93: Forward declaration looks fineThe forward declaration for snd_get_arraystruct matches the implementation signature below and avoids implicit declaration warnings. No issues.
Co-authored-by: Borishkof <[email protected]>
oddbookworm
force-pushed
the
sound-copy-bytes
branch
from
12c9952 to
9f82126
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
buildconfig/stubs/pygame/mixer.pyi (1)
75-76: Return Self for copy/copy to reflect subtype-preserving semanticsThe runtime implementation preserves the original subclass type by using
Py_TYPE(self)->tp_new(...). The stubs should model that by returningSelfinstead ofSound.Apply this diff:
- def copy(self) -> Sound: ... - def __copy__(self) -> Sound: ... + def copy(self) -> Self: ... + def __copy__(self) -> Self: ...And add
Selfto the imports (outside this hunk), for example:from typing_extensions import Buffer, deprecated, Self # add Selftest/mixer_test.py (2)
1352-1356: Ensure all filenames are exercised even when MP3 is added
zip(filenames, old_volumes, new_volumes)truncates to the shortest sequence. When MP3 is appended, the last file isn’t tested. Iterate by index or cycle volumes so every file is covered.One simple refactor:
- for f, old_vol, new_vol in zip(filenames, old_volumes, new_volumes): - filename = example_path(os.path.join("data", f)) + for i, f in enumerate(filenames): + filename = example_path(os.path.join("data", f)) + old_vol = old_volumes[i % len(old_volumes)] + new_vol = new_volumes[i % len(new_volumes)]Apply the same change to the subclass loop below.
1333-1409: Solid coverage of copy semantics and independence
- Validates length, channels, volume, and raw data parity between original and copy.
- Confirms independent volume after mutation.
- Exercises playback on the copy and ensures channel accounting is sane.
- Extends coverage to a
Soundsubclass.Consider adding a quick check for
copy.copy(sound)to exercise__copy__as well.Example addition outside this hunk:
import copy # ... cpy = copy.copy(sound) self.assertIsInstance(cpy, mixer.Sound) self.assertEqual(sound.get_raw(), cpy.get_raw())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
buildconfig/stubs/pygame/mixer.pyi(1 hunks)docs/reST/ref/mixer.rst(2 hunks)src_c/doc/mixer_doc.h(1 hunks)src_c/mixer.c(4 hunks)test/mixer_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src_c/doc/mixer_doc.h
- docs/reST/ref/mixer.rst
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
test/mixer_test.pybuildconfig/stubs/pygame/mixer.pyisrc_c/mixer.c
🧬 Code Graph Analysis (1)
test/mixer_test.py (3)
buildconfig/stubs/pygame/mixer.pyi (17)
Sound(48-76)init(16-23)set_volume(70-70)set_volume(96-96)set_volume(98-98)copy(75-75)get_length(73-73)get_num_channels(40-40)get_num_channels(72-72)get_volume(71-71)get_volume(99-99)get_raw(74-74)play(53-58)play(82-88)stop(35-35)stop(68-68)stop(89-89)test/test_utils/__init__.py (1)
example_path(36-37)buildconfig/stubs/pygame/base.pyi (1)
error(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Debian (Bookworm - 12) [build - armv7, test - armv6]
- GitHub Check: x86_64
- GitHub Check: arm64
🔇 Additional comments (4)
src_c/mixer.c (4)
92-94: LGTM: forward declaration is fineThe static forward declaration for
snd_get_arraystructis appropriate and matches the later definition.
852-860: Nice touch: copy preserves the original chunk’s volumeMirroring the source volume onto the new chunk maintains parity and matches user expectations. Good call.
861-863: Confirm refcounting: no extra INCREF should be needed when returning newSoundReturning the result of
tp_newwith refcount 1 is the normal pattern; an extra INCREF here would leak. However, a prior debug build report mentioned undercount on 3.14 rc. Please re-validate on your target build to ensure no premature deallocation occurs before merging.If you still observe undercounting with a Python debug build:
- Share a minimal reproducer that drops the copy too early.
- We can revisit whether an INCREF is warranted in this specific code path.
922-924: Public API surface looks correctExposing both
copy()and__copy__()to back the Python copy protocol is consistent and ergonomic.
src_c/mixer.c
Outdated
| if (channeldata) { | ||
| for (i = 0; i < numchanneldata; ++i) { | ||
| Py_XDECREF(channeldata[i].sound); | ||
| if (channeldata[i].queue) { | ||
| Mix_HaltGroup( | ||
| (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); | ||
| } | ||
| else { | ||
| Mix_HaltGroup(-1); | ||
| } | ||
| Py_XDECREF(channeldata[i].queue); |
There was a problem hiding this comment.
Leak: channeldata[i].sound is never decref’d in mixer_quit
Inside the loop, channeldata[i].sound holds a reference to a Python object. The code decrefs queue but never decrefs sound, leaking a ref per channel. Add Py_XDECREF(channeldata[i].sound); before freeing channeldata.
Apply this diff:
if (channeldata[i].queue) {
Mix_HaltGroup(
(int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
}
else {
Mix_HaltGroup(-1);
}
+ Py_XDECREF(channeldata[i].sound);
Py_XDECREF(channeldata[i].queue);Nit (optional): Calling Mix_HaltGroup(-1) inside the loop repeats the same work N times; you can do it once outside the loop for the non-queue case.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (channeldata) { | |
| for (i = 0; i < numchanneldata; ++i) { | |
| Py_XDECREF(channeldata[i].sound); | |
| if (channeldata[i].queue) { | |
| Mix_HaltGroup( | |
| (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); | |
| } | |
| else { | |
| Mix_HaltGroup(-1); | |
| } | |
| Py_XDECREF(channeldata[i].queue); | |
| if (channeldata) { | |
| for (i = 0; i < numchanneldata; ++i) { | |
| if (channeldata[i].queue) { | |
| Mix_HaltGroup( | |
| (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); | |
| } | |
| else { | |
| Mix_HaltGroup(-1); | |
| } | |
| Py_XDECREF(channeldata[i].sound); | |
| Py_XDECREF(channeldata[i].queue); |
🤖 Prompt for AI Agents
In src_c/mixer.c around lines 518 to 527, the loop in mixer_quit decrefs
channeldata[i].queue but never decrefs channeldata[i].sound, leaking one Python
reference per channel; add a Py_XDECREF(channeldata[i].sound) for each iteration
before you free channeldata. Also, to avoid repeating work, call
Mix_HaltGroup(-1) once outside the loop for the non-queue case instead of
calling it on every iteration. Ensure the added Py_XDECREF is used (not
Py_DECREF) to safely handle NULL values.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src_c/mixer.c (1)
520-527: Fix refcount leak in mixer_quit: DECREF channeldata[i].soundWe halt groups and DECREF queue, but never decref the per-channel Python Sound reference. This leaks one Python ref per channel on quit.
Apply this diff:
if (channeldata[i].queue) { Mix_HaltGroup( (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); } else { Mix_HaltGroup(-1); } + Py_XDECREF(channeldata[i].sound); Py_XDECREF(channeldata[i].queue);Optional nit: Mix_HaltGroup(-1) inside the loop repeats work N times. Consider hoisting it outside the loop for the non-queue case.
🧹 Nitpick comments (2)
src_c/mixer.c (1)
810-879: Avoid double-copying large audio buffers (optional)Current flow copies audio data twice: (1) Sound -> PyBytes, (2) PyBytes -> new Mix_Chunk memory. If you want to optimize large-copy paths, you could allocate the destination chunk and memcpy directly from orig->abuf. That would keep deep-copy semantics but with one fewer allocation/copy.
If you want, I can sketch a variant that bypasses the intermediate PyBytes and calls a small helper parallel to _chunk_from_buf.
test/mixer_test.py (1)
1334-1441: Thorough coverage; consider guarding playback asserts under “disk” driver and using subTestsGreat validation across formats, method paths (copy() and copy.copy()), and subclass preservation. Two small improvements to reduce flakiness and improve diagnostics:
- The “disk” audio driver can make get_busy() assertions unreliable. Guard the playback assertions when SDL_AUDIODRIVER=disk (similar to other tests).
- Use subTest per file to pinpoint failures faster.
Apply this minimal guard in each playback block:
- channel = sound_copy.play() + channel = sound_copy.play() if channel is None: continue + if os.environ.get("SDL_AUDIODRIVER") == "disk": + continue self.assertTrue(channel.get_busy())Optionally wrap each (f, old_vol, new_vol) iteration with self.subTest(file=f) for clearer failure reporting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
buildconfig/stubs/pygame/mixer.pyi(2 hunks)src_c/mixer.c(4 hunks)test/mixer_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- buildconfig/stubs/pygame/mixer.pyi
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
test/mixer_test.pysrc_c/mixer.c
🧬 Code Graph Analysis (1)
test/mixer_test.py (3)
buildconfig/stubs/pygame/mixer.pyi (18)
copy(76-76)Sound(49-77)init(17-24)get_sdl_mixer_version(47-47)set_volume(71-71)set_volume(97-97)set_volume(99-99)get_length(74-74)get_num_channels(41-41)get_num_channels(73-73)get_volume(72-72)get_volume(100-100)get_raw(75-75)play(54-59)play(83-89)stop(36-36)stop(69-69)stop(90-90)test/test_utils/__init__.py (1)
example_path(36-37)buildconfig/stubs/pygame/base.pyi (1)
error(6-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: arm64 deps
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: i686
- GitHub Check: build (macos-14)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: AMD64
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: dev-check
- GitHub Check: x86
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: build (ubuntu-22.04)
🔇 Additional comments (4)
src_c/mixer.c (3)
92-93: Forward declaration is appropriateAdding the static forward declaration for snd_get_arraystruct resolves ordering/visibility and avoids implicit-int-style warnings. Looks good.
810-879: Sound.copy implementation is correct and robustHighlights:
- Preserves subclass type via Py_TYPE(self)->tp_new — good.
- Proper NULL checks and full error-path cleanup (no leaks).
- Uses snd_get_raw + sound_init(buffer=...) to deep-copy bytes — simple and consistent.
- Preserves original chunk volume on the copy — nice parity touch.
- Correctly returns the new reference without an extra INCREF.
Re: refcounting — returning (PyObject *)newSound after tp_new + successful init is correct; adding a Py_INCREF here would leak a reference.
938-940: API surface wiring for copy/copy looks correctMethod table exports both .copy() and copy to Python with the shared implementation and appropriate doc macro. Good addition and Python protocol compliance.
test/mixer_test.py (1)
1-1: LGTM: needed import for copy.copyImport is required for exercising copy via copy.copy(). All good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src_c/mixer.c (1)
520-528: Leak: channeldata[i].sound is never decref’d in mixer_quitThe loop decrefs
queuebut notsound, leaking a Python ref per channel. Add a DECREF forchanneldata[i].soundon each iteration.Apply this minimal fix:
if (channeldata[i].queue) { Mix_HaltGroup( (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); } else { Mix_HaltGroup(-1); } + Py_XDECREF(channeldata[i].sound); Py_XDECREF(channeldata[i].queue);Optional nit: calling
Mix_HaltGroup(-1)once before the loop (instead of in every else) avoids redundant work. I can draft that change if you’d like.
🧹 Nitpick comments (1)
src_c/mixer.c (1)
816-886: Sound.copy implementation: preserves subclass, propagates errors, and keeps volume — LGTMHighlights:
- Constructs via
Py_TYPE(self)->tp_newto preserve subclass type.- Robust error handling and cleanup across all failure paths (including
snd_get_rawandPyDict_SetItemString).- Calls
sound_initwith a buffer kwarg; exceptions are propagated (good use of PG_SAVE/UNSAVE).- Copies
Mix_Chunkvolume from original to copy to maintain parity.- Returns a new reference without leaking.
Minor nit only: consider renaming
dicttokwargsfor readability since it’s used as keyword args forsound_init. No functional change needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src_c/mixer.c(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/mixer.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: build (macos-14)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: build (windows-latest)
- GitHub Check: aarch64
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: dev-check
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
🔇 Additional comments (4)
src_c/mixer.c (4)
92-93: Forward declaration for snd_get_arraystruct looks correctPrototype matches the definition later; unblocks usage in sound_getset. No issues.
291-319: Switch to Py_DECREF in endsound_callback is safe hereGiven queue is only non-NULL when a sound is currently playing (and callback fires on channel end),
channeldata[channel].soundshould never be NULL in either branch. Using Py_DECREF avoids silently ignoring logic errors. Good tightening.If you want to be extra defensive in debug builds, consider an assertion before Py_DECREF to document this invariant.
944-946: Expose both copy() and copy() in the method table — good API ergonomicsThis aligns with Python’s copy protocol and keeps the direct method. Ensure docs/stubs reflect Self returns (which your PR notes indicate). Looks good.
681-685: Confirmed: Mix_HaltGroup performs synchronous DECREF via endsound_callbackVerification of the SDL_mixer documentation shows that Mix_HaltChannel (and by extension Mix_HaltGroup) invokes any registered Mix_ChannelFinished callback for halted channels before the halt call returns, running under the audio lock. This callback (endsound_callback) will perform the DECREF, so removing the direct DECREF here is safe and will not leak or race.
No code changes needed.
oddbookworm
force-pushed
the
sound-copy-bytes
branch
from
b3ff4cc to
4e11fad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src_c/mixer.c (1)
522-529: Leak on shutdown: channeldata[i].sound is never decref’d (and redundant group halts in loop)The loop DECREFs
queuebut notsound. This leaks one Python ref per channel on mixer quit. Also, callingMix_HaltGroup(-1)inside the loop repeats the same global halt N times (minor perf nit).Apply at least the DECREF fix; consider hoisting the global halt outside the loop.
Proposed diff:
if (channeldata[i].queue) { Mix_HaltGroup( (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); } else { Mix_HaltGroup(-1); } + Py_XDECREF(channeldata[i].sound); Py_XDECREF(channeldata[i].queue);Nit (optional): Move
Mix_HaltGroup(-1)out of the loop and call it once for the non-queue case.
🧹 Nitpick comments (1)
src_c/mixer.c (1)
302-314: Switch to Py_DECREF assumes non-NULL; safeguard or assert invariant
endsound_callbacklikely runs only when a Sound was playing, sochanneldata[channel].soundshould be non-NULL. If that invariant can ever be violated (edge cases, future changes),Py_DECREF(NULL)will crash.If you want robustness without sacrificing intent, either add an assert or use
Py_XDECREFhere.Proposed diff:
- Py_DECREF(channeldata[channel].sound); + Py_XDECREF(channeldata[channel].sound); ... - Py_DECREF(channeldata[channel].sound); + Py_XDECREF(channeldata[channel].sound);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src_c/mixer.c(7 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/mixer.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: dev-check
- GitHub Check: msys2 (clang64, clang-x86_64)
- GitHub Check: msys2 (ucrt64, ucrt-x86_64)
- GitHub Check: msys2 (mingw64, x86_64)
- GitHub Check: build (ubuntu-22.04)
- GitHub Check: build (ubuntu-24.04)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: build (macos-14)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
🔇 Additional comments (3)
src_c/mixer.c (3)
92-93: Good forward declaration to satisfy prototype usageAdding the
snd_get_arraystructprototype avoids implicit declarations when referenced earlier (e.g., in getset) and keeps C99/C11 compilers happy.
816-885: snd_copy: Solid implementation (subclass-preserving, safe error paths, preserves volume)Highlights:
- Preserves subclass type via
Py_TYPE(self)->tp_new.- Robust NULL/error checks with proper cleanup and exception preservation.
- Uses
snd_get_raw+sound_initand then mirrors the original chunk’s volume onto the copy.Returning
newSoundwithout an extra INCREF is correct —tp_newreturns a new reference andtp_initdoes not consume it.
944-946: Method table wiring for copy and copy looks goodExposes both
copyand__copy__backed by the same implementation with proper doc macro.
oddbookworm
force-pushed
the
sound-copy-bytes
branch
from
19a3b7e to
0a65766
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src_c/mixer.c (2)
302-306: Double-check DECREF safety in endsound_callbackThese paths now unconditionally Py_DECREF(channeldata[channel].sound). In practice, this should always be non-NULL when the callback fires, but if an edge case slips through, this will crash in release builds.
- If you want the safety of guarding against unexpected NULL without changing semantics, prefer Py_XDECREF here; or
- Keep Py_DECREF but add an assert to catch invariants in debug builds.
Apply this minimal safety tweak if you prefer robustness over strictness:
- Py_DECREF(channeldata[channel].sound); + Py_XDECREF(channeldata[channel].sound);And in the else-path:
- Py_DECREF(channeldata[channel].sound); + Py_XDECREF(channeldata[channel].sound);Also applies to: 312-317
829-839: Nit: kwargs namingdict here is acting as kwargs for sound_init. Consider renaming to kwargs for clarity, in line with surrounding code.
- PyObject *dict = PyDict_New(); + PyObject *kwargs = PyDict_New();And adjust subsequent references accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src_c/mixer.c(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/mixer.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: dev-check
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [s390x]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
🔇 Additional comments (7)
src_c/mixer.c (7)
92-93: Forward declaration looks goodDeclaring snd_get_arraystruct before use eliminates prototype warnings and keeps the getset table cleanly typed.
818-827: Subclass-preserving allocation and error handling: LGTMUsing Py_TYPE(self)->tp_new preserves subclass type. The NULL checks and error setting are correct and defensive.
840-858: Robust error paths on snd_get_raw and dict populationGood use of PG_SAVE_EXCEPTION/PG_UNSAVE_EXCEPTION and proper DECREF on all failure paths. Using PyDict_SetItemString avoids a temporary key allocation.
861-869: Propagate sound_init errors correctlyThe code cleanly propagates sound_init’s exception and avoids masking with a secondary error. DECREF ordering is correct.
870-877: Nice: preserve original chunk volumeMirroring the chunk volume keeps copy() semantically aligned with the original Sound state. Good call.
879-881: Refcount on return: please re-verify whether an extra INCREF is requiredReturning a newly created PyObject normally should not require an extra Py_INCREF. However, previous discussion on this PR mentioned undercounting observed with a debug build (Py 3.14.0rc1) unless an extra INCREF was added in snd_copy. Current code returns without INCREF.
Before we lock this in, can you re-run the failing scenario you had previously and confirm that no premature deallocation occurs with the current implementation? If it still undercounts, we need to add a balanced INCREF; if not, we should keep it as-is to avoid a leak.
If verification indicates undercounting, apply:
- Py_DECREF(dict); - return (PyObject *)newSound; + Py_DECREF(dict); + Py_INCREF(newSound); // Only if undercount is reproducible; otherwise omit + return (PyObject *)newSound;Please share the exact reproducer (Python version, debug flags, minimal script) if you still observe the undercount so we can document the rationale in a code comment.
940-942: Method registrations for copy and copy look correctBoth methods mapped to the same implementation with shared docs; aligns with Python’s copy protocol.
| if (channeldata) { | ||
| for (i = 0; i < numchanneldata; ++i) { | ||
| Py_XDECREF(channeldata[i].sound); | ||
| Py_XDECREF(channeldata[i].queue); | ||
| if (channeldata[i].sound) { | ||
| Mix_HaltGroup( | ||
| (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); | ||
| } | ||
| if (channeldata[i].queue) { | ||
| Mix_HaltGroup( | ||
| (int)(intptr_t)pgSound_AsChunk(channeldata[i].queue)); | ||
| } |
There was a problem hiding this comment.
Leak: channeldata[i].sound and .queue never DECREF’d in mixer_quit
Each channel can hold references to Python Sound objects in both fields. The loop halts groups but never releases those Python references, leaking one ref per non-NULL field per channel.
Proposed fix:
if (channeldata) {
for (i = 0; i < numchanneldata; ++i) {
- if (channeldata[i].sound) {
- Mix_HaltGroup(
- (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
- }
- if (channeldata[i].queue) {
- Mix_HaltGroup(
- (int)(intptr_t)pgSound_AsChunk(channeldata[i].queue));
- }
+ if (channeldata[i].sound) {
+ Mix_HaltGroup(
+ (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
+ Py_DECREF(channeldata[i].sound);
+ channeldata[i].sound = NULL;
+ }
+ if (channeldata[i].queue) {
+ Mix_HaltGroup(
+ (int)(intptr_t)pgSound_AsChunk(channeldata[i].queue));
+ Py_DECREF(channeldata[i].queue);
+ channeldata[i].queue = NULL;
+ }
}
free(channeldata);Optional: Instead of per-chunk Mix_HaltGroup inside the loop, a single Mix_HaltChannel(-1) or Mix_HaltGroup(-1) outside the loop would be more efficient; but the above keeps current behavior intact.
🤖 Prompt for AI Agents
In src_c/mixer.c around lines 520 to 529, the loop halts groups for
channeldata[i].sound and channeldata[i].queue but never Py_DECREFs those Python
Sound object references; add Py_XDECREF(channeldata[i].sound) and
Py_XDECREF(channeldata[i].queue) immediately after the Mix_HaltGroup calls (and
set the fields to NULL to avoid dangling references). Ensure you include
Python.h reference semantics (use Py_XDECREF to handle NULL safely) and keep the
existing Mix_HaltGroup behavior; alternatively you may replace the per-item
halting with a single Mix_HaltChannel(-1)/Mix_HaltGroup(-1) outside the loop but
still perform the DECREFs inside the loop if you retain the current iteration.
There was a problem hiding this comment.
How about not changing this block of code, but doing Mix_ChannelFinished(NULL) (disable the callback and let this function do the cleaning as intended) just when mixer quit is called, does that solve this issue? I believe that would be the cleanest fix to this problem.
There was a problem hiding this comment.
🧩 Analysis chain
@ankith26, that's an insightful suggestion! Let me analyze this approach:
Your suggestion to call Mix_ChannelFinished(NULL) to disable the callback has merit, but there are a few considerations:
-
Timing issue: The
Mix_HaltGroup()calls are synchronous, but the callback cleanup would be asynchronous. Sincemixer_quitneeds to immediately freechanneldata, we can't rely on asynchronous cleanup. -
Original behavior: Looking at the diff, the original code actually had
Py_XDECREF()calls that were replaced withMix_HaltGroup()calls. The issue is that the DECREFs were removed entirely. -
Callback vs direct cleanup: The
endsound_callbackdoes handle proper cleanup withPy_XDECREF, but it's designed for normal channel completion, not shutdown.
I think the cleanest solution would be a hybrid approach:
// Disable callback to prevent interference during shutdown
Mix_ChannelFinished(NULL);
if (channeldata) {
for (i = 0; i < numchanneldata; ++i) {
if (channeldata[i].sound) {
Mix_HaltGroup((int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
Py_XDECREF(channeldata[i].sound);
}
if (channeldata[i].queue) {
Mix_HaltGroup((int)(intptr_t)pgSound_AsChunk(channeldata[i].queue));
Py_XDECREF(channeldata[i].queue);
}
}
free(channeldata);
channeldata = NULL;
numchanneldata = 0;
}This combines your suggestion (disabling the callback) with restoring the reference management that was inadvertently removed. What do you think about this approach?
🏁 Script executed:
#!/bin/bash
# Let's look at the endsound_callback function to understand how it handles cleanup
ast-grep --pattern 'static void endsound_callback($_) {
$$$
}'
Length of output: 66
@ankith26, thank you for the clarification! Now I see the full picture. Looking at the endsound_callback function, you're absolutely right that calling Mix_ChannelFinished(NULL) would be the cleanest approach. Here's why:
The endsound_callback properly handles Py_XDECREF for both sound and queue fields, but there's a race condition risk: if Mix_HaltGroup() triggers the callback asynchronously, it could access channeldata after mixer_quit has already freed it.
Your suggestion to disable the callback first eliminates this race condition. The complete fix would be:
// Disable callback to prevent race conditions during shutdown
Mix_ChannelFinished(NULL);
if (channeldata) {
for (i = 0; i < numchanneldata; ++i) {
if (channeldata[i].sound) {
Mix_HaltGroup((int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
Py_XDECREF(channeldata[i].sound);
}
if (channeldata[i].queue) {
Mix_HaltGroup((int)(intptr_t)pgSound_AsChunk(channeldata[i].queue));
Py_XDECREF(channeldata[i].queue);
}
}
free(channeldata);
channeldata = NULL;
numchanneldata = 0;
}This approach:
- Prevents the callback from interfering during shutdown
- Still halts the audio groups properly
- Manually handles the Python reference cleanup that was removed from the original code
- Maintains thread safety
You're right that this is much cleaner than trying to coordinate with the callback mechanism during shutdown. Great insight!
🧬 Code Graph Analysis Results
- src_c/mixer.c: ChannelData struct and related globals
struct ChannelData {
PyObject *sound;
PyObject *queue;
int endevent;
};
static struct ChannelData *channeldata = NULL;
static int numchanneldata = 0;
- src_c/mixer.c: endsound_callback function
static void
endsound_callback(int channel)
{
if (channeldata) {
if (channeldata[channel].endevent && SDL_WasInit(SDL_INIT_VIDEO)) {
_pg_push_mixer_event(channeldata[channel].endevent, channel);
}
if (channeldata[channel].queue) {
PyGILState_STATE gstate = PyGILState_Ensure();
int channelnum;
Mix_Chunk *sound = pgSound_AsChunk(channeldata[channel].queue);
Py_XDECREF(channeldata[channel].sound);
channeldata[channel].sound = channeldata[channel].queue;
channeldata[channel].queue = NULL;
PyGILState_Release(gstate);
channelnum = Mix_PlayChannelTimed(channel, sound, 0, -1);
if (channelnum != -1) {
Mix_GroupChannel(channelnum, (int)(intptr_t)sound);
}
}
else {
PyGILState_STATE gstate = PyGILState_Ensure();
Py_XDECREF(channeldata[channel].sound);
channeldata[channel].sound = NULL;
PyGILState_Release(gstate);
Mix_GroupChannel(channel, -1);
}
}
}
- src_c/mixer.c: Mix_ChannelFinished callback registration (in mixer initialization)
Mix_ChannelFinished(endsound_callback);
- src_c/mixer.c: mixer_quit function
static PyObject *
mixer_quit(PyObject *self, PyObject *_null)
{
int i;
if (SDL_WasInit(SDL_INIT_AUDIO)) {
Py_BEGIN_ALLOW_THREADS;
Mix_HaltMusic();
Py_END_ALLOW_THREADS;
if (channeldata) {
for (i = 0; i < numchanneldata; ++i) {
if (channeldata[i].sound) {
Mix_HaltGroup(
(int)(intptr_t)pgSound_AsChunk(channeldata[i].sound));
}
if (channeldata[i].queue) {
Mix_HaltGroup(
(int)(intptr_t)pgSound_AsChunk(channeldata[i].queue));
}
}
free(channeldata);
channeldata = NULL;
numchanneldata = 0;
}
if (mx_current_music) {
if (*mx_current_music) {
Py_BEGIN_ALLOW_THREADS;
Mix_FreeMusic(*mx_current_music);
Py_END_ALLOW_THREADS;
*mx_current_music = NULL;
}
mx_current_music = NULL;
}
if (mx_queue_music) {
if (*mx_queue_music) {
Py_BEGIN_ALLOW_THREADS;
Mix_FreeMusic(*mx_queue_music);
Py_END_ALLOW_THREADS;
*mx_queue_music = NULL;
}
mx_queue_music = NULL;
}
Py_BEGIN_ALLOW_THREADS;
Mix_CloseAudio();
SDL_QuitSubSystem(SDL_INIT_AUDIO);
Py_END_ALLOW_THREADS;
}
Py_RETURN_NONE;
}
✏️ Learnings added
Learnt from: ankith26
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:520-529
Timestamp: 2025-08-17T17:23:06.756Z
Learning: In pygame-ce's mixer.c, when fixing reference leaks in mixer_quit, the cleanest approach is to first call Mix_ChannelFinished(NULL) to disable the callback and prevent race conditions, then manually handle Py_XDECREF calls for channeldata[i].sound and channeldata[i].queue after the Mix_HaltGroup calls.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src_c/mixer.c (1)
522-529: Leak: channeldata[i].sound and .queue are never DECREF’d during mixer_quitThe loop halts groups but does not release the Python references stored in channeldata[i].sound and channeldata[i].queue. This leaks one ref per non-NULL field per channel on quit.
Apply this minimal fix to release those references and null the fields:
if (channeldata[i].sound) { Mix_HaltGroup( (int)(intptr_t)pgSound_AsChunk(channeldata[i].sound)); + Py_XDECREF(channeldata[i].sound); + channeldata[i].sound = NULL; } if (channeldata[i].queue) { Mix_HaltGroup( (int)(intptr_t)pgSound_AsChunk(channeldata[i].queue)); + Py_XDECREF(channeldata[i].queue); + channeldata[i].queue = NULL; }Optional: Calling Mix_HaltGroup(-1) or Mix_HaltChannel(-1) once outside the loop would avoid repeated halts and improve efficiency, but the above is sufficient to fix the leak.
🧹 Nitpick comments (2)
src_c/mixer.c (2)
812-881: snd_copy: Great error handling, subclass preservation, and volume parity; please re-verify refcount under 3.14 debug; consider copying Python-level attrs
- Good: Uses Py_TYPE(self)->tp_new to preserve subclass; robust NULL checks and cleanup; propagates exceptions; mirrors Mix_Chunk volume.
- Refcount: Returning without an extra Py_INCREF(newSound) is the standard pattern (tp_new returns a new reference, and returning from the C method transfers it). Given earlier reports of “undercount” on Python 3.14 debug, please re-run under that environment to confirm behavior. If an extra INCREF is empirically required, add it with a brief comment explaining why to avoid regressions.
Optional: If users subclass Sound and add Python-level attributes, shallow-copying dict preserves that state on copies.
Apply this (optional) diff just before Py_DECREF(dict):
- Py_DECREF(dict); + /* Best-effort: copy Python-level attributes if present on a subclass. */ + { + PyObject *src_dict = PyObject_GetAttrString(self, "__dict__"); + if (src_dict) { + PyObject *dst_dict = + PyObject_GetAttrString((PyObject *)newSound, "__dict__"); + if (dst_dict) { + if (PyDict_Update(dst_dict, src_dict) < 0) { + /* Don't make copy fail due to attribute copy; clear and continue. */ + PyErr_Clear(); + } + Py_DECREF(dst_dict); + } + else { + PyErr_Clear(); + } + Py_DECREF(src_dict); + } + else { + PyErr_Clear(); + } + } + Py_DECREF(dict); return (PyObject *)newSound;
940-942: Consider adding deepcopy to integrate with copy.deepcopyYou’ve implemented copy() and copy() — nice. For completeness with Python’s copy protocol, consider adding deepcopy(self, memo) that delegates to the same implementation. It prevents surprising behavior when users call copy.deepcopy(sound).
Proposed table entry (insert alongside the others):
{"copy", snd_copy, METH_NOARGS, DOC_MIXER_SOUND_COPY}, {"__copy__", snd_copy, METH_NOARGS, DOC_MIXER_SOUND_COPY}, + {"__deepcopy__", (PyCFunction)snd_deepcopy, METH_O, DOC_MIXER_SOUND_COPY},And add this simple wrapper (anywhere near snd_copy):
static PyObject *snd_deepcopy(PyObject *self, PyObject *memo) { /* Deep-copy semantics are identical for Sound; memo is unused. */ return snd_copy(self, NULL); }If you adopt this, remember to update stubs/docs accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src_c/mixer.c(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
📚 Learning: 2025-08-13T12:39:19.107Z
Learnt from: oddbookworm
PR: pygame-community/pygame-ce#3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.107Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/mixer.c
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: arm64
- GitHub Check: x86_64
- GitHub Check: aarch64
- GitHub Check: x86_64
- GitHub Check: i686
- GitHub Check: debug_coverage (ubuntu-24.04, 3.14.0rc1)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.13.5)
- GitHub Check: debug_coverage (ubuntu-24.04, 3.9.23)
- GitHub Check: x86
- GitHub Check: AMD64
- GitHub Check: Debian (Bookworm - 12) [armv7]
- GitHub Check: Debian (Bookworm - 12) [armv6]
- GitHub Check: Debian (Bookworm - 12) [ppc64le]
- GitHub Check: Debian (Bookworm - 12) [s390x]
🔇 Additional comments (1)
src_c/mixer.c (1)
92-94: Forward declaration for snd_get_arraystruct looks correctPrototype matches the implementation below. No concerns.
…into sound-copy-bytes
|
@ankith26 comments addressed |
I added https://github.com/Borishkof as a co-author because I took the test and tweaked the docs from their old pull request
Summary by CodeRabbit
New Features
Documentation
Tests