IAudioStreamPlayback::mix parameters could be made safe

[djcsdy]

IAudioStreamPlayback::mix is marked unsafe because it takes a raw pointer:

unsafe fn mix(&mut self, buffer: * mut AudioFrame, rate_scale: f32, frames: i32,) -> i32;

The first thing I do when implementing this function is to convert buffer to a slice:

unsafe fn mix(&mut self, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32 {
    let buffer = unsafe { std::slice::from_raw_parts_mut(buffer, frames as usize) }

    // ... body of function here (can be entirely safe code)

    frames
}

It'd be nice if godot-rust did this conversion for me before calling mix, which could then be made safe:

fn mix(&mut self, buffer: &mut [AudioFrame], rate_scale: f32) -> i32;

Godot calls mix from a non-main thread. I am making the assumption that Godot promises not to read or mutate buffer until mix returns. This seems a safe assumption, because it would be very silly if Godot violated this assumption, but I have not actually checked.

The fact that Godot calls mix from a non-main thread might also be justification on its own for keeping mix marked unsafe, but that's a separate issue.

[Bromeon]

Good idea! Like in #1129, we cannot automate this, but we could introduce a new mechanism that allows us to rewrite a custom-defined virtual function.

Do you know other APIs that would benefit from this? Maybe we could look at some taking pointers 🤔

---

Btw @Yarwin typically I use c: engine for anything coming from generated APIs. The label c: register is more used for anything user-defined that is registered with the engine (i.e. proc-macro API and related symbols).

[Bromeon]

This will likely have to wait until v0.4, as I'd prefer not having larger modifications for the soon-to-be-released v0.3, and it's breaking due to signature change. At least unless we rename it, but then that opens another can of works with two coexisting methods.

That shouldn't discourage you from experimenting though -- in fact, having a proof-of-concept early in the v0.3 cycle would give us some confidence to break the API in 0.4.0 release.

Furthermore, it would be good to batch changes of this kind together -- so if anyone comes across more pointer-based APIs that can be easily made safe, please add a comment here! 🙂

[Bromeon]

Worth noting that #649 faces a similar issue and could likely be addressed in the same vein, by manually defining safe APIs that hide the unsafe ones.

Another one, which also lists several virtual methods accepting raw pointers: #677

cc @lilizoey

[djcsdy]

I'm planning to collect together a list of functions that can be made safe in the same way as this one. Just from a brief scan of the code there are quite a few.

If anyone else has already done this, please let me know.

[Bromeon]

@djcsdy I did, added another link to my previous post.

[djcsdy]

Thanks!

I made a list of functions that take a pointer to a buffer and a length.

It should be possible to make all of these safe by wrapping the buffer in unsafe { std::slice::from_raw_parts(buffer, length) } or unsafe { std::slice::from_raw_parts_mut(buffer, length) }.

Function	ptr arg	length arg
IAudioEffectInstance::process	dst_buffer	frame_count
IAudioEffectSpectrumAnalyzerInstance::process	dst_buffer	frame_count
IAudioStreamGeneratorPlayback::mix_resampled	dst_buffer	frame_count
IAudioStreamGeneratorPlayback::mix	buffer	frames
IAudioStreamPlaybackInteractive::mix	buffer	frames
IAudioStreamPlaybackOggVorbis::mix	buffer	frames
IAudioStreamPlaybackPlaylist::mix	buffer	frames
IAudioStreamPlaybackResampled::mix_resampled	dst_buffer	frame_count
IAudioStreamPlaybackResampled::mix	buffer	frames
IAudioStreamPlaybackSynchronized::mix	buffer	frames
IMultiplayerPeerExtension::put_packet	p_buffer	p_buffer_size
IPacketPeerExtension::put_packet	p_buffer	p_buffer_size
IPhysicsDirectSpaceState2DExtension::intersect_point	results	max_results
IPhysicsDirectSpaceState2DExtension::intersect_shape	result	max_results
IPhysicsDirectSpaceState3DExtension::intersect_point	results	max_results
IPhysicsDirectSpaceState3DExtension::intersect_shape	result_count	max_results
IScriptLanguageExtension::profiling_get_accumulated_data	info_array	info_max
IScriptLanguageExtension::profiling_get_frame_data	info_array	info_max
IStreamPeerExtension::get_data	r_buffer	r_bytes
IStreamPeerExtension::get_partial_data	r_buffer	r_bytes
IStreamPeerExtension::put_data	p_data	p_bytes
IStreamPeerExtension::put_partial_data	p_data	p_bytes
ITextServerAdvanced::font_set_data_ptr	data_ptr	data_size
ITextServerExtension::font_set_data_ptr	data_ptr	data_size
IWebRtcDataChannelExtension::put_packet	p_buffer	p_buffer_size

IAudioStreamGeneratorPlayback::mix_resampled and IAudioStreamGeneratorPlayback::mix are listed for completeness but see #1129.

A lot of these are effectively duplicates because of inheritance.

I've only inspected the signature of these functions. I still need to verify that the semantics are as described.

There are also some functions that provide a pointer to a pointer to a buffer, and a pointer to a length. I haven't listed these as they'll need to be handled differently.

I've also excluded functions where the buffer is a *void as these will require more investigation.

I'll look into making a PR to make these functions safe.

[Bromeon]

Thanks so much, that's very appreciated! 👍

Maybe before jumping straight into implementation, we should check the approach. It's a number of methods that might be worth semi-automating (i.e. listing just the method and parameter names, and letting codegen handle the conversion to safe parameter), but if we find an easy way to write that code manually in a central place, that's probably also OK -- and likely more extensible for any sort of "special" semantics (void* or whatever).

[djcsdy]

Agreed, at the moment I am not sure if the best approach is to automate these or implement special cases.

I still need to double check the semantics of each of these functions. I think it'll become clearer after I've done that.

[Bromeon]

An idea:

We would list somewhere in special_cases.rs (or a submodule) the method names to be replaced, and their new signatures:

let replaced_virtual_methods = &[
    ("IAudioStreamPlayback", quote! { 
        fn mix(&mut self, buffer: &mut [AudioFrame], rate_scale: f32) -> usize;
        fn another_method(&self, ...);
    }),
    ...
];

(note that the method name is inferred from the signature).

The godot-codegen crate could then use the new signature for mix instead of the unsafe one. That covers the user-facing API, which is now safe.

What remains is the bridge from unsafe raw API to safe user-facing API. This part could also be generated by the compiler, in the form of a mapper trait:

trait VirtualMethodMapper {
    // This is the original signature, as declared by Godot.
    // It is parametrized by T, implementing the user-facing trait.
    fn mix<T>(this: &mut T, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32
    where T: IAudioStreamPlayback;

    // Here, all methods defined in `replaced_virtual_methods` are listed.
    fn another_method<T>(this: &T, ...)
    where T: IAudioStreamPlayback;

    ...
}

The resulting trait would land somewhere in godot-core. There could be a predefined type (e.g. TheOneMapper here), which the library calls to look up replacements.

enum TheOneMapper {} // uninhabitable

impl VirtualMethodMapper for TheOneMapper {
    fn mix<T>(this: &mut T, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32
    where
        T: IAudioStreamPlayback
    {
        // Manual code written by us.

        let buffer = unsafe { std::slice::from_raw_parts_mut(buffer, frames as usize) };

        // Forward to user-facing trait, convert return type.
        this.mix(buffer, rate_scale) as i32
    }

    // repeat for all trait functions
}

godot-rust code on dispatching virtual call:

<TheOneMapper as VirtualMethodMapper>::mix(self, buffer, rate_scale, frames)

It needs quite a bit of machinery to get going, but it would likely be very flexible. In contrast to fully auto-generated APIs, it would allow to take specific semantics into account, or do signature changes beyond ptr/len -> slice conversion. And possibly this mechanism could be reused elsewhere...

Anyway, just some brainstorming 🙂

[Bromeon]

I collected all Godot 4.4 methods that contained at least one pointer (basically * appearing in parameter or return type's name). Most of them are virtual (recognized from leading _), but a few aren't.

I didn't look at semantics, it's really just pointers with any sort of meaning. So as a start, your table of functions specifically working with (ptr, len) parameters may be more productive. But I thought it can't hurt to have both for completeness 🙂

The entries look like this:

MultiplayerPeerExtension::_get_packet

! (p)       r_buffer: const uint8_t **
! (p)  r_buffer_size: int32_t*
  (>)                 enum::Error

OpenXRAPIExtension::transform_from_pose

! (p)  pose: const void*
  (>)        Transform3D

with:

(p)         parameter
(>)         return type
!           contains pointer
[required]  virtual method must be overridden
[const]     &self instead of &mut self access

Just to be clear, I don't think an initial contribution should aim at changing all those signatures. On the contrary, we should probably start small with a handful methods and see if such an approach works out.

Here is the list.

[Bromeon]

Compatibility

One problem with my previous proposal is that every change from

unsafe fn mix(&mut self, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32;

to

fn mix(&mut self, buffer: &mut [AudioFrame], rate_scale: f32) -> usize;

is breaking. This means that we can only introduce such changes across minor versions, rather than gradually, and it may also be annoying for users.

A way forward

An idea that would allow us to incrementally migrate APIs, and also not run into problems when Godot adds new pointer methods that we don't yet handle:

1. All pointer methods are now renamed to get a special suffix, e.g. _raw, _unsafe or so:

   unsafe fn mix_unsafe(&mut self, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32;

2. We can gradually add safe "overloads" in the same trait, without breaking changes. These need to have a default implementation (to not break code), which can do nothing, as the methods are not directly invoked:

   fn mix(&mut self, buffer: &mut [AudioFrame], rate_scale: f32) -> usize {
       unimplemented!()
   }

3. Now the problem is that some of the unsafe methods are still required, meaning the method must be overridden in the user impl. We cannot make the safe overload required, and making it non-required risks introducing bugs if overriding is forgotten.

   We could however add some special code in the proc macro, that when a safe method is overridden, it will auto-override the unsafe one delegating to it. Overriding both will not be allowed.

   // impl ITrait for MyClass:
   fn mix(&mut self, buffer: &mut [AudioFrame], rate_scale: f32) -> usize {
       // user code
   }
   
   // proc-macro generated code:
   unsafe fn mix_unsafe(&mut self, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32 {
       // Delegates to outside impl that defines how to forward unsafe -> safe.
       <TheOneMapper as VirtualMethodMapper>::mix(self, buffer, rate_scale, frames)
   }

   This method is generated outside and provided by the user (as above):

   impl VirtualMethodMapper for TheOneMapper {
       fn mix<T>(this: &mut T, buffer: *mut AudioFrame, rate_scale: f32, frames: i32) -> i32
       where
           T: IAudioStreamPlayback
       {
           // manually written
           let buffer = unsafe { std::slice::from_raw_parts_mut(buffer, frames as usize) };
           this.mix(buffer, rate_scale) as i32
       }
   }

4. As soon as a safe method is available, we can add #[deprecated] attributes to the unsafe method, to create awareness of the new API.

5. In next minor versions, deprecated unsafe methods can be removed from the public API (maybe still declared internally but outside the trait, to allow dispatching).

[Bromeon]

@djcsdy any feedback on my latest proposal?

The only action for now is that all pointer-based virtual functions would be renamed to {function}_unchecked (or similar).

It's a breaking change now in 0.3.0, but should easy the path towards safe functions in 0.3.x and beyond.

[djcsdy]

I definitely agree with the renaming as a first step. It'll be a few days at least before I can look at this again in any detail.