After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796875 - rtsp-client: always allocate both IPV4 and IPV6 sockets
rtsp-client: always allocate both IPV4 and IPV6 sockets
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
unspecified
Other All
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-07-25 18:03 UTC by Mathieu Duponchelle
Modified: 2018-08-01 18:49 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
rtsp-client: always allocate both IPV4 and IPV6 sockets (3.23 KB, patch)
2018-07-25 18:03 UTC, Mathieu Duponchelle
none Details | Review
rtsp-client: always allocate both IPV4 and IPV6 sockets (4.59 KB, patch)
2018-07-27 17:22 UTC, Mathieu Duponchelle
committed Details | Review

Description Mathieu Duponchelle 2018-07-25 18:03:11 UTC
See commit message
Comment 1 Mathieu Duponchelle 2018-07-25 18:03:30 UTC
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.
Comment 2 Sebastian Dröge (slomo) 2018-07-26 10:56:48 UTC
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.
Comment 3 Mathieu Duponchelle 2018-07-27 17:22:04 UTC
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.
Comment 4 Mathieu Duponchelle 2018-07-27 17:23:17 UTC
(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 :)
Comment 5 Sebastian Dröge (slomo) 2018-07-30 06:52:33 UTC
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.
Comment 6 Mathieu Duponchelle 2018-07-31 15:40:55 UTC
(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?
Comment 7 Sebastian Dröge (slomo) 2018-07-31 15:43:24 UTC
(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
Comment 8 Mathieu Duponchelle 2018-07-31 18:01:48 UTC
accepted then ? :)
Comment 9 Sebastian Dröge (slomo) 2018-07-31 21:57:12 UTC
Comment on attachment 373192 [details] [review]
rtsp-client: always allocate both IPV4 and IPV6 sockets

Oh, yes :)
Comment 10 Mathieu Duponchelle 2018-08-01 18:46:13 UTC
Attachment 373192 [details] pushed as 12f8abb - rtsp-client: always allocate both IPV4 and IPV6 sockets