Add fd callback registration for vst3 for linux

[chairbender]

Relates to #7
Adds to #8

This is my attempt to make the gain example work on Linux for VST3 only. I AM able to get it running and see the GUI with this.

However, as this is my first PR for this repo, and in fact my first ever Rust PR, I highly expect this to have problems! I'm fine with closing it if it's completely off base, but would definitely appreciate any feedback to help me improve. As a mitigating factor, this is only a PR for another PR branch, not for the main branch. I absolutely do not want to compromise the principals of this project!

My approach was to look at the old code where the fd-based callback registration was in place and working and basically get it wired up within the new structure while making my best guess to try to match the organizing principles of the new code.

Please see the TODOs which call out places where I was especially unsure if it was the right approach. I do not want to litter the codebase with todos, these are only for the purposes of review and helping me remember what areas I was uncertain about. They will be removed.

Changes:

• add the file_descriptor / poll functions to the View trait (as it has access to the event loop structs which implement the as_raw_fd methods - it seemed the natural place to put these methods)
• add plug_frame to Vst3ViewHost to hold the IPlugFrame necessary for registering callbacks
• add back in linux::EventHandler, storing it directly under PlugView (seemed to match up best with where it lived in the old code).
  • its implementation is pretty similar to the old version, it just has to access the underlying poll method at a different location (get at the View)
  • one key distinction is that the EventHandler.state is wrapped in an UnsafeCell. The old code did not do this, but it seemed like the only way to allow the functions in EventHandler to access the state.view in order to call poll. I am really not sure if this is safe to do, or there is a better / safer way to do the same thing.
• event handler registration added to PlugView::new

EDIT:
One big question I have about this approach overall is that it seems to push boilerplate into the plugin, as it now has to always store event_loop, and always implement file_descriptor() and poll(). Shouldn't those be things that it's possible for the plugin to avoid having to configure? It seems like it's a way to avoid having coupler depend on portlight though, which certainly has its advantages.

I wonder though, is there a way to avoid the need for that boilerplate (small though it may be) while still keeping portlight and coupler themselves decoupled?

[chairbender]

was my biggest concern with adding EventHandler - I wasn't sure if its prior removal implied there was a better approach in mind that just hadn't been added yet.

[micahrj]

It wasn't removed for any explicit reason, it's just that more or less the entire VST3 backend was rewritten and it was never added back. I think it still makes the most sense to have an EventHandler struct like this.

Fundamentally what's necessary is for there to be some object implementing the IEventHandler and ITimerHandler interfaces, and beyond that we have a lot of freedom to split up or combine objects in the backend. For instance, it would be possible to have the PlugView implement those interfaces directly in addition to the IPlugView interface. It would even be possible to get rid of the separate PlugView object and have the main Component object implement all of the view-related interfaces as well.

[chairbender]

Hmm, I'm not sure how to do that (have PlugView implement the interfaces is straightforward, but registering the PlugView has been the tricky part...)

Today we register the handler inside of attached. Within that context, we don't seem to have the ComWrapper<PlugView> we would need in order to register the plugView ("self" only refers to the PlugView, not the ComWrapper<PlugView>). The only place that has the ComWrapper is the Component (via createView - and even in that case, it isn't seemingly saved anywhere within the framework).

Still trying to figure it out, probably missing some simple approach...

This compiles, (within .attach) but segfaults, and seems obviously wrong. Presented only to illustrate my issue:

if let Some(run_loop) = frame.cast::<IRunLoop>() {
                let stupid_view = ComWrapper::new(PlugView::new(&self.main_thread_state));
                run_loop.registerTimer(stupid_view.as_com_ref::<ITimerHandler>().unwrap().as_ptr(), 16);

                if let Some(fd) =
                    (*self.main_thread_state.get()).view.as_ref().unwrap().file_descriptor()
                {
                    run_loop.registerEventHandler(stupid_view.as_com_ref::<IEventHandler>().unwrap().as_ptr(), 16);
                }
            }

And here's the actual interface implementations I added FYI:

    impl<P: Plugin> IEventHandlerTrait for PlugView<P> {
        unsafe fn onFDIsSet(&self, _fd: FileDescriptor) {
            let state = unsafe { &mut *self.main_thread_state.get() };
            state.view.as_mut().unwrap().poll();
        }
    }

[chairbender]

I may have found a way, but it requires saving a raw "self pointer" in PlugView. It seems like this might be a recurring issue with COM objects - not being able to access themselves as COM objects. Requiring the need for a self pointer. Unless I'm missing something (seems like it'd be much easier if all the VST3 hooks also provided the com ptr themselves...).

[micahrj]

Yeah, this is specifically a limitation of the way the bindings are set up in the vst3 crate. The traits all take &self, but there isn't a good way to go from a &self reference to an interface pointer like *mut IEventHandler. I've spent some time thinking about what it would take to make it possible in the vst3 bindings, but for now I think that saving a raw self pointer is probably the best way to go about it.

[chairbender]

Not exactly clear on how the fd registration stuff works yet, but is it really necessary for MainEventLoop to act as the fd for the purposes of x11 event handlers? Could the fd just be any old Rc that coupler maintains instead and avoid this need to depend on the portlight MainEventLoop (indirectly)? Would also seemingly bypass this boilerplate if that's an option.

[chairbender]

FYI, I got this working for CLAP as well in another branch but I'll keep those changes separate for now to keep this PR from getting out of control.

[micahrj]

EDIT: One big question I have about this approach overall is that it seems to push boilerplate into the plugin, as it now has to always store event_loop, and always implement file_descriptor() and poll(). Shouldn't those be things that it's possible for the plugin to avoid having to configure? It seems like it's a way to avoid having coupler depend on portlight though, which certainly has its advantages.

I wonder though, is there a way to avoid the need for that boilerplate (small though it may be) while still keeping portlight and coupler themselves decoupled?

My eventual plan for this is to introduce an adapter with a generic helper implementation of the View trait that plugins can use instead of implementing it manually. Something like this:

impl Plugin for MyPlugin {
    // ...

    type View = adapter::PluginView<MyView>;

    fn view(&mut self, host: ViewHost, parent: &ParentWindow) -> Self::View {
        adapter::PluginView::new(parent, MyView::new())
    }

    // ...
}

I definitely don't intend on having every plugin manually implement all of the event loop boilerplate, but I think that can be solved with helpers like this rather than by explicitly tying coupler and portlight together.

[micahrj]

Not exactly clear on how the fd registration stuff works yet, but is it really necessary for MainEventLoop to act as the fd for the purposes of x11 event handlers? Could the fd just be any old Rc that coupler maintains instead and avoid this need to depend on the portlight MainEventLoop (indirectly)? Would also seemingly bypass this boilerplate if that's an option.

It is necessary to hook up the fd from portlight's X11 connection to the host's event loop in some way or other, because events from that connection need to be able to notify the host to call the plugin's onFDIsSet/on_fd callback.

[chairbender]

Thanks for all the great comments, exactly what I was hoping to learn. I've been suddenly busier than usual so haven't gotten a chance to follow up on this but I'm still planning to once able!

[chairbender]

oookay finally found some time to sit down and make some improvements....

I wanted to try your suggestion of implementing the callbacks directly on PlugView. I've removed EventHandler and done so.

The big challenge I ran into was figuring out how to take the PlugView self reference in attached and turn it into an IEventHandler COM object. It seems like a general issue with COM hooks / callbacks. The pattern that I used to solve it was a self_ptr reference:

pub struct PlugView<P: Plugin> {
    main_thread_state: Arc<UnsafeCell<MainThreadState<P>>>,
    self_ptr: Cell<Option<*mut IPlugView>>,
}

Then populating that in the new method (and refactoring new to also handle the various COM shenanigans instead of directly returning a new PlugView

impl<P: Plugin> PlugView<P> {
    pub fn new(main_thread_state: &Arc<UnsafeCell<MainThreadState<P>>>) -> ComPtr<IPlugView> {
        let view = ComWrapper::new(PlugView {
            main_thread_state: main_thread_state.clone(),
            self_ptr: Cell::new(None),
        });
        let com_ptr = view.to_com_ptr::<IPlugView>().unwrap();
        let raw_ptr = com_ptr.as_ptr();
        view.self_ptr.set(Some(raw_ptr));
        com_ptr
    }
}

I opted to call it new as opposed to something like new_com even though it returns a ComPtr because I figured, if you're trying to construct a PlugView outside of COM, probably you are doing something wrong, so it's good if you're trying to use ::new and get an unexpected ComPtr as it makes you stop and think.

Then this abomination is needed to actually safely turn that raw ptr into the needed interface. I was only able to figure it out thanks to the other usage of .queryInterface I saw in this repo.

macro_rules! query_interface {
    ($source_ptr:expr, $interface_type:ty) => {{
        let mut result_obj: *mut c_void = std::ptr::null_mut();
        let unknown_ptr = $source_ptr as *mut FUnknown;
        let iid = <$interface_type>::IID;

        let result = unsafe {
            ((*(*unknown_ptr).vtbl).queryInterface)(
                unknown_ptr,
                iid.as_ptr() as *const TUID,
                &mut result_obj,
            )
        };

        if result == kResultOk && !result_obj.is_null() {
            Some(result_obj as *mut $interface_type)
        } else {
            None
        }
    }};
}

// usage
 if let Some(timer_handler_ptr) = query_interface!(ptr, ITimerHandler) {
                        run_loop.registerTimer(timer_handler_ptr, 16);
                        ...

WDYT of this approach?

I cleaned up the other todos and stuff, let me know what else should be changed to hopefully make this merge-worthy!

[micahrj]

Then this abomination is needed to actually safely turn that raw ptr into the needed interface. I was only able to figure it out thanks to the other usage of .queryInterface I saw in this repo.

There are a couple of ways that you can use the machinery in the vst3 crate to take care of the interface casting rather than doing it completely manually like in that query_interface! macro. If you have a ComWrapper<PlugView>, you can use ComWrapper::to_com_ptr::<ITimerHandler>, and if you start with a ComPtr<IPlugView>, you can use ComPtr::cast::<ITimerHandler> (and the same goes for ComRef).

[chairbender]

There are a couple of ways that you can use the machinery in the vst3 crate to take care of the interface casting rather than doing it completely manually like in that query_interface! macro. If you have a ComWrapper<PlugView>, you can use ComWrapper::to_com_ptr::<ITimerHandler>, and if you start with a ComPtr<IPlugView>, you can use ComPtr::cast::<ITimerHandler> (and the same goes for ComRef).

Ah, okay. I should have investigated the vst3 crate more. I was expecting it to be a little tricky to work with the vst3 api but I suppose that weird macro was beyond what I should've expected.

I've refactored it to use ComRef and cast.

Anything else I should fix?

[micahrj]

Moving this impl Class into the linux module will cause the build to fail on other platforms since it is now conditional on #[cfg(target_os = "linux")]. You could fix this by instead changing the outer impl Class to:

impl<P: Plugin> Class for PlugView<P> {
    #[cfg(not(target_os = "linux"))]
    type Interfaces = (IPlugView,);
    #[cfg(target_os = "linux")]
    type Interfaces = (IEventHandler, ITimerHandler, IPlugView);
}

However, to be honest, after seeing both implementations, I think it would be cleaner to just define a separate EventHandler struct holding an Arc<UnsafeCell<MainThreadState<P>>>. I'm sorry to ask you to make that change after you put in the time to get this version working!

[micahrj]

It's a relatively minor thing, but I think I would rather store a flag (or maybe an Option<*mut IEventHandler*>) for whether we've registered an event handler and use that to determine whether to unregister it, rather than calling file_descriptor again. My reasoning is that a misbehaving Plugin impl could in principle return different things from subsequent calls to file_descriptor, so the return value of file_descriptor is not necessarily a reliable indicator as to whether we registered one when initializing the view.