Rust sendrecv #20

maxmcd · 2018-05-31T15:11:22Z

There's more work to be done here (No flag support, no cleanup, a few shortcuts) but the rust script seems fully functional. Wanted to inquire about two things:

Is this rust code welcome (I'm just writing this up as a personal exercise) and is there any feedback?
Are the docker-compose and related Dockerfiles desired? I can move them off the branch, but figured they might be helpful.

Can complete the rust code and clean up/document docker-compose upon confirmation of #2.

…dia_stream

sdroege · 2018-05-31T15:41:59Z

sendrecv/gst-rust/src/main.rs

+}
+
+fn sdp_message_as_text(offer: gstreamer_webrtc::WebRTCSessionDescription) -> Option<String> {
+    unsafe {


sdroege/gstreamer-rs#108 (comment) for why this is necessary currently. We should fix that before merging this PR :)

I've added those now here sdroege/gstreamer-rs@85ca3b9

Lovely timing, as mine was incorrect: maxmcd/gstreamer-rs@dd0918b

:)

A mutable borrow is not needed here as all these are immutable fields after all, and from_glib_none is needed here because we don't take ownership of the field (otherwise we would steal it from the struct). Ideally we would just borrow it, but there's no borrowing mechanism in glib-rs at this point that could be used for this (and generally borrowing from GLib types is dangerous by default except for some special cases), so we have to copy here.

Let me know if my version works and if there are any other problems :) And thanks again for working on this

Your version works perfectly. Replaced the unsafe call.

sdroege · 2018-05-31T15:43:37Z

sendrecv/gst-rust/src/main.rs

+    return AppState::PeerConnecting;
+}
+
+fn register_with_server(app_state: &Arc<AtomicUsize>, out: &ws::Sender) -> AppState {


Might be nicer to move the app state into a struct for extensibility later instead of directly passing around that single integer everywhere (and maybe wrap that struct into an Rc<Mutex<_>>)

sdroege · 2018-05-31T15:44:50Z

sendrecv/gst-rust/src/main.rs

+    webrtc
+        .emit(
+            "set-local-description",
+            &[&offer.to_value(), &promise.to_value()],


Hmm what's the point of the promise if it's not used anywhere afterwards?

I was mostly parroting the original c implementation: webrtc-sendrecv.c#L252-L255

It does seem though that no promise is allowed: https://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-bad/html/gst-plugins-bad-plugins-webrtcbin.html#GstWebRTCBin-set-local-description

On that point, I couldn't figure out how to pass null in a situation like this. Is there some stand-in in the library?

You should be able to pass &None::<gst::Promise> here

sdroege · 2018-05-31T15:45:45Z

sendrecv/gst-rust/src/main.rs

+    });
+    let options = gst::Structure::new_empty("options");
+    webrtc_clone
+        .emit("create-offer", &[&options.to_value(), &promise.to_value()])


The to_value() in all the emit calls should not be needed, only the &

sdroege · 2018-05-31T15:46:58Z

sendrecv/gst-rust/src/main.rs

+        sink.sync_state_with_parent().unwrap();
+        gst::Element::link_many(&[&q, &conv, &resample, &sink]).unwrap();
+    } else {
+        pipe.clone()


You could move this dynamic_cast outside the if and have it only once then, storing the bin in a local variable

sdroege · 2018-05-31T15:49:00Z

sendrecv/gst-rust/src/main.rs

+        queue !
+        application/x-rtp,media=audio,encoding-name=OPUS,payload=97 ! sendrecv.
+        ",
+    ).unwrap();


Explicitly creating the elements would seem nicer than string programming, but I guess it's fine :)

sdroege · 2018-05-31T15:49:24Z

sendrecv/gst-rust/src/main.rs

+        .unwrap();
+
+    let ret = pipe.set_state(gst::State::Playing);
+    assert_ne!(ret, gst::StateChangeReturn::Failure);


You could also do into_result().unwrap() on the return value

sdroege · 2018-05-31T15:50:32Z

sendrecv/gst-rust/src/main.rs

+                x if x == AppState::ServerRegistering as usize => AppState::ServerRegisteringError,
+                x if x == AppState::PeerConnecting as usize => AppState::PeerConnectionError,
+                x if x == AppState::PeerConnected as usize => AppState::PeerCallError,
+                x if x == AppState::PeerCallNegotiating as usize => AppState::PeerCallError,


See above, if you make the app struct a Rc<Mutex<_>> then you could store the state directly as an enum and match properly on it, exhaustively

Can we get away with Rc with the promises? I assumed that would behave the same way as spawning a new thread and a sync::Arc would be needed.

Yes, Arc is needed, sorry :)

sdroege · 2018-05-31T15:54:21Z

sendrecv/gst-rust/src/main.rs

+                .unwrap();
+            self.update_state(AppState::PeerCallStarted);
+        }
+        if json_msg.get("ice").is_some() {


Maybe we could use serde-json here for directly converting between a Rust struct and JSON?

sdroege · 2018-05-31T15:55:51Z

Looks quite nice, thanks :) I have no opinions about the Docker stuff and how the webrtc library is used is for someone else to review.

From the Rust side, in addition to the comments above it might be nice to do proper error handling in various places instead of just unwrapping/expecting

maxmcd · 2018-05-31T16:58:43Z

Thanks so much for the detailed feedback @sdroege!

maxmcd · 2018-06-05T22:08:06Z

I'm done with edits for the moment. The mutex stuff is a little messy in places and it's still quite panicky. Had a difficult time consolidating the async nature of the code with proper error handling.

Also, cleaned up docker and added peer-id/server flag support to gain parity with the c version.

sdroege · 2018-06-06T06:55:47Z

So this is working and basically equivalent with the C version now, and could get another full review?

nirbheek · 2018-06-06T09:06:47Z

signalling/Dockerfile

+RUN sed -i 's/sslctx.load_cert_chain(chain_pem, keyfile=key_pem)/pass/g' \
+    ./simple-server.py
+RUN sed -i 's/ssl=sslctx,//g' \
+    ./simple-server.py


Maybe you want to add an option to simple-server.py to not use ssl instead? If you set ssl=None when calling serve(), it's the same as not passing an sslctx.

nirbheek · 2018-06-06T09:07:40Z

sendrecv/gst/Dockerfile

+RUN sed -i 's/wss:\/\/webrtc.nirbheek.in:8443/ws:\/\/signalling:8443/g' \
+    /opt/webrtc-sendrecv.c
+RUN sed -i 's/strict_ssl = TRUE/strict_ssl = FALSE/g' \
+    /opt/webrtc-sendrecv.c


Here, too. I think it would be generally useful for people (while running local servers) if we do that.

maxmcd · 2018-06-06T16:27:23Z

@sdroege yes absolutely

sdroege · 2018-06-13T06:43:09Z

Added some minor fixes. Is set_property_from_str acceptable when constructing the pipeline? As an example: seems like I would need GstAudioTestSrcWave to be imported to properly replace: audiotestsrc.set_property_from_str("wave", "red-noise");

That's fine. The alternative would be something like this https://github.com/sdroege/gstreamer-rs/blob/f4d57a66522183d4927b47af422e8f321182111f/examples/src/bin/playbin.rs#L27-L35 (there's basically the same API for enums)

sdroege · 2018-06-13T06:52:00Z

For how to integrate GstBus and proper error handling here, see #20 (comment). That's a general architectural problem right now with how the ws crate is used.

sdroege · 2018-06-13T06:56:12Z

For how to integrate GstBus and proper error handling here, see #20 (comment). That's a general architectural problem right now with how the ws crate is used.

You could basically have another thread that does the ws stuff, and the main thread runs the GstBus main loop. And from the ws handler you could put messages on the bus also, or otherwise call things on the main thread via the glib main context API.

The other way around, calling things from elsewhere into the ws thread/event loop does not seem to be possible from a short look over the API. But maybe you also don't need that?

Otherwise there are some tokio based websocket libraries that might be possible to use, but that complicates everything quite a lot :)

maxmcd · 2018-06-14T14:28:31Z

Not sure how I'm missing this. How would I create a message for the bus and what MessageView type would you recommend using for posting websocket messages to the bus?

sdroege · 2018-06-14T14:35:20Z

Not sure how I'm missing this. How would I create a message for the bus and what MessageView type would you recommend using for posting websocket messages to the bus?

You would use bus.post() for posting the message, and you could either create an application message, or directly an error. Application would probably be nicer.

See https://github.com/sdroege/gstreamer-rs/blob/db3fe694154c697afdaf3efb6ec65332546942e0/tutorials/src/bin/basic-tutorial-5.rs#L264-L267 for an example involving an element, you can simply do bus.post(msg) in your case instead of using element.post_message(msg). The structure from the application message can contain anything you want apart from its name.

You can catch the message on the bus like https://github.com/sdroege/gstreamer-rs/blob/db3fe694154c697afdaf3efb6ec65332546942e0/tutorials/src/bin/basic-tutorial-5.rs#L241-L244, for example.

maxmcd · 2018-06-15T19:45:42Z

Rewrote the script with rust-websocket and have better error handling with no panics and (I think) much less blocking. Much happier with where this is now, doesn't seem to be fighting against itself as much.

The error handling within the callback closures is a little hacky, and looking at some of the gstreamer-rs examples my bus/loop setup could be a little cleaner.

Ready for another round of feedback/review.

sdroege · 2018-06-16T07:41:20Z

sendrecv/gst-rust/src/main.rs

+            MessageView::Element(_) => {}
+            MessageView::Tag(_) => {}
+            MessageView::NewClock(_) => {}
+            MessageView::Warning(warning) => {


You also want to handle Error messages, but all the others above are not really needed :) An Error message is always fatal

sdroege · 2018-06-16T07:42:31Z

sendrecv/gst-rust/src/main.rs

+            println!("Got error message! {}", msg);
+            return self.handle_error();
+        }
+        let json_msg: JsonMsg = serde_json::from_str(&msg).unwrap();


Instead of unwrapping, failing to deserialize the JSON should probably just be an error like other errors

sdroege · 2018-06-16T07:44:05Z

Looks very good, apart from the minor things above I didn't see anything wrong while shortly looking over it. Will do another proper review in the next days but this looks like it's done. Much better with the other websocket library, and the need for 2 threads instead of 1 for that can probably be refactored later (e.g. by using the tokio-based async API on it). But that's for later

sdroege · 2018-06-16T07:46:23Z

sendrecv/gst-rust/src/main.rs

+        }
+    }
+
+    fn app_state_lt(&self, state: AppState, error_msg: &'static str) -> bool {


Maybe this could mirror the Ord trait a bit more and be called app_state_cmp and return a std::cmp::Ordering. But details. Just lt seems a bit short :)

sdroege

Looks good to me, thanks a lot for your work :)

sdroege · 2018-06-18T06:03:22Z

All merged now :)

maxmcd · 2018-06-18T15:44:22Z

Nice! Thanks so much for your patience and feedback @sdroege

MathieuDuponchelle · 2018-10-15T18:54:05Z

@maxmcd , nice work, as far as I can tell this only works when the signalling server is launched with --disable-ssl , or am I missing something?

maxmcd · 2018-10-15T20:17:24Z

@MathieuDuponchelle correct, at least that's what I used when I worked on this, I'm sure you could get ssl working again. This is somewhat documented here: https://github.com/centricular/gstwebrtc-demos/blob/master/signalling/Dockerfile

The priority was getting a quick docker demo working so would check the dockerfiles and docker-compose file for quick reference on how to run.

sdroege · 2018-10-15T21:06:19Z

This will need some refactoring to do all the ws communication on a single thread via tokio/futures instead of splitting the socket in its sender and receiver side

sdroege · 2018-10-16T07:26:12Z

@maxmcd would you be interested in working on this? In any case, we should probably open a new issue for this :)

maxmcd · 2018-10-17T14:08:09Z

Yep, would be happy to.

sdroege · 2018-10-17T16:52:50Z

Great, let us know how it goes and if you need help or want someone else to take over

maxmcd added 9 commits May 29, 2018 12:22

Add docker-compose demo

ad8a9d5

Beginnings of gst-rust, plugin validation and basic WS event loop

5f3d57b

Promise and event progress

3f32c7f

Add send ice, create offer, send sdp and others

6ecf017

Json message parsing and Sdp message parse_buffer fix

a6359d9

Video to browser working

3d03d52

Add appstate handling with Atomic and Arc

ab61ee6

Implement on_incoming_stream, on_incoming_decodebin_stream, handle_me…

656ec1b

…dia_stream

Remove debugging statements and some cleanup

e03100a

sdroege reviewed May 31, 2018

View reviewed changes

Added minor fixes and improvements related to PR

59a5616

maxmcd force-pushed the rust branch from 5511985 to 59a5616 Compare May 31, 2018 19:11

maxmcd added 2 commits June 5, 2018 13:17

Add some error handling, add more to Mutex, minor fixes

e778ca1

Add peer-id and server flags, clean up some mutex use, general cleanup

d4165e4

Remove unsafe fetching of sdp as text

37d2714

maxmcd force-pushed the rust branch from cf2de09 to 37d2714 Compare June 5, 2018 22:26

nirbheek reviewed Jun 6, 2018

View reviewed changes

Minor refactoring and cleanup

76fb8a7

Replace ws lib, use bus and better error handling, remove all panics

03bf984

maxmcd added 2 commits June 15, 2018 15:57

Cleanup and more idiomatic error handling for missing elements

b392f4b

Use enum for json serialization as well

23850c6

maxmcd force-pushed the rust branch from bac7344 to 23850c6 Compare June 15, 2018 20:04

Better handle signalling server going away

7ed8ddf

sdroege reviewed Jun 16, 2018

View reviewed changes

maxmcd added 2 commits June 16, 2018 09:49

Move caught bus message types to catchall, lt => cmp, handle bus errors

e06c8d3

Consolidate app_control cloning

1f80d35

sdroege approved these changes Jun 18, 2018

View reviewed changes

sdroege closed this in ad9b875 Jun 18, 2018

sdroege mentioned this pull request Dec 5, 2018

Rust sendrecv should get support for TLS websockets #70

Closed

Jackovic mentioned this pull request Jan 19, 2019

Cannot parse SDP message in python demo #25

Closed

Rust sendrecv #20

Rust sendrecv #20

Conversation

maxmcd commented May 31, 2018

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 May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmcd May 31, 2018 • edited Loading

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 May 31, 2018

maxmcd commented May 31, 2018

maxmcd commented Jun 5, 2018

sdroege commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxmcd commented Jun 6, 2018

sdroege commented Jun 13, 2018

sdroege commented Jun 13, 2018

sdroege commented Jun 13, 2018 • edited Loading

maxmcd commented Jun 14, 2018

sdroege commented Jun 14, 2018

maxmcd commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdroege commented Jun 16, 2018

Choose a reason for hiding this comment

sdroege left a comment

Choose a reason for hiding this comment

sdroege commented Jun 18, 2018

maxmcd commented Jun 18, 2018

MathieuDuponchelle commented Oct 15, 2018

maxmcd commented Oct 15, 2018

sdroege commented Oct 15, 2018

sdroege commented Oct 16, 2018

maxmcd commented Oct 17, 2018

sdroege commented Oct 17, 2018

sdroege May 31, 2018 •

edited

Loading

maxmcd May 31, 2018 •

edited

Loading

sdroege commented Jun 13, 2018 •

edited

Loading