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

AudioStream(Playback)WAV: Use LocalVectors instead of pointers #96017

Merged
merged 1 commit into from
Sep 8, 2024

Conversation

DeeJayLSP
Copy link
Contributor

@DeeJayLSP DeeJayLSP commented Aug 23, 2024

This PR originally used Vectors, but were changed to LocalVectors on reduz's request.


Changes the types of (AudioStreamPlaybackWAV::qoa.dec and AudioStreamWAV::data) to use Vectors instead of pointers. This was done with AudioStreamMP3's data between versions 3.x and 4.x of Godot.

Only private variables and function structures were modified, without altering return type, therefore does not break compatibility.

Alongside #95463, this PR makes the AudioStreamWAV and AudioStreamPlaybackWAV classes free of direct memalloc/free calls, and consequently the only memalloc/free calls within the /scene directory.

@Chaosus Chaosus added this to the 4.4 milestone Aug 24, 2024
@fire fire requested a review from a team August 26, 2024 18:16
@fire
Copy link
Member

fire commented Aug 26, 2024

Does anyone know if using Vector instead of pointers causes audio corruption?

I don't understand why AudioStreamMP3 doesn't match AudioStreamWAV and AudioStreamPlaybackWAV.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 26, 2024

Does anyone know if using Vector instead of pointers causes audio corruption?

Most of audio playback works in two ways.

In case of uncompressed (WAV 8/16 bit) it simply copies samples from the data, converts into 32 bit float, then goes into the audio buffer. And with compressed (MP3, QOA, IMA ADPCM) the data goes through a decode process before being sent to the buffer. All access uses some kind of uint8_t pointer.

The only change with this PR is how data is stored in memory. Everything is passed to buffer/decoders using the Vector's pointer anyway, with the advantage of not having explicit memalloc/memfree calls within the class.

Shouldn't cause corruption at all.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 28, 2024

One thing I noticed: p_src in do_resample() is essentially the same as base->data + AudioStreamWAV::DATA_PAD, so I changed the code to use it directly.

@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Aug 30, 2024

Rebased on top of master following #95463 merge.

@DeeJayLSP DeeJayLSP force-pushed the wav-vec branch 2 times, most recently from 6289213 to 2816e87 Compare September 7, 2024 15:01
@DeeJayLSP DeeJayLSP changed the title AudioStream(Playback)WAV: Use Vectors instead of pointers AudioStream(Playback)WAV: Use LocalVectors instead of pointers Sep 7, 2024
@DeeJayLSP DeeJayLSP force-pushed the wav-vec branch 4 times, most recently from 80f7b12 to 2fda1f3 Compare September 7, 2024 15:29
@DeeJayLSP
Copy link
Contributor Author

DeeJayLSP commented Sep 8, 2024

One last force push. Forgot to set all values in the vector to 0 when setting data, this was causing an audible pop when looping a 16-bit stream.

@akien-mga akien-mga merged commit 0e307f8 into godotengine:master Sep 8, 2024
20 checks passed
@DeeJayLSP DeeJayLSP deleted the wav-vec branch September 8, 2024 21:25
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants