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

Add FLOAT32 formats #9852

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

TimDaub
Copy link

@TimDaub TimDaub commented Nov 17, 2019

Fixes #9851

@welcome
Copy link

welcome bot commented Nov 17, 2019

Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS.

@kripken
Copy link
Member

kripken commented Nov 19, 2019

Thanks @TimDaub !

Yes, this looks right to me, I'm puzzled why it wasn't there before... perhaps @juj has an idea?

I suspect we didn't want to add extra changes to al.h which is an upstream header? But as we do support that - and looks like we have tests for it, too - we should have it in the header.

I think we can also remove the #defines for those two constants from the two tests that define them, tests/openal_capture_sanity.c and tests/openal_capture.c.

@TimDaub TimDaub force-pushed the add-float32-formats branch 2 times, most recently from 7ee3daa to 86771ab Compare November 20, 2019 14:48
@TimDaub
Copy link
Author

TimDaub commented Nov 20, 2019

I think we can also remove the #defines for those two constants from the two tests that define them, tests/openal_capture_sanity.c and tests/openal_capture.c.

OK, I removed them.

- Added AL_FORMAT_MONO_FLOAT32 and AL_FORMAT_STEREO_FLOAT32 to al.h
- Remove custom definition of these values from tests
@juj
Copy link
Collaborator

juj commented Nov 21, 2019

The FLOAT32 #defines are not part of OpenAL (https://github.com/kcat/openal-soft/blob/master/include/AL/al.h), but they are an extension to OpenAL, see

https://github.com/kcat/openal-soft/blob/f2eab3e919aa06b72ad467118db3269fe2571bbb/include/AL/alext.h#L70

If we want to do this proper, we should also add a file alext.h in https://github.com/emscripten-core/emscripten/tree/incoming/system/include/AL where this should live.

Perhaps we should take openal-soft headers as-is from https://github.com/kcat/openal-soft/tree/master/include/AL ? (although that will run into similar problems as WebGL currently does with mismatching/unrecognized extensions, but with OpenAL it may be easier to add support to each)

@juj juj added the OpenAL label Nov 21, 2019
@kripken
Copy link
Member

kripken commented Nov 22, 2019

Thanks @juj, yes, that sounds like the way to go!

@sbc100 sbc100 closed this Jan 30, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

@sbc100 sbc100 reopened this Jan 30, 2020
@sbc100 sbc100 changed the base branch from incoming to master January 30, 2020 20:14
@stale
Copy link

stale bot commented Jan 31, 2021

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Jan 31, 2021
Base automatically changed from master to main March 8, 2021 23:49
@stale stale bot removed the wontfix label Mar 8, 2021
@Laguna1989
Copy link

Any progress on this one?

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

Successfully merging this pull request may close these issues.

AL_FORMAT_STEREO_FLOAT32 is not defined
5 participants