Fix the AudioStream update and Sound update to use element size rather than size in bytes #212

meisei4 · 2025-07-07T03:33:07Z

This is a fix to correct the the AudioStream update and Sound update to use element size rather than size in bytes. I have tested it with a new sample that is the equivalent of the raylib core examples audio_raw_stream.c:

https://github.com/raysan5/raylib/blob/master/examples/audio/audio_raw_stream.c

AmityWilder · 2025-07-07T21:18:19Z

raylib/src/core/audio.rs

+            ffi::UpdateSound(
+                self.0,
+                data.as_ptr() as *const std::os::raw::c_void,
+                data.len() as i32,


I would prefer if these could be .try_into().unwrap() since usize as i32 can overflow without warning, but as i32 is so prevalent in the library that this isn't a deal breaker. It will probably be addressed on its own in the future.

will address with this https://github.com/raylib-rs/raylib-rs/pull/212/files#r2191223482
I have made the try_into().unwrap() update, but need to investigate the other comment further to understand.

AmityWilder · 2025-07-07T21:33:17Z

samples/audio_raw_stream.rs

+use raylib::prelude::MouseButton::MOUSE_BUTTON_LEFT;
+
+const MAX_SAMPLES: usize = 512;
+const DATA_ELEMENTS_PER_CYCLE: usize = MAX_SAMPLES * 2;


What is DATA_ELEMENTS_PER_CYCLE and where is the 2 coming from? If you are calculating the size of data in bytes, it would be more clear to use size_of::<i16>() to show why 2 in particular is being used.

Fixed. removed DATA_ELEMENTS_PER_CYCLE, now using only MAX_SAMPLES, with halving throughout to follow raylib c version

AmityWilder · 2025-07-07T21:44:39Z

samples/audio_raw_stream.rs

+    let raylib_audio = RaylibAudio::init_audio_device().unwrap();
+    raylib_audio.set_audio_stream_buffer_size_default(MAX_SAMPLES_PER_UPDATE as i32);
+    let mut audio_stream = raylib_audio.new_audio_stream(SAMPLE_RATE, SAMPLE_SIZE, 1);
+    let mut data: [i16; DATA_ELEMENTS_PER_CYCLE] = [0; DATA_ELEMENTS_PER_CYCLE];


This line appears to be related to the original C version's line:

short *data = (short *)malloc(sizeof(short)*MAX_SAMPLES);

malloc does not allocate shorts. It allocates bytes, and then has the byte-array pointer cast to a short-array pointer. In other words,

(T *)malloc(sizeof(T)*N)

allocates the equivalent of

// this [T; N] // NOT this [T; size_of::<T>()*N]

DATA_ELEMENTS_PER_CYCLE is MAX_SAMPLES*2 (2 presumably being the size of a short in bytes).
I think you may have accidentally created an array with as many elements as it was intended to have bytes.

This isn't unsafe or anything, but it is allocating double the space that is intended to be used, and may confuse readers.

updated to use MAX_SAMPLES outright (still concerned if this is a sign that my understanding of the original AudioStream::update() function is incorrect with regards to handling buffers byte size vs element size, i would like to address that in the many places that involve halving and treating the MAX_SAMPLES as a byte count vs element count, perhaps could be helpful to request clarity for this in the original raylib example even?)

MAX_SAMPLES is in samples. Raylib multiplies sampleCount by the size of a sample itself.

samples/audio_raw_stream.rs

AmityWilder · 2025-07-07T21:58:27Z

samples/audio_raw_stream.rs

+        if raylib_handle.is_mouse_button_down(MOUSE_BUTTON_LEFT) {
+            frequency = 40.0 + mouse_position.y;
+        }


The C version of the example also calls SetAudioStreamPan after updating the frequency.

float pan = (float)(mousePosition.x) / (float)screenWidth; SetAudioStreamPan(stream, pan);

introduced pan, but with a mouse x position inversion to have left<->right mouse position match the pans effect. Included a comment. this might be optional, but it bugged me (can remove it to follow raylib closer if the pan behavior is identical in the rust bindings, I will check)

If you would like, I'm sure you could try proposing that change to Raylib itself and find out if there's a reason for it

AmityWilder · 2025-07-07T22:45:31Z

samples/audio_raw_stream.rs

+        let mut draw_handle = raylib_handle.begin_drawing(&raylib_thread);
+        draw_handle.clear_background(Color::RAYWHITE);
+        draw_handle.draw_text(
+            &format!("sine frequency: {:.1} Hz", frequency),


The C version floors to an integer instead of tenths. I won't be picky about this though, I just want to point out that it isn't equivalent in behavior.

I have fixed this after investigating the difference between:

(int)float_val

and these rust fucntions i just learned float_val.floor() as i32, float_val.trunc() as i32, float_val as i32, and I also learned that float_val.floor() has behavior I didnt know about for negatives:
assert_eq!(-3.3_f32 as i32, -3); vs assert_eq!(-3.3_f32.floor() as i32, -4);
https://doc.rust-lang.org/std/primitive.f32.html#method.floor, https://doc.rust-lang.org/reference/expressions/operator-expr.html#numeric-cast

(unrelated here because frequency is never negative) so the best version is to match the C behavior is simply:

&format!("sine frequency: {}", frequency as i32)

AmityWilder · 2025-07-07T22:51:18Z

samples/audio_raw_stream.rs

+            audio_stream.update(&write_buf);
+
+        }
+        let width = raylib_handle.get_screen_width();


This example does not enable resizing of the window. Instead of requesting the width every frame, the width could have been stored in a constant or an immutable variable at the start of the program—like the C version does.

Also, "Width? Width of what? The wave? Its period? The text? Some rectangle I missed?"
Width is a very generic name, especially in a function as expansive as main.

You could have used the name screen_width like the original, making its intent far clearer at a glance without needing to check the definition to know what width it's referring to.

Updated! I have no excuse for this, it was sloppy and the result of my own projects use of ambigious width things where i have to constantly switch between render_width and screen_width because of testing on apple machine and linux machine with DPI things that im still trying to grasp...

AmityWilder · 2025-07-07T22:57:38Z

samples/audio_raw_stream.rs

+            20,
+            Color::RED,
+        );
+        draw_handle.draw_text("click mouse button to change frequency", 10, 10, 20, Color::DARKGRAY);


The C version also mentions panning in this string. However, you also removed that feature from this example (I'm not sure why), so it might be intentional. Just remember to update this string if you decide to re-enable panning.

Updated to properly add the pan feature.

this is related to the raylib-rs example copy not having the panning. I mixed up what i was implementing https://github.com/raysan5/raylib/blob/master/examples/audio/audio_raw_stream.c vs https://github.com/raylib-rs/raylib-rs/blob/unstable/showcase/original/audio/audio_raw_stream.c

AmityWilder · 2025-07-07T23:29:07Z

samples/audio_raw_stream.rs

This example diverges quite a bit from the C version. It looks like you did a bit more than just rewrite it in Rust...

First of all, thank you again for the review!

I am going to organize my thoughts as a reponse to this first, cause i think it will help me. I think we are discussing in discord still, but there is some very strange status of these repositories that lead to this (not making excuses for myself, just trying to explain what i think happened to me I think):

versions of this raylib sample are a bit confusing:

main branch raylib core repo: https://github.com/raysan5/raylib/blob/master/examples/audio/audio_raw_stream.c

commented out code confusion here (I think you mentioned following up on this in the official raylib repo, so that sounds good)

raylib-rs project corresponding sample: https://github.com/raylib-rs/raylib-rs/blob/unstable/showcase/original/audio/audio_raw_stream.c

perhaps out of date? potential sign of abandonment of the rewriting the original raylib samples? (not part of the actual samples/bin style raylib-rs "examples" at least)

potentially old rewrite attempt of the original raylib example: https://github.com/raylib-rs/raylib-rs/blob/unstable/showcase/src/example/audio/audio_raw_stream.rs

similar sign as previous comment, something about me not understanding what the current status of this showcase directory is vs the samples/bin directory (I cant seem to get the showcase stuff to work, as i think its out of date and abandoned im unsure though)

I will continue with a new rewrite of the audio_raw_stream.c from the main repo (but with no commented out code, just to have something up and running for me to learn from this review), so we might need to discuss more about how and where this example should be placed, and honestly whether or not my "size in bytes" -> "size in elements" is even a "fix" in the first place or if it is potentially a misunderstanding of the original design

I will include references to the new code (much less verbose than this current reply) in responses to the review comments to hopefully have a fresh look at a less divergent rewrite of the original audio_raw_stream.c

AmityWilder · 2025-07-08T00:06:16Z

raylib/src/core/audio.rs

-    // }}
+    /// Updates sound buffer with new data.
+    #[inline]
+    pub fn update<T: AudioSample>(&mut self, data: &[T]) {


It might be a good idea to either assert that size_of::<T>() is equal to self.0.sampleSize or somehow make that a restriction on T. Otherwise a user may pass an array of u8 to an audio stream formatted for f32.

Because samples is passed in elements instead of bytes, Raylib will multiply the element count by the sample size.

Consider the case of self.0.sampleSize=32, data=&[u8; 14]:
data contains 14 bytes of data.
UpdateSound sees that 14 elements are to be read.
self.0.sampleSize tells UpdateSound to expect each element to be 4 bytes.
14 elements, 4 bytes each, means UpdateSound expects data to contain 14*4=56 bytes.
The first 3 elements will contain incorrect, though within-bounds data.
Suppose UpdateSound attempts to read the f32 (not u8) at index 4: 4*4=16 bytes. data only contains 14 bytes. This results in an out-of-bounds position being accessed, which would be Undefined Behavior.

Additionally:
ffi::UpdateSound performs a memcpy into (&*self.0.stream.buffer).data without performing a bounds check nor resizing. If data contains more bytes than the buffer, the buffer will be overrun resulting in another case of UB.

Because Raylib doesn't perform this bounds check, Rust needs to do so instead to prevent unsoundness.

Example:

assert_eq!( self.0.stream.sampleSize.try_into() .expect("sampleSize should be 8, 16, or 32"), std::mem::size_of::<T>()*u8::BITS as usize, "update data format must match sound", ); assert!( data.len() <= self.0.frameCount.try_into() .expect("frameCount should be a valid memory allocation size"), "frames to update must fit in sound", );

Alternatively:

#[derive(Error, Debug)] pub enum UpdateSoundError { #[error("update data format must match sound")] SampleSizeMismatch, #[error("frames to update must fit in sound")] TooManyFrames, } pub fn update<T: AudioSample>(&mut self, data: &[T]) -> Result<(), UpdateSoundError> { if self.0.stream.sampleSize.try_into().expect("sampleSize should be 8, 16, or 32") != std::mem::size_of::<T>()*u8::BITS as usize { Err(UpdateSoundError::SampleSizeMismatch) } else if data.len() > self.0.frameCount.try_into().expect("frameCount should be a valid memory allocation size") { Err(UpdateSoundError::TooManyFrames) } else { // ... Ok(()) } }

I have made my final draft i think of the full review and fixes. The code itself embeds the notes that remain to be addressed, optionally involving raylib repo side/upstream discussion. these notes are a kind of gathering of the discord discussions

raylib-rs/raylib/src/core/error.rs

Line 38 in 5c8f8e1

/// **Notes** (iann): if raylib upstream discussion introduces any of these checks, we might simplify these to avoid any redundancy i think

raylib-rs/raylib/src/core/audio.rs

Line 417 in 5c8f8e1

/// **Notes** (iann):

I will address the AudioStreamCallback in a separate PR I think, unless you think it would be better to include in this PR

…ginal raylib c example

….com/raylib-rs/raylib-rs/pull/212/files#r2191026194

match the

Unavailable

meisei4 added 3 commits July 7, 2025 12:03

update AudioStream update func with element size instead of byte size

2b7b9fc

proper commit, no commented out code

7a529df

remove commented out ffi code

a1f51bc

AmityWilder self-requested a review July 7, 2025 21:09

AmityWilder previously requested changes Jul 8, 2025

View reviewed changes

meisei4 added 7 commits July 8, 2025 15:54

beginnings of corrections from amy's review, moving closer to the ori…

cac5590

…ginal raylib c example

update closer to raylib core example

2dc5dc6

fix misunderstanding about casting issue locaiton from https://github…

98a3052

….com/raylib-rs/raylib-rs/pull/212/files#r2191026194

update Sound update and AudioStream Update with added test

7d14cda

Merge branch 'raylib-rs:unstable' into unstable

c473ade

match the original raylib manual audio stream update behavior exactly

9cb1f7d

match the

Last review draft for PR, update comments, and test

5c8f8e1

Dacode45 approved these changes Sep 2, 2025

View reviewed changes

Dacode45 added this pull request to the merge queue Sep 2, 2025

Merged via the queue into raylib-rs:unstable with commit b6eff72 Sep 2, 2025

meisei4 mentioned this pull request Sep 8, 2025

Fixing raw AudioStream callback functionality #233

Merged

meisei4 mentioned this pull request Nov 21, 2025

[examples] Review audio examples audio_raw_stream(split) and audio_sound_loading to add more clarity raysan5/raylib#5350

Open

Fix the AudioStream update and Sound update to use element size rather than size in bytes #212

Fix the AudioStream update and Sound update to use element size rather than size in bytes #212

Uh oh!

Conversation

meisei4 commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisei4 Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AmityWilder Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meisei4 Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

meisei4 commented Jul 7, 2025 •

edited

Loading

meisei4 Jul 8, 2025 •

edited

Loading

AmityWilder Jul 9, 2025 •

edited

Loading

meisei4 Jul 14, 2025 •

edited

Loading