GNOME Bugzilla – Bug 788340
Dynamically reconfigure pipeline in PLAY based on transports
Last modified: 2017-11-15 17:59:59 UTC
Created attachment 360667 [details] [review] Dynamically reconfigure pipeline in PLAY based on transports This patch proposes a solution how to dynamically reconfigure pipeline in PLAY based on transports. The pipeline initially created in DESCRIBE does not contain specific transport elements. The src pads on rtpbin are blocked so that no data can flow. The receiver and the sender parts are added and configured after PLAY based on the transports requested in SETUP. If the media is shared, the streams are dynamically reconfigured after each PLAY. No queues are added to the pipeline unless there are multiple transports per stream, case for shared medias. We have tested the code and all unit tests pass except one, test_record in rtspclientsink.c and we need your help on order to fix it.
Can you give some information about why/how the record test is failing, and what kind of help you need? What questions do you have? :) Also would it be possible to split this patch into smaller ones that are easier to review? This one is huge! :)
Review of attachment 360667 [details] [review]: Why are you adding the receiver/sender parts after PLAY and not during SETUP? SETUP is basically when the client tells the server to set up these pieces. In PLAY/RECORD you would only finish the setup and consider the session complete Also all this seems like an ABI change at least, behaviour is different now (see e.g. the stream.c test changes) and other users of the rtsp infrastructure will need updating. rtspclientsink probably needs updating for example. We should probably increase the soname from 0 to 1 here. ::: gst/rtsp-server/rtsp-media.c @@ -595,2 @@ if (gst_rtsp_stream_query_position (stream, &tmp)) { - data->position = MAX (data->position, tmp); The part about the position reporting could probably be a separate commit @@ +611,3 @@ priv = media->priv; + data.position = G_MAXINT64; Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing else can be found @@ +2181,3 @@ + + if (gst_rtsp_stream_is_complete (stream)) { + complete = TRUE; Shouldn't it only be considered complete once *all* streams are complete? Also this part with the seeking looks like a separate, independent change that could be a separate commit @@ +2711,3 @@ + + /* start blocked since it is possible that there are no sink elements yet */ + media_streams_set_blocked (media, TRUE); Previously this was only done for non-RECORD streams. It seems problematic to block and wait for RECORD streams as they would only receive packets once the other side got the RECORD response. @@ +3655,2 @@ switch (priv->suspend_mode) { case GST_RTSP_SUSPEND_MODE_NONE: Why go through blocking and preparing state here, instead of directly going to prepared as before? This seems to change behaviour, which might be unexpected. And could also be a separate change probably ::: gst/rtsp-server/rtsp-session-media.c @@ +415,3 @@ + * + * Returns: (transfer none): a list of #GstRTSPStreamTransport that is valid + * until the session of @media is unreffed. This is not thread-safe: the transport could be changed at any time after they were returned to the caller in theory. You need to return a full copy of the list ::: gst/rtsp-server/rtsp-stream.c @@ +311,3 @@ + g_object_unref (priv->mcast_socket_v6[0]); + if (priv->mcast_socket_v6[1]) + g_object_unref (priv->mcast_socket_v6[1]); By using a little for loop, this can be half the number of lines ;) Why was this not necessary before, what exactly changed here? @@ +1260,3 @@ static gboolean +create_and_configure_udpsources (GstElement ** udpsrc, + GSocket * socket) It's only configuring 1 udpsource now @@ +1381,3 @@ + g_clear_object (&inetaddr); + if (multicast) + inetaddr = g_inet_address_new_any (family); The problem with allocating ports ourselves is that the IP_MULTICAST_ALL and control message stuff from udpsrc won't work properly, or is it still doing that here? @@ +2678,3 @@ + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); + if (priv->appsink[1]) + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); Why don't we do this right after creation of the sinks? Why for both indizes? @@ +2722,3 @@ + +static void +plug_mcast_sink (GstRTSPStream * stream, guint index) plug_udp_sink() and plug_mcast_sink() look very similar. Maybe code can be shared? @@ +2734,3 @@ + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); + if (priv->appsink[1]) + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); Why for both indizes? @@ +2796,3 @@ + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); + if (priv->appsink[1]) + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); Why for both indizes? @@ +2826,3 @@ + /* when its only TCP, we need to set sync and preroll to FALSE + * for the sink to avoid deadlock. And this is only needed for + * sink used for RTCP data, not the RTP data. */ Elsewhere you do it for all appsinks @@ +2903,3 @@ + * | rtpbin | | tee | | sink | + * | send->sink src->sink | + * '--------' '-----' '---------' Isn't the tee also only added when needed below? @@ +3012,3 @@ + tcp = transport->lower_transport == GST_RTSP_LOWER_TRANS_TCP; + udp = transport->lower_transport == GST_RTSP_LOWER_TRANS_UDP; + mcast = transport->lower_transport == GST_RTSP_LOWER_TRANS_UDP_MCAST; Maybe with is_ prefix for these kind of flags @@ +4255,3 @@ priv->blocking = TRUE; + + if (info->type & GST_PAD_PROBE_TYPE_BUFFER) Add some additional parenthesis here, some compilers warn about this otherwise. (i.e. do "if ((a & b))") @@ +4257,3 @@ + if (info->type & GST_PAD_PROBE_TYPE_BUFFER) + buffer = gst_pad_probe_info_get_buffer (info); + else if (info->type & GST_PAD_PROBE_TYPE_BUFFER_LIST) { Add some {} above @@ +4261,3 @@ + buffer = gst_buffer_list_get (list, 0); + } else + g_assert_not_reached (); ... and a } here @@ +4351,3 @@ + g_mutex_lock (&priv->lock); + if (priv->send_src[0] && gst_pad_is_linked (priv->send_src[0])) + set_blocked (stream, FALSE); Why is it important that it is linked? and when is it important? @@ +4444,3 @@ + } + gst_event_parse_segment (event, &segment); + gst_event_parse_segment (event, &segment); Double line @@ +4447,3 @@ + if (segment->format != GST_FORMAT_TIME) + *position = -1; + else { Some {} above @@ +4498,3 @@ + else if (priv->send_src[0]) + pad = gst_object_ref (priv->send_src[0]); + else { {} above @@ +4578,3 @@ + goto create_receiver_error; + + /* in the RECORD case, we only create the receiver part */ And the sender part (RTCP) is added where? :) Just add that to the comment here @@ +4604,3 @@ + * @stream: a #GstRTSPStream + * + * Checks whether the stream is complete, contains at least one sink element. Why does that make it complete? Was does complete mean? Would it be different for RECORD, probably not? ::: tests/check/gst/client.c @@ +731,3 @@ + * The client transport settings is allowed and the requested destination + * is allocated by the server regardless if the requested address is present in + * the configured address pool or not. */ Why is this correct and why was the previous behaviour (to reject this) wrong? You change tests from previously testing failure to testing for success here ::: tests/check/gst/media.c @@ +207,3 @@ g_object_set (G_OBJECT (media), "reusable", TRUE, NULL); + pool = gst_rtsp_thread_pool_new (); A thread pool shouldn't really be needed, the server will create one. Why is it now needed? @@ +272,3 @@ g_object_unref (factory); +if (0) { +} Why?
> Why are you adding the receiver/sender parts after PLAY and not during > SETUP? SETUP is basically when the client tells the server to set up these > pieces. > In PLAY/RECORD you would only finish the setup and consider the session > complete > We presented/suggested this solution in our earlier mail conversation with you. We do not need any pipeline in SETUP anyway. Let us know if you want us to change this approach and complete the pipeline in SETUP. > ::: gst/rtsp-server/rtsp-media.c > @@ -595,2 @@ > if (gst_rtsp_stream_query_position (stream, &tmp)) { > - data->position = MAX (data->position, tmp); > > The part about the position reporting could probably be a separate commit Yes, you are right. The change is included now in 0001-rtsp-media-return-minimum-value-in-query-position-ca.patch > > @@ +611,3 @@ > priv = media->priv; > > + data.position = G_MAXINT64; > > Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing > else can be found GST_CLOCK_TIME_NONE is not a proper initial value because we are actually using MIN function now.
Created attachment 361715 [details] [review] rtsp-media: return minimum value in query position case
> @@ +1381,3 @@ > + g_clear_object (&inetaddr); > + if (multicast) > + inetaddr = g_inet_address_new_any (family); > > The problem with allocating ports ourselves is that the IP_MULTICAST_ALL and > control message stuff from udpsrc won't work properly, or is it still doing > that here? It's done in a similar way as the case where no address pool is provided (lines 1282-1284 on master). What's the proper solution?
(In reply to Patricia Muscalu from comment #3) > > Why are you adding the receiver/sender parts after PLAY and not during > > SETUP? SETUP is basically when the client tells the server to set up these > > pieces. > > In PLAY/RECORD you would only finish the setup and consider the session > > complete > > > > We presented/suggested this solution in our earlier mail conversation with > you. > We do not need any pipeline in SETUP anyway. Let us know if you want us to > change this approach and complete the pipeline in SETUP. I think it's fine but it needs some explanation IMHO. Basically, it makes no sense to set up all the elements before PLAY/RECORD because until that point everything can still change anyway. > > @@ +611,3 @@ > > priv = media->priv; > > > > + data.position = G_MAXINT64; > > > > Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing > > else can be found > > GST_CLOCK_TIME_NONE is not a proper initial value because we are actually > using MIN function now. G_MAXINT64 is not ideal either though, for position queries in TIME format values above that are valid too). I'm not sure what to suggest here though, the position/duration query API is broken by the usage of int64 but actually using uint64 for TIME format. Maybe take the code as it is in gstbin.c or gstelement.c for the generic query handling. (In reply to Patricia Muscalu from comment #5) > > @@ +1381,3 @@ > > + g_clear_object (&inetaddr); > > + if (multicast) > > + inetaddr = g_inet_address_new_any (family); > > > > The problem with allocating ports ourselves is that the IP_MULTICAST_ALL and > > control message stuff from udpsrc won't work properly, or is it still doing > > that here? > > It's done in a similar way as the case where no address pool is provided > (lines > 1282-1284 on master). What's the proper solution? I'm not sure and it was broken like that before already. But maybe you can add a FIXME comment there.
(In reply to Sebastian Dröge (slomo) from comment #2) > @@ +2181,3 @@ > + > + if (gst_rtsp_stream_is_complete (stream)) { > + complete = TRUE; > > Shouldn't it only be considered complete once *all* streams are complete? But what if not all media streams are active (<=> media contains two streams but only one is requested). The missing part here is, to make sure that all active streams are complete before performing a seek on media pipeline. Do you agree?
(In reply to Sebastian Dröge (slomo) from comment #6) > > > @@ +611,3 @@ > > > priv = media->priv; > > > > > > + data.position = G_MAXINT64; > > > > > > Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing > > > else can be found > > > > GST_CLOCK_TIME_NONE is not a proper initial value because we are actually > > using MIN function now. > > G_MAXINT64 is not ideal either though, for position queries in TIME format > values above that are valid too). I'm not sure what to suggest here though, > the position/duration query API is broken by the usage of int64 but actually > using uint64 for TIME format. The initial value of data.position is set to G_MAXINT64 and if the query position fails (<=> data.ret == FALSE), the position is set to GST_CLOCK_TIME_NONE.
(In reply to Patricia Muscalu from comment #7) > (In reply to Sebastian Dröge (slomo) from comment #2) > > @@ +2181,3 @@ > > + > > + if (gst_rtsp_stream_is_complete (stream)) { > > + complete = TRUE; > > > > Shouldn't it only be considered complete once *all* streams are complete? > > But what if not all media streams are active (<=> media contains two streams > but only one is requested). The missing part here is, to make sure that all > active streams are complete before performing a seek on media pipeline. Do > you agree? The problem here is to find a mapping between active transports (session media has this information) and active streams.
(In reply to Sebastian Dröge (slomo) from comment #2) > @@ +2678,3 @@ > + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); > + if (priv->appsink[1]) > + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); > > Why don't we do this right after creation of the sinks? > Why for both indizes? In the current implementation on master: * in case of udp && tcp: both indices are used * in case of tcp: the async and sync flags are set to false only for rtcp appsink.
(In reply to Patricia Muscalu from comment #7) > (In reply to Sebastian Dröge (slomo) from comment #2) > > @@ +2181,3 @@ > > + > > + if (gst_rtsp_stream_is_complete (stream)) { > > + complete = TRUE; > > > > Shouldn't it only be considered complete once *all* streams are complete? > > But what if not all media streams are active (<=> media contains two streams > but only one is requested). The missing part here is, to make sure that all > active streams are complete before performing a seek on media pipeline. Do > you agree? You know from the SETUPs that happened before PLAY/RECORD which streams are requested, and all those need to be complete. Or not? (In reply to Patricia Muscalu from comment #9) > (In reply to Patricia Muscalu from comment #7) > > (In reply to Sebastian Dröge (slomo) from comment #2) > > > @@ +2181,3 @@ > > > + > > > + if (gst_rtsp_stream_is_complete (stream)) { > > > + complete = TRUE; > > > > > > Shouldn't it only be considered complete once *all* streams are complete? > > > > But what if not all media streams are active (<=> media contains two streams > > but only one is requested). The missing part here is, to make sure that all > > active streams are complete before performing a seek on media pipeline. Do > > you agree? > > The problem here is to find a mapping between active transports (session > media has this information) and active streams. Can we expose that information easily? (In reply to Patricia Muscalu from comment #8) > (In reply to Sebastian Dröge (slomo) from comment #6) > > > > @@ +611,3 @@ > > > > priv = media->priv; > > > > > > > > + data.position = G_MAXINT64; > > > > > > > > Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing > > > > else can be found > > > > > > GST_CLOCK_TIME_NONE is not a proper initial value because we are actually > > > using MIN function now. > > > > G_MAXINT64 is not ideal either though, for position queries in TIME format > > values above that are valid too). I'm not sure what to suggest here though, > > the position/duration query API is broken by the usage of int64 but actually > > using uint64 for TIME format. > > The initial value of data.position is set to G_MAXINT64 and if the query > position fails (<=> data.ret == FALSE), the position is set to > GST_CLOCK_TIME_NONE. Ok :) (In reply to Patricia Muscalu from comment #10) > (In reply to Sebastian Dröge (slomo) from comment #2) > > @@ +2678,3 @@ > > + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); > > + if (priv->appsink[1]) > > + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); > > > > Why don't we do this right after creation of the sinks? > > Why for both indizes? > > In the current implementation on master: > * in case of udp && tcp: both indices are used > * in case of tcp: the async and sync flags are set to false only for rtcp > appsink. The RTCP sinks are always sync=async=false, the RTP sinks are always sync=async=true or not?
> (In reply to Patricia Muscalu from comment #8) > > (In reply to Sebastian Dröge (slomo) from comment #6) > > > > > @@ +611,3 @@ > > > > > priv = media->priv; > > > > > > > > > > + data.position = G_MAXINT64; > > > > > > > > > > Why this change? Generally, -1 is returned (GST_CLOCK_TIME_NONE) if nothing > > > > > else can be found > > > > > > > > GST_CLOCK_TIME_NONE is not a proper initial value because we are actually > > > > using MIN function now. > > > > > > G_MAXINT64 is not ideal either though, for position queries in TIME format > > > values above that are valid too). I'm not sure what to suggest here though, > > > the position/duration query API is broken by the usage of int64 but actually > > > using uint64 for TIME format. > > > > The initial value of data.position is set to G_MAXINT64 and if the query > > position fails (<=> data.ret == FALSE), the position is set to > > GST_CLOCK_TIME_NONE. > > Ok :) Ok, so I guess that the query position patch is correct, or? > > (In reply to Patricia Muscalu from comment #10) > > (In reply to Sebastian Dröge (slomo) from comment #2) > > > @@ +2678,3 @@ > > > + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); > > > + if (priv->appsink[1]) > > > + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); > > > > > > Why don't we do this right after creation of the sinks? > > > Why for both indizes? > > > > In the current implementation on master: > > * in case of udp && tcp: both indices are used > > * in case of tcp: the async and sync flags are set to false only for rtcp > > appsink. > > The RTCP sinks are always sync=async=false, the RTP sinks are always > sync=async=true or not? This is the correct behavior, so this means that we have a bug on master.
(In reply to Patricia Muscalu from comment #12) > > Ok :) > > Ok, so I guess that the query position patch is correct, or? Yes > This is the correct behavior, so this means that we have a bug on master. Ok, so let's fix that while we're at it. Apart from that it's all clear what is to be changed with the patches?
(In reply to Sebastian Dröge (slomo) from comment #13) > (In reply to Patricia Muscalu from comment #12) > > > > Ok :) > > > > Ok, so I guess that the query position patch is correct, or? > > Yes > > > This is the correct behavior, so this means that we have a bug on master. > > Ok, so let's fix that while we're at it. > Ok. I'll provide a separate patch. > > Apart from that it's all clear what is to be changed with the patches? Yes, I guess. I'll send new patches. Thanks for reviewing.
(In reply to Patricia Muscalu from comment #10) > > > (In reply to Sebastian Dröge (slomo) from comment #2) > > > > @@ +2678,3 @@ > > > > + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); > > > > + if (priv->appsink[1]) > > > > + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); > > > > > > > > Why don't we do this right after creation of the sinks? > > > > Why for both indizes? > > > > > > In the current implementation on master: > > > * in case of udp && tcp: both indices are used > > > * in case of tcp: the async and sync flags are set to false only for rtcp > > > appsink. > > > > The RTCP sinks are always sync=async=false, the RTP sinks are always > > sync=async=true or not? > > This is the correct behavior, so this means that we have a bug on master. What about RECORD streams? Don't we need to set async to FALSE for RTP sinks as well?
Yes
(In reply to Patricia Muscalu from comment #15) > What about RECORD streams? Don't we need to set async to FALSE for RTP sinks > as well? Sorry for my stupid question :). In RECORD streams we do not have any udpsinks or appsinks anymore.
You should have some for RTCP at least. And for RTP I think there should be some that are sending some dummy packets for NAT breaking, like rtspsrc does.
Yes, you absolutely right: the RTCP branch is constructed as well in the rtsp-server code (we need to correct our patch). Regarding dummy-packet-generation ... As the <depayloader->sink/whatever> part of the pipeline is provided by the client side, so I guess it's up to the application to provide some keepalive mechanism when using NAT, correct?
It's not about keep-alive, but for opening ports. See gst_rtspsrc_send_dummy_packets()
Ok, I'll of course read about it. But do you agree that sending dummy packets is not a part the rtsp-server implementation?
Probably not, but something that should be done in rtspsrc/rtspclientsink
Created attachment 362154 [details] [review] stream: set async=sync=false only for RTCP appsink
Created attachment 362157 [details] [review] stream: add functions to get rtp and rtcp multicast
Created attachment 362159 [details] [review] session-media: add function to get a list of transports
Created attachment 362160 [details] [review] stream: obtain stream position from pad
Created attachment 362161 [details] [review] Dynamically reconfigure pipeline in PLAY based on transport
Created attachment 362162 [details] [review] media: seek on media pipelines that are complete
(In reply to Sebastian Dröge (slomo) from comment #2) > @@ +2711,3 @@ > + > + /* start blocked since it is possible that there are no sink elements yet > */ > + media_streams_set_blocked (media, TRUE); > > Previously this was only done for non-RECORD streams. It seems problematic > to block and wait for RECORD streams as they would only receive packets once > the other side got the RECORD response. Now, the RECORD and LIVE streams are treated in the same way, meaning that media streams are blocked in DESCRIBE and unblocked when PLAY comes. > @@ +3655,2 @@ > switch (priv->suspend_mode) { > case GST_RTSP_SUSPEND_MODE_NONE: > > Why go through blocking and preparing state here, instead of directly going > to prepared as before? This seems to change behaviour, which might be > unexpected. And could also be a separate change probably Added a comment in the code. This change is logically depended on the whole dynamic-pipeline change so it's difficult to provide a separate change for it. > ::: gst/rtsp-server/rtsp-stream.c > @@ +311,3 @@ > + g_object_unref (priv->mcast_socket_v6[0]); > + if (priv->mcast_socket_v6[1]) > + g_object_unref (priv->mcast_socket_v6[1]); > > By using a little for loop, this can be half the number of lines ;) Done. > Why was this not necessary before, what exactly changed here? It's a new socket attribute. > @@ +1381,3 @@ > + g_clear_object (&inetaddr); > + if (multicast) > + inetaddr = g_inet_address_new_any (family); > > The problem with allocating ports ourselves is that the IP_MULTICAST_ALL and > control message stuff from udpsrc won't work properly, or is it still doing > that here? Added a FIXME note. > > @@ +2678,3 @@ > + g_object_set (priv->appsink[0], "async", FALSE, "sync", FALSE, NULL); > + if (priv->appsink[1]) > + g_object_set (priv->appsink[1], "async", FALSE, "sync", FALSE, NULL); > > Why don't we do this right after creation of the sinks? > Why for both indizes? Fixed: stream-set-async-sync-false-only-for-RTCP-appsink.patch > > @@ +2722,3 @@ > + > +static void > +plug_mcast_sink (GstRTSPStream * stream, guint index) > > plug_udp_sink() and plug_mcast_sink() look very similar. Maybe code can be > shared? Fixed, however I think that the code is less readable after this change. > @@ +2903,3 @@ > + * | rtpbin | | tee | | sink | > + * | send->sink src->sink | > + * '--------' '-----' '---------' > > Isn't the tee also only added when needed below? The tee element is always present. The code is less complected that way. > @@ +4351,3 @@ > + g_mutex_lock (&priv->lock); > + if (priv->send_src[0] && gst_pad_is_linked (priv->send_src[0])) > + set_blocked (stream, FALSE); > > Why is it important that it is linked? and when is it important? It covers the case when not all media streams are active. > @@ +4604,3 @@ > + * @stream: a #GstRTSPStream > + * > + * Checks whether the stream is complete, contains at least one sink > element. > > Why does that make it complete? Was does complete mean? Added a more descriptive information. > > Would it be different for RECORD, probably not? Not really, removed an unnecessary if statement . > > ::: tests/check/gst/client.c > @@ +731,3 @@ > + * The client transport settings is allowed and the requested destination > + * is allocated by the server regardless if the requested address is > present in > + * the configured address pool or not. */ > > Why is this correct and why was the previous behaviour (to reject this) > wrong? You change tests from previously testing failure to testing for > success here Reverted this change. If we need more changes a new bug will be filed as this change is not really relevant to the current problem. > > ::: tests/check/gst/media.c > @@ +207,3 @@ > g_object_set (G_OBJECT (media), "reusable", TRUE, NULL); > > + pool = gst_rtsp_thread_pool_new (); > > A thread pool shouldn't really be needed, the server will create one. Why is > it now needed? The thread pool is actually created in the test code at line 173 (master version). The pool is created by the server, yes, but we do not have any access to the server instance from the server code. I realize now that this change can provided in a separate patch.
(In reply to Sebastian Dröge (slomo) from comment #1) > Can you give some information about why/how the record test is failing, and > what kind of help you need? What questions do you have? :) The only request that reaches the server is OPTIONS and the test terminates with an error code: Failure 'GST_MESSAGE_TYPE (msg) != GST_MESSAGE_EOS' occurred How does it really work? _join_bin() is called on the client side from gst_rtsp_client_sink_collect_streams, I'm a bit confused ...
(In reply to Patricia Muscalu from comment #29) > (In reply to Sebastian Dröge (slomo) from comment #2) > > @@ +2711,3 @@ > > + > > + /* start blocked since it is possible that there are no sink elements yet > > */ > > + media_streams_set_blocked (media, TRUE); > > > > Previously this was only done for non-RECORD streams. It seems problematic > > to block and wait for RECORD streams as they would only receive packets once > > the other side got the RECORD response. > > Now, the RECORD and LIVE streams are treated in the same way, meaning that > media streams are blocked in DESCRIBE and unblocked when PLAY comes. I meant of course when PLAY or RECORD comes.
(In reply to Patricia Muscalu from comment #30) > (In reply to Sebastian Dröge (slomo) from comment #1) > > Can you give some information about why/how the record test is failing, and > > what kind of help you need? What questions do you have? :) > > The only request that reaches the server is OPTIONS and the test terminates > with an error code: > > Failure 'GST_MESSAGE_TYPE (msg) != GST_MESSAGE_EOS' occurred > > How does it really work? _join_bin() is called on the client side from > gst_rtsp_client_sink_collect_streams, I'm a bit confused ... I don't know, how did this ever work before? The test seems a bit weird also. Apart from this failure, does it all look good in your testing? From my side the patches look good (except for that broken test), but I didn't run the testsuite myself.
Yes, all the tests pass. This is actually the only test failing.
How do we proceed?
Maybe let's just disable the test, or check for the new behaviour? It seems like the behaviour it's testing for makes no sense.
gst/rtspclientsink.c:185:F:general:test_record:0: Failure 'GST_MESSAGE_TYPE (msg) != GST_MESSAGE_EOS' occurred This error also happens for me with latest master btw.
commit d51f8abe567587343cad0ad096e4740e9969b17d (HEAD -> master, origin/master, origin/HEAD) Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Nov 15 19:52:29 2017 +0200 rtsp-stream: Only update the RTP udpsink if it actually exists For send-only streams it does not exist, but the RTCP udpsink might. commit bed93b915d8a4be12a2ab71ed10f919d55519b0a Author: Sebastian Dröge <sebastian@centricular.com> Date: Wed Nov 15 18:15:53 2017 +0200 win32: Update exports commit efdb795c86d5e9fbc25da52406c0791ebccb0b05 Author: Patricia Muscalu <patricia@axis.com> Date: Mon Oct 23 09:49:09 2017 +0200 rtsp-media: seek on media pipelines that are complete Make sure that a seek is performed on pipelines that contain at least one sink element. Change-Id: Icf398e10add3191d104b1289de612412da326819 https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit a7732a68e8bc6b4ba15629c652056c240c624ff0 Author: Patricia Muscalu <patricia@axis.com> Date: Tue Oct 17 10:44:33 2017 +0200 Dynamically reconfigure pipeline in PLAY based on transports The initial pipeline does not contain specific transport elements. The receiver and the sender parts are added after PLAY. If the media is shared, the streams are dynamically reconfigured after each PLAY. https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit 930a602e17e4bfdf55522011d3d132bc6777452b Author: Patricia Muscalu <patricia@axis.com> Date: Mon Oct 16 12:40:57 2017 +0200 rtsp-stream: obtain stream position from pad If no sinks have been added yet, obtain the current and the stop position of the stream from the send_src pad. Change-Id: Iacd4ab4bdc69f6b49370d06012880ce48a7d595a https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit 5ec1b809895b35d103fbef5f20ebef0ddefd2c59 Author: Patricia Muscalu <patricia@axis.com> Date: Mon Oct 16 11:35:10 2017 +0200 rtsp-session-media: add function to get a list of transports Change-Id: I817e10624da0f3200f24d1b232cff481099278e3 https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit 51d670f73bb4d4fd8fbfde2daf191bc8f7a74218 Author: Patricia Muscalu <patricia@axis.com> Date: Mon Oct 16 11:15:55 2017 +0200 rtsp-stream: add functions to get rtp and rtcp multicast sockets Change-Id: Iddfe6e0bd250cb0159096d5eba9e4202d22b56db https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit c9605cc5e1c36a2839df389d5fa002aaef43f786 Author: Patricia Muscalu <patricia@axis.com> Date: Fri Oct 20 12:21:48 2017 +0200 stream: set async=sync=false only for RTCP appsink Change-Id: I929a218a9adf4759f61322b6f2063aacc5595f90 https://bugzilla.gnome.org/show_bug.cgi?id=788340 commit b5c3ef8d53e0768ee4677887bb4808ef1f169ca0 Author: Patricia Muscalu <patricia@axis.com> Date: Mon Oct 16 10:10:17 2017 +0200 rtsp-media: return minimum value in query position case The minimum position should be returned as we are interested in the whole interval. Change-Id: I30e297fc040c995ae40c25dee8ff56321612fe2b https://bugzilla.gnome.org/show_bug.cgi?id=788340