From 658be807c156dfdc0b6be96845d2575eb65da643 Mon Sep 17 00:00:00 2001 From: fengalin Date: Sat, 27 Jan 2018 12:52:03 +0100 Subject: [PATCH] Fix building argument-less messages Building an argument-less message such as eos yields an assertion failure due to the inner structure being null. The short term solution consists in checking that the inner `structure` is not `null` before attempting to insert `other_fields`. The consequence is that `others_fields` defined for argument-less messages will be ignored. A correction will be applied when GStreamer 1.14 is released thank to the introduction of `gst_message_writable_structure` (see https://bugzilla.gnome.org/show_bug.cgi?id=792928). Due to the dependency on GStreamer 1.14, the correction will be only available under the activation of a feature "v1_14". Events are not affected as the build method the availability of `gst_event_writable_structure` and this function "will never return NULL". However, we can avoid a `structure` allocation for argument-less messages without `other_fields`. --- gstreamer/src/event.rs | 48 ++++++++++++++++++++++++++++++++++- gstreamer/src/message.rs | 54 +++++++++++++++++++++++++++++++++++----- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/gstreamer/src/event.rs b/gstreamer/src/event.rs index dfe820138..973a1cafd 100644 --- a/gstreamer/src/event.rs +++ b/gstreamer/src/event.rs @@ -895,7 +895,7 @@ macro_rules! event_builder_generic_impl { ffi::gst_event_set_running_time_offset(event, running_time_offset); } - { + if !self.other_fields.is_empty() { let s = StructureRef::from_glib_borrow_mut( ffi::gst_event_writable_structure(event) ); @@ -1667,3 +1667,49 @@ impl<'a> CustomBothOobBuilder<'a> { ev }); } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_simple() { + ::init().unwrap(); + + // Event without arguments + let flush_start_evt = Event::new_flush_start().build(); + match flush_start_evt.view() { + EventView::FlushStart(flush_start_evt) => { + assert!(flush_start_evt.0.get_structure().is_none()); + }, + _ => panic!("flush_start_evt.view() is not an EventView::FlushStart(_)"), + } + + let flush_start_evt = Event::new_flush_start() + .other_fields(&[("extra-field", &true)]) + .build(); + match flush_start_evt.view() { + EventView::FlushStart(flush_start_evt) => { + assert!(flush_start_evt.0.get_structure().is_some()); + if let Some(other_fields) = flush_start_evt.0.get_structure() { + assert!(other_fields.has_field("extra-field")); + } + }, + _ => panic!("flush_start_evt.view() is not an EventView::FlushStart(_)"), + } + + // Event with arguments + let flush_stop_evt = Event::new_flush_stop(true) + .other_fields(&[("extra-field", &true)]) + .build(); + match flush_stop_evt.view() { + EventView::FlushStop(flush_stop_evt) => { + assert_eq!(flush_stop_evt.get_reset_time(), true); + assert!(flush_stop_evt.0.get_structure().is_some()); + if let Some(other_fields) = flush_stop_evt.0.get_structure() { + assert!(other_fields.has_field("extra-field")); + } + } + _ => panic!("flush_stop_evt.view() is not an EventView::FlushStop(_)"), + } + } +} diff --git a/gstreamer/src/message.rs b/gstreamer/src/message.rs index 8f0f714db..5cf572df2 100644 --- a/gstreamer/src/message.rs +++ b/gstreamer/src/message.rs @@ -1165,6 +1165,8 @@ macro_rules! message_builder_generic_impl { } } + // Warning: other_fields are ignored with argument-less messages + // until GStreamer 1.14 is released pub fn other_fields(self, other_fields: &[(&'a str, &'a ToSendValue)]) -> Self { Self { other_fields: self.other_fields.iter().cloned() @@ -1183,13 +1185,19 @@ macro_rules! message_builder_generic_impl { ffi::gst_message_set_seqnum(msg, seqnum.to_glib()); } - { - let s = StructureRef::from_glib_borrow_mut( - ffi::gst_message_get_structure(msg) as *mut _ - ); + if !self.other_fields.is_empty() { + // issue with argument-less messages. We need the function + // ffi::gst_message_writable_structure to sort this out + // and this function will be available in GStreamer 1.14 + // See https://github.com/sdroege/gstreamer-rs/pull/75 + // and https://bugzilla.gnome.org/show_bug.cgi?id=792928 + let structure = ffi::gst_message_get_structure(msg); + if !structure.is_null() { + let structure = StructureRef::from_glib_borrow_mut(structure as *mut _); - for (k, v) in self.other_fields { - s.set_value(k, v.to_send_value()); + for (k, v) in self.other_fields { + structure.set_value(k, v.to_send_value()); + } } } @@ -2442,3 +2450,37 @@ impl<'a> RedirectBuilder<'a> { msg }); } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_simple() { + ::init().unwrap(); + + // Message without arguments + let eos_msg = Message::new_eos().build(); + match eos_msg.view() { + MessageView::Eos(eos_msg) => { + assert!(eos_msg.0.get_structure().is_none()); + }, + _ => panic!("eos_msg.view() is not a MessageView::Eos(_)"), + } + + // Note: can't define other_fields for argument-less messages before GStreamer 1.14 + + // Message with arguments + let buffering_msg = Message::new_buffering(42) + .other_fields(&[("extra-field", &true)]) + .build(); + match buffering_msg.view() { + MessageView::Buffering(buffering_msg) => { + assert_eq!(buffering_msg.get_percent(), 42); + assert!(buffering_msg.0.get_structure().is_some()); + assert!(buffering_msg.0.get_structure().unwrap().has_field("extra-field")); + } + _ => panic!("buffering_msg.view() is not a MessageView::Buffering(_)"), + } + } +}