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 5.1 Vorbis files #18

Closed
duysqubix opened this issue Oct 10, 2023 · 6 comments · Fixed by #163
Closed

Support 5.1 Vorbis files #18

duysqubix opened this issue Oct 10, 2023 · 6 comments · Fixed by #163
Labels
enhancement New feature or request

Comments

@duysqubix
Copy link
Contributor

The code no longer assumes Vorbis files are stereo (they may have up to 255 channels), which caused e.g. mono files to play at twice their speed.

The current strategy for files with more than 2 channels is to only play the first and last channels, and drop the rest.

Original issue: faiface/beep#154

@MarkKremer
Copy link
Contributor

The mono part is implemented in #10.

In the original PR I said that if we would like to support 3+ channels that we should pick the front left and front right channels instead of the first and last channels. However, that drops some channels still which I'm not sure if I'm a fan of. I propose we wait until 3+ channel support (#9) is implemented and then this can be implemented the proper way.

If someone needs this sooner, please comment. It's not a decision set in stone.

@MarkKremer MarkKremer added the enhancement New feature or request label Oct 11, 2023
@MarkKremer MarkKremer changed the title Fix support for mono and 5.1 Vorbis files Support 5.1 Vorbis files Oct 11, 2023
@K4thos
Copy link

K4thos commented May 15, 2024

If someone needs this sooner, please comment. It's not a decision set in stone.

We're considering switching from our beep fork to gopxl/beep, but not having support for 5.1 would be a regression compared to the current release of our engine. Please consider merging this PR as a temporary solution, until a better one is ready: faiface/beep#154

@MarkKremer
Copy link
Contributor

Sure!

Does your fork happen to have implemented my suggestions (faiface/beep#154 (comment))?

@K4thos
Copy link

K4thos commented May 15, 2024

Thanks! I'm afraid not - original content of PR 154 matches what's in our fork.

@MarkKremer
Copy link
Contributor

MarkKremer commented May 22, 2024

Does #163 work for you and your usecase @K4thos?

@K4thos
Copy link

K4thos commented Jun 8, 2024

yes, looks good, thanks!

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

Successfully merging a pull request may close this issue.

3 participants