GNOME Bugzilla – Bug 796875
rtsp-client: always allocate both IPV4 and IPV6 sockets
Last modified: 2018-08-01 18:49:04 UTC
See commit message
Created attachment 373163 [details] [review] rtsp-client: always allocate both IPV4 and IPV6 sockets multiudpsink does not support setting the socket* properties after it has started, which meant that rtsp-server could no longer serve on both IPV4 and IPV6 sockets since the patches from https://bugzilla.gnome.org/show_bug.cgi?id=757488 were merged. When first connecting an IPV6 client then an IPV4 client, multiudpsink fell back to using the IPV6 socket. When first connecting an IPV4 client, then an IPV6 client, multiudpsink errored out, released the IPV4 socket, then crashed when trying to send a message on NULL nevertheless, that is however a separate issue. This could probably be fixed by handling the setting of sockets in multiudpsink after it has started, that will however be a much more significant effort. For now, this commit simply partially reverts the behaviour of rtsp-stream: it will continue to only create the udpsinks when needed, as was the case since the patches were merged, it will however when creating them, always allocate both sockets and set them on the sink before it starts, as was the case prior to the patches.
Review of attachment 373163 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +1880,1 @@ use_client_settings)) Does this make use of the address pool properly? It might be that there are only v4 or v6 addresses in the pool, then allocation of the other would fail here and we should probably not error out then but continue with only that one family (assuming it's the same family as was requested here, "family" variable) @@ +1895,3 @@ + GSocketFamily family; + + family = priv->is_ipv6 ? G_SOCKET_FAMILY_IPV6 : G_SOCKET_FAMILY_IPV4; Same thing here as above, but also the family actually depends on what multicast address is selected and available in the pool at all. I don't think this is correct as is. Also see https://bugzilla.gnome.org/show_bug.cgi?id=793441, there will have to be multiple udpsinks in the end. One for each multicast destination, and corresponding udpsrcs. Due to ports.
Created attachment 373192 [details] [review] rtsp-client: always allocate both IPV4 and IPV6 sockets multiudpsink does not support setting the socket* properties after it has started, which meant that rtsp-server could no longer serve on both IPV4 and IPV6 sockets since the patches from https://bugzilla.gnome.org/show_bug.cgi?id=757488 were merged. When first connecting an IPV6 client then an IPV4 client, multiudpsink fell back to using the IPV6 socket. When first connecting an IPV4 client, then an IPV6 client, multiudpsink errored out, released the IPV4 socket, then crashed when trying to send a message on NULL nevertheless, that is however a separate issue. This could probably be fixed by handling the setting of sockets in multiudpsink after it has started, that will however be a much more significant effort. For now, this commit simply partially reverts the behaviour of rtsp-stream: it will continue to only create the udpsinks when needed, as was the case since the patches were merged, it will however when creating them, always allocate both sockets and set them on the sink before it starts, as was the case prior to the patches. Transport configuration will only error out if the allocation of UDP sockets fails for the actual client's family, this also downgrades the GST_ERRORs in alloc_ports_one_family to GST_WARNINGs, as failing to allocate is no longer necessarily fatal.
(In reply to Sebastian Dröge (slomo) from comment #2) > Review of attachment 373163 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ +1880,1 @@ > use_client_settings)) > > Does this make use of the address pool properly? It might be that there are > only v4 or v6 addresses in the pool, then allocation of the other would fail > here and we should probably not error out then but continue with only that > one family (assuming it's the same family as was requested here, "family" > variable) > > @@ +1895,3 @@ > + GSocketFamily family; > + > + family = priv->is_ipv6 ? G_SOCKET_FAMILY_IPV6 : > G_SOCKET_FAMILY_IPV4; > > Same thing here as above, but also the family actually depends on what > multicast address is selected and available in the pool at all. > > I don't think this is correct as is. > Good catch, thanks, updated the patch (it also was making the unit tests fail) > > Also see https://bugzilla.gnome.org/show_bug.cgi?id=793441, there will have > to be multiple udpsinks in the end. One for each multicast destination, and > corresponding udpsrcs. Due to ports. Right, that's a different issue however :)
Review of attachment 373192 [details] [review]: ::: gst/rtsp-server/rtsp-client.c @@ +1880,3 @@ + + if (!gst_rtsp_stream_allocate_udp_sockets (ctx->stream, G_SOCKET_FAMILY_IPV6, ct, + use_client_settings) && family == G_SOCKET_FAMILY_IPV6) This still fails if allocating one of the two ports fails in the case where the pool only contains one kind of addresses. That seems wrong Also you still have the problem you're fixing here with multicast now as currently there is only one multicast sink. You at least have to add one for both families.
(In reply to Sebastian Dröge (slomo) from comment #5) > Review of attachment 373192 [details] [review] [review]: > > ::: gst/rtsp-server/rtsp-client.c > @@ +1880,3 @@ > + > + if (!gst_rtsp_stream_allocate_udp_sockets (ctx->stream, > G_SOCKET_FAMILY_IPV6, ct, > + use_client_settings) && family == G_SOCKET_FAMILY_IPV6) > > This still fails if allocating one of the two ports fails in the case where > the pool only contains one kind of addresses. That seems wrong > This fails configuring the client transport if we couldn't set it up for the requested family, but that is the current behaviour too, are you saying the current behaviour is wrong wrt that as well? > Also you still have the problem you're fixing here with multicast now as > currently there is only one multicast sink. You at least have to add one for > both families. I'm not sure I get what you mean, for udp the udp sinks can have both IPV6 and IPV4 , it simply needs to be done before the sinks are started, are you saying this is not the case for multicast?
(In reply to Mathieu Duponchelle from comment #6) > (In reply to Sebastian Dröge (slomo) from comment #5) > > Review of attachment 373192 [details] [review] [review] [review]: > > > > ::: gst/rtsp-server/rtsp-client.c > > @@ +1880,3 @@ > > + > > + if (!gst_rtsp_stream_allocate_udp_sockets (ctx->stream, > > G_SOCKET_FAMILY_IPV6, ct, > > + use_client_settings) && family == G_SOCKET_FAMILY_IPV6) > > > > This still fails if allocating one of the two ports fails in the case where > > the pool only contains one kind of addresses. That seems wrong > > > > This fails configuring the client transport if we couldn't set it up for the > requested family, but that is the current behaviour too, are you saying the > current behaviour is wrong wrt that as well? Oh I missed that additional condition. No it's fine I guess > > Also you still have the problem you're fixing here with multicast now as > > currently there is only one multicast sink. You at least have to add one for > > both families. > > I'm not sure I get what you mean, for udp the udp sinks can have both IPV6 > and IPV4 , it simply needs to be done before the sinks are started, are you > saying this is not the case for multicast? I guess that's irrelevant and part of the other bugs, so let's ignore that here
accepted then ? :)
Comment on attachment 373192 [details] [review] rtsp-client: always allocate both IPV4 and IPV6 sockets Oh, yes :)
Attachment 373192 [details] pushed as 12f8abb - rtsp-client: always allocate both IPV4 and IPV6 sockets