servo / media

Mozilla Public License 2.0
82 stars 54 forks source link

Upgrade GStreamer dependencies and switch to Rust 2021 edition #393

Closed rafaelcaricio closed 1 year ago

rafaelcaricio commented 1 year ago

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.

jdm commented 1 year ago

This is wonderful; thank you!

rafaelcaricio commented 1 year ago

Maybe the other changes left here should be done in separated PRs? They sound out of the scope for the upgrade of GStreamer bindings version. What do you think?

sdroege commented 1 year ago

Sure that's OK for me. I just commented everything I noticed. You might want to add TODO comments so it's not forgotten.

mrobinson commented 1 year ago

I rebased this and added a new commit updating the gstreamer dependency to 0.21.

mrobinson commented 1 year ago

It seems this change is causing one of the examples which are all run by the CI to not exit.

rafaelcaricio commented 1 year ago

Yes, I don't know how to fix the CI. But this should be good to go.

mrobinson commented 1 year ago

Unfortunately, it cannot land until the CI passes.

I am able to reproduce this issue locally though, by running the same command the CI does:

 ls examples/*.rs | xargs -I{} basename  {} .rs  | grep -v params_connect | RUST_BACKTRACE=1 GST_DEBUG=3 xargs -I{} cargo ex {} --all-features

It seems that the problem example is media_element_source_node, which I can confirm by trying to run this manually.

RUST_BACKTRACE=1 GST_DEBUG=3 xargs -I{} cargo ex media_element_source_node --all-features
rafaelcaricio commented 1 year ago

Ok, I will try to check this over the weekend.

rafaelcaricio commented 1 year ago

Oops, I did something wrong... 😨

rafaelcaricio commented 1 year ago

It seems like the CI is stuck in the examples' execution.

jdm commented 1 year ago

What's the next step here? Solving the CI failures?

mrobinson commented 1 year ago

Yes, this just needs to pass on the CI.

ceyusa commented 1 year ago

We were looking on this and we spot the problem is servo/media using GstPlayer, which is deprecated in GStreamer 1.20, and will be removed in 1.24. And, as far as we saw, the event callbacks aren't called anymore.

So, most of the work to upgrade GStreamer as dependency implies to change the inner player from GstPlayer to GstPlay API.

mrobinson commented 1 year ago

The examples are deadlocking because the end of stream message never arrives. This is because in the implementation of the media player we use, it relies on GstPlayer properly sending signals. These message are now sent via the bus. GstPlay has backwards compatibility support to forward the messages to signals, but there is no backwards compatibility support for GstPlayer.

Thus, it seems we can either listen to the bus signals and continue using GstPlayer or upgrade to GstPlay. It doesn't seem like GstPlayer has the necessary APIs to listen to bus signals. While it is deprecated, GstPlayer is still part of the GStreamer API. Is there a way to adapt to the new bus message refactor while continuing to use GstPlayer, or is GstPlayer simply unable to listen to messages like "end of stream" in newer versions of GStreamer?

sdroege commented 1 year ago

If there's a breaking change in behaviour in GstPlayer then that's a bug and needs to be addressed.

sdroege commented 1 year ago

@lu-zero pointed me to https://github.com/servo/project/blob/main/governance/tsc/tsc-2023-11-14.md#multimedia-status , which contains some wrong information.

newer version of gstreamer have been making api changes

That's not the case. You can stay at GstPlayer just fine, it's not going away anytime soon and definitely not for 1.24 like written in this comment. We want to get rid of it at some point, but for now there are far too many users left and it's not going to happen before GstPlay gets moved to gst-plugins-base and becomes officially API-stable.

There shouldn't be any API changes other than the ones in the bindings, which is what this PR is about.

If you want to move to GstPlay then you need to update to a newer version of GStreamer, but I think that's out of scope for this PR and would better be done independently. There's no reason for making it part of this PR, it just makes it bigger.


However, it seems like something is not working correctly with GstPlayer but it's not clear to me what. Something related to message handling. Also unclear if that only affects newer versions of GStreamer (from which to which version did it break?), or if it somehow gets introduced by the bindings update in this PR.

Both with GStreamer 1.22.6 and GStreamer git main the player.rs is working fine for me, and with both GStreamer versions it's just a compat API around the new GstPlay. The end-of-stream signal at least is emitted correctly there.

Can someone shed some light on the problems here?

mrobinson commented 1 year ago

newer version of gstreamer have been making api changes

That's not the case. You can stay at GstPlayer just fine, it's not going away anytime soon and definitely not for 1.24 like written in this comment. We want to get rid of it at some point, but for now there are far too many users left and it's not going to happen before GstPlay gets moved to gst-plugins-base and becomes officially API-stable.

Apologies, this is mistake in the minutes. I believe the person taking minutes at that moment either misheard or misinterpreted me. What I was said there was essentially a summary of what I wrote in the comment above (https://github.com/servo/media/pull/393#issuecomment-1802273171). I will correct the minutes.

sdroege commented 1 year ago

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 :)

ceyusa commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago
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!

rafaelcaricio commented 1 year ago

Happy to rebase when it lands.