-
-
Notifications
You must be signed in to change notification settings - Fork 201
Add Sound.copy and Sound.__copy__ #3556
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
Open
oddbookworm
wants to merge
11
commits into
pygame-community:main
Choose a base branch
from
oddbookworm:sound-copy-bytes
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
319be11
Add Sound.copy and Sound.__copy__
oddbookworm 48181d3
Fixed refcounting (hopefully) and addressed other review comments fro…
oddbookworm d5e9157
Added note about __bytes__ to Sound docs
oddbookworm f71f0c3
Address comments
oddbookworm 9f82126
Halt playback instead of decrefing the sound in mixer_quit
oddbookworm eae8383
Review comments from bot, good :bot:
oddbookworm 4e11fad
More refcount edits
oddbookworm 0a65766
Put decref back
oddbookworm 669f06e
Use XDECREF where it is potentially unsafe to use normal DECREF
oddbookworm 24348bc
Review comments
oddbookworm 3902608
Merge branch 'main' of https://github.com/pygame-community/pygame-ce …
oddbookworm File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 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_quit
needs 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_callback
does 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:
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:
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 callingMix_ChannelFinished(NULL)
would be the cleanest approach. Here's why:The
endsound_callback
properly handlesPy_XDECREF
for bothsound
andqueue
fields, but there's a race condition risk: ifMix_HaltGroup()
triggers the callback asynchronously, it could accesschanneldata
aftermixer_quit
has already freed it.Your suggestion to disable the callback first eliminates this race condition. The complete fix would be:
This approach:
You're right that this is much cleaner than trying to coordinate with the callback mechanism during shutdown. Great insight!
🧬 Code Graph Analysis Results
✏️ Learnings added
🧠 Learnings used