GNOME Bugzilla – Bug 738298
souphttpsrc: add 'pause-mode' property
Last modified: 2018-11-03 14:54:42 UTC
The DLNA CVP-2 spec defines different 'pause' modes regarding the way client should handle the HTTP connection when being paused: - stalling pause: keep the connection open and just stop reading from it. When resuming just resume reading from the connection. This is the current behavior in souphttpsrc. - time/range pause: close the HTTP connection when pausing. When resuming re-establish a new HTTP connection and perform a time or range seek request to the server. In order to support this new mode, we need some API controlling the behaviour of souphttpsrc when pausing.
Created attachment 288224 [details] [review] souphttpsrc: implement 'disconnect pause' Some pause modes in DLNA CVP-2 require the HTTP connection to be disconnected while the pipeline is paused. Add a property to implement this type of pause. The default is the current behaviour ('stalling').
Here is a potential implementation adding a 'pause-mode' enum property. That does the job but maybe having a 'pause-timeout' gint would be better so we can control after how long we want to close the connection when being in pause (with 0 as default being the current behaviour (stalling)). Thoughts?
I think this should only be done if accept-ranges is declared by the server, and also disconnecting should probably wait a little bit. Like only disconnect if you're in paused for >10 seconds. Independent of that you should not look at the pipeline state, but instead look if downstream is blocked. Data processing happens (and should!) in paused.
(In reply to comment #3) > I think this should only be done if accept-ranges is declared by the server, This makes sense for the "pure" HTTP use case but is going to be problematic when using the dlnasrc element. DLNA defines its own HTTP field for servers to advertise that they support "byte range" (see "7.5.4.3.2.22 MT HTTP header: Range (server)" in the spec). Some servers (like the MCVT tool) only use this field so we wouldn't be able to use "pause range" on those. Also, in theory, a DLNA server could support only "time range" requests (which is DLNA specific) so we'd still want to be able to support "time pause" in that case. This behaviour won't be the default anyway, so maybe we could argue that if you enable it through a property you are supposed to know if the server is going to support it or not? > and also disconnecting should probably wait a little bit. Like only disconnect > if you're in paused for >10 seconds. Yeah that's what I suggested by having a 'pause-timeout' property instead. > Independent of that you should not look at the pipeline state, but instead look > if downstream is blocked. Data processing happens (and should!) in paused. Do you mean I shouldn't catch the state change in gst_soup_http_src_change_state()? What's the proper way to know if the src pad is accepting data or not?
(In reply to comment #4) > (In reply to comment #3) > > I think this should only be done if accept-ranges is declared by the server, > > This makes sense for the "pure" HTTP use case but is going to be problematic > when using the dlnasrc element. DLNA defines its own HTTP field for servers to > advertise that they support "byte range" (see "7.5.4.3.2.22 MT HTTP header: > Range (server)" in the spec). Some servers (like the MCVT tool) only use this > field so we wouldn't be able to use "pause range" on those. > > Also, in theory, a DLNA server could support only "time range" requests (which > is DLNA specific) so we'd still want to be able to support "time pause" in that > case. Yeah, that's why I think all this DLNA mess should be handled differently without adding DLNA specifics to souphttpsrc but by making souphttpsrc more extensible. > > Independent of that you should not look at the pipeline state, but instead look > > if downstream is blocked. Data processing happens (and should!) in paused. > > Do you mean I shouldn't catch the state change in > gst_soup_http_src_change_state()? What's the proper way to know if the src pad > is accepting data or not? I don't know unfortunately... maybe timeout if create was not called for a longer time.
Imho, the idea of checking byte range related headers to automatically enable that feature is nice, but likely we should still let users force it through properties. This way one can cover dlnasrc use cases. My worry would be to not be able to disable it, e.g. what if the server fails after stating it support ranges ?
Then we would error out, like we do now if the server claims to support seeks but does not.
(In reply to comment #7) > Then we would error out, like we do now if the server claims to support seeks > but does not. Good point, I agree. About that timeout, did you (Guillaume) has something in mind ? I have the impression you'd need some side thread to handle it, as the source thread is generally blocked downstream.
(In reply to comment #8) > (In reply to comment #7) > > Then we would error out, like we do now if the server claims to support seeks > > but does not. > > Good point, I agree. About that timeout, did you (Guillaume) has something in > mind ? I have the impression you'd need some side thread to handle it, as the > source thread is generally blocked downstream. Not really, one of my goal when I opened this bug was to discuss the proper way this should be implemented. Maybe we could have a g_timeout (living in its own thread) of $prop-value seconds being reset each time gst_soup_http_src_create() is called to detect the pause?
Ideally we would refactor souphttpsrc to use the new SoupRequest API, and then we would always have another thread with its own main context/loop running... which could also be used for this kind of stuff. The code in souphttpsrc currently is rather fragile and hard to understand.
(In reply to comment #10) > Ideally we would refactor souphttpsrc to use the new SoupRequest API, and then > we would always have another thread with its own main context/loop running... > which could also be used for this kind of stuff. > > The code in souphttpsrc currently is rather fragile and hard to understand. I don't recall having seen API to gain access to that internal context though. Soup will still call you back on the thread default context. So while we are pushing, there is no way to timeout unless we have a extra extra thread too. It's not an issue from my point of view.
You would create and loop the thread default main context that libsoup is using ;)
(In reply to comment #12) > You would create and loop the thread default main context that libsoup is using > ;) I don't think you have access to that, or at least I don't see an API.
You don't need access to it. You create a main context, set it as the thread default and then call libsoup API. Then libsoup is using that main context you just created.
(In reply to comment #14) > You don't need access to it. You create a main context, set it as the thread > default and then call libsoup API. Then libsoup is using that main context you > just created. ok, I was just saying the same but differently, sorry for this cycle ;-P And prior to that, create a thread for running that context, a thread that is not the one the pad_push(buffer).
This case was also trying to achieve the same goal: https://bugzilla.gnome.org/show_bug.cgi?id=724786 The attached patch is good enough for our use case, but it does not work correctly when paused event comes when session_io_status is queued.
We have had the same issue regarding to support DLNA pause/resume. I have a concern regarding to thread management since it does not protect the src when invoking gst_soup_http_src_cancel_message() .Maybe mutex?
-- 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/133.