Let's do magic with GDBus instead.
Created attachment 433748 [details] Patch
Created attachment 433751 [details] Patch
I'm against this change. We are only going to depend on more and more portals and libportal does abstract complexity we shouldn't need to care about such as org.freedesktop.portal.Request objects which are easy to get wrong.
To be clear, I just mean we shouldn't *avoid* libportal. Not a review of the rest of the changes which appear to fix issues.
I don't mind either way, we just need to agree on one approach or the other. Two things that would be nice to have in libportal, support for embedding the mouse cursor in screencast sessions and a way to know which device type was selected by the user.
Comment on attachment 433751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433751&action=review There's too much sync DBus, I think, does that happen in the main thread? > Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:97 > + public: You don't need public for a struct > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:65 > + if (m_sessions.contains(device.persistentId())) { > + const auto it = m_sessions.find(device.persistentId()); Instead of constains + find you can do only find and check it != end
(In reply to Carlos Garcia Campos from comment #6) > Comment on attachment 433751 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433751&action=review > > There's too much sync DBus, I think, does that happen in the main thread? > Yes. And this is a synchronous method, no way to avoid sync dbus with the current architecture.
(In reply to Patrick Griffis from comment #4) > To be clear, I just mean we shouldn't *avoid* libportal. Not a review of the > rest of the changes which appear to fix issues. If Phil doesn't want to use it for this particular portal, we can always bring it back later when we have something else that we want to use it for. I agree that we should use libportal whenever it's convenient to do so. (In reply to Philippe Normand from comment #7) > Yes. And this is a synchronous method, no way to avoid sync dbus with the > current architecture. Sync D-Bus on the main thread is awful, but sometimes it's very hard to avoid. :/
Comment on attachment 433751 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433751&action=review >> Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:97 >> + public: > > You don't need public for a struct clang disagrees :) ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:67:24: error: 'fd' is a private member of 'WebCore::GStreamerDisplayCaptureDeviceManager::Session' it->value->fd, { }, constraints, device.type()); ^ ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:101:13: note: declared private here int fd; ^ ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:159:47: error: 'path' is a private member of 'WebCore::GStreamerDisplayCaptureDeviceManager::Session' "org.freedesktop.portal.Desktop", it->path.ascii().data(), "org.freedesktop.portal.Session", nullptr, nullptr)); ^ ../../Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:102:16: note: declared private here String path; ^
Created attachment 433868 [details] Patch
Comment on attachment 433868 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=433868&action=review > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:147 > + fd = g_unix_fd_list_get(fdList.get(), fdOut, nullptr); Where is this fd closed? > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:156 > + auto it = m_sessions.take(persistentID); Take doesn't return an iterator, this is session. I guess we should close the fd here, or in the Session destructor and it will be closed here too when out of scope.
Created attachment 434004 [details] Patch
Comment on attachment 434004 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434004&action=review > Source/WebCore/platform/mediastream/gstreamer/GStreamerCaptureDeviceManager.h:96 > + WTF_MAKE_FAST_ALLOCATED; Why did you remove the macro for structs? Can we also make this non copyable? It this is copied we would end up closing the fd twice
I messed up the patch, I'll re-upload one. Sorry!
Created attachment 434008 [details] Patch
Comment on attachment 434008 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=434008&action=review > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:72 > + m_proxy = adoptGRef(g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION, > + static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES), nullptr, > + "org.freedesktop.portal.Desktop", "/org/freedesktop/portal/desktop", "org.freedesktop.portal.ScreenCast", nullptr, nullptr)); Sorry that I didn't notice this in previous reviews but we should handle errors here. > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:83 > + GUniqueOutPtr<GError> error; > + auto result = adoptGRef(g_dbus_proxy_call_sync(m_proxy.get(), "CreateSession", g_variant_new("(a{sv})", &options), > + G_DBUS_CALL_FLAGS_NONE, s_dbusCallTimeout.millisecondsAs<int>(), nullptr, &error.outPtr())); This error here isn't handled either. > Source/WebCore/platform/mediastream/gstreamer/GStreamerDisplayCaptureDeviceManager.cpp:159 > + auto proxy = adoptGRef(g_dbus_proxy_new_for_bus_sync(G_BUS_TYPE_SESSION, > + static_cast<GDBusProxyFlags>(G_DBUS_PROXY_FLAGS_DO_NOT_CONNECT_SIGNALS | G_DBUS_PROXY_FLAGS_DO_NOT_LOAD_PROPERTIES), nullptr, > + "org.freedesktop.portal.Desktop", session->path.ascii().data(), "org.freedesktop.portal.Session", nullptr, nullptr)); And here proxy could null too if g_dbus_proxy_new_for_bus_sync fails
Committed r280239 (239909@main): <https://commits.webkit.org/239909@main>
<rdar://problem/81011828>