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

Upgrade GStreamer dependencies and switch to Rust 2021 edition #393

Merged
merged 9 commits into from
Nov 18, 2023

Conversation

rafaelcaricio
Copy link
Contributor

I've upgraded the version of the GStreamer Rust bindings. I've tried to make the minimal possible changes to the code, keeping all previous logic and behavior.

Please let me know if I've missed anything.

backends/auto/Cargo.toml Outdated Show resolved Hide resolved
backends/gstreamer/lib.rs Outdated Show resolved Hide resolved
backends/gstreamer/lib.rs Outdated Show resolved Hide resolved
backends/gstreamer/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@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 much better :)

backends/auto/Cargo.toml Outdated Show resolved Hide resolved
@jdm
Copy link
Member

jdm commented May 16, 2023

This is wonderful; thank you!

Copy link
Contributor

@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.

Just a few minor things

backends/gstreamer/audio_decoder.rs Show resolved Hide resolved
backends/gstreamer/lib.rs Outdated Show resolved Hide resolved
backends/gstreamer/media_stream.rs Outdated Show resolved Hide resolved
backends/gstreamer/media_stream.rs Show resolved Hide resolved
let proxy_src = gst::ElementFactory::make("proxysrc", None).unwrap();
let proxy_sink = gst::ElementFactory::make("proxysink", None).unwrap();
proxy_src.set_property("proxysink", &proxy_sink).unwrap();
let proxy_sink = gst::ElementFactory::make("proxysink").build().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

For a later time but you should probably use gstreamer-utils::StreamProducer instead of proxysrc / proxysink

@@ -585,17 +568,15 @@ impl GStreamerPlayer {
let observer = self.observer.clone();
// Handle `position-update` signal.
player!(inner).connect_position_updated(move |_, position| {
if let Some(seconds) = position.seconds() {
if let Some(seconds) = position.map(|p| p.seconds()) {
notify!(observer, PlayerEvent::PositionChanged(seconds));
Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, with gst_play you would get exactly this behaviour. There's a bus with messages instead of callback-based signals

backends/gstreamer/player.rs Show resolved Hide resolved
backends/gstreamer/render-unix/lib.rs Show resolved Hide resolved
backends/gstreamer/render-unix/lib.rs Show resolved Hide resolved
@@ -1,13 +1,14 @@
use super::BACKEND_BASE_TIME;
use crate::datachannel::GStreamerWebRtcDataChannel;
use crate::media_stream::GStreamerMediaStream;
use boxfnonce::SendBoxFnOnce;
Copy link
Contributor

Choose a reason for hiding this comment

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

This crate is not needed anymore nowadays

@ceyusa
Copy link
Contributor

ceyusa commented Nov 15, 2023

Thanks @sdroege

Thanks! Also if you can tell me some details about what exactly the problem here is then I can try helping with that. But in any case, don't move to GstPlay yet unless you want to make 1.22 your minimum supported version, and even if you do please do it separate from the update here as it's a rather independent change :)

There are several examples reaching a deadlock, for example (I'm using this branch, rebased, and gstreamer 1.22 devenv):

 $ GST_DEBUG=gst-play:4 cargo ex media_element_source_node --all-features
                                                                      
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
warning: the following packages contain code that will be rejected by a future version of Rust: traitobject v0.1.0
note: to see what the problems were, use the option `--future-incompat-report`, or run `cargo report future-incompatibilities --id 1`
     Running `target/debug/media_element_source_node`
0:00:00.034696937 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_URI_LOADED
0:00:00.034747665 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_STATE_CHANGED
Finished pushing data
0:00:00.046928869 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_STATE_CHANGED

NeedData
0:00:00.071634133 706958 0x7fa6cc0013e0 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_VOLUME_CHANGED
0:00:00.071669843 706958 0x7fa6cc0013e0 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_MUTE_CHANGED
0:00:00.074206330 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_MEDIA_INFO_UPDATED
0:00:00.074235309 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_VIDEO_DIMENSIONS_CHANGED
0:00:00.074285157 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_DURATION_CHANGED
0:00:00.074297480 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_MEDIA_INFO_UPDATED
0:00:00.074849127 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_STATE_CHANGED
0:00:00.575495749 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:01.076228800 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:01.576932199 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:02.077619324 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:02.578301983 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:03.078978215 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:03.579673278 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:04.080358822 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:04.581035893 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:05.074728212 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_POSITION_UPDATED
0:00:05.074791796 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_END_OF_STREAM
0:00:05.074815642 706958 0x55c342e53680 INFO                gst-play gstplay.c:310:api_bus_post_message: Posting API-bus message-type: GST_PLAY_MESSAGE_STATE_CHANGED

As far as we digged, in the code we rely on receive the gstplayer's end_of_stream signal to stop the loop, but it never came, even though gstplay posts the message.

@sdroege
Copy link
Contributor

sdroege commented Nov 15, 2023

See https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5672 . No signals were emitted at all in your case.

You can work around this for now (without any negative effects with any GStreamer version other than wasting a some resources) by having a thread run a main loop with the default GLib main context.

@mrobinson
Copy link
Member

See https://gitlab.freedesktop.org/gstreamer/gstreamer/-/merge_requests/5672 . No signals were emitted at all in your case.

You can work around this for now (without any negative effects with any GStreamer version other than wasting a some resources) by having a thread run a main loop with the default GLib main context.

Thanks so much for this fix!

@sdroege
Copy link
Contributor

sdroege commented Nov 16, 2023

by having a thread run a main loop with the default GLib main context

To be more concrete here, a single thread for the whole process is enough. Just something like

thread::spawn(|| glib::MainLoop::new(None, false).run());

This would only be a problem if the same process is also using e.g. GTK. If embedding servo like this is a concern then I'll think of another work-around.

mrobinson added a commit that referenced this pull request Nov 17, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)
@mrobinson
Copy link
Member

thread::spawn(|| glib::MainLoop::new(None, false).run());

Thank you so much! This seems to have worked (https://github.com/servo/media/actions/runs/6904007056/job/18783825835). Once #408 lands, we should be able to rebase this and land it!

mrobinson added a commit that referenced this pull request Nov 17, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)
mrobinson added a commit that referenced this pull request Nov 17, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)

Co-authored-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
mrobinson added a commit that referenced this pull request Nov 17, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)

Co-authored-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
@rafaelcaricio
Copy link
Contributor Author

Happy to rebase when it lands.

mrobinson added a commit that referenced this pull request Nov 18, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)

Co-authored-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
github-merge-queue bot pushed a commit that referenced this pull request Nov 18, 2023
This is a workaround for an issue where GStreamer does not deliver end
of stream signals unless there is a main loop running. See [1] for
more information.

1. #393 (comment)

Co-authored-by: Víctor Manuel Jáquez Leal <vjaquez@igalia.com>
@mrobinson mrobinson changed the title Upgrade GStreamer dependencies Upgrade GStreamer dependencies and switch to Rust 2021 edition Nov 18, 2023
@mrobinson mrobinson added this pull request to the merge queue Nov 18, 2023
Merged via the queue into servo:main with commit 354a187 Nov 18, 2023
2 checks passed
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.

5 participants