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

Support native playback on Windows #360

Merged
merged 8 commits into from
Feb 27, 2025

Conversation

wilt00
Copy link
Contributor

@wilt00 wilt00 commented Feb 23, 2025

This PR adds support for playing generated mp3 files on Windows with no external dependencies, just built-in Python standard library functions and native Windows APIs, so users on Windows no longer need to have mpv installed.

One drawback of this method is that isn't possible to stop playback with ctrl-C while the audio is playing. In theory, it should be possible to fix this with an additional dependency (pywin32), although I haven't tried that out yet, and since it's a windows-only library it'd take some fiddling in setuptools, so I opted for the simpler version here. (An even simpler version would be passing the file name to winsound.PlaySound, but unfortunately that only takes .wav files)

Copy link
Owner

@rany2 rany2 left a comment

Choose a reason for hiding this comment

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

The subtitle file shouldn't be generated and the path to it shouldn't be printed out when we're not using MPV.

@wilt00
Copy link
Contributor Author

wilt00 commented Feb 26, 2025

It did just occur to me that this would be a breaking change - I can flip the default back to preferring mpv if you prefer

@rany2
Copy link
Owner

rany2 commented Feb 26, 2025

It did just occur to me that this would be a breaking change - I can flip the default back to preferring mpv if you prefer

I wouldn't really consider it a breaking change but I think it's deserving of at least a minor version bump.

@rany2
Copy link
Owner

rany2 commented Feb 26, 2025

At any rate we just need to clear the remaining CI complaints and I'll merge this. Thanks a lot for bearing with me btw

Signed-off-by: rany <rany2@riseup.net>
Signed-off-by: rany <rany2@riseup.net>
@rany2
Copy link
Owner

rany2 commented Feb 27, 2025

any comments before I merge this?

@wilt00
Copy link
Contributor Author

wilt00 commented Feb 27, 2025

Nope, thanks for cleaning it up!

@rany2 rany2 merged commit ecf50b9 into rany2:master Feb 27, 2025
3 checks passed
@rany2
Copy link
Owner

rany2 commented Feb 27, 2025

Thanks for putting in the work for this feature!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants