Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Support devicename=None in _sdl2.AudioDevice #3617

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

Merged

Conversation

@rhendric
Copy link
Contributor

@rhendric rhendric commented Oct 17, 2025

The docstring already claimed that this was supported, but it didn't work.

@rhendric rhendric requested a review from a team as a code owner October 17, 2025 05:02
Copy link
Contributor

coderabbitai bot commented Oct 17, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

Allow devicename to be None when opening audio devices: pass NULL to SDL_OpenAudioDevice for the default device or a UTF-8 encoded C string when a name is provided; update type hints in stubs to Optional[str]; preserve existing error handling.

Changes

Cohort / File(s) Summary
Audio device initialization
src_c/cython/pygame/_sdl2/audio.pyx		 Accept devicename as None or str; convert to a C string pointer (NULL when None, UTF-8 pointer otherwise) and pass it to SDL_OpenAudioDevice. Preserve existing error handling and use the new pointer at the call site.
Type hints / stubs
buildconfig/stubs/pygame/_sdl2/audio.pyi		 Update typings: import Optional; change AudioDevice.__init__ and AudioDevice.devicename signatures to accept/return Optional[str] instead of str.

Sequence Diagram(s)

sequenceDiagram
 participant Py as Python caller
 participant C as Cython wrapper
 participant SDL as SDL library
 Py->>C: open_audio(devicename)
 alt devicename is None
 C->>C: devicename_ptr = NULL
 else devicename provided
 C->>C: encode devicename -> utf8_ptr
 C->>C: devicename_ptr = utf8_ptr
 end
 C->>SDL: SDL_OpenAudioDevice(devicename_ptr, ...)
 alt success
 SDL-->>C: device_id
 C-->>Py: device object / id
 else failure
 SDL-->>C: NULL / error
 C-->>Py: raise error
 end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "Support devicename=None in _sdl2.AudioDevice" directly and accurately summarizes the main change in the changeset. The modifications to both the Cython implementation and type stubs are specifically about enabling the devicename parameter to accept None in addition to strings, changing its type from str to Optional[str]. The title is concise, clear, and sufficiently specific for a teammate reviewing commit history to understand the primary intent of the change.
Description Check ✅ Passed The pull request description "The docstring already claimed that this was supported, but it didn't work" is directly related to the changeset and provides meaningful context for the changes. It explains the motivation behind adding None support: there was a documentation-implementation mismatch where the docstring claimed the feature was supported but the code didn't actually implement it. This context is relevant and meaningful, making the description appropriate for the PR objectives.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3dd631d and e0b4055.

📒 Files selected for processing (2)
  • buildconfig/stubs/pygame/_sdl2/audio.pyi (3 hunks)
  • src_c/cython/pygame/_sdl2/audio.pyx (2 hunks)
🔇 Additional comments (3)
buildconfig/stubs/pygame/_sdl2/audio.pyi (1)

2-2: LGTM! Type annotations correctly reflect optional devicename.

The stub updates properly annotate devicename as Optional[str] in both the constructor parameter and property return type, aligning with the runtime implementation that now supports None.

Also applies to: 29-29, 43-43

src_c/cython/pygame/_sdl2/audio.pyx (2)

134-136: LGTM! Type validation correctly allows None and str.

The updated type check properly validates that devicename is either None or a str, rejecting any other types with a clear error message.

145-155: LGTM! Encoding logic and bytes object lifetime are correct.

The implementation properly handles both cases:

  • When devicename is None, passes NULL to SDL for the default device
  • When devicename is a string, encodes to UTF-8 and maintains the bytes object lifetime correctly

The devicename_bytes local variable ensures the C string pointer remains valid throughout the SDL_OpenAudioDevice call.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src_c/cython/pygame/_sdl2/audio.pyx (1)

145-160: Consider refactoring to reduce duplication.

The SDL_OpenAudioDevice call is duplicated in both branches with only the first parameter differing. You could reduce duplication with:

+ cdef bytes device_name_bytes
+ cdef const char* device_name_ptr
 if self._devicename is None:
- self._deviceid = SDL_OpenAudioDevice(
- NULL,
- self._iscapture,
- &self.desired,
- &self.obtained,
- allowed_changes
- )
+ device_name_ptr = NULL
 else:
- self._deviceid = SDL_OpenAudioDevice(
- self._devicename.encode("utf-8"),
- self._iscapture,
- &self.desired,
- &self.obtained,
- allowed_changes
- )
+ device_name_bytes = self._devicename.encode("utf-8")
+ device_name_ptr = device_name_bytes
+
+ self._deviceid = SDL_OpenAudioDevice(
+ device_name_ptr,
+ self._iscapture,
+ &self.desired,
+ &self.obtained,
+ allowed_changes
+ )

Note: Keeping device_name_bytes alive ensures the pointer remains valid during the C call.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3fc1dbe and e42b76c.

📒 Files selected for processing (1)
  • src_c/cython/pygame/_sdl2/audio.pyx (2 hunks)
🔇 Additional comments (2)
src_c/cython/pygame/_sdl2/audio.pyx (2)

134-135: LGTM! Type validation correctly supports None.

The type check properly allows None and validates that non-None values are strings. The error message accurately reflects the accepted types.

145-160: Correctly implements None support for default device.

The branching logic properly passes NULL to SDL_OpenAudioDevice when devicename is None, which selects the default device per SDL2 API. UTF-8 encoding for non-None device names is appropriate.

@rhendric rhendric force-pushed the rhendric/AudioDevice-None branch from e42b76c to 3dd631d Compare October 17, 2025 05:12
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

Hello, thanks for contributing to pygame-ce! 🥳

The code changes are looking good, but the type stubs (at buildconfig/stubs/pygame/_sdl2/audio.pyi) are still incorrect, could you fix that too in this PR?
The devicename arg of __init__ and devicename property of the class should both be typed as Optional[str]

rhendric reacted with thumbs up emoji 
The docstring already claimed that this was supported, but it didn't
work.
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again for the PR! 😎

Copy link
Member

@rhendric Can you provide some details about your use of the _sdl2.audio API? I'm curious. I was unaware anyone used it, besides from the list device names function.

Copy link
Contributor Author

I'm only using it for the async audio API. I'm playing with generating music dynamically in real time, so I don't have pre-recorded audio files to play, and so getting a callback invoked when SDL needs audio data is much more straightforward than racing to proactively queue up Sounds through the mixer API.

Copy link
Contributor Author

I should probably also mention that I'm in the very early stages of experimenting with this, and while my first demo script showed promise, in the context of a larger application I'm currently struggling with some skipping issues that might force me to switch to synchronous after all (though there seems to be a different bug in the mixer API that is getting in the way of queuing dynamic Sounds repeatedly). So don't move any mountains based solely on my use case just yet.

Copy link
Member

@rhendric You might be interested in what I've been working on for SDL3 audio support then, where it will hopefully not be hidden away in a private undocumented module. #3581

though there seems to be a different bug in the mixer API that is getting in the way of queuing dynamic Sounds repeatedly

Are you running into the default limit of 8 sounds playing at once? That can be raised.

Copy link
Contributor Author

SDL3 audio support

Looks good! The open_stream function in particular would be a match for my use case, I think, provided that something like the overhead of waiting for the GIL on every callback isn't the source of my current woes.

Are you running into the default limit of 8 sounds playing at once?

No, I think I'm running into #531. There's a race condition between the main thread rapidly queuing sound data and the SDL thread moving sounds from the queue field to the sound field for the channel. The root issue seems to be that endsound_callback in mixer.c tests whether queue is NULL before locking on anything. I have a patch that I'm in the process of stress testing.

@Starbuck5 Starbuck5 added New API This pull request may need extra debate as it adds a new class or function to pygame _sdl2 pygame._sdl2 labels Nov 1, 2025
Copy link
Member

@Starbuck5 Starbuck5 left a comment

Choose a reason for hiding this comment

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

Thanks!

Tested it out using examples/audiocapture.

@Starbuck5 Starbuck5 added bugfix PR that fixes bug and removed New API This pull request may need extra debate as it adds a new class or function to pygame labels Nov 1, 2025
@Starbuck5 Starbuck5 merged commit 3c8f666 into pygame-community:main Nov 1, 2025
29 checks passed
@Starbuck5 Starbuck5 added this to the 2.5.7 milestone Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments

@Starbuck5 Starbuck5 Starbuck5 approved these changes

@ankith26 ankith26 ankith26 approved these changes

Assignees

No one assigned

Labels

bugfix PR that fixes bug _sdl2 pygame._sdl2

Projects

None yet

Milestone

2.5.7

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /