From 88a52d9ea14512acda8982a6617cf909891b3945 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fran=C3=A7ois=20Laignel?= Date: Wed, 11 Sep 2024 11:11:28 +0200 Subject: [PATCH] gst: anticipate GQuark to GstIdStr lifetime changes GStreamer fixes a memory leak due to GQuarks by switching to GstIdStr. The consequence is that strings previously backed by a GQuark returned by a function will now get their lifetime bound to that of its owner, while the GQuark version ensured static lifetime. Because some functions return a string with the assumption that they are static and because we can't alter the API for existing versions of the bindings, this MR temporarily forces affected strings as GQuarks, thus gaining static lifetime regardless of the GStreamer version actually being used. For newer versions of the bindings, the API will be fixed and GQuarks will be removed in favor a leakless solution. See: https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/7432 Part-of: --- gstreamer/src/caps_features.rs | 14 ++++++++++++++ gstreamer/src/structure.rs | 32 ++++++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gstreamer/src/caps_features.rs b/gstreamer/src/caps_features.rs index 226c5d70c..239f1b60c 100644 --- a/gstreamer/src/caps_features.rs +++ b/gstreamer/src/caps_features.rs @@ -401,6 +401,8 @@ impl CapsFeaturesRef { return None; } + // Safety: we can return a GStr based on the feature here because + // the lifetime of the returned value is constrained by &self. Some(glib::GStr::from_ptr(feature)) } } @@ -573,6 +575,8 @@ impl<'a> Iterator for Iter<'a> { self.idx += 1; + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(glib::GStr::from_ptr(feature)) } } @@ -599,6 +603,9 @@ impl<'a> Iterator for Iter<'a> { let feature = ffi::gst_caps_features_get_nth(self.caps_features.as_ptr(), end as u32); debug_assert!(!feature.is_null()); + + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(glib::GStr::from_ptr(feature)) } } @@ -614,6 +621,9 @@ impl<'a> Iterator for Iter<'a> { self.n_features as u32 - 1, ); debug_assert!(!feature.is_null()); + + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(glib::GStr::from_ptr(feature)) } } @@ -633,6 +643,8 @@ impl<'a> DoubleEndedIterator for Iter<'a> { ffi::gst_caps_features_get_nth(self.caps_features.as_ptr(), self.n_features as u32); debug_assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(glib::GStr::from_ptr(feature)) } } @@ -651,6 +663,8 @@ impl<'a> DoubleEndedIterator for Iter<'a> { ); debug_assert!(!feature.is_null()); + // Safety: we can return a GStr based on the feature here because the lifetime + // of the returned Item is constrained by the underlying CapsFeatureRef. Some(glib::GStr::from_ptr(feature)) } } diff --git a/gstreamer/src/structure.rs b/gstreamer/src/structure.rs index 365582b6a..c38ea15d2 100644 --- a/gstreamer/src/structure.rs +++ b/gstreamer/src/structure.rs @@ -637,7 +637,11 @@ impl StructureRef { #[doc(alias = "get_name")] #[doc(alias = "gst_structure_get_name")] pub fn name<'a>(&self) -> &'a glib::GStr { - unsafe { glib::GStr::from_ptr(ffi::gst_structure_get_name(&self.0)) } + unsafe { + let name = ffi::gst_structure_get_name(&self.0); + // Ensure the name is static whatever the GStreamer version being used. + glib::GStr::from_ptr(glib::ffi::g_intern_string(name)) + } } #[doc(alias = "gst_structure_get_name_id")] @@ -744,7 +748,8 @@ impl StructureRef { let field_name = ffi::gst_structure_nth_field_name(&self.0, idx as u32); debug_assert!(!field_name.is_null()); - Some(glib::GStr::from_ptr(field_name)) + // Ensure the name is static whatever the GStreamer version being used. + Some(glib::GStr::from_ptr(glib::ffi::g_intern_string(field_name))) } } @@ -1099,6 +1104,8 @@ impl<'a> Iterator for FieldIterator<'a> { return None; } + // Safety: nth_field_name() ensures static lifetime for the returned string, + // whatever the GStreamer version being used. let field_name = self.structure.nth_field_name(self.idx).unwrap(); self.idx += 1; @@ -1119,6 +1126,8 @@ impl<'a> DoubleEndedIterator for FieldIterator<'a> { } self.n_fields -= 1; + // Safety: nth_field_name() ensures static lifetime for the returned string, + // whatever the GStreamer version being used. Some(self.structure.nth_field_name(self.n_fields).unwrap()) } } @@ -1129,6 +1138,8 @@ impl<'a> std::iter::FusedIterator for FieldIterator<'a> {} #[derive(Debug)] pub struct Iter<'a> { + // Safety: FieldIterator ensures static lifetime for the returned Item, + // whatever the GStreamer version being used. iter: FieldIterator<'a>, } @@ -1480,4 +1491,21 @@ mod tests { assert!(!s.has_field("array")); assert!(!s.has_field("list")); } + + #[test] + fn test_name_and_field_name_lt() { + crate::init().unwrap(); + + let (name, field_name) = { + let s = Structure::builder("name") + .field("field0", "val0") + .field("field1", "val1") + .build(); + + (s.name(), s.nth_field_name(0).unwrap()) + }; + + assert_eq!(name, "name"); + assert_eq!(field_name, "field0"); + } }