GNOME Bugzilla – Bug 780140
souphttpsrc: Implement soup session sharing
Last modified: 2018-11-03 15:17:27 UTC
Also needs some patches for adaptivedemux to make use of that, but with that it's then possible to use exactly the same connection for the initial manifest, any variant manifests and the fragments. Which speeds up time-to-play a lot especially if using HTTPS where every connection setup is very expensive.
Created attachment 348079 [details] [review] souphttpsrc: Implement soup session sharing souphttpsrc now shares its SoupSession with other elements in the pipeline via GstContext if possible (session-wide settings are all the defaults), or if the context was forced by the application. This allows multiple souphttpsrcs to reuse connections, cookies, etc.
Created attachment 348080 [details] [review] uridownloader: add new gst_uri_downloader_set_parent If set, the parent is used to proxy need-context messages from uridownloader's http source in order to get cookies/headers from the pipeline. Based on a patch from Philippe Normand https://bugzilla.gnome.org/show_bug.cgi?id=726314
Created attachment 348081 [details] [review] adaptivedemux: set uridownloader's parent Allows internal http source to request contexts and share cookies with the pipeline in case the server imposes any restriction based on cookies.
Created attachment 348085 [details] [review] souphttpsrc: Use a in-memory cookie jar by default in sessions we created This ensures that cookies are stored and used as set by the server, and shared with other souphttpsrc that use the same SoupSession.
Note that this also allows the application to set a SoupSession on the pipeline (with "force=true"), and then whatever the application wants can be done from there.
*** Bug 721807 has been marked as a duplicate of this bug. ***
Should this go into 1.12, possible disabled by default unless GST_SOUP_SESSION_SHARE=1 is defined in the environment to easily test it?
Review of attachment 348081 [details] [review]: ::: gst-libs/gst/adaptivedemux/gstadaptivedemux.c @@ +441,3 @@ demux->priv->input_adapter = gst_adapter_new (); demux->downloader = gst_uri_downloader_new (); + gst_uri_downloader_set_parent (demux->downloader, GST_ELEMENT_CAST (demux)); This creates a circular reference cycle between demux and downloader. The downloader should probably just use a GWeakRef here.
Created attachment 348162 [details] [review] uridownloader: Use a GWeakRef to the parent to prevent a reference cycle
Attachment 348079 [details] pushed as 7cb70e7 - souphttpsrc: Implement soup session sharing Attachment 348085 [details] pushed as eee911b - souphttpsrc: Use a in-memory cookie jar by default in sessions we created
This is causing various deadlocks and refcount issues on the context. One way to reproduce is gst-validate-launcher -m -f -t validate.dash.playback.scrub_forward_seeking.dash_exMPD_BIP_TC1 https://ci.gstreamer.net/job/GStreamer-master-validate/4932/testReport/junit/validate.dash.playback/scrub_forward_seeking/dash_exMPD_BIP_TC1/
I don't see refcount issue, but that deadlock I can reproduce. Something seems to be locking the main context's mutex but not releasing it again. Nothing in our code though, we don't do anything with that mutex (it's private to glib).
Ah the problem is that something destroys the context or session in the meantime.
Remaining problem is that there's either a bug in libsoup, or we use the API wrong (even before my patches). commit 1f2e48852ee07bfb40e8fa2df9b11835a136a7ca Author: Sebastian Dröge <sebastian@centricular.com> Date: Thu May 18 15:10:30 2017 +0300 souphttpsrc: Make session sharing thread-safe on our side https://bugzilla.gnome.org/show_bug.cgi?id=780140
It looks like libsoup is using the thread default main context for some reason here, which it shouldn't because we only use the sync libsoup API. And things randomly explode in libsoup, which might also be a thread-safety bug in there. I don't see anything wrong with reference counting, etc. on our side.
Updating title accordingly.
We should disable session sharing for any libsoup version that does not include https://bugzilla.gnome.org/show_bug.cgi?id=762138#c3
*** Bug 751372 has been marked as a duplicate of this bug. ***
(In reply to Sebastian Dröge (slomo) from comment #17) > We should disable session sharing for any libsoup version that does not > include https://bugzilla.gnome.org/show_bug.cgi?id=762138#c3 Is this a blocker then?
Yes but Edward opened another bug for that, or not? This one here is solved and not a blocker, it's a new feature.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/issues/356.