pygame.Sound.copy implementation

[Borishkof]

Create a function for deep copying sound.
Create some tests (failing tests on .xm and .opus for un unknown reason).

issue : #1328 (comment)

[ankith26]

Thanks for contributing to pygame-ce!

This change would need documentation and type stubs for complete approval

[damusss]

Things I noticed beside what ankith said. Also some error messages could be improved grammar wise.

[Borishkof]

Thanks for your help, I will update my PR.

[bilhox]

Overall LGTM, I left some requested changes.

[Borishkof]

It seems like xm or opus is "Unrecognized audio format" (using mixer.Sound() ) for the distribution that does the automatic test I failed.
But it works on my distrib and some examples in mixer_music_test use those formats (without specific version checking).
What should I do ?

[yunline]

It seems like xm or opus is "Unrecognized audio format" (using mixer.Sound() ) for the distribution that does the automatic test I failed.
But it works on my distrib and some examples in mixer_music_test use those formats (without specific version checking).
What should I do ?

You are writing unittest for Sound.copy, not for other functionalities.
The test case should be decoupled with other functionalities like "reading files" since those should be taken care of by other test cases.

[Starbuck5]

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.

[Starbuck5]

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.

[Starbuck5]

Thank you for working on this @Borishkof! I can tell this is a high effort contribution.

However, I feel that there is no safe way to do this within the SDL_mixer API, and I don't really see why this is a useful feature. I feel bad I didn't see the earlier discussion and say so then.

Copying a surface? Important, because they are editable. Edit one copy, leave the other copy the same.
Sounds on the other hand? The only thing you can edit currently is the volume. Not exactly as fundamental.

Talking myself through it I guess you could attach sounds to objects in a game. So like different sprites have different volume explosion noises. So you would want to give them each a copy of the sound with a different preset volume. But that would be very wasteful on the memory front. The ideal solution I think would be a bunch of Mix_Chunks all pointed at the same audio buffer, with different volumes set. So we already have Quickload_RAW from SDL_mixer, and we already can get the raw bytes using the routine get_raw uses right?

So maybe this can be done without this allocated branching? Whether or not the mix_chunk controls the allocation, the allocation can be copied out. We'd need some way of managing the life cycle of this new buffer that only some Sound objects have. If we wanted to share it between Sound copies for efficiency's sake it could be another Python object so we can ref-count it for garbage collection purposes.

OK new idea on this train of thought, can't this be implemented safely and simply in Python? I haven't tested this at all but couldn't you take the output of Sound.get_raw() and pipe it back into the Sound constructor with the array= keyword. If it's written in Python you could do my crazy memory proxy Python object idea with very low complexity.

[Borishkof]

Thank you for the review :)

As I am a beginner in Pygame, I can't argue much about the solutions you propose and will leave it to the community to decide which is the most efficient way to tackle the issue.
That being said, I believe you are right about the poor memory management (I forgot to check it).

If the community reaches a consensus on a reasonably simple solution, I would be glad to try and implement it. Otherwise, I am happy to let anyone with good ideas take a shot at addressing this issue. 👍

[bilhox]

I find the arguments gave by starbucks understandable, so I'm for an implementation in python side now.

[Starbuck5]

On second thought maybe sharing the memory is a bit too far. Sound data is mutable through https://pyga.me/docs/ref/sndarray.html#pygame.sndarray.samples, so that would need to protected against. Also I'm not sure the public Sound API supports zero copying the data.

But anyways here is a 1 line Python implementation of sound.copy, in a demonstration program.

import pygame
pygame.init()

window = pygame.Window(resizable=True)
screen = window.get_surface()

class MySound(pygame.Sound):
    def copy(self):
        return self.__class__(buffer=self.get_raw())

a = MySound(r"C:\Users\charl\Desktop\pygame-ce\examples\data\secosmic_lo.wav")
b = a.copy()
b.set_volume(0.1)

clock = pygame.Clock()

while True:
    for event in pygame.event.get():
        if event.type == pygame.QUIT:
            pygame.quit()
            raise SystemExit
        if event.type == pygame.KEYDOWN and event.key == pygame.K_1:
            a.play()

        if event.type == pygame.KEYDOWN and event.key == pygame.K_2:
            b.play()

    window.flip()
    clock.tick(144)

[Starbuck5]

I've set this to a draft because it is not ready to merge in its current state. I was going to close it fully and send it back to the issue for a new PR, but the docs/tests/stubs bit of this PR is probably good. I just think the actual implementation needs to be overhauled.

I showed a Python example of how this could be achieved in one line. This same logic could be applied in C (it would take more than one line), or it could be monkey patched into the type in __init__.py.