After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 705371 - souphttpsrc: Does network operations from the state change thread
souphttpsrc: Does network operations from the state change thread
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal blocker
: 1.1.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-08-02 18:32 UTC by Greg Rutz
Modified: 2013-09-18 16:48 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Minimal testcase (1.29 KB, text/x-csrc)
2013-08-03 09:50 UTC, Jens Georg
  Details
souphttpsrc: do not do http requests in READY (2.21 KB, patch)
2013-09-16 17:56 UTC, Thiago Sousa Santos
committed Details | Review

Description Greg Rutz 2013-08-02 18:32:21 UTC
For development purposes, I am using Rygel as both the client and the server (single process on the same machine).  This bug ONLY occurs when playing media in this scenario.

With the following change in GStreamer (gst-plugins-good, souphttpsrc), Rygel hangs when playing any media content:

--
5bc5f4a0f6d9b82f72b39926d8ad81600dac4604 is the first bad commit
commit 5bc5f4a0f6d9b82f72b39926d8ad81600dac4604
Author: Thiago Santos <thiago.sousa.santos@collabora.com>
Date:   Wed Jun 19 14:06:40 2013 -0300

    souphttpsrc: allow seeks in ready
    
    On is_seekable, check if the server's headers have already been
    received. If not, do a HEAD request to get them before responding
    to basesrc.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=702206
--

The reason for the hang is that souphttpsrc now makes an HTTP request (HEAD) during pipeline setup prior to moving to PLAY state.  It does this so that it can determine whether or not the element can report itself as "seekable" (by looking for range request support in the HTTP HEAD response headers).  This request is being initiated on the Rygel main g_main_loop thread.  Once the request has been submitted to libsoup, the thread enters a new g_main_loop and waits for a response.  Since the Rygel main loop thread is now blocked in a new main loop in souphttpsrc, the request is never processed by the Rygel server and we are now deadlocked.
Comment 1 Jens Georg 2013-08-03 09:26:23 UTC
We do a HEAD reaquest in rygel for the exact same reason, wonder if it's possible to pass that information to souphttpsrc, at least as a work-around...
Comment 2 Jens Georg 2013-08-03 09:50:26 UTC
Created attachment 250755 [details]
Minimal testcase

Minimal testcase showing the problem without rygel
Comment 3 Tim-Philipp Müller 2013-08-03 10:08:59 UTC
Anything souphttpsrc does should not depend on or be run in Rygel's main loop, but its own custom main loop / main context.

Regression, so marking as blocker.
Comment 4 Greg Rutz 2013-08-05 16:40:02 UTC
Tim's latest comments confuse me.  Jens marked this bug as being a GStreamer problem.  But Tim's comment seems to indicate that Rygel should not be running GStreamer from its main loop.  So, is this problem really a GStreamer bug or a Rygel bug?
Comment 5 Tim-Philipp Müller 2013-08-06 08:28:04 UTC
*If* Jens's comment that

> This [HEAD] request is being initiated on the Rygel main g_main_loop thread

is correct, then that would indicate a bug in souphttpsrc (or - less likely - libsoup).

The unit tests also fail because of this.
Comment 6 Tim-Philipp Müller 2013-08-06 08:32:38 UTC
> *If* Jens's comment that

Sorry, that was your comment of course Greg. Anyway, I'm quite sure it's a souphttpsrc problem.
Comment 7 Sebastian Dröge (slomo) 2013-08-20 14:18:06 UTC
Well, this testcase obviously hangs because the HEAD request is done when souphttpsrc is set from READY to PAUSED, which is done from the main thread when setting playbin to PLAYING. That means that the server can't reply yet because its main loop is not running yet. souphttpsrc then waits (apparently forever) for the server to reply to the request (opening a connection works it seems).

Is that the same problem in your original case of this bug?
Comment 8 Greg Rutz 2013-08-20 14:21:04 UTC
(In reply to comment #7)
> Is that the same problem in your original case of this bug?

Yes, thats why I originally posted this bug in Rygel.  I wasn't sure whether the playbin initialization (client) and server threads should be the same.
Comment 9 Sebastian Dröge (slomo) 2013-08-20 14:29:40 UTC
Ok, so that's just another case of doing synchronous network operations from the main thread. Our network elements should not do that
Comment 10 Sebastian Dröge (slomo) 2013-08-21 06:53:17 UTC
The souphttpsrc code before the HEAD request was added did all network operations from different threads, so this is actually a regression. Should be moved to a different thread (without waiting for it on the main thread) or be reverted. CC'ing Thiago who wrote the original patch
Comment 11 Sebastian Dröge (slomo) 2013-09-12 10:28:47 UTC
Thiago, any ideas here? In the worst case we just have to revert the commits that added the HEAD requests before 1.2.
Comment 12 Thiago Sousa Santos 2013-09-13 04:26:33 UTC
The objective of the patch was allowing gsturidownloader (used by dashdemux) to fetch only the range of the file that was defined by the dash fragment. So doing this operation in READY isn't acceptable.

It is also possible for dashdemux to set souphttpsrc to playing and then issue the seek for the desired range, but this would make it start downloading from 0 until the seek was processed. How can souphttpsrc fetch only a range of bytes without attempting to start from 0?

Use properties to define the range? Set it to live to prevent producing data in paused and seek in paused?
Comment 13 Sebastian Dröge (slomo) 2013-09-13 07:29:47 UTC
What about storing the seek in READY and then handle this seek when going to PAUSED?
Comment 14 Thiago Sousa Santos 2013-09-16 17:56:02 UTC
Created attachment 255057 [details] [review]
souphttpsrc: do not do http requests in READY

Patch that prevents doing HEAD requests if in READY. This still keeps dash
working for range requests as the seek requested start will be stored for
later use when doing the GET.

I'm still unsure if we need to keep the 'is_seekable' call to the
new check_seekable function as I'm not sure if it is acceptable to do the
request from query/event handlers as its when basesrc seems to call
is_seekable
Comment 15 Sebastian Dröge (slomo) 2013-09-18 16:37:04 UTC
commit c84282b9a6bfe9383172c4f8ed28b655cfe84f2f
Author: Thiago Santos <thiago.sousa.santos@collabora.com>
Date:   Mon Sep 16 13:53:45 2013 -0300

    souphttpsrc: do not do http requests in READY
    
    HEAD requests to discover if the server is seekable shouldn't be done in
    READY as it might lock the main thread that is doing the state change.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=705371