fzwoch / obs-gstreamer

GStreamer OBS Studio plugin
GNU General Public License v2.0
349 stars 34 forks source link

source: implement OBS media status and control callbacks #94

Closed kamalmostafa closed 1 year ago

kamalmostafa commented 1 year ago

I needed these features for my websocket-controlled OBS application. Looks like other folks are looking for them too!

Fixes #70


Enables multiple OBS-websocket methods and OBS GUI "Source Toolbar" controls for obs-gstreamer sources.

Implements the following OBS media status callbacks:

    media_get_state
    media_get_time
    media_get_duration
    media_play_pause
    media_stop
    media_restart
    media_set_time (for seek-enabled pipelines)
fzwoch commented 1 year ago

Thanks, this looks quite nice already.

I could imagine to extend the state report with OBS_MEDIA_STATE_BUFFERING and OBS_MEDIA_STATE_ENDED. It would require a set and clear of a state variable, coming into the bus callback. Can quickly slip to cover all cases though. The first one would be for GST_MESSAGE_BUFFERING and the second one GST_MESSAGE_EOS. But I would accept it as it is already.

kamalmostafa commented 1 year ago

Thanks for your review Florian :+1: ...

[v2 notes]

I took a start at implementing a special tracking variable to deal with EOS and BUFFERING (and fussing about when to clear those), but then it occurred to me that we could just track all the obs_media_states within bus_callback(). So I've implemented that scheme instead and verified that it allows OBS to detect EOS and some additional error conditions as well.

I've also applied the changes you requested -- and fixed _restart to not pass through GST_STATE_NULL (which works better).

I'm pushing a (rebased) stack of commits here. The first commit (source: implement OBS media status and control callbacks) is unchanged in this stack, to ease your review of the new changes -- but my expectation is that I (or you) will squash them all down into the original commit, if you approve of the bus_callback scheme.

fzwoch commented 1 year ago

I commented a lot.. but I think it is actually quite close..

kamalmostafa commented 1 year ago

I'll comment on the whole "buffering" issue later -- but first -- a new general problem with my OBS media callback design:

It turns out that it is unsafe to destroy/reconstruct directly from the OBS media callbacks as I've been doing. Those callbacks are being called from some OBS thread -- I don't really know if its the same thread that initially started the plugin -- it might be some other GUI thread attached to the media-control buttons (?). So once we click the OBS "Stop" and "Restart" buttons, the new bus callback we set up ends up running in the context of "that thread". We've been getting away with it anyway, because whatever thread that is keeps running as long as OBS is running (?). That's pretty icky right there, but it appears to function normally anyway.

BUT ... A serious problem is exposed when we use OBS-websocket to invoke our stop and restart. The callbacks run okay, and the pipeline restarts and video plays -- but now our newly created bus callback has ended up running in the context of the OBS-websocket thread. And if the application connected to OBS-websocket then chooses to disconnect from OBS, then that thread just goes poof! Weirdly, in this state, our pipeline will still be playing video just fine -- but our bus callback disappears, and we lose the ability to read the media state altogether.

Upshot of all this: Our OBS media stop/restart callbacks must arrange for the teardown and reconstruct to run in the context of the main loop thread, like e.g.

        g_main_context_invoke(g_main_loop_get_context(data->loop),
                              start_pipe, data);

I have that core idea working, but I'll need to re-think about when the media states get reset and clean it up. So [v4] will be on the way soon.

fzwoch commented 1 year ago

Sounds like something I started to do for https://github.com/fzwoch/obs-gnome-screencast.

But I forgot why I actually needed it to do there. But yeah, sounds like the right thing to do, something like the g_idle_add() in the callback itself, just that you additionally need to select the correct context.

kamalmostafa commented 1 year ago

Here's my [v4], which addresses all of your comments from last round (I think) -- and fixes all of the obs_media_set_* callbacks to run their work in the obs-gstreamer loop context.

It's getting to be quite a bit of code, so I recommend leaving it as separate commits; each commit here is a valid functionality point.

I'm not aware of any problems with this [v4], but I'll advise you whether I find any over the next couple of days running in my real-world application.

kamalmostafa commented 1 year ago

FYI, The extra force-push of the indicate "buffering" commit there was just to amend the git comment (no code change).

fzwoch commented 1 year ago

Looks good to me and sounds like reasonable tested. If we notice a bug it can always be fixed. Good work, thank you!

kamalmostafa commented 1 year ago

:+1: Thanks for obs-gstreamer, Florian -- and thanks for merging this!

rodrigograca31 commented 1 year ago

Just wanted to report its working great! (at least the restart 😂)(on Linux) Thanks @kamalmostafa for the hard work!