arcnmx / wireplumber.rs

wireplumber rust bindings
https://arcnmx.github.io/wireplumber.rs/main/wireplumber/
MIT License
21 stars 3 forks source link

Problem with Metadata::find #29

Closed saivert closed 1 year ago

saivert commented 1 year ago

Give this code:

pub(crate) fn default_target(&self) -> Option<PwNodeObject> {
        let app = PwvucontrolApplication::default();
        let om = app.imp().wp_object_manager.get().unwrap();
        if let Some(metadata) = app.imp().metadata.borrow().as_ref() {

            if let (Some(target_serial), _) = metadata.find(self.boundid(), "target.object") {
                if target_serial != "-1" {
                    if let Some(sinknode) = om.lookup([
                        Constraint::compare(ConstraintType::PwProperty, "object.serial", target_serial.as_str(), true),
                    ].iter().collect::<Interest<wp::pw::Node>>()) {
                        return app.imp().nodemodel.get_node(sinknode.bound_id()).ok();
                    };
                }
            }
        } else {
            wp::log::warning!("Cannot get metadata object");
        };
        None
    }

This always leads to a assert failure in from_glib_full where it looks like the pointer is always null.

I tried fixing it like this:

    fn find(&self, subject: u32, key: &str) -> (Option<glib::GString>, glib::GString) {
        use glib::translate::*;
        use std::{mem::MaybeUninit, ffi::c_char};
        unsafe {
            let mut type_: MaybeUninit<*const c_char> = MaybeUninit::uninit();

            let ret = from_glib_none(
                wp::ffi::wp_metadata_find(self.as_ref().to_glib_none().0, subject, key.to_glib_none().0, type_.as_mut_ptr())
            );
            dbg!(type_);
            (ret, from_glib_none(type_.assume_init())) // This should really be from_glib_none according to docs (transfer none)
        }
    }

But then I get this error:

thread 'main' panicked at 'C string is not valid utf-8', /home/saivert/Projects/pwvucontrol/builddir/cargo-home/registry/src/index.crates.io-6f17d22bba15001f/glib-0.18.1/src/gstring.rs:566:9

I think this is the correct way to fix it:

    fn find(&self, subject: u32, key: &str) -> (Option<glib::GString>, glib::GString) {
        use glib::translate::*;
        unsafe fn get_type(type_: *const i8) -> glib::GString {
            if type_ == std::ptr::null() {
                "".into()
            } else {
                from_glib_none(type_)
            }
        }
        unsafe {
            let mut type_ = std::ptr::null();

            let ret: Option<glib::GString> = from_glib_none(
                wp::ffi::wp_metadata_find(self.as_ref().to_glib_none().0, subject, key.to_glib_none().0, &mut type_)
            );
            if ret.is_some() {
                return (ret, get_type(type_));
            }
            (None, "".into())
        }
    }

Not valid to try to get the type name unless you actually find a metadata item and also not valid if the type is a null pointer. This should handle both cases.

Maybe it is better if the return type is simply Option<(glib::GString, glib::GString)> since the whole thing is either valid or not.

Yes I'm not decided on whether this code should use MaybeUninit instead. I was trying to fix this code using as few modifications to the existing code as possible.

arcnmx commented 1 year ago

I pushed a corrected return type that should now catch null values as None, let me know if you still encounter problems.

Maybe it is better if the return type is simply Option<(glib::GString, glib::GString)> since the whole thing is either valid or not.

This part is now being tracked as part of #13; there are a number of functions with awkward return types that need cleaning up.