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

Added basic-tutorial-2 #33

Closed
wants to merge 3 commits into from
Closed

Added basic-tutorial-2 #33

wants to merge 3 commits into from

Conversation

Jouan
Copy link
Contributor

@Jouan Jouan commented Sep 12, 2017

Contributes to solve #32.

I have never used gstreamer-rs before, so I'm not entirely sure if this is correct.
It compiles, runs and prints "Unexpected message received."

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.

Thanks a lot :) This looks very good already, I mostly have cosmetic comments below. Generally, I'd recommend taking a look at the examples in the examples repository for getting some idea how things could be done, possibly simpler.

use gst::GstObjectExt;
use gst::ElementExt;
use gst::MessageView;

Copy link
Owner

Choose a reason for hiding this comment

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

Ideally you would "use gst;" and "use gst::prelude::*;" here

}
}
else {
println!("Not all elements could be created.");
Copy link
Owner

Choose a reason for hiding this comment

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

This else case could be at the very top and an early return from main(). Create both elements, check if both could be created and if not just error out

},
MessageView::Eos(..) => println!("End-Of-Stream reached."),
// We should not reach here because we only asked for ERRORs and EOS
_ => eprintln!("Unexpected message received."),
Copy link
Owner

Choose a reason for hiding this comment

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

There is no filtered API for GstBus in the Rust bindings currently, so you would just ignore all other messages here

// Wait until error or EOS
let bus = pipeline.get_bus().unwrap();
if let Some(msg) = bus.timed_pop(gst::CLOCK_TIME_NONE) {
match msg.view() {
Copy link
Owner

Choose a reason for hiding this comment

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

Here I would "use gst::MessageView;". Check the other examples

gst::GObjectExtManualGst::set_property_from_str(
&source,
"pattern",
"0");
Copy link
Owner

Choose a reason for hiding this comment

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

You can even give the pattern a name here: 0 is "smpte"

Copy link
Contributor Author

@Jouan Jouan Sep 12, 2017

Choose a reason for hiding this comment

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

This part is mimicking: g_object_set (source, "pattern", 0, NULL);

My understanding was that "pattern" is the property to set, and "0" is the value for that property.

Could you explain what you mean by "give the pattern a name"?

Copy link
Owner

Choose a reason for hiding this comment

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

The equivalent to the C code would be with set_property but we can do better here. Either you could use that in combination with glib::EnumClass (see playbin.rs for an example with flags, enums work very similar). That would be most explicit. Or you could use source.set_property_from_str("pattern", "smpte")

The advantage with using the string is that it is more descriptive. You don't have to look up in the docs that 0 is the SMPTE pattern but the code just says so. And you could replace it directly with e.g. " snow" without looking up the numerical value of it.

gst::BinExtManual::add_many(
&pipeline,
&[&source, &sink]).unwrap();
if let Err(_) = gst::ElementExt::link(&source, &sink) {
Copy link
Owner

Choose a reason for hiding this comment

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

source.link(&sink) or gst::Element::link_many(&source, &sink)

Copy link
Owner

Choose a reason for hiding this comment

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

Also on error here, you could as well expect("Failed to link elements) instead of manually unwrapping the Result and exiting the process (same in the other places: element creation for example).

// Build the pipeline
gst::BinExtManual::add_many(
&pipeline,
&[&source, &sink]).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

pipeline.add_many(&[&source, &sink])

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.

Two more things, then we're good to go :) Thanks for the update

eprintln!("Debugging information: {:?}", err.get_debug());
},
MessageView::Eos(..) => println!("End-Of-Stream reached."),
// We should not reach here because we only asked for ERRORs and EOS
Copy link
Owner

Choose a reason for hiding this comment

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

This comment is wrong: other than in the C code, we will reach here because the filtering of messages happens in our match statement and not before that already


// Wait until error or EOS
let bus = pipeline.get_bus().unwrap();
if let Some(msg) = bus.timed_pop(gst::CLOCK_TIME_NONE) {
Copy link
Owner

Choose a reason for hiding this comment

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

You want a loop around this to fetch all messages until EOS or Error, i.e. replace the if let with a while let and break on EOS and Error.

@@ -29,16 +29,15 @@ fn main() {

// Wait until error or EOS
let bus = pipeline.get_bus().unwrap();
if let Some(msg) = bus.timed_pop(gst::CLOCK_TIME_NONE) {
while let Some(msg) = bus.timed_pop(gst::CLOCK_TIME_NONE) {
use gst::MessageView;
match msg.view() {
MessageView::Error(err) => {
eprintln!("Error received from element {}: {}",
msg.get_src().get_path_string(), err.get_error());
eprintln!("Debugging information: {:?}", err.get_debug());
Copy link
Owner

Choose a reason for hiding this comment

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

On Error you also want to break. I'll fix that up and merge it, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I completely missed that obvious part!
Thank you for all your feedback, it's super helpful!

Copy link
Owner

Choose a reason for hiding this comment

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

I'm glad to hear that it was useful for you :) Thanks again for this PR, it's merged now

@sdroege sdroege closed this in a1679f6 Sep 13, 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