gstreamer: Use more accurate types for Seqnum, GroupId and MetaSeqnum

For the latter introduce an actual opaque type that allows using them
for comparison purposes but is not just a plain u64.

For the former represent them as opaque type around an NonZeroU32. 0 is
the invalid case and does not happen in the majority of functions. Where
it can happen, represent this case by using an Option<_> instead.

This makes it harder to mis-use these types.

Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/issues/209

gstreamer/src/event.rs

@@ -15,11 +15,12 @@ use std::cmp;
 use std::ffi::CStr;
 use std::fmt;
 use std::mem;
+use std::num::NonZeroU32;
 use std::ops::Deref;
 use std::ptr;
 
 use glib;
-use glib::translate::{from_glib, from_glib_full, from_glib_none, FromGlib, ToGlib, ToGlibPtr};
+use glib::translate::{from_glib, from_glib_full, from_glib_none, ToGlib, ToGlibPtr};
 use glib::value::ToSendValue;
 
 #[cfg(any(feature = "v1_10", feature = "dox"))]
@@ -27,34 +28,27 @@ use glib::translate::FromGlibPtrContainer;
 
 use EventType;
 
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct Seqnum(pub u32);
+pub struct Seqnum(pub(crate) NonZeroU32);
-pub const SEQNUM_INVALID: Seqnum = Seqnum(0);
+
+impl Seqnum {
+    pub fn next() -> Self {
+        unsafe {
+            let v = gst_sys::gst_util_seqnum_next();
+            if v == 0 {
+                Seqnum::next()
+            } else {
+                Seqnum(NonZeroU32::new_unchecked(v))
+            }
+        }
+    }
+}
 
 impl ToGlib for Seqnum {
     type GlibType = u32;
 
     fn to_glib(&self) -> u32 {
-        self.0
+        self.0.get()
-    }
-}
-
-impl FromGlib<u32> for Seqnum {
-    fn from_glib(val: u32) -> Seqnum {
-        skip_assert_initialized!();
-        Seqnum(val)
-    }
-}
-
-impl Into<u32> for Seqnum {
-    fn into(self) -> u32 {
-        self.0
-    }
-}
-
-impl From<u32> for Seqnum {
-    fn from(v: u32) -> Seqnum {
-        Seqnum(v)
     }
 }
 
@@ -67,40 +61,25 @@ impl cmp::PartialOrd for Seqnum {
 impl cmp::Ord for Seqnum {
     fn cmp(&self, other: &Seqnum) -> cmp::Ordering {
         unsafe {
-            let ret = gst_sys::gst_util_seqnum_compare(self.0, other.0);
+            let ret = gst_sys::gst_util_seqnum_compare(self.0.get(), other.0.get());
             ret.cmp(&0)
         }
     }
 }
 
-#[derive(Debug, Clone, Copy, Default, PartialEq, Eq)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub struct GroupId(pub u32);
+pub struct GroupId(pub(crate) NonZeroU32);
-pub const GROUP_ID_INVALID: GroupId = GroupId(0);
 
-impl ToGlib for GroupId {
+impl GroupId {
-    type GlibType = u32;
+    pub fn next() -> Self {
-
+        unsafe {
-    fn to_glib(&self) -> u32 {
+            let v = gst_sys::gst_util_group_id_next();
-        self.0
+            if v == 0 {
-    }
+                GroupId::next()
-}
+            } else {
-
+                GroupId(NonZeroU32::new_unchecked(v))
-impl Into<u32> for GroupId {
+            }
-    fn into(self) -> u32 {
+        }
-        self.0
-    }
-}
-
-impl From<u32> for GroupId {
-    fn from(v: u32) -> GroupId {
-        GroupId(v)
-    }
-}
-
-impl FromGlib<u32> for GroupId {
-    fn from_glib(val: u32) -> GroupId {
-        skip_assert_initialized!();
-        GroupId(val)
     }
 }
 
@@ -164,7 +143,11 @@ gst_define_mini_object_wrapper!(Event, EventRef, gst_sys::GstEvent, [Debug,], ||
 
 impl EventRef {
     pub fn get_seqnum(&self) -> Seqnum {
-        unsafe { from_glib(gst_sys::gst_event_get_seqnum(self.as_mut_ptr())) }
+        unsafe {
+            let seqnum = gst_sys::gst_event_get_seqnum(self.as_mut_ptr());
+            assert_ne!(seqnum, 0);
+            Seqnum(NonZeroU32::new_unchecked(seqnum))
+        }
     }
 
     pub fn get_running_time_offset(&self) -> i64 {
@@ -544,13 +527,18 @@ impl<'a> StreamStart<'a> {
         }
     }
 
-    pub fn get_group_id(&self) -> GroupId {
+    pub fn get_group_id(&self) -> Option<GroupId> {
         unsafe {
             let mut group_id = mem::MaybeUninit::uninit();
 
             gst_sys::gst_event_parse_group_id(self.as_mut_ptr(), group_id.as_mut_ptr());
 
-            from_glib(group_id.assume_init())
+            let group_id = group_id.assume_init();
+            if group_id == 0 {
+                None
+            } else {
+                Some(GroupId(NonZeroU32::new_unchecked(group_id)))
+            }
         }
     }
 }
@@ -658,7 +646,9 @@ impl<'a> StreamGroupDone<'a> {
 
             gst_sys::gst_event_parse_stream_group_done(self.as_mut_ptr(), group_id.as_mut_ptr());
 
-            from_glib(group_id.assume_init())
+            let group_id = group_id.assume_init();
+            assert_ne!(group_id, 0);
+            GroupId(NonZeroU32::new_unchecked(group_id))
         }
     }
 }
@@ -1000,7 +990,7 @@ macro_rules! event_builder_generic_impl {
             unsafe {
                 let event = $new_fn(&mut self);
                 if let Some(seqnum) = self.builder.seqnum {
-                    gst_sys::gst_event_set_seqnum(event, seqnum.to_glib());
+                    gst_sys::gst_event_set_seqnum(event, seqnum.0.get());
                 }
 
                 if let Some(running_time_offset) = self.builder.running_time_offset {
@@ -1092,7 +1082,7 @@ impl<'a> StreamStartBuilder<'a> {
             gst_sys::gst_event_set_stream_flags(ev, flags.to_glib());
         }
         if let Some(group_id) = s.group_id {
-            gst_sys::gst_event_set_group_id(ev, group_id.to_glib());
+            gst_sys::gst_event_set_group_id(ev, group_id.0.get());
         }
         ev
     });
@@ -1233,7 +1223,7 @@ impl<'a> StreamGroupDoneBuilder<'a> {
     }
 
     event_builder_generic_impl!(|s: &Self| gst_sys::gst_event_new_stream_group_done(
-        s.group_id.to_glib()
+        s.group_id.0.get()
     ));
 }
 

gstreamer/src/functions.rs

@@ -83,28 +83,6 @@ pub fn parse_launchv_full(
     }
 }
 
-pub fn util_group_id_next() -> ::GroupId {
-    assert_initialized_main_thread!();
-    unsafe {
-        let v = from_glib(gst_sys::gst_util_group_id_next());
-        if v == ::GROUP_ID_INVALID {
-            return from_glib(gst_sys::gst_util_group_id_next());
-        }
-        v
-    }
-}
-
-pub fn util_seqnum_next() -> ::Seqnum {
-    assert_initialized_main_thread!();
-    unsafe {
-        let v = from_glib(gst_sys::gst_util_seqnum_next());
-        if v == ::SEQNUM_INVALID {
-            return from_glib(gst_sys::gst_util_seqnum_next());
-        }
-        v
-    }
-}
-
 #[cfg(any(feature = "v1_12", feature = "dox"))]
 pub fn calculate_linear_regression(
     xy: &[(u64, u64)],

gstreamer/src/lib.rs

@@ -116,6 +116,8 @@ pub use tags::{
 mod tags_serde;
 
 pub mod meta;
+#[cfg(any(feature = "v1_16", feature = "dox"))]
+pub use meta::MetaSeqnum;
 #[cfg(any(feature = "v1_14", feature = "dox"))]
 pub use meta::ReferenceTimestampMeta;
 pub use meta::{Meta, MetaAPI, MetaRef, MetaRefMut, ParentBufferMeta};
@@ -141,7 +143,7 @@ mod bufferlist_serde;
 pub mod query;
 pub use query::{Query, QueryRef, QueryView};
 pub mod event;
-pub use event::{Event, EventRef, EventView, GroupId, Seqnum, GROUP_ID_INVALID, SEQNUM_INVALID};
+pub use event::{Event, EventRef, EventView, GroupId, Seqnum};
 pub mod context;
 pub use context::{Context, ContextRef};
 mod static_caps;

gstreamer/src/message.rs

@@ -20,6 +20,7 @@ use TagList;
 use std::ffi::CStr;
 use std::fmt;
 use std::mem;
+use std::num::NonZeroU32;
 use std::ops::Deref;
 use std::ptr;
 
@@ -39,7 +40,11 @@ impl MessageRef {
     }
 
     pub fn get_seqnum(&self) -> Seqnum {
-        unsafe { from_glib(gst_sys::gst_message_get_seqnum(self.as_mut_ptr())) }
+        unsafe {
+            let seqnum = gst_sys::gst_message_get_seqnum(self.as_mut_ptr());
+            assert_ne!(seqnum, 0);
+            Seqnum(NonZeroU32::new_unchecked(seqnum))
+        }
     }
 
     pub fn get_structure(&self) -> Option<&StructureRef> {
@@ -1040,7 +1045,12 @@ impl<'a> StreamStart<'a> {
                 self.as_mut_ptr(),
                 group_id.as_mut_ptr(),
             )) {
-                Some(from_glib(group_id.assume_init()))
+                let group_id = group_id.assume_init();
+                if group_id == 0 {
+                    None
+                } else {
+                    Some(GroupId(NonZeroU32::new_unchecked(group_id)))
+                }
             } else {
                 None
             }
@@ -1308,7 +1318,7 @@ macro_rules! message_builder_generic_impl {
                 let src = self.builder.src.to_glib_none().0;
                 let msg = $new_fn(&mut self, src);
                 if let Some(seqnum) = self.builder.seqnum {
-                    gst_sys::gst_message_set_seqnum(msg, seqnum.to_glib());
+                    gst_sys::gst_message_set_seqnum(msg, seqnum.0.get());
                 }
 
                 #[cfg(any(feature = "v1_14", feature = "dox"))]
@@ -2179,7 +2189,7 @@ impl<'a> StreamStartBuilder<'a> {
     message_builder_generic_impl!(|s: &mut Self, src| {
         let msg = gst_sys::gst_message_new_stream_start(src);
         if let Some(group_id) = s.group_id {
-            gst_sys::gst_message_set_group_id(msg, group_id.to_glib());
+            gst_sys::gst_message_set_group_id(msg, group_id.0.get());
         }
         msg
     });
@@ -2473,10 +2483,11 @@ mod tests {
         ::init().unwrap();
 
         // Message without arguments
-        let eos_msg = Message::new_eos().seqnum(Seqnum(1)).build();
+        let seqnum = Seqnum::next();
+        let eos_msg = Message::new_eos().seqnum(seqnum).build();
         match eos_msg.view() {
             MessageView::Eos(eos_msg) => {
-                assert_eq!(eos_msg.get_seqnum(), Seqnum(1));
+                assert_eq!(eos_msg.get_seqnum(), seqnum);
                 assert!(eos_msg.get_structure().is_none());
             }
             _ => panic!("eos_msg.view() is not a MessageView::Eos(_)"),
@@ -2497,13 +2508,14 @@ mod tests {
     fn test_other_fields() {
         ::init().unwrap();
 
+        let seqnum = Seqnum::next();
         let eos_msg = Message::new_eos()
             .other_fields(&[("extra-field", &true)])
-            .seqnum(Seqnum(1))
+            .seqnum(seqnum)
             .build();
         match eos_msg.view() {
             MessageView::Eos(eos_msg) => {
-                assert_eq!(eos_msg.get_seqnum(), Seqnum(1));
+                assert_eq!(eos_msg.get_seqnum(), seqnum);
                 if let Some(other_fields) = eos_msg.get_structure() {
                     assert!(other_fields.has_field("extra-field"));
                 }

gstreamer/src/meta.rs

@@ -71,6 +71,10 @@ pub unsafe trait MetaAPI: Sync + Send + Sized {
     }
 }
 
+#[cfg(any(feature = "v1_16", feature = "dox"))]
+#[derive(Debug, Clone, Copy, PartialOrd, Ord, PartialEq, Eq)]
+pub struct MetaSeqnum(u64);
+
 pub struct MetaRef<'a, T: MetaAPI + 'a> {
     meta: &'a T,
     buffer: &'a BufferRef,
@@ -148,10 +152,10 @@ impl<'a, T: MetaAPI> MetaRef<'a, T> {
     }
 
     #[cfg(any(feature = "v1_16", feature = "dox"))]
-    pub fn get_seqnum(&self) -> u64 {
+    pub fn get_seqnum(&self) -> MetaSeqnum {
         unsafe {
             let meta = self.meta as *const _ as *const gst_sys::GstMeta;
-            gst_sys::gst_meta_get_seqnum(meta)
+            MetaSeqnum(gst_sys::gst_meta_get_seqnum(meta))
         }
     }