GNOME Bugzilla – Bug 790412
unit test failure: rtspclientsink test_record fails with GST_MESSAGE_TYPE (msg) != GST_MESSAGE_EOS
Last modified: 2017-12-19 09:18:15 UTC
Created attachment 363758 [details] pipeline dump for test_record not-linked flow error The message on the bus is a "not-linked" internal flow error message. culprit: send_rtp_src_0:proxypad4 Pipeline dump attached as SVG.
The problem here now is that the send_rtp_src from rtpbin inside the rtspclientsink is never ever connected to anything anymore.
gst_rtsp_stream_complete_stream() is never called, but even before we do SETUP it already fails because of the unlinked pad.
AFAIU we need to call gst_rtsp_stream_set_blocked() for blocking the stream, before unblocking the payloader. And then at a later time complete_stream() and unblock(). However we can only call complete_stream() once we have the transports, and we only have the transports once we went to PLAYING (that's when ANNOUNCE, SETUP, RECORD is called and we need SETUP), but we will never preroll because the streams are blocked.
In complete_stream() only the RTCP sender part is added to the pipeline in the RECORD case.
The RECORD receiving part of the pipeline (depay -> appsink, in the test code) is not transport depended and should be probably added to the pipeline when DESCRIBE is called. What do you think?
There is no DESCRIBE, this is on the sink side. It is the sink sending the RTSP requests to the server (in PAUSED->PLAYING) :) It would have to be added in _open() already at the end, is that possible?
I meant ANNOUNCE ... on the server side ;)
Yeah, that doesn't help here :) I think on the server side it is all fine, it's the client that falls apart. And adding the sinks in _open() is not possible actually, during SETUP you might have to add new UDP sinks and if there was already an appsink before that is pre-rolled, you would lose the first buffer(s) :) Some somehow we need to pre-roll the rtspclientsink without any RTP/RTCP sinks, and add those in later when we know which are needed.
It would be much easier and cleaner to have a separate _join_bin() implementation for rtspclientsink.
What would it do different? As I see it, we would need to somehow pre-roll the sink while the GstRTSPStreams are blocked. And then once SETUP is done, we would add the remaining sinks (which complete() does, no?) and unblock the streams.
(In reply to Sebastian Dröge (slomo) from comment #10) > What would it do different? There is a difference in rtsp-stream implementation when acting in client and server mode, as far as I understand it. In the client mode there is no need for dynamic pipeline support at all. The clientsink knows what transport is supposed to be used, so after _join_bin() has been called (on the client side) the pipeline controlled by the client, is well known and should be fully configured (as all relevant information is in place, no?). If it's not desired to provide a separate implementation of rtsp-stream, it should be enough to add transport related parts of the pipeline after _join_bin().
In client-mode, the user-supplied part is known (payloader / depayloader, etc). But at join_bin() time the transports are not known yet, those are only known at a later time once all the SETUP has happened. But at that point the user-supplied parts must've joined the bin already so that rtpbin, etc is all there and the SDPs can be generated. What is needed is that after join_bin() the pipeline can pre-roll, and then the transport elements are added later and then the stream is completed.
(In reply to Sebastian Dröge (slomo) from comment #12) > In client-mode, the user-supplied part is known (payloader / depayloader, > etc). But at join_bin() time the transports are not known yet, those are > only known at a later time once all the SETUP has happened. But at that > point the user-supplied parts must've joined the bin already so that rtpbin, > etc is all there and the SDPs can be generated. You are talking about the server side implementation, I guess. _join_bin() on the client side is called before issuing any RTSP request: _sink_open -> _collect_streams -> _join_bin(). At that point the payloader part is added to the pipeline on the client side.
(In reply to Patricia Muscalu from comment #13) > (In reply to Sebastian Dröge (slomo) from comment #12) > > In client-mode, the user-supplied part is known (payloader / depayloader, > > etc). But at join_bin() time the transports are not known yet, those are > > only known at a later time once all the SETUP has happened. But at that > > point the user-supplied parts must've joined the bin already so that rtpbin, > > etc is all there and the SDPs can be generated. > > You are talking about the server side implementation, I guess. > _join_bin() on the client side is called before issuing any RTSP request: > _sink_open -> _collect_streams -> _join_bin(). At that point the payloader > part is added to the pipeline on the client side. I'm talking about the client side and I'm talking about exactly what you mention above :) That's required so that the client can send the correct ANNOUNCE request later to the server. Now at a later point (after ANNOUNCE), we do the SETUPs, and only at that point the transport elements would have to be added.
Regarding SETUP. I see now, that you can configure the rtspclientsink with a list of protocols *not* just one specific protocol (my bad, sorry for confusion ;)) So yes we need to wait until SETUP before adding a specific transport parts as the client will retry with a new allowed protocol in case of the failure of the current one. Actually _join_bin() should be called before issuing a SETUP request. There is not need to call it in OPEN, correct?
Also the udpsink destination in our case would have to be configured after SETUP. So _join_bin() should be called before SETUP (it is now, in _open()). And _complete() should be called after all SETUPs to add the transport elements (that's what it does, right?). And between _join_bin() and _complete() we need to block the streams (there's the API for that), but also ensure that somehow the whole thing pre-rolls so that we actually can go to PLAYING (which is what does SETUP). Do you agree?
(In reply to Sebastian Dröge (slomo) from comment #16) > Also the udpsink destination in our case would have to be configured after > SETUP. > > So _join_bin() should be called before SETUP (it is now, in _open()). And Correct. > _complete() should be called after all SETUPs to add the transport elements > (that's what it does, right?). Yes, as long as we talk about the server side it's true. So when the stream part is provided by the client (client_side set on rtsp-stream) only the rtcp sending part is added to the stream in _complete_stream(). "client_side" attribute should be probably changed to "receiving_mode": * in receiving_mode recv_rtcp_src pad of rtpbin is active and connected to depay * in !receiving_mode send_rtp_src is active and connected to sender part (created in create_sender_part()). If this code is supposed to be called from the rtspclientsink, we need to introduce more complexity, because in this case we want to add the sender part connected to send_rtp_src (I tried to describe the idea of client/server mode in the text above). The current design, limits the rtspclientsink's functionality (this is my interpretation, correct me if I'm wrong). It would be nice to use this sink element as a standalone plugin (it's functionality is comparable to rtspsrc I guess).
(In reply to Patricia Muscalu from comment #17) > (In reply to Sebastian Dröge (slomo) from comment #16) > > Also the udpsink destination in our case would have to be configured after > > SETUP. > > > > So _join_bin() should be called before SETUP (it is now, in _open()). And > > Correct. > > > _complete() should be called after all SETUPs to add the transport elements > > (that's what it does, right?). > > Yes, as long as we talk about the server side it's true. > So when the stream part is provided by the client (client_side set on > rtsp-stream) only the rtcp sending part is added to the stream in > _complete_stream(). > "client_side" attribute should be probably changed to "receiving_mode": > * in receiving_mode recv_rtcp_src pad of rtpbin is active and connected to > depay > * in !receiving_mode send_rtp_src is active and connected to sender part > (created in create_sender_part()). > If this code is supposed to be called from the rtspclientsink, we need to > introduce more complexity, because in this case we want to add the sender > part connected to send_rtp_src (I tried to describe the idea of > client/server mode in the text above). _complete_stream() may actually work without any additonal changes as the function (create_sender_part()) checks if we have priv->srcpad.
(In reply to Patricia Muscalu from comment #17) > (In reply to Sebastian Dröge (slomo) from comment #16) > > Also the udpsink destination in our case would have to be configured after > > SETUP. > > > > So _join_bin() should be called before SETUP (it is now, in _open()). And > > Correct. > > > _complete() should be called after all SETUPs to add the transport elements > > (that's what it does, right?). > > Yes, as long as we talk about the server side it's true. I'm talking about the client, in RECORD mode. I don't care about the server for now, it's the client that completely falls apart currently :) > So when the stream part is provided by the client (client_side set on > rtsp-stream) only the rtcp sending part is added to the stream in > _complete_stream(). > "client_side" attribute should be probably changed to "receiving_mode": What does receiving mode mean exactly? Receiving an RTP stream, so RECORD (server) and PLAY (client)? Or client side as in the one who connects to the RTSP server and sends requests? > * in receiving_mode recv_rtcp_src pad of rtpbin is active and connected to > depay > * in !receiving_mode send_rtp_src is active and connected to sender part > (created in create_sender_part()). > If this code is supposed to be called from the rtspclientsink, we need to > introduce more complexity, because in this case we want to add the sender > part connected to send_rtp_src (I tried to describe the idea of > client/server mode in the text above). Yes, it's supposed to be called from rtspclientsink. I'm also planning a rtspsrc2 around the RTSP infrastructure from the server, as it's more well-designed than what exists in the rtspsrc plugin right now. > The current design, limits the rtspclientsink's functionality (this is my > interpretation, correct me if I'm wrong). It would be nice to use this sink > element as a standalone plugin (it's functionality is comparable to rtspsrc > I guess). It is a standalone sink. It connects to an RTSP server and sends a RECORD stream to it. So now I'm more confused, or we're maybe talking about different things :) Generally what would have to happen for rtspclientsink is: 1) In READY->PAUSED the rtsp-streams are created, added to the rtpbin and blocked. This allows us to get all the information needed for sending ANNOUNCE to the server (i.e. the SDP) 2) In PAUSED->PLAYING the sink is doing SETUP(s) and RECORD. Between the last SETUP and RECORD, all transports are known and can be configured on the rtsp-streams. Which can then be "completed" (-> add udpsink, appsink, etc. as needed for the RTP streams but also for the RTCP streams. How would we know before this where to send the RTCP?), and once RECORD has happened we can unblock the rtsp-streams so that data gets send to the RTP sinks, and thus to the RTSP server.
> Generally what would have to happen for rtspclientsink is: > > 1) In READY->PAUSED the rtsp-streams are created, added to the rtpbin and > blocked. This allows us to get all the information needed for sending > ANNOUNCE to the server (i.e. the SDP) Yes, it's correct. > > 2) In PAUSED->PLAYING the sink is doing SETUP(s) and RECORD. Between the > last SETUP and RECORD, all transports are known and can be configured on the > rtsp-streams. Which can then be "completed" (-> add udpsink, appsink, etc. > as needed for the RTP streams but also for the RTCP streams. How would we > know before this where to send the RTCP?), and once RECORD has happened we > can unblock the rtsp-streams so that data gets send to the RTP sinks, and > thus to the RTSP server. Yes, before SETUP we need to allocate UDP ports for RTP and RTCP and complete the streams with the receiver and the sender parts.
(In reply to Patricia Muscalu from comment #20) > > 2) In PAUSED->PLAYING the sink is doing SETUP(s) and RECORD. Between the > > last SETUP and RECORD, all transports are known and can be configured on the > > rtsp-streams. Which can then be "completed" (-> add udpsink, appsink, etc. > > as needed for the RTP streams but also for the RTCP streams. How would we > > know before this where to send the RTCP?), and once RECORD has happened we > > can unblock the rtsp-streams so that data gets send to the RTP sinks, and > > thus to the RTSP server. > > Yes, before SETUP we need to allocate UDP ports for RTP and RTCP and complete > the streams with the receiver and the sender parts. Hm but *before* SETUP we might not know yet where to send the UDP packets as the server will tell us in its reply to SETUP. How would that work? However before SETUP we need to allocate sockets so that in SETUP we can tell the server about our own ports (for sending RTCP there, etc).
(In reply to Sebastian Dröge (slomo) from comment #21) > (In reply to Patricia Muscalu from comment #20) > > > > 2) In PAUSED->PLAYING the sink is doing SETUP(s) and RECORD. Between the > > > last SETUP and RECORD, all transports are known and can be configured on the > > > rtsp-streams. Which can then be "completed" (-> add udpsink, appsink, etc. > > > as needed for the RTP streams but also for the RTCP streams. How would we > > > know before this where to send the RTCP?), and once RECORD has happened we > > > can unblock the rtsp-streams so that data gets send to the RTP sinks, and > > > thus to the RTSP server. > > > > Yes, before SETUP we need to allocate UDP ports for RTP and RTCP and complete > > the streams with the receiver and the sender parts. > > Hm but *before* SETUP we might not know yet where to send the UDP packets as > the server will tell us in its reply to SETUP. How would that work? > Good point. I think this problem already exists in 1.12 as _join_bin() -> alloc_ports() is called before SETUP. Let me check how it works ;) I have a patch that actually works <=> the test passes. However, I need to check all the details before providing a proper solution.
What's the status here?
I'm working on a final patch.
Created attachment 365093 [details] [review] adapt to new rtsp-stream API - wip version
The attached patch adapts the clientsink code to the new rtsp-stream API. There is a timing issue. The test hangs when running in valgrind and I guess it's because of an incorrect async handling.
Just summarizing the failing test result: Currently there is no synchronization mechanism between the sink and sink thread. The sink starts tasks but does not await the task completion. There is no queuing mechanism for tasks to be sent as well. What's actually happening here is that OPEN command is "scheduled" but never executed - OPEN is cancelled by WAIT command sent on the change transition PAUSED->PLAYING. It's a pure luck that the test passes in 1.12.
Not sure I understand. You mean it's a problem with how the test works?
The problem I described above addresses thread/communication internally in the clientsink code.
Ah ok, so it was always broken and now just happens to always run into the broken case.
If WAIT command is postponed (by sleep()) the test passes in valgrind as well. WAIT is triggered by transition change PAUSED->PLAYING and when this happens the ongoing OPEN task is cancelled.
I guess it's ok to cancel some commands, however when triggering OPEN we should check if the client successfully opened connection to the server, check for errors etc. before proceeding.
Ah I forgot to reply here, sorry. From what I remember, OPEN happens in READY->PAUSED, and then all the other commands happen in PAUSED->PLAYING. It might make sense to ensure that OPEN can actually be handled/performed before going to PAUSED->PLAYING, and to maybe even block the READY->PAUSED transition on its completion.
Would that make sense, Patricia?
I think so. I'll try to provide a new patch this week.
Created attachment 365628 [details] [review] Wait until connection to server has been started
Created attachment 365629 [details] [review] Use the new rtsp-stream API
commit 64f1a3ab8527a33512682355ddc8adb604d26148 (HEAD -> master, origin/master, origin/HEAD) Author: Patricia Muscalu <patricia@dovakhiin.com> Date: Sat Dec 16 21:46:53 2017 +0100 rtspclientsink: Use the new rtsp-stream API https://bugzilla.gnome.org/show_bug.cgi?id=790412 commit 96cfed48bfd06d4c620fa0630131ac0d5aa33db7 Author: Patricia Muscalu <patricia@dovakhiin.com> Date: Sat Dec 16 21:01:43 2017 +0100 rtspclientsink: Wait until OPEN has been scheduled Make sure that the sink thread has started opening connection to the server before continuing. https://bugzilla.gnome.org/show_bug.cgi?id=790412
Thanks Patricia!