teltek / gst-plugin-ndi

GStreamer NDI Plugin for Linux
GNU Lesser General Public License v2.1
155 stars 27 forks source link

Lots of improvements #34

Closed sdroege closed 5 years ago

sdroege commented 5 years ago

This fixes various memory unsafety issues, memory leaks and also cleans up the code otherwise.

I'll do more work on the FIXME comments in the next days and also fix various other issues that exist currently.

sdroege commented 5 years ago

And generally I would always recommend to keep the unsafe code very localized in safe abstractions, and not spread out all over the codebase, or otherwise there's not much difference to writing everything in C.

sdroege commented 5 years ago

Lots of further updates here but I'm not done yet.

sdroege commented 5 years ago

Next step is to make all the waiting interruptable and make use of the NDI SDK timestamps in a meaningful way.

sdroege commented 5 years ago

And then proper error handling everywhere.

sdroege commented 5 years ago

This is now finished from my side.

rubenrua commented 5 years ago

I'm very pleased to review this code next week.

andiempettJISC commented 5 years ago

which ndi SDK is this against? great work btw

sdroege commented 5 years ago

On 19 July 2019 21:49:21 EEST, androidwiltron notifications@github.com wrote:

which ndi SDK is this against?

I don't think that I needs a newer version than before, no special new SDK API is used. I was using the latest release of the SDK as of two weeks ago.

It should work with pre-3 streams and at least any 3 version of the SDK, if not older. I'd have to check in detail next week.

Are you running into specific problems?

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

rubenrua commented 5 years ago

Strange error with cargo build --no-default-features using ubuntu 18.04 (gstreamer version 1.14.4-1~ubuntu18.04.1) and cargo 1.36.0 (c4fcfb725 2019-05-15)

Error ``` Compiling gst-plugin-ndi v1.0.0 (/home/rubenrua/src/github.com/teltek/gst-plugin-ndi) error[E0573]: expected type, found static `TIMECODE_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0574]: expected struct, variant or union type, found static `TIMECODE_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a struct, variant or union type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0573]: expected type, found static `TIMECODE_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0573]: expected type, found static `TIMECODE_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0573]: expected type, found static `TIMESTAMP_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0574]: expected struct, variant or union type, found static `TIMESTAMP_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a struct, variant or union type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0573]: expected type, found static `TIMESTAMP_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error[E0573]: expected type, found static `TIMESTAMP_CAPS` --> src/lib.rs:46:1 | 46 | / lazy_static! { 47 | | static ref DEFAULT_RECEIVER_NDI_NAME: String = { 48 | | format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID")) 49 | | }; ... | 59 | | }; 60 | | } | |_^ not a type | = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error: aborting due to 8 previous errors error: Could not compile `gst-plugin-ndi`. To learn more, run the command again with --verbose. ```

A possible solution could be using two lazy_static blocks:

diff --git a/src/lib.rs b/src/lib.rs
index 596496e..61b5850 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -47,13 +47,14 @@ lazy_static! {
     static ref DEFAULT_RECEIVER_NDI_NAME: String = {
         format!("GStreamer NDI Source {}-{}", env!("CARGO_PKG_VERSION"), env!("COMMIT_ID"))
     };
+}

-    #[cfg(feature = "reference-timestamps")]
+#[cfg(feature = "reference-timestamps")]
+lazy_static! {
     static ref TIMECODE_CAPS: gst::Caps = {
         gst::Caps::new_simple("timestamp/x-ndi-timecode", &[])
     };

-    #[cfg(feature = "reference-timestamps")]
     static ref TIMESTAMP_CAPS: gst::Caps = {
         gst::Caps::new_simple("timestamp/x-ndi-timestamp", &[])
     };
sdroege commented 5 years ago

Thanks, apparently I didn't test without the default features in the later commits. Will do so now!

sdroege commented 5 years ago

Should be fine now, sorry :)

rubenrua commented 5 years ago

Thank you @sdroege for your work on this.