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

pygame.Sound.copy implementation #3238

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions src_c/mixer.c
Original file line number Diff line number Diff line change
Expand Up @@ -831,6 +831,72 @@ snd_get_samples_address(PyObject *self, PyObject *closure)
#endif
}

static PyObject *
snd_copy(pgSoundObject *self, PyObject *_null)
damusss marked this conversation as resolved.
Show resolved Hide resolved
{
Mix_Chunk *chunk = pgSound_AsChunk(self);
pgSoundObject *new_sound;
Mix_Chunk *new_chunk;

// Validate the input chunk
CHECK_CHUNK_VALID(chunk, NULL);

// Create a new sound object
new_sound =
(pgSoundObject *)pgSound_Type.tp_new(Py_TYPE(self), NULL, NULL);
if (!new_sound) {
PyErr_SetString(PyExc_RuntimeError,
damusss marked this conversation as resolved.
Show resolved Hide resolved
"Failed to allocate memory for new sound object");
return NULL;
}

// Handle chunk allocation type
if (chunk->allocated) {
// Create a deep copy of the audio buffer for allocated chunks
Uint8 *buffer_copy = (Uint8 *)malloc(chunk->alen);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is a memory leak. This is allocated but never freed, and it can't be freed here because it needs to live as long as the chunk does.

if (!buffer_copy) {
Py_DECREF(new_sound);
PyErr_SetString(PyExc_MemoryError,
"Failed to allocate memory for sound buffer");
return NULL;
}
memcpy(buffer_copy, chunk->abuf, chunk->alen);

// Create a new Mix_Chunk
new_chunk = Mix_QuickLoad_RAW(buffer_copy, chunk->alen);
if (!new_chunk) {
free(buffer_copy);
Py_DECREF(new_sound);
PyErr_SetString(PyExc_RuntimeError,
damusss marked this conversation as resolved.
Show resolved Hide resolved
"Failed to create new sound chunk");
return NULL;
}
new_chunk->volume = chunk->volume;
new_sound->chunk = new_chunk;
}
else {
// For non-allocated chunks (e.g., formats like .xm), create a full
// copy
new_chunk = (Mix_Chunk *)malloc(sizeof(Mix_Chunk));
if (!new_chunk) {
Py_DECREF(new_sound);
PyErr_SetString(PyExc_MemoryError,
"Failed to allocate memory for sound chunk");
return NULL;
}
*new_chunk = *chunk; // Copy the entire structure
Comment on lines +880 to +887
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying an entire struct like this may work in this scenario but in my opinion is not something that is safe in general. From SDL's perspective these are all internal fields and they can do whatever they want with it. They're barely documented.


// For safety, ensure the copied chunk doesn't share pointers
new_chunk->abuf =
NULL; // Prevent double-free if original gets deallocated
new_chunk->allocated = 0;

new_sound->chunk = new_chunk;
}

return (PyObject *)new_sound;
}

PyMethodDef sound_methods[] = {
{"play", (PyCFunction)pgSound_Play, METH_VARARGS | METH_KEYWORDS,
DOC_MIXER_SOUND_PLAY},
Expand All @@ -842,6 +908,7 @@ PyMethodDef sound_methods[] = {
{"get_volume", snd_get_volume, METH_NOARGS, DOC_MIXER_SOUND_GETVOLUME},
{"get_length", snd_get_length, METH_NOARGS, DOC_MIXER_SOUND_GETLENGTH},
{"get_raw", snd_get_raw, METH_NOARGS, DOC_MIXER_SOUND_GETRAW},
{"copy", snd_copy, METH_NOARGS, ""},
{NULL, NULL, 0, NULL}};

static PyGetSetDef sound_getset[] = {
Expand Down
32 changes: 32 additions & 0 deletions test/mixer_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -711,6 +711,38 @@ def test_get_sdl_mixer_version__linked_equals_compiled(self):

self.assertTupleEqual(linked_version, compiled_version)

def test_snd_copy(self):
mixer.init()

filenames = [
"house_lo.mp3",
"house_lo.ogg",
"house_lo.wav",
"house_lo.flac",
# "house_lo.opus", unsupported
# "surfonasinewave.xm" unsupported
]

for f in filenames:
filename = example_path(os.path.join("data", f))
sound = mixer.Sound(file=filename)
sound_copy = sound.copy()
self.assertEqual(sound.get_length(), sound_copy.get_length())
self.assertEqual(sound.get_num_channels(), sound_copy.get_num_channels())
self.assertEqual(sound.get_volume(), sound_copy.get_volume())
self.assertEqual(sound.get_raw(), sound_copy.get_raw())

del sound

# Test on the copy
channel = sound_copy.play()
self.assertTrue(channel.get_busy())
sound_copy.stop()
self.assertFalse(channel.get_busy())

sound_copy.play()
self.assertEqual(sound_copy.get_num_channels(), 1)


############################## CHANNEL CLASS TESTS #############################

Expand Down
Loading