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

Fix deadlock on macOS when running GStreamer in the main thread #86

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

amyspark
Copy link
Collaborator

@amyspark amyspark commented Jul 4, 2024

Hi,

This addresses the issue in #83 (comment) where CEF would deadlock on launch on macOS. I traced it to an incorrect assumption in my code, that GStreamer would always run in a separate thread; in the supplied app, GStreamer is handled from the Cocoa event loop.

While this was already addressed for the CEF startup itself, it didn't account for GStreamer's code flow before CEF initialization (GstElement's start handler runs before the call to run_cef) and on shutdown (there was no check for main thread, so the deadlock would've occurred on the call to dispatch_async_f).

Let me know if this is OK; I tested it locally with gst-launch-1.0 and observed no issues.

cc @reinismu , can you check with your Nix recipe?

@amyspark amyspark requested a review from sdroege July 4, 2024 18:00
gstcefsrc.cc Outdated Show resolved Hide resolved
gstcefsrc.cc Outdated Show resolved Hide resolved
@amyspark amyspark force-pushed the fix-deadlock-shutdown-macos branch from 9198e83 to 96c1381 Compare July 4, 2024 21:47
@amyspark
Copy link
Collaborator Author

amyspark commented Jul 4, 2024

I found another one, this one comes from existing code. CefPostTask does not seem to if it's running in the main thread, so it'll post the task asynchronously. Then the semaphore will wait endlessly. I've added the same checking code as in src_start and src_stop.

With that I get the app running, it seems to be waiting for an outside connection -- I take that's expected @reinismu .

@amyspark amyspark requested a review from sdroege July 4, 2024 21:51
gstcefsrc.cc Show resolved Hide resolved
@sdroege
Copy link
Contributor

sdroege commented Jul 5, 2024

set_caps() and set_property() are calling browser APIs but those would be called from arbitrary threads. Is that OK?

@amyspark amyspark force-pushed the fix-deadlock-shutdown-macos branch from 96c1381 to dcd105a Compare July 5, 2024 15:29
@amyspark
Copy link
Collaborator Author

amyspark commented Jul 5, 2024

set_caps() and set_property() are calling browser APIs but those would be called from arbitrary threads. Is that OK?

Yes, that's OK. They get processed by separate threads within CEF.

@amyspark amyspark requested a review from sdroege July 5, 2024 15:48
@sdroege
Copy link
Contributor

sdroege commented Jul 5, 2024

Yes, that's OK. They get processed by separate threads within CEF.

This API is very confusing :)

@@ -778,13 +789,22 @@ gst_cef_src_start(GstBaseSrc *base_src)
GST_OBJECT_UNLOCK (src);
Copy link
Contributor

Choose a reason for hiding this comment

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

The g_cond_wait() 10 lines above would also wait forever if we end up here on macOS on the main thread or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not after this MR. If GStreamer is managed from the main thread, this function gets called by line 762 above, by which time either CEF has initialized (skipping the wait altogether), or it has failed (and this function will not be called at all). The wait is still necessary if the app handles CEF in a different event loop from the GStreamer pipeline's, like with gst-launch-1.0.

@sdroege sdroege merged commit 002f660 into centricular:master Jul 6, 2024
1 check passed
aiden-jeffrey added a commit to aiden-jeffrey/gstcefsrc that referenced this pull request Oct 3, 2024
…shutdown-macos"

This reverts commit 002f660, reversing
changes made to 8ee7844.
@amyspark amyspark deleted the fix-deadlock-shutdown-macos branch October 4, 2024 16:42
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.

2 participants