Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tutorial 5, similar to the one from gst-docs. #41

Closed
wants to merge 1 commit into from

Conversation

thiblahute
Copy link
Contributor

And "Add a Bus.async_signal_func helper function"

@thiblahute thiblahute changed the title Add tutorial 5, similare from the one from gst-docs. Add tutorial 5, similar to the one from gst-docs. Oct 11, 2017
Copy link
Owner

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want to run all this once through rustfmt-nightly :)

@@ -0,0 +1,377 @@
#[cfg(feature = "tutorial5")]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to move all this into a sub-module so that you have to put the feature gate only once or twice :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

fn add_streams_info(
playbin: &gst::Element,
textbufcell: &SendCell<gtk::TextBuffer>,
stype: String,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

&str would be nicer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

// Extract metadata from all the streams and write it to the text widget in the GUI
fn analyze_streams(playbin: &gst::Element, textbufcell: &SendCell<gtk::TextBuffer>) {
let text = "";
{
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text is only used once, and only in this block here, not outside

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


match pipeline.query_duration(gst::Format::Time) {
Some(x) => {
let seconds = (x as u64) / gst::SECOND;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let Some(dur) = pipeline.query_...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

gst::SeekFlags::FLUSH | gst::SeekFlags::KEY_UNIT,
((value * gst::SECOND) as i64),
)
.is_err()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let Err(_) = pipeline.seek_simple(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

let position = pipeline.query_position(gst::Format::Time);
if let Some(position) = position {
let seconds = (position as u64) / gst::SECOND;
signal::signal_handler_block(lslider, from_glib(slider_update_signal_id));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The from_glib should not be needed at all here. If connect_value_changed returns an u64 instead, you found a bug in gtk-rs and can submit a PR there :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It returns a SignalHandlerId no a u64 as all other signals iiuc.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fixed then :)


// Initialize GTK
if gtk::init().is_err() {
eprintln!("Failed to initialize GTK.");
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if let Err(err) = gtk::init() and then also print the error. Same for GStreamer. is_err, is_some and all these are usually not nice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

let playbin = gst::ElementFactory::make("playbin", None).unwrap();
if playbin
.set_property("uri", &glib::Value::from(uri))
.is_err()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap(), this never fails unless you fail to type :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


let bus = playbin.get_bus().unwrap();
let pipeline = playbin.clone();
bus.add_watch(gst::Bus::async_signal_func);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't that the same as bus.add_signal_watch()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, not sure how I forgot and ended up adding the API :-)

Removed Bus::async_signal_func it should not be so useful, let me know if you want me to open a PR to re add it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's fine :) I didn't add it before because it seemed a bit useless

@thiblahute thiblahute force-pushed the tutorial-5 branch 2 times, most recently from 615458c to ec79ffd Compare October 11, 2017 17:42
.connect_value_changed(move |slider| {
let pipeline = &pipeline;
let value = slider.get_value() as u64;
if let Err(_) = pipeline .seek_simple(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space before .seek_simple

eprintln!("Seeking to {} failed", value);
}
})
.to_glib();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_glib() also shouldn't be here. The from_glib_* and to_glib_* functions are all meant for the bindings code only, or when you interact with the C functions directly

let pipeline = playbin.clone();
bus.add_signal_watch();
playbin.get_bus().unwrap().connect_message(move |_, msg| {
//let streams_view = &streams_view;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out, why?

@thiblahute
Copy link
Contributor Author

This now depends on gtk-rs/glib#240

Copy link
Owner

@sdroege sdroege left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just need to get the glib change merged first

@sdroege
Copy link
Owner

sdroege commented Oct 14, 2017

I'll merge this later, thanks a lot :)

@sdroege sdroege closed this in faae914 Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants