GNOME Bugzilla – Bug 630046
sdpdemux: Add optional support for rtspsrc as session element
Last modified: 2010-10-07 10:18:28 UTC
in bug# 628214, support for the rtsp-sdp protocol through redirection has been added to sdpdemux. In the same place it has been concluded that a way to use rtspsrc as session element would not decrease stability and reliability of the sdpdemux element. As redirection is not yet handled by playbin2/uridecodebin (and it's not clear if and when such a support will be added) a way to hide the complexity of handling a redirection may be desirable from applications developers. The attached patches provide a way to: - use rtspsrc as session element the "traditional" way (traditional from the sdpdemux pov). - optionally choose between rtsp redirection and rtspsrc session element.
Created attachment 170583 [details] [review] rtsp session element patch rebased on top of Mark's redirection The patch is taken from the rtsp session on from bug #628214
Created attachment 170584 [details] [review] Add an option to select between redirect or session element
Created attachment 170585 [details] [review] add an option to select between redirect or session element Same patch as before, just modified the author's email address
Improved patch pushed: commit 528f6e0573bb2856d719349c27c7c0d340b128e6 Author: Wim Taymans <wim.taymans@collabora.co.uk> Date: Tue Sep 21 19:07:05 2010 +0200 sdpdemux: add property to disable redirect Add a property to avoid redirection to the rtsp-sdp:// url but instead embeds an rtspsrc element inside sdpdemux as the session manager. Based on patch by Marco Ballesio. Fixes #630046
Just tested this last patch and it seems the sdp file as in bug #628214 is not playing. I'll perform some more tests over the weekend and eventually reopen this bug with more appropriate patches if needed.
Reopening this as well. The pipeline freezes when going to playing with the integrated patches, while the ones proposed by me are instead working properly. My suspect is that we have a race condition, but I've not yet checked into the code, but it must be triggered by the differences between https://bugzilla.gnome.org/attachment.cgi?id=170583 and 528f6e0573bb2856d719349c27c7c0d340b128e6 See also comments in bug#628214
Actually, it appears I can set the bug only to "verified" state. Does it imply that when a bug is supposedly fixed it must always surely work?
The bug is marked as fixed when the patch has been committed and verified my the committer. if the problem reappears one can file a noew bug as a regression or reopen this bug.
This is the point.. there's no "reopen" in the drop down box. I can move the bug to "UNCONFIRMED" or "VERIFIED". Doing the former, I guess it means that somebody in the community will have to confirm it again..
> This is the point.. there's no "reopen" in the drop down box. I can move the > bug to "UNCONFIRMED" or "VERIFIED". Doing the former, I guess it means that > somebody in the community will have to confirm it again.. You can't set it to NEW again as second step yourself after you've re-opened it? But in any case, it doesn't matter (to us). We don't care much about the UNCONFIRMED vs. NEW distinction; we do try to set it, but that's mostly to make the bug reporter feel better ;-)
(In reply to comment #10) > > This is the point.. there's no "reopen" in the drop down box. I can move the > > bug to "UNCONFIRMED" or "VERIFIED". Doing the former, I guess it means that > > somebody in the community will have to confirm it again.. > > You can't set it to NEW again as second step yourself after you've re-opened > it? But in any case, it doesn't matter (to us). We don't care much about the > UNCONFIRMED vs. NEW distinction; we do try to set it, but that's mostly to make > the bug reporter feel better ;-) No I can't.. maybe I need some super powers for this ;)
Created attachment 171749 [details] [review] sdpdemux: workaround rtspsrc failing state change (continued from bug #628214) The only difference (that matters here) between the original patch and the one committed is that the original one actually performs _set_state twice. That (accidentally) happens to workaround rtspsrc failing to reach PLAYING (in one call). The attached patch (hackishly) arranges for 2 _set_state calls, which suffices to eliminate race/deadlock. A "cleaner fix" may require (tricky?) core modifications, e.g. not having async_done wipe out the pending state if not a toplevel bin (??)
(In reply to comment #12) > Created an attachment (id=171749) [details] [review] > sdpdemux: workaround rtspsrc failing state change > > (continued from bug #628214) > > The only difference (that matters here) between the original patch and the one > committed is that the original one actually performs _set_state twice. > That (accidentally) happens to workaround rtspsrc failing to reach PLAYING (in > one call). The attached patch (hackishly) arranges for 2 _set_state calls, > which suffices to eliminate race/deadlock. So it all goes back to my original comment in https://bugzilla.gnome.org/show_bug.cgi?id=628214#c0 that "Still the key issue of state change violations in sdpdemux are remaining" (which is actually based on your observations ;) ). > > A "cleaner fix" may require (tricky?) core modifications, e.g. not having > async_done wipe out the pending state if not a toplevel bin (??) Yep we've already discussed about this and I guess the only way would be to set back the pipeline to PAUSED at some point, add the rtspsrc and then going again to PLAYING. I dunno how hackish it would be when compared with the current implementation. Btw your patch looks quite good to me, I'll test it as soon as I get to my hacking laptop @ home.
I can confirm Mark's patch is working well in my case as well. I'd only wish to make the rtspsrc session the default instead of using redirection. It would leverage the application developers from implementing protocol-specific redirections when using sdpdemux.
commit 161761651667a2580290fb6e7a01b0330954dedf Author: Mark Nauwelaerts <mark.nauwelaerts@collabora.co.uk> Date: Thu Oct 7 11:59:30 2010 +0200 sdpdemux: workaround internal rtspsrc failing state change Fixes #630046. --- Unfortunately, it seems advisable to perform redirection by default to remain consistent with other cases/places where a redirection is emitted.