commit a3e1db12929ed26a6d589e796e38ac8627471fb6
Author: Tim-Philipp Müller <tim@centricular.net>
Date:   Sat Feb 16 15:47:02 2013 +0000

    soup: use default proxy resolver instead of deprecated GNOME proxy resolver
    
    Apparently there's no reason to use it any longer. Drop libsoup-gnome
    dependency while at it, now that we don't need anything from it any
    more (it only consists entirely of deprecated API now anyways).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693911

I hope this fixes the deprecation warning for the allocator stuff:

 commit 048d256041a68d0f4e64ddbb73a6bcf325e6ff17
 Author: Tim-Philipp Müller <tim@centricular.net>
 Date:   Sat Feb 16 16:49:22 2013 +0000

    souphttpsrc: set SOUP_VERSION_{MIN_REQUIRED,MAX_ALLOWED} to suppress deprecations with newer versions
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693911


As I understand it, we should really rewrite the plugin based on the newer API which might also simplify things.

(In reply to comment #2)
> As I understand it, we should really rewrite the plugin based on the newer API
> which might also simplify things.

I suspect "might also simplify things" is a huge understatement. souphttpsrc has to jump through a lot of hoops to turn the old push-based libsoup API into a pull-based API for gstreamer.

In particular, you could completely get rid of all the callbacks and signal handlers and GMainContext/GMainLoop fiddling, and just use synchronous I/O instead. And you don't need to do the chunk allocator stuff, because you can just g_input_stream_read() directly into your buffers, and (assuming you aren't using SSL or compression), that operation shouldn't involve any additional copying of the data (beyond the eventual read(2) call itself).


gst-plugins-good currently requires libsoup >= 2.38 (which was released in March 2012). You would want to require at least 2.40 (from September 2012; the request API existed in 2.38, but it was doing the same hacks souphttpsrc does).

With 2.40, you can #define SOUP_USE_UNSTABLE_REQUEST_API, and use SoupRequester and SoupRequest.

Alternatively, you could bump the requirement to 2.42 (March 2013), and then you don't need a #define, and you can bypass SoupRequest and use soup_session_send() (which takes a SoupMessage and returns a GInputStream).

Created attachment 327866 [details] [review]
Port soup to use sync API

Still needs some more testing but the code flow looks much simpler

gst-validate and unit tests still happy after this. Decided to merge as unlikely to get wider testing without landing in master.

souphttpsrc: refactor to use Soup's sync API
    
    Replace the async API with the sync API to remove all the extra mainloop
    and context handling. Currently it blocks reading until 'blocksize'
    bytes are available but that can be improved by using:
    
    https://developer.gnome.org/gio/unstable/GPollableInputStream.html#g-pollable-input-stream-read-nonblocking
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693911

Awesome... skimming the diff I see a few possible problems:

>-          soup_session_async_new_with_options (SOUP_SESSION_ASYNC_CONTEXT,
>+          soup_session_new_with_options (SOUP_SESSION_USER_AGENT,

Be sure to read https://developer.gnome.org/libsoup/stable/libsoup-session-porting.html. In particular, you don't need to add a SOUP_TYPE_PROXY_RESOLVER_DEFAULT feature any more, since that's the default behavior (if you don't set SOUP_SESSION_PROXY_URI), and you don't need to add a SOUP_TYPE_CONTENT_DECODER (though you still want to remove the automatically-added one if !src->compress)

(OK, neither of those are actually *problems*, they're just places where the code is doing stuff it doesn't have to do any more.)

>+  src->input_stream =
>+      soup_session_send (src->session, src->msg, src->cancellable, NULL);
>+  gst_soup_http_src_got_headers (src, src->msg);

That won't do the right thing if the message fails or is cancelled before reading the headers. Note that in the old code, "got-headers" wouldn't be emitted in that case and it would go right to "finished". So some of the error checking code that used to be in gst_soup_http_src_finished_cb() needs to happen before you call gst_soup_http_src_got_headers().

Thanks for the pointers, will address those till the end of the hackfest (tomorrow).

(In reply to Dan Winship from comment #6)
> Awesome... skimming the diff I see a few possible problems:
> 
> >-          soup_session_async_new_with_options (SOUP_SESSION_ASYNC_CONTEXT,
> >+          soup_session_new_with_options (SOUP_SESSION_USER_AGENT,
> 
> Be sure to read
> https://developer.gnome.org/libsoup/stable/libsoup-session-porting.html. In
> particular, you don't need to add a SOUP_TYPE_PROXY_RESOLVER_DEFAULT feature
> any more, since that's the default behavior (if you don't set
> SOUP_SESSION_PROXY_URI), and you don't need to add a
> SOUP_TYPE_CONTENT_DECODER (though you still want to remove the
> automatically-added one if !src->compress)

Removed the proxy entry but kept the compress as it was already being explicit about the choice, even though !compress was the default before.

> 
> (OK, neither of those are actually *problems*, they're just places where the
> code is doing stuff it doesn't have to do any more.)
> 
> >+  src->input_stream =
> >+      soup_session_send (src->session, src->msg, src->cancellable, NULL);
> >+  gst_soup_http_src_got_headers (src, src->msg);
> 
> That won't do the right thing if the message fails or is cancelled before
> reading the headers. Note that in the old code, "got-headers" wouldn't be
> emitted in that case and it would go right to "finished". So some of the
> error checking code that used to be in gst_soup_http_src_finished_cb() needs
> to happen before you call gst_soup_http_src_got_headers().

Added some code to handle the cancellation, but what exactly happens when the message fails before the headers? I'm guessing no GInputStream is created there. So we should only parse the headers if we got a GInputStream?

Will also look at the other changes in that porting document.

(In reply to Thiago Sousa Santos from comment #8)
> Added some code to handle the cancellation, but what exactly happens when
> the message fails before the headers? I'm guessing no GInputStream is
> created there. So we should only parse the headers if we got a GInputStream?

right

This is introducing a race condition. This command line freezes when it tries to reach the PAUSED state:

  gst-launch-1.0 souphttpsrc location=http://<some-stream> ! fakesink

With 1.8.0 this always works. Reverting the commits above also fixes it (thanks slomo :) ).

I'll attach a backtrace.

Created attachment 328713 [details]
Backtrace when souphttpsrc hangs

(In reply to Carlos Rafael Giani from comment #11)
> Created attachment 328713 [details]
> Backtrace when souphttpsrc hangs

This looks like that glib/dconf initialization issue, we eliminated it in the tests with 8e2c1d1de56bddbff22170f8b17473882e0e63f9 but I guess it can happen with gst-launch itself.

Could you try getting another backtrace with dconf and libsoup symbols so we have the full stack?

Created attachment 328870 [details]
updated backtrace

Here's the new backtrace. Unfortunately, in Ubuntu 16.04 there doesn't seem to be a dconf-gsettings-backend debug package, so I couldn't get dconf debug symbols. I was able to get symbols for soup though.

Funny thing: I tried playing with my glib version by installing it to the uninstalled prefix. Installed master, 2.48.1 (same version as my system) and also installed from the debian source I also have installed in my system.

They all stop hanging after this. When I remove the prefix dir it fails before the 20th run. When installed it runs up to 100 times without an issue.

Here it usually happens always when playing a DASH stream, but apparently only for me. Not sure what to do about this one, Carlos' backtrace looks like it is a race during g_socket_get_type(), which should all be guarded by g_once_init_*()... and which is apparently causing the deadlock?

It doesn't look like fork() is involved here, why would gst-launch create a new process (other than for the plugin scanner)?

(In reply to Sebastian Dröge (slomo) from comment #15)
> Here it usually happens always when playing a DASH stream, but apparently
> only for me. Not sure what to do about this one, Carlos' backtrace looks
> like it is a race during g_socket_get_type(), which should all be guarded by
> g_once_init_*()... and which is apparently causing the deadlock?

sounds like probably bug 674885; the workaround is to manually g_ensure_type() the types earlier on so they don't both get initialized at the same time in different threads

Which types though?

This seems to work well enough, any missing?

  g_type_ensure (G_TYPE_SOCKET);
  g_type_ensure (G_TYPE_SOCKET_ADDRESS);
  g_type_ensure (G_TYPE_SOCKET_SERVICE);
  g_type_ensure (G_TYPE_SOCKET_FAMILY);
  g_type_ensure (G_TYPE_SOCKET_CLIENT);
  g_type_ensure (G_TYPE_RESOLVER);
  g_type_ensure (G_TYPE_PROXY_RESOLVER);
  g_type_ensure (G_TYPE_PROXY_ADDRESS);
  g_type_ensure (G_TYPE_TLS_CERTIFICATE);
  g_type_ensure (G_TYPE_TLS_CONNECTION);
  g_type_ensure (G_TYPE_TLS_DATABASE);
  g_type_ensure (G_TYPE_TLS_INTERACTION);

Do we have to do this in all other plugins that use GSocket as well then?

There are only a few cases that cause problems. It's enough to just pre-register one of the types that shows up in the deadlock in the backtrace

Ok. I tried just G_TYPE_SOCKET first, that wasn't enough (then it got stuck in SOCKET_ADDRESs) :)

Does that mean that the get_type() functions are actually not thread-safe, and g_once_init_enter/leave() is just useless? :)

g_once itself is fine. The problem is a deadlock inside the type machinery that is triggered by certain pairs of types. The referenced bug has the details.

Ok, let's close this again and see if it's enough.

commit 33fb50b308481bc31962f0eeba7a38786cf9f55b
Author: Tim-Philipp Müller <tim@centricular.com>
Date:   Fri Jun 17 19:59:13 2016 +0100

    soup: work around frequent deadlocks in GLib type initialisation
    
    .. by registering the types from the plugin init function. This
    seems to help, but we'll see if it's enough (might need similar
    things elsewhere).
    
    https://bugzilla.gnome.org/show_bug.cgi?id=693911
    https://bugzilla.gnome.org/show_bug.cgi?id=674885