From e1a964fb006caeeca272331d75003762eb2e8d91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Dr=C3=B6ge?= Date: Wed, 3 Feb 2021 22:38:02 +0200 Subject: [PATCH] video/timecode: Use mem::ManuallyDrop to prevent a double unref of the daily jam As ffi::GstVideoTimeCode implements Copy, assignments don't move it out of the passed in value but just copy it. This doesn't increase the reference count of the daily jam, still runs the Drop impl of the passed in value to decrease the daily jam and then causes a second unref of it later when the returned value is dropped. Fixes https://gitlab.freedesktop.org/gstreamer/gstreamer-rs/-/issues/310 --- gstreamer-video/src/video_time_code.rs | 67 ++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 3 deletions(-) diff --git a/gstreamer-video/src/video_time_code.rs b/gstreamer-video/src/video_time_code.rs index 5c95e0133..bfd4b21dc 100644 --- a/gstreamer-video/src/video_time_code.rs +++ b/gstreamer-video/src/video_time_code.rs @@ -134,6 +134,10 @@ impl TryFrom for ValidVideoTimeCode { fn try_from(v: VideoTimeCode) -> Result { skip_assert_initialized!(); if v.is_valid() { + // Use ManuallyDrop here to prevent the Drop impl of VideoTimeCode + // from running as we don't move v.0 out here but copy it. + // GstVideoTimeCode implements Copy. + let v = mem::ManuallyDrop::new(v); Ok(ValidVideoTimeCode(v.0)) } else { Err(v) @@ -504,6 +508,10 @@ impl Ord for ValidVideoTimeCode { impl From for VideoTimeCode { fn from(v: ValidVideoTimeCode) -> VideoTimeCode { skip_assert_initialized!(); + // Use ManuallyDrop here to prevent the Drop impl of VideoTimeCode + // from running as we don't move v.0 out here but copy it. + // GstVideoTimeCode implements Copy. + let v = mem::ManuallyDrop::new(v); VideoTimeCode(v.0) } } @@ -538,10 +546,11 @@ impl VideoTimeCodeMeta { #![allow(clippy::cast_ptr_alignment)] unsafe { ffi::gst_video_time_code_clear(&mut self.0.tc); + // Use ManuallyDrop here to prevent the Drop impl of VideoTimeCode + // from running as we don't move tc.0 out here but copy it. + // GstVideoTimeCode implements Copy. + let tc = mem::ManuallyDrop::new(tc); self.0.tc = tc.0; - if !self.0.tc.config.latest_daily_jam.is_null() { - glib::ffi::g_date_time_ref(self.0.tc.config.latest_daily_jam); - } } } } @@ -561,3 +570,55 @@ impl fmt::Debug for VideoTimeCodeMeta { .finish() } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_add_get_set_meta() { + gst::init().unwrap(); + + let mut buffer = gst::Buffer::new(); + { + let datetime = + glib::DateTime::new_utc(2021, 02, 04, 10, 53, 17.0).expect("can't create datetime"); + let time_code = crate::VideoTimeCode::from_date_time( + gst::Fraction::new(30, 1), + &datetime, + crate::VideoTimeCodeFlags::empty(), + 0, + ) + .expect("can't create timecode"); + drop(datetime); + + let mut meta = crate::VideoTimeCodeMeta::add( + buffer.get_mut().unwrap(), + &time_code.try_into().expect("invalid timecode"), + ); + + let datetime = + glib::DateTime::new_utc(2021, 02, 04, 10, 53, 17.0).expect("can't create datetime"); + let mut time_code_2 = crate::ValidVideoTimeCode::try_from( + crate::VideoTimeCode::from_date_time( + gst::Fraction::new(30, 1), + &datetime, + crate::VideoTimeCodeFlags::empty(), + 0, + ) + .expect("can't create timecode"), + ) + .expect("invalid timecode"); + + assert_eq!(meta.get_tc(), time_code_2); + + time_code_2.increment_frame(); + + assert_eq!(meta.get_tc().get_frames() + 1, time_code_2.get_frames()); + + meta.set_tc(time_code_2.clone()); + + assert_eq!(meta.get_tc(), time_code_2); + } + } +}