macos: Support user-created wgpu surfaces. about baseview HOT 5 OPEN

Several months ago, there was a discussion on the Rust Audio Discord about a change to Baseview's API contract that would resolve this type of issue in a more principled way, by requiring that windows only be closed via an explicit call to WindowHandle::close() or Window::close(). I'll outline the reasoning for this proposal below.

Fundamentally, the reason for this issue is that on macOS, when running as a guest window in a plugin scenario, Baseview relies on the signal of the host releasing its reference(s) to the NSView to know when to perform cleanup. This decision was made because of the way the Audio Unit API handles the editor UI: the plugin implements a particular factory method, uiViewForAudioUnit, which returns an NSView *, and ownership of that NSView then passes to the host. It's then up to the host to manage the lifetime of the NSView, so the only fully reliable notification that the host is finished with it is when its retain count reaches 0 and dealloc is called. From AUCocoaUIView.h:

/*!
	@function	uiViewForAudioUnit:withSize:
	@abstract	Return the NSView responsible for displaying the interface for the provided AudioUnit.
	@param		inAudioUnit
					The Audio Unit the view is associated with.
	@param		inPreferredSize
					The preferred size of the view to be returned.
	@result		An NSView.
	
	@discussion
				This method is a factory function: each call to it must return a unique view.
				Each view must be returned with a retain count of 1 and autoreleased.
				It is the client's responsibility to retain the returned view and to release the view when it's no longer needed.
*/
- (NSView *)uiViewForAudioUnit:(AudioUnit)inAudioUnit withSize:(NSSize)inPreferredSize;

However, this situation is completely unique to the Audio Unit API; this is not how things work in any other situation. When Baseview is used to create a standalone window, client code is entirely in charge of when that window is closed, so there's no need to watch the retain count to know when to perform cleanup. Likewise, in every single plugin API that isn't Audio Unit (including VST2, VST3, CLAP, LV2, and AAX), there is an explicit call in the plugin API by which the host tells the plugin to close its editor GUI. Waiting until the retain count hits zero happens to work in most situations, since when the host's container view is destroyed it releases its reference to Baseview's NSView, but it's still semantically incorrect, since in each of those APIs the plugin should finish destroying and cleaning up its editor GUI before returning from the "close editor" call, and the host will only destroy the container view after that call.

So, the current design implements every other plugin API incorrectly on macOS, and introduces other issues besides (like the retain cycle with wgpu surfaces), because it's based around the exceptional case of the AU API. There's an alternate design which allows Baseview to expose the same window lifetime behavior on all platforms, which still supports the AU use case: we declare that Baseview does not support the use case of creating an unparented NSView and passing ownership to a host application (so, we get rid of the open_as_if_parented method), and make it the responsibility of the plugin framework to create its own wrapper NSView to be returned from uiViewForAudioUnit. Then, the plugin framework can expose the same API around opening and closing the editor UI for AU as it does for every other plugin format: at UI creation time, the framework can pass the wrapper NSView as the parent window, and then it can override dealloc for that wrapper to explicitly make a "close editor" call. This is actually the approach used by JUCE.

This design resolves the issue with circular references in wgpu: since the window is always destroyed due to an explicit call from client code, Baseview isn't reliant on release or dealloc for knowing when to clean up, and it doesn't cause issues that wgpu increments the retain count. Requiring that window cleanup can only be initiated by client code fixes some other outstanding issues as well. Currently, Baseview leaks its NSView subclass, since final cleanup happens in its overridden implementation of release, and it's not possible to unregister a class while inside a method defined on that class. There's a similar issue on Windows, where Baseview's call to UnregisterClassW always fails, since it happens from inside the WNDPROC, so the WNDCLASS ends up getting leaked there as well.

Implementing this change would require some careful thought about the order of operations during window destruction, especially on macOS, but it wouldn't actually involve a lot of code, and I think there's a strong case for it as the correct solution to these issues.

from baseview.

There's one hacky option: require users to decrement retainCount when adding a wgpu surface, initially via a direct msg_send! snippet or via an API call.

from baseview.

I think the proper solution here is to just remove all of the (broken) retain_count_after_build logic and add an explicit integration with wgpu to handle breaking the reference cycle (it could be in a separate "baseview_wgpu" crate, but I think it would work fine just being behind a feature the way OpenGL surface creation currently is). The create_surface method in wgpu is unsafe anyway, so it makes sense for there to have to be an explicit integration.

from baseview.

I like that idea a lot.

I looked at how iced_baseview works, and for wgpu it calls iced_graphics' compositor trait method:

https://github.com/iced-rs/iced/blob/260bbc5690b921e678d02eeb7a558c48874544d0/graphics/src/window/compositor.rs#L25-L31

Which just wraps create_surface, as you can imagine. For something like a baseview_wgpu crate, I'm not as familiar with these moving parts to figure out how best to work with iced_graphics. Do you have any thoughts here?

Or, alternatively, is the issue that wgpu surfaces don't get correctly "registered" in an autorelease pool (again, not familiar with this problem space at all, so these are probably naive questions)? It seems like freeing the surface would break any circular references, so if we could somehow fix upstream resource management, would that solve our problem?

from baseview.

Thank you for the context and thorough answer! I agree with your comment on #137 and that we should probably close it -- I don't need to target AU for now, and as you said, all other plugin APIs (and specifically, their hosts) reliably send "close" requests.

For additional context on my end, I originally ran into this problem when integrating baseview into other Rust toolkits that previously relied on winit's CloseRequested event to manage cleanup and drop resources. I've since worked out better ways to address that problem, so I'm no longer blocked by this issue. And, as you said, the correct way to handle this is to fix the API. For now, I'll plan my "host-to-GUI" handshake around explicit close requests.

from baseview.