Add basic-tutorial-8.r #52

gdesmott · 2017-11-08T16:39:20Z

No description provided.

sdroege · 2017-11-08T17:06:09Z

Awesome, thanks! I'll review this tomorrow

sdroege

Generally looks good, just a couple of cosmetic things. Not for everything I added a comment to each instance of it, please check if the same cosmetic change would also make sense elsewhere :)

sdroege · 2017-11-09T13:45:07Z

tutorials/src/bin/basic-tutorial-8.rs

+        .unwrap();
+    let audio_caps = info.to_caps().unwrap();
+    app_source.set_property("caps", &audio_caps).unwrap();
+    app_source.set_property_from_str("format", "time");


Setting gst::Format::Time would be nicer

I missed that; thanks.

sdroege · 2017-11-09T13:46:39Z

tutorials/src/bin/basic-tutorial-8.rs

+            println!("start feeding");
+
+            let data_clone = data.clone();
+            d.source_id = Some(idle_add(move || {


glib::idle_add would be nicer. I don't like globally imported functions very much :)

sdroege · 2017-11-09T14:22:27Z

tutorials/src/bin/basic-tutorial-8.rs

+
+    let appsink = app_sink.clone().dynamic_cast::<AppSink>().expect(
+        "Sink element is expected to be an appsink!",
+    );


Maybe instead of having two variables that have almost the same name (appsrc and app_source), just cast here and use that in the future?

Done. I just had to move code around a bit to do so.

sdroege · 2017-11-09T14:28:08Z

tutorials/src/bin/basic-tutorial-8.rs

+            let data_clone = data.clone();
+            d.source_id = Some(idle_add(move || {
+                let data = &data_clone;
+                let mut samples: Vec<u8> = vec![0; CHUNK_SIZE];


You could also just allocate a buffer and then go over its data. It's potentially faster as the memory doesn't have to be zero-initialized first

Indeed. Done.

sdroege · 2017-11-09T14:31:35Z

tutorials/src/bin/basic-tutorial-8.rs

+                    let samples = samples.as_mut_slice_of::<i16>().unwrap();
+                    pts = data.num_samples
+                        .mul_div_floor(gst::SECOND, SAMPLE_RATE as u64)
+                        .unwrap();


If you wait long enough, this will panic as u64 overflows ;)

What would be the proper way to handle that?

Use expect instead of unwrap for now, or if it returns None (i.e. overflows), you could just go EOS and stop doing anything

sdroege · 2017-11-09T14:33:53Z

tutorials/src/bin/basic-tutorial-8.rs

+                    }
+
+                    data.num_samples += num_samples as u64;
+                    appsrc = data.appsrc.clone();


You could do let (appsrc, pts) = { the long block };. That's IMHO nicer than having "uninitialized" variable declarations

Indeed. That's the kind of Rust trick I'm not yet used to. :)

sdroege · 2017-11-09T14:36:09Z

tutorials/src/bin/basic-tutorial-8.rs

+        let mut data = data.lock().unwrap();
+        if let Some(source) = data.source_id.take() {
+            println!("stop feeding");
+            source_remove(source);


Same here, glib::source_remove

sdroege · 2017-11-09T14:36:33Z

tutorials/src/bin/basic-tutorial-8.rs

+
+    // configure appsink
+    app_sink.set_property("emit-signals", &true).unwrap();
+    app_sink.set_property("caps", &audio_caps).unwrap();


Doensn't appsink have proper functions for settings these, instead of stringly-typed properties? (Same for appsrc btw)

Indeed; fixed.
Should appsrc::set_caps() take a Option(Caps) as gst_app_src_set_caps() allows 'caps' being NULL?

Yes, same for appsink. Fixing right now, thanks!

Fixed in c9636bc

sdroege · 2017-11-09T14:37:46Z

tutorials/src/bin/basic-tutorial-8.rs

+        {
+            let data = &data_clone.lock().unwrap();
+            appsink = data.appsink.clone();
+        }


Also here, let the block return the appsink clone

sdroege · 2017-11-15T14:54:39Z

@gdesmott What's the status here? :)

gdesmott · 2017-11-15T15:18:07Z

I have a big train trip on Friday, that should give me some time to fix your comments. :)

sdroege · 2017-11-17T14:46:35Z

Travis is unhappy with the changes :)

sdroege · 2017-11-17T14:58:25Z

There were some API improvements/changes in GIT master to improve type-safety and convenience a bit, that's why it fails now.

gdesmott · 2017-11-17T15:47:45Z

Branch updated.

sdroege

Looks very nice now, just 3 minor things left :)

sdroege · 2017-11-17T16:09:09Z

tutorials/src/bin/basic-tutorial-8.rs

+    );
+
+    let data: Arc<Mutex<CustomData>> =
+        Arc::new(Mutex::new(CustomData::new(appsrc.clone(), appsink.clone())));


You could let new() take the two elements by reference and clone internally, and directly return the Arc<Mutex<_>> instead of cluttering the code with that here :)

sdroege · 2017-11-17T16:10:38Z

tutorials/src/bin/basic-tutorial-8.rs

+    let data_clone = data.clone();
+    appsrc.connect_need_data(move |_, _size| {
+        let data = &data_clone;
+        let mut d = data_clone.lock().unwrap();


Might be nicer to also use data here instead of data_clone

sdroege · 2017-11-17T16:12:07Z

tutorials/src/bin/basic-tutorial-8.rs

+                let num_samples = CHUNK_SIZE / 2; /* Each sample is 16 bits */
+                let mut buffer = gst::Buffer::with_size(CHUNK_SIZE).unwrap();
+
+                let appsrc = {


let (appsrc, buffer) = { ... } would seem nicer, and you can also move the num_samples variable into the block

sdroege · 2017-11-17T18:43:46Z

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add basic-tutorial-8.r #52

Add basic-tutorial-8.r #52

gdesmott commented Nov 8, 2017

sdroege commented Nov 8, 2017

sdroege left a comment

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege Nov 17, 2017

sdroege Nov 17, 2017

sdroege Nov 9, 2017

gdesmott Nov 17, 2017

sdroege commented Nov 15, 2017 •

edited

Loading

gdesmott commented Nov 15, 2017

sdroege commented Nov 17, 2017

sdroege commented Nov 17, 2017

gdesmott commented Nov 17, 2017

sdroege left a comment

sdroege Nov 17, 2017

sdroege Nov 17, 2017

sdroege Nov 17, 2017

sdroege commented Nov 17, 2017

gdesmott commented Nov 20, 2017

Add basic-tutorial-8.r #52

Add basic-tutorial-8.r #52

Conversation

gdesmott commented Nov 8, 2017

sdroege commented Nov 8, 2017

sdroege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdroege commented Nov 15, 2017 • edited Loading

gdesmott commented Nov 15, 2017

sdroege commented Nov 17, 2017

sdroege commented Nov 17, 2017

gdesmott commented Nov 17, 2017

sdroege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdroege commented Nov 17, 2017

gdesmott commented Nov 20, 2017

sdroege commented Nov 15, 2017 •

edited

Loading