GNOME Bugzilla – Bug 793441
rtsp-stream: client transport is not updated for multicast clients
Last modified: 2018-08-15 06:35:47 UTC
Created attachment 368335 [details] [review] rtsp-stream: Set the unicast TTL parameter on multicast udp sinks If a new client requests receiving rtp data from the different multicast group than the previous multicast client, this new request is ignored <=> the new client configuration (host and ports) is not propagated to the multicast multiudpsink. The provided 2 patches suggest the solution.
Created attachment 368336 [details] [review] rtsp-stream: Update transport for multicast clients as well
I would like to mention additional problems: 1. The public API gst_rtsp_stream_get_multicast_address() has been introduced a long time ago, I know, but what's the real intention of this function? The server is just sending rtp/rtcp data to the multicast groups. It does not have any multicast address itself. Which address is supposed to be returned in the shared media case with clients requesting different multicast addresses? This question is tightly related to the logic in the check_mcast_part_for_transport() function. The function will always fail for the multicast clients requesting another transport settings than the first multicast client. 2. update_transport() function will change ttl-mc value. Do we want to do this? I added a FIXME.
(In reply to Patricia Muscalu from comment #2) > I would like to mention additional problems: > > 1. The public API gst_rtsp_stream_get_multicast_address() has been introduced > a long time ago, I know, but what's the real intention of this function? The > server is just sending rtp/rtcp data to the multicast groups. It does not > have any multicast address itself. Which address is supposed to be returned > in the shared media case with clients requesting different multicast > addresses? This question is tightly related to the logic in the > check_mcast_part_for_transport() function. The function will always fail for > the multicast clients requesting another transport settings than the first > multicast client. It should probably have never been a public function, like many things in gst-rtsp-server that are public API. Feel free to deprecate that function and let it continue to return something useless as it does since the beginning of time :) > 2. update_transport() function will change ttl-mc value. Do we want to do > this? > I added a FIXME. What does that mean exactly? The TTL of all multicast clients will be defined by the latest (or first?) multicast client to join?
> What does that mean exactly? The TTL of all multicast clients will be > defined by the latest (or first?) multicast client to join? A good question. As the media is shared it means that any socket changes requested by the new clients will impact an already existing client. It's not allowed to change mtu in the case of a shared media, for example.
Review of attachment 368336 [details] [review]: Generally looks good ::: gst/rtsp-server/rtsp-stream.c @@ +3737,3 @@ priv->transports = g_list_prepend (priv->transports, trans); + /* FIXME: Is it ok to set ttl-mc if media is shared? */ Maybe we should take the max of all current ttl-mc and update that on add/remove?
But we should already respond with the correct ttl value on the SETUP request.
(In reply to Patricia Muscalu from comment #6) > But we should already respond with the correct ttl value on the SETUP > request. But we don't? Do you have a suggestion how to solve this? Instead of taking the maximum or minimum ttl-mc, we could also take only the one of the first for example.
(In reply to Sebastian Dröge (slomo) from comment #7) > (In reply to Patricia Muscalu from comment #6) > > But we should already respond with the correct ttl value on the SETUP > > request. > > But we don't? We respond, but the value might not be the one, the current client is requesting. Example: * SETUP 1: ttl=1 => the value will be set on the rtp socket RTSP OK ttl=1 * SETUP 2: ttl=3 RTSP OK ttl=3 => but this value is never set on the socket! > Do you have a suggestion how to solve this? Instead of taking the maximum or > minimum ttl-mc, we could also take only the one of the first for example. I think, that your first suggestion of taking the maximum value is the one we should implement. I'll send you a patch.
(In reply to Patricia Muscalu from comment #8) > (In reply to Sebastian Dröge (slomo) from comment #7) > > (In reply to Patricia Muscalu from comment #6) > > > But we should already respond with the correct ttl value on the SETUP > > > request. > > > > But we don't? > We respond, but the value might not be the one, the current client is > requesting. > Example: > * SETUP 1: ttl=1 => the value will be set on the rtp socket > RTSP OK ttl=1 > * SETUP 2: ttl=3 > RTSP OK ttl=3 => but this value is never set on the socket! > > > Do you have a suggestion how to solve this? Instead of taking the maximum or > > minimum ttl-mc, we could also take only the one of the first for example. > I think, that your first suggestion of taking the maximum value is the one > we should implement. I'll send you a patch. The drawback of this approach is that, that the first client will suddenly receive IP packets with ttl=3 (if we follow the example above). So, you are probably right, that the first client should decide.
Created attachment 368443 [details] [review] Update transport for multicast clients as well
Created attachment 368444 [details] [review] Let the first multicast request decide the ttl socket value
(In reply to Patricia Muscalu from comment #2) > I would like to mention additional problems: > > 1. The public API gst_rtsp_stream_get_multicast_address() has been introduced > a long time ago, I know, but what's the real intention of this function? The > server is just sending rtp/rtcp data to the multicast groups. It does not > have any multicast address itself. Which address is supposed to be returned > in the shared media case with clients requesting different multicast > addresses? This question is tightly related to the logic in the > check_mcast_part_for_transport() function. The function will always fail for > the multicast clients requesting another transport settings than the first > multicast client. > I'm currently working on this issue.
In order to proceed, I need to ask a couple of questions regarding gst_rtsp_stream_reserve_address() API: 1. What does it really mean to reserve a multicast address? What's supposed to be achieved by this API? 2. Why is this function called from default_configure_client_transport() in the specific transport case? Does it mean that the default implementation does not allow multiple mcast requests with the same multicast group? It's allowed in the normal case anyway. 3. In the client settings case, why do we require the existence of an address pool? If "transport.client-settings" parameter is set to true, the client is allowed to specify destination etc. There is no need for pre-configured address pool, I guess.
> 1. What does it really mean to reserve a multicast address? What's supposed to be achieved by this API? I don't know either :) It should be enough to take the multicast address from the pool once it's needed > 2. [...] I also don't know. It should be allowed indeed. > 3. In the client settings case, why do we require the existence of an address pool? If "transport.client-settings" parameter is set to true, the client is allowed to specify destination etc. There is no need for pre-configured address pool, I guess. Yes, correct AFAIU
Created attachment 369077 [details] [review] Set the unicast TTL parameter on multicast udp sinks
Created attachment 369078 [details] [review] Update transport for multicast clients as well
Created attachment 369079 [details] [review] Let the first multicast request decide the ttl socket value
Created attachment 369080 [details] [review] Don't reserve multicast address in the client settings case
Created attachment 369081 [details] [review] Added test case for multicast ttl
Created attachment 369082 [details] [review] Don't require address pool in the transport specific case
Created attachment 369083 [details] [review] Corrected logic in check_mcast_part_for_transport
Comment on attachment 369077 [details] [review] Set the unicast TTL parameter on multicast udp sinks The commit message is confusing. You want to set the multicast-ttl on multicast sinks, not the unicast-ttl, or not? :)
Review of attachment 369080 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ -1883,3 @@ - * the one requested by the client */ - addr = gst_rtsp_stream_reserve_address (ctx->stream, ct->destination, - ct->port.min, ct->port.max - ct->port.min + 1, ct->ttl); If use_client_settings is TRUE, shouldn't we make sure to reserve the address that the (new) client wants instead of just doing nothing? How can we otherwise be sure that we can later use it?
(In reply to Sebastian Dröge (slomo) from comment #23) > Review of attachment 369080 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ -1883,3 @@ > - * the one requested by the client */ > - addr = gst_rtsp_stream_reserve_address (ctx->stream, > ct->destination, > - ct->port.min, ct->port.max - ct->port.min + 1, ct->ttl); > > If use_client_settings is TRUE, shouldn't we make sure to reserve the > address that the (new) client wants instead of just doing nothing? How can > we otherwise be sure that we can later use it? It's not the multicast address we want to reserve. What we want to to here is to reserve a unicast address that the socket is bound to. There is a FIXME in gst_rtsp_stream_reserve_address().
Comment on attachment 369083 [details] [review] Corrected logic in check_mcast_part_for_transport Why is this stored as a GString instead of storing it as an array/list of address/ports? Also please explain in the commit message what exactly this is fixing
Comment on attachment 369081 [details] [review] Added test case for multicast ttl The commit message is wrong. It's more like you're removing all kinds of tests and add one :)
(In reply to Sebastian Dröge (slomo) from comment #25) > Comment on attachment 369083 [details] [review] [review] > Corrected logic in check_mcast_part_for_transport > > Why is this stored as a GString instead of storing it as an array/list of > address/ports? > > Also please explain in the commit message what exactly this is fixing Inspired by PROP_CLIENTS in gstmultiudpsink.
(In reply to Sebastian Dröge (slomo) from comment #26) > Comment on attachment 369081 [details] [review] [review] > Added test case for multicast ttl > > The commit message is wrong. It's more like you're removing all kinds of > tests and add one :) I agree, the commit message should be more precise.
(In reply to Sebastian Dröge (slomo) from comment #22) > Comment on attachment 369077 [details] [review] [review] > Set the unicast TTL parameter on multicast udp sinks > > The commit message is confusing. You want to set the multicast-ttl on > multicast sinks, not the unicast-ttl, or not? :) Yes. It should be "... unicast TTL ..." -> "... multicast TTL ...".
(In reply to Patricia Muscalu from comment #27) > (In reply to Sebastian Dröge (slomo) from comment #25) > > Comment on attachment 369083 [details] [review] [review] [review] > > Corrected logic in check_mcast_part_for_transport > > > > Why is this stored as a GString instead of storing it as an array/list of > > address/ports? > > > > Also please explain in the commit message what exactly this is fixing > > Inspired by PROP_CLIENTS in gstmultiudpsink. That's because there it is a GObject property. Here we have proper C API and can make use more better types :) That would also make the strstr() unnecessary that is currently there to check if client is there, and makes it easier to remove a client.
(In reply to Sebastian Dröge (slomo) from comment #30) > (In reply to Patricia Muscalu from comment #27) > > (In reply to Sebastian Dröge (slomo) from comment #25) > > > Comment on attachment 369083 [details] [review] [review] [review] [review] > > > Corrected logic in check_mcast_part_for_transport > > > > > > Why is this stored as a GString instead of storing it as an array/list of > > > address/ports? > > > > > > Also please explain in the commit message what exactly this is fixing > > > > Inspired by PROP_CLIENTS in gstmultiudpsink. > > That's because there it is a GObject property. Here we have proper C API and > can make use more better types :) That would also make the strstr() > unnecessary that is currently there to check if client is there, and makes > it easier to remove a client. Yes, good point. We should have a list of addresses instead. Ok with GList?
Sure, or GQueue. Whatever you prefer :)
Created attachment 369328 [details] [review] Set the unicast TTL parameter on multicast udp sinks
Created attachment 369329 [details] [review] Update transport for multicast clients as well
Created attachment 369330 [details] [review] Let the first multicast request decide the ttl socket value
Created attachment 369331 [details] [review] Don't reserve multicast address in the client settings case
Created attachment 369332 [details] [review] Removed test_client_multicast_invalid_transport_specific
Created attachment 369333 [details] [review] Added test for multicast ttl
Created attachment 369334 [details] [review] Don't require address pool in the transport specific case
Created attachment 369335 [details] [review] Added a list of multicast client addresses
Created attachment 369778 [details] [review] Don't require presence of sinks in _get_*_socket()
Created attachment 372691 [details] [review] Don't require address pool in the transport specific case
Created attachment 372692 [details] [review] Added a list of multicast client addresses
Comment on attachment 369330 [details] [review] Let the first multicast request decide the ttl socket value The spec says the following about this (RTSP 1.0): This request header indicates which transport protocol is to be used and configures its parameters such as destination address, compression, multicast time-to-live and destination port for a single stream. It sets those values not already determined by a presentation description. For RTSP 2.0 it says the same but additionally also: ttl: multicast time-to-live for IPv4. When included in requests, the value indicates the TTL value that the client requests the server to use. In a response, the value actually being used by the server is returned. A server will need to consider what values that are reasonable and also the authority of the user to set this value. Corresponding functions are not needed for IPv6 as the scoping is part of the IPv6 multicast address [RFC4291]. So I think always using the first one is not necessarily right. We should probably use the maximum that is requested by a client, and have a server-side limit for the maximum value a client can possibly request. What do you think? This also seems relatively easy to implement. The other important part is that we must report the actual TTL in the response to the client, but I think that's already done correctly or not?
Review of attachment 369333 [details] [review]: ::: tests/check/gst/client.c @@ +753,3 @@ + "RTP/AVP;multicast;destination=233.252.0.1;ttl=0;port=5000-5001;"); + + /* make sure that ttl value in the response is 1 */ We should also do another check here that a second client with a valid different ttl gets the actual ttl value in the response. So with the current behaviour of your patches (see comment above for that), you could send another request with ttl=2 and check that the response contains ttl=1 (the actual value used).
Comment on attachment 369332 [details] [review] Removed test_client_multicast_invalid_transport_specific It seems like here something is also missing, let's see the spec: destination: The address to which a stream will be sent. The client may specify the multicast address with the destination parameter. To avoid becoming the unwitting perpetrator of a remote- controlled denial-of-service attack, a server SHOULD authenticate the client and SHOULD log such attempts before allowing the client to direct a media stream to an address not chosen by the server. This is particularly important if RTSP commands are issued via UDP, but implementations cannot rely on TCP as reliable means of client identification by itself. A server SHOULD not allow a client to direct media streams to an address that differs from the address commands are coming from. And even more strict with RTSP 2.0: perpetrator of a remote-controlled DoS attack, a server MUST perform security checks (see Section 21.2.1) and SHOULD log So we should not just blindly use addresses provided by the client if they differ from the client's own IP, but need to do some sanity checks. Section 21.2.1 of RTSP 2.0 has some useful guidance for that. I think the old behaviour of checking the address pool was a poor-man's solution for that. Only the destinations that are in the pool were allowed to be selected by the client, not arbitrary ones. Maybe we need some new API in the server to allow/disallow specific destinations. A new signal/vfunc. But just allowing everything unconditionally seems potentially dangerous and wrong for RTSP 2.0 at least.
(In reply to Sebastian Dröge (slomo) from comment #44) > > So I think always using the first one is not necessarily right. We should > probably use the maximum that is requested by a client, and have a > server-side limit for the maximum value a client can possibly request. > > What do you think? This also seems relatively easy to implement. So some kind of set_max_ttl() on the RTSP media factory, media and stream.
Review of attachment 369331 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +1873,1 @@ goto error_allocating_ports; gst_rtsp_stream_allocate_udp_sockets() here does not actually make use of the use_client_settings boolean that is passed to it. There seems to be some kind of problem here. Especially if there is already socket of the right type, nothing at all is going to happen here. But for use_client_settings and multicast we at least need to listen on the same port (for RTCP, etc) as the given multicast destination. It is not enough to just send data there, i.e. not enough to just put a new destination to the multiudpsink, but instead we might need a new socket! @@ +1880,3 @@ + /* FIXME: We want actually make sure at this point, that the rtp and + * rtcp ports are reserved with the unicast address that our rtp/rtcp + * sockets are bound to. */ I don't understand the FIXME comment. We're in the multicast case currently, why do we care about the RTP/RTCP ports of the unicast address (I assume of the unicast socket)? You mean that we should check here what I mentioned above? That the selected port for the socket is actually the right one? I believe we need separate sockets here for each client if they select different ports. @@ -1883,3 @@ - * the one requested by the client */ - addr = gst_rtsp_stream_reserve_address (ctx->stream, ct->destination, - ct->port.min, ct->port.max - ct->port.min + 1, ct->ttl); So this was checking that a client can only ever select at most once a specific multicast address. This is weird, at least for shared medias it should be fine for multiple clients to select the same values here. For non-shared medias it's not but there this does not add much. Other medias might have different address pools, or even in different processes, and might also allow to select the same multicast destination. It can still conflict. And clients would have to filter this based on the SSRC then in the end. We probably also want a different bug report here for adding some API that allows the server to allow/disallow specific multicast destinations. Doing that via the address pool seems weird, in any case. For the non-client-settings case, gst_rtsp_stream_get_multicast_address() returns the next free multicast destination that exists (or the one that was previously selected). That seems fine. I believe the pool should only be used for selecting server-selected address/port pairs. @@ +1905,3 @@ ct->port.max = addr->port + addr->n_ports - 1; ct->ttl = addr->ttl; + gst_rtsp_address_free (addr); For the unicast case, it seems like there's also the potential of the client maliciously redirecting the traffic to some other place. Nothing new here, that was already the case before your changes, but it should at least get a FIXME comment and ideally a separate bug report ::: tests/check/gst/client.c @@ -526,3 @@ fail_unless (session_pool != NULL); - fail_unless (gst_rtsp_session_pool_get_n_sessions (session_pool) == 1); Why is this invalid now?
Comment on attachment 369778 [details] [review] Don't require presence of sinks in _get_*_socket() Had to merge this one here manually as it seems to depend on some of your local changes. Especially the multicast functions were very different.
Review of attachment 372691 [details] [review]: This seems like it can be merged with the " Don't reserve multicast address in the client settings case " patch. Same thing really, or not? ::: gst/rtsp-server/rtsp-stream.c @@ +1384,3 @@ + /* multicast and transport specific case */ + if (multicast && ct->destination != NULL && use_transport_settings) { What's with the case of !multicast and client settings? check_address_and_ports() also checks for multicast although it is only ever called for multicast @@ +1388,3 @@ + if (!check_address_and_ports (ct->destination, ct->port.min, ct->port.max, + TRUE)) + goto invalid_client_settings; Is it actually invalid for a client to specify an odd RTP port and an even RTCP port? @@ +1464,3 @@ + if (!transport_settings_defined) + tmp_rtp = + g_inet_socket_address_get_port (G_INET_SOCKET_ADDRESS (rtp_sockaddr)); Shouldn't this give the same value if transport_settings_defined, and if not it would be a bug somewhere?
Review of attachment 372691 [details] [review]: ::: tests/check/gst/client.c @@ +908,3 @@ + + + /* simple SETUP with a valid URI and multicast, but an invalid ip */ Where is the invalid IP here? I see no IP :) @@ +1096,3 @@ + +/* test if two multicast clients can choose different transport settings. + * CASE: media is shared */ This test should currently fail as both clients can't select different multicast ports. The server will only ever bind to the port of the first
Review of attachment 372692 [details] [review]: As mentioned above, this is actually not enough but a good start. We need to potentially use multiple multicast sockets unfortunately ::: gst/rtsp-server/rtsp-stream.c @@ +1627,3 @@ + client->rtp_port = rtp_port; + client->add_count = 1; + priv->mcast_clients = g_list_append (priv->mcast_clients, client); g_list_append() is O(n). Either prepend() or use a GQueue or similar @@ +1654,3 @@ + + if (!check_address_and_ports (destination, rtp_port, rtcp_port, TRUE)) + goto invalid_address; Why do you need to check here? There will be nothing to remove if it's invalid :) @@ +3338,3 @@ + + if ((g_strcmp0 (cli->address, tr->destination) == 0) && + (cli->rtp_port == tr->port.min)) What is this function actually checking, what is it good for? @@ +4397,3 @@ + */ +gchar * +gst_rtsp_stream_get_multicast_client_addresses (GstRTSPStream * stream) What is this actually used for, and why a comma-separated list in a string?
(In reply to Sebastian Dröge (slomo) from comment #52) > Review of attachment 372692 [details] [review] [review]: > > As mentioned above, this is actually not enough but a good start. We need to > potentially use multiple multicast sockets unfortunately Let's handle that in a separate ticket. It adds a whole lot of other problems: we need multiple udpsrcs, multiple multiudpsinks, etc. and it also does not work yet today either. However this means that if multiple multicast destinations are selected, RTCP will only ever work for the first one.
Created attachment 373231 [details] [review] rtsp-stream: avoid duplicating the first multicast client In dcb4533fedae3ac62bc25a916eb95927b7d69aec , we made it so clients were dynamically added and removed to the multicast udp sinks, as such we should no longer add a first client in set_multicast_socket_for_udpsink
Created attachment 373249 [details] [review] Add new API for setting/getting maximum multicast ttl value
Created attachment 373250 [details] [review] Don't reserve multicast address in the client settings case
Created attachment 373251 [details] [review] Don't require address pool in the transport specific case
Created attachment 373252 [details] [review] Choose the maximum ttl value provided by multicast clients
Created attachment 373253 [details] [review] Added a list of multicast client addresses
Ah sorry, I forgot to mark "Removed test_client_multicast_invalid_transport_specific" as obsolete and I don't see any option to remove it afterwards :). However, the above 5 patches are the ones to be reviewed. Thanks in advance!
Thanks Patricia! Can you take a look at Mathieu's patch in comment 54, does that make sense to you? Also please double-check that your patches still apply and function correctly after dcb4533fedae3ac62bc25a916eb95927b7d69aec in master (a few days ago).
I've just retested the provided patches on the top of the current master and I haven't noticed any problems. Regarding Mathieu's patch. The patch is correct as there is no point of duplicating the host information for the first client. Shouldn't the code in multiudpsink prevent this case as well?
(In reply to Patricia Muscalu from comment #62) > I've just retested the provided patches on the top of the current master and > I haven't noticed any problems. > > Regarding Mathieu's patch. The patch is correct as there is no point of > duplicating > the host information for the first client. Shouldn't the code in > multiudpsink prevent this case as well? See the "send-duplicates" property on multiudpsink :) I don't know what it is good for though.
Review of attachment 373249 [details] [review]: ::: gst/rtsp-server/rtsp-media.c @@ +379,3 @@ GST_TYPE_CLOCK, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); + g_object_class_install_property (gobject_class, PROP_MAX_MCAST_TTL, Why is it here a property but not in rtsp-stream?
Review of attachment 373250 [details] [review]: Looks good
Review of attachment 373251 [details] [review]: generally looks good, just one question ::: gst/rtsp-server/rtsp-stream.c @@ +1475,3 @@ g_clear_object (&rtp_socket); + if (transport_settings_defined) + goto transport_settings_error; What if we already have been bound to that UDP port and the corresponding elements could be used for this new client too? Is that covered somewhere?
Review of attachment 373252 [details] [review]: Looks good
Review of attachment 373253 [details] [review]: This should probably get a link to https://bugzilla.gnome.org/show_bug.cgi?id=796917 in the commit message and a FIXME comment somewhere in the code? Looks fine otherwise ::: gst/rtsp-server/rtsp-stream.c @@ +1642,3 @@ + client->rtp_port = rtp_port; + client->add_count = 1; + priv->mcast_clients = g_list_prepend (priv->mcast_clients, client); Shouldn't this also be configured on multiudpsink at some point, or does that happen elsewhere already? Same for removal. It looks like this code only keeps track of the addresses but does not actually configure them on the sink (and the source for RTCP or RTP RECORD or backchannel is broken anyway, see above, but it was so before anyway)
(In reply to Sebastian Dröge (slomo) from comment #64) > Review of attachment 373249 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-media.c > @@ +379,3 @@ > GST_TYPE_CLOCK, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); > > + g_object_class_install_property (gobject_class, PROP_MAX_MCAST_TTL, > > Why is it here a property but not in rtsp-stream? I've just followed the way how it's done for the buffer-size property. There is no buffer-size property for the stream object itself.
(In reply to Sebastian Dröge (slomo) from comment #66) > Review of attachment 373251 [details] [review] [review]: > > generally looks good, just one question > > ::: gst/rtsp-server/rtsp-stream.c > @@ +1475,3 @@ > g_clear_object (&rtp_socket); > + if (transport_settings_defined) > + goto transport_settings_error; > > What if we already have been bound to that UDP port and the corresponding > elements could be used for this new client too? Is that covered somewhere? Good point. Let me check.
(In reply to Patricia Muscalu from comment #69) > (In reply to Sebastian Dröge (slomo) from comment #64) > > Review of attachment 373249 [details] [review] [review] [review]: > > > > ::: gst/rtsp-server/rtsp-media.c > > @@ +379,3 @@ > > GST_TYPE_CLOCK, G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS)); > > > > + g_object_class_install_property (gobject_class, PROP_MAX_MCAST_TTL, > > > > Why is it here a property but not in rtsp-stream? > > I've just followed the way how it's done for the buffer-size property. There > is no buffer-size property for the stream object itself. ok then :)
(In reply to Sebastian Dröge (slomo) from comment #68) > Review of attachment 373253 [details] [review] [review]: > > This should probably get a link to > https://bugzilla.gnome.org/show_bug.cgi?id=796917 in the commit message and > a FIXME comment somewhere in the code? > > Looks fine otherwise > > ::: gst/rtsp-server/rtsp-stream.c > @@ +1642,3 @@ > + client->rtp_port = rtp_port; > + client->add_count = 1; > + priv->mcast_clients = g_list_prepend (priv->mcast_clients, client); > > Shouldn't this also be configured on multiudpsink at some point, or does > that happen elsewhere already? Same for removal. > > It looks like this code only keeps track of the addresses but does not > actually configure them on the sink (and the source for RTCP or RTP RECORD > or backchannel is broken anyway, see above, but it was so before anyway) Your are absolutely right that some parts are missing here. The whole idea is to add the mcast address on SETUP (during the client transport configuration). What's missing here is that add_client and remove_client functions, that actually send signals to multiudpsink, don't check if the requested destination exists in the mcast_clients array. The destination should be removed as well "remove" is sent to the sink.
(In reply to Patricia Muscalu from comment #72) > What's missing here is that add_client and remove_client functions, > that actually send signals to multiudpsink, don't check if the > requested destination exists in the mcast_clients array. The destination > should be removed as well "remove" is sent to the sink. Ok, so that would need an update then. it seems without, this change is not very meaningful?
(In reply to Sebastian Dröge (slomo) from comment #73) > (In reply to Patricia Muscalu from comment #72) > > What's missing here is that add_client and remove_client functions, > > that actually send signals to multiudpsink, don't check if the > > requested destination exists in the mcast_clients array. The destination > > should be removed as well "remove" is sent to the sink. > > Ok, so that would need an update then. it seems without, this change is not > very meaningful? Yes, of course. I'll update the patch.
(In reply to Patricia Muscalu from comment #72) > (In reply to Sebastian Dröge (slomo) from comment #68) > > Review of attachment 373253 [details] [review] [review] [review]: > > > > This should probably get a link to > > https://bugzilla.gnome.org/show_bug.cgi?id=796917 in the commit message and > > a FIXME comment somewhere in the code? > > > > Looks fine otherwise > > > > ::: gst/rtsp-server/rtsp-stream.c > > @@ +1642,3 @@ > > + client->rtp_port = rtp_port; > > + client->add_count = 1; > > + priv->mcast_clients = g_list_prepend (priv->mcast_clients, client); > > > > Shouldn't this also be configured on multiudpsink at some point, or does > > that happen elsewhere already? Same for removal. > > > > It looks like this code only keeps track of the addresses but does not > > actually configure them on the sink (and the source for RTCP or RTP RECORD > > or backchannel is broken anyway, see above, but it was so before anyway) > > Your are absolutely right that some parts are missing here. The whole idea > is to add the mcast address on SETUP (during the client transport > configuration). What's missing here is that add_client and remove_client > functions, that actually send signals to multiudpsink, don't check if the > requested destination exists in the mcast_clients array. The destination > should be removed as well "remove" is sent to the sink. Wait, there already exists remove_mcast_client_addr() function, provided in this patch, that actually removes the address on client removal. I'll only add additional checks in add_client()
Created attachment 373325 [details] [review] Added a list of multicast client addresses
(In reply to Patricia Muscalu from comment #75) > (In reply to Patricia Muscalu from comment #72) > > (In reply to Sebastian Dröge (slomo) from comment #68) > > > Review of attachment 373253 [details] [review] [review] [review] [review]: > > > > > > This should probably get a link to > > > https://bugzilla.gnome.org/show_bug.cgi?id=796917 in the commit message and > > > a FIXME comment somewhere in the code? > > > > > > Looks fine otherwise > > > > > > ::: gst/rtsp-server/rtsp-stream.c > > > @@ +1642,3 @@ > > > + client->rtp_port = rtp_port; > > > + client->add_count = 1; > > > + priv->mcast_clients = g_list_prepend (priv->mcast_clients, client); > > > > > > Shouldn't this also be configured on multiudpsink at some point, or does > > > that happen elsewhere already? Same for removal. > > > > > > It looks like this code only keeps track of the addresses but does not > > > actually configure them on the sink (and the source for RTCP or RTP RECORD > > > or backchannel is broken anyway, see above, but it was so before anyway) > > > > Your are absolutely right that some parts are missing here. The whole idea > > is to add the mcast address on SETUP (during the client transport > > configuration). What's missing here is that add_client and remove_client > > functions, that actually send signals to multiudpsink, don't check if the > > requested destination exists in the mcast_clients array. The destination > > should be removed as well "remove" is sent to the sink. > > Wait, there already exists remove_mcast_client_addr() function, provided in > this patch, that actually removes the address on client removal. I'll only > add additional checks in add_client() The checking part of the multicast client address exists as well, so I've just rearranged the code a bit to make it more clear ... I hope.
(In reply to Patricia Muscalu from comment #70) > (In reply to Sebastian Dröge (slomo) from comment #66) > > Review of attachment 373251 [details] [review] [review] [review]: > > > > generally looks good, just one question > > > > ::: gst/rtsp-server/rtsp-stream.c > > @@ +1475,3 @@ > > g_clear_object (&rtp_socket); > > + if (transport_settings_defined) > > + goto transport_settings_error; > > > > What if we already have been bound to that UDP port and the corresponding > > elements could be used for this new client too? Is that covered somewhere? > > Good point. Let me check. This is actually the case when bind() fails. So in the client-settings case we just return with an error message, otherwise we try to find any free address/ports pairs in the address pool.
(In reply to Patricia Muscalu from comment #78) > (In reply to Patricia Muscalu from comment #70) > > (In reply to Sebastian Dröge (slomo) from comment #66) > > > Review of attachment 373251 [details] [review] [review] [review] [review]: > > > > > > generally looks good, just one question > > > > > > ::: gst/rtsp-server/rtsp-stream.c > > > @@ +1475,3 @@ > > > g_clear_object (&rtp_socket); > > > + if (transport_settings_defined) > > > + goto transport_settings_error; > > > > > > What if we already have been bound to that UDP port and the corresponding > > > elements could be used for this new client too? Is that covered somewhere? > > > > Good point. Let me check. > > This is actually the case when bind() fails. So in the client-settings case > we just return with an error message, otherwise we try to find any free > address/ports pairs in the address pool. But the bind fails although there is a stream bound to those ports already and it could be used for this client? Or do we only ever go into this code if there is no stream for that configuration yet?
(In reply to Sebastian Dröge (slomo) from comment #79) > (In reply to Patricia Muscalu from comment #78) > > (In reply to Patricia Muscalu from comment #70) > > > (In reply to Sebastian Dröge (slomo) from comment #66) > > > > Review of attachment 373251 [details] [review] [review] [review] [review] [review]: > > > > > > > > generally looks good, just one question > > > > > > > > ::: gst/rtsp-server/rtsp-stream.c > > > > @@ +1475,3 @@ > > > > g_clear_object (&rtp_socket); > > > > + if (transport_settings_defined) > > > > + goto transport_settings_error; > > > > > > > > What if we already have been bound to that UDP port and the corresponding > > > > elements could be used for this new client too? Is that covered somewhere? > > > > > > Good point. Let me check. > > > > This is actually the case when bind() fails. So in the client-settings case > > we just return with an error message, otherwise we try to find any free > > address/ports pairs in the address pool. > > But the bind fails although there is a stream bound to those ports already > and it could be used for this client? Or do we only ever go into this code > if there is no stream for that configuration yet? Yes, we enter this code only if there is no configuration done yet. So the socket creation is only done for the first multicast client (shared media case) The logic has to be changed after a proper solution has been provided in https://bugzilla.gnome.org/show_bug.cgi?id=796917.
Pushed all
Ugh, and something went wrong while merging the patches from here. Some commits where squashed with others. I did a git rebase to get rid of all the "Change-Id" markers, gst-indent then complained and I mixed up "git commit --amend" and "git add && git rebase --continue" it seems. All changes seem to be there though.
Thanks! The last two have been squashed: stream: Choose the maximum ttl value provided by multicast clients and stream: Added a list of multicast client addresses Is it possible to add the following commit message to the last one? When media is shared, the same media stream can be sent to multiple multicast groups. Currently, there is no API to retrieve multicast addresses from the stream. When calling gst_rtsp_stream_get_multicast_address() function, only the first multicast address is returned. With this patch, each multicast destination requested in SETUP will be stored in an internal list (call to gst_rtsp_stream_add_multicast_client_address()). The list of multicast groups requested by the clients can be retrieved by calling gst_rtsp_stream_get_multicast_client_addresses(). There still exist some problems with the current implementation in the multicast case: 1) The receiving part is currently only configured with regard to the first multicast client (see https://bugzilla.gnome.org/show_bug.cgi?id=796917). 2) Secondly, of security reasons, some constraints should be put on the requested multicast destinations (see https://bugzilla.gnome.org/show_bug.cgi?id=796916). Thank you!
Fixed that now but please in the future don't include these "Change-ID" things in the commit message.
Thanks Sebastian!
Hi Patricia, thanks for your contribution ! It would be nice for your next contribution to remove the "Change-id:" marks in your commit message before submission. It's a lot of work for us to remove and don't belong to public repository.
I apologize for having caused extra unnecessary work for you. It wasn't my intention. I'll ask my colleges as well to double check the commit messages before providing their patches. Sorry!