GNOME Bugzilla – Bug 721807
souphttpsrc: expose SoupSocket
Last modified: 2017-03-16 15:55:16 UTC
I am currently trying to implement some Trafic Control management when playing medias from remote locations. To achieve that, I need to now on which socket GstSoupHTTPSrc is current operating so I can tell the kernel to give that socket enough resources. I think it can be done by adding a "soup-socket" property on GstSoupHTTPSrc. That socket can be known by listening to the "request-started" signal on SoupSession. If that sounds good to gstreamer devs, I could work on a patch.
Created attachment 265766 [details] [review] GstSoupHTTPSrc: Add a soup-socket property That property can be used to know the socket on wich current request is operating.
I did not test yet the patch yet, but that gives an idea of what I need.
Review of attachment 265766 [details] [review]: ::: ext/soup/gstsouphttpsrc.c @@ +751,3 @@ + + g_clear_object (&src->socket); + src->socket = g_object_ref (socket); You have to either make the socket volatile and use g_atomic_int_set() here.. Or use GST_OBJECT_LOCK()/UNLOCK() to protect the socket everywhere.
Created attachment 266389 [details] [review] GstSoupHTTPSrc: Add a soup-socket property That property can be used to know the socket on wich current request is operating.
Wouldn't this be better achieved by some other mechanism such as cgroups or somesuch?
@tim: They want to know which socket in the browser is doing video and which is doing HTML.
Our use case is something like that: You're watching a video in firefox. You don't want other firefox downloads to use the bandwidth needed for the video stream. So I get from gst the bitrate of audio/video streams, and tell the kernel to allow at least that for the given socket. If needed it will drop packets from others sockets so the tcp flow control will slow down those.
We want to control the rate of the incoming traffic rather than the outgoing traffic. But on Linux, the traffic control module for incoming traffic ("ingress qdisq") is before Netfilter and socket lookups: http://upload.wikimedia.org/wikipedia/commons/3/37/Netfilter-packet-flow.svg So when we have to decide whether to accept, drop or slow down a packet, we don't know which socket, socket or cgroup it might belong to, and we don't even know yet whether it is for a local process or a forward. We can just look at ip/port etc. in the packet. There are kernel patches which could make things different (either IMQ InterMediate Queueing, or openwrt's TC connmark action) but they are unlikely to be accepted in mainline so I don't want to rely on that.
I'm not so keen on exposing this either, for the same reason I wouldn't like to expose the SoupSession object. It seems like a thread-safety nightmare (Soup and GIO are generally not threadsafe) and an invitation for people to do weird things...
Would exposing the socket address (just have to save a copy of it), would be sufficient for your use case ?
"soup-local-address" and "soup-remote-address" would be enough, yes.
(In reply to comment #10) > Would exposing the socket address (just have to save a copy of it), would be > sufficient for your use case ? Yes. Ideally we need to know the tuple (source ip, source port, dest ip, dest port(=80 likely), protocol(=TCP)). Would it help with thread-safety if it was advertised on the gst bus rather than as a GObject property? I don't know whether gstsouphttpsrc does reconnections (if it is the case, the tuple can change and the app would need to get notified about it).
Out of curiosity, would it be possible to do something with the souphttpsrc thread instead? (e.g. move the thread to a certain cgroup) - you should be able to do that using the STREAM_STATUS messages then I think. (Sorry to be so insistent, I just dislike the idea of exposing this stuff)
Also, the atomic pointer code looks a bit bogus. I don't think the atomic get won't guarantee that the object will actually be alive for long enough for g_value_set_object to acquire a ref, g_clear_object() might unref it in the streaming thread(?) before that.
* will guarantee
And even just exposing the addresses as mentioned in comment 11 and comment 12 is not really useful. It's never threadsafe, souphttpsrc could decide at any moment to restart the connection.
(In reply to comment #13) > Out of curiosity, would it be possible to do something with the souphttpsrc > thread instead? (e.g. move the thread to a certain cgroup) - you should be able > to do that using the STREAM_STATUS messages then I think. - The thread-level granularity of cgroups is annoying. - Even though it should be possible to shape *egress* traffic with cgroups, we can't shape ingress traffic with cgroups because of the reason in Comment #8. > (Sorry to be so insistent, I just dislike the idea of exposing this stuff) No worries, I would like to hear if there is a better way :) (In reply to comment #16) > And even just exposing the addresses as mentioned in comment 11 and comment 12 > is not really useful. It's never threadsafe, souphttpsrc could decide at any > moment to restart the connection. It is not critical if there is some delay between a new libsoup connection and installing new traffic control rules with the correct 5-tuple (ip/port src/dest, protocol). The worst which can happen is that the first incoming packets are handled with the wrong rate. In theory it should be possible to implement this without delay: get the 5-tuple and install the new TC rules before sending anything on the socket. But in practice, it might be easier to get the information serialised from the gst bus (or GObject property notification?) and send it asynchronously with a D-Bus message to the TC daemon.
Maybe you want a signal that proxies the soupsession signal? And then you can connect to that signal and synchronously get the SoupSocket and do whatever you want... and also notice when it goes away. Still not liking the exposal of these internal details :)
Might be better to bus messages because the current approach will ignore GstURIDownloader in HLS/DASH/etc
Bus messages are async and have the thread safety problem again
Not if you just send the ip/port/ip/port in the message.
Of course, souphttpsrc could already use a completely different connection when you get this message in your application. Even if this does not matter for your very specific use case, it's extremely ugly for a public API on an element and nobody else using this API will expect it to behave like this.
slomo: If the app needs it the ip/port/ip/port/proto right now, it can just connect a sync handler. You could say the same thing about state change messages, by the time they arrive to the application, the state could have already moved on.
Another approach is to add a GstNetAddress meta to the buffers. Possibly extending it with the local ip/port (and not only the remote one).
More context how I am using the patch: http://alban-apinc.blogspot.co.uk/2014/02/traffic-control-for-multimedia-devices.html
Created attachment 280049 [details] [review] GstSoupHTTPSrc: Add a soup-socket property That property can be used to know the socket on wich current request is operating.
I wanted to test this patch so here is an updated version of top of master.
What should happen with this? WONTFIX? :)
Well, I think we provided enough explanations and alternative implementations, if you still don't want to see it in gst, yes close it won't fix and we'll keep the patch in our branch.
Bug #780140 implements sharing of the SoupSession. Would that solve something for you?
If app can get the SoupSession object, then it can connect "request-started" signal on it and get sockets. That should be fine, thanks. *** This bug has been marked as a duplicate of bug 780140 ***