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 791743 - Multicast TTL not set on stream sockets correctly
Multicast TTL not set on stream sockets correctly
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
1.x
Other Linux
: Normal blocker
: 1.13.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-12-18 17:13 UTC by Arkver
Modified: 2018-01-07 14:42 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Example trace showing an address pool with TTL=16 being used, but the multiudpsink "clients" being set to TTL=1 (7.05 KB, text/plain)
2017-12-18 17:13 UTC, Arkver
  Details
Patch re v1.12.3 which doesn't fix the problem. (2.36 KB, patch)
2017-12-18 17:31 UTC, Arkver
none Details | Review
Patch to set ttl-mc property on udpsink objects (on 1.12.4) (1.30 KB, patch)
2018-01-04 13:06 UTC, Arkver
committed Details | Review

Description Arkver 2017-12-18 17:13:48 UTC
Created attachment 365710 [details]
Example trace showing an address pool with TTL=16 being used, but the multiudpsink "clients" being set to TTL=1

When a media factory has a multicast address pool assigned with a non-default TTL and the protocols are set to GST_RTSP_LOWER_TRANS_UDP_MCAST, the TTL is not applied to the stream sockets. These are set to use the default multicast TTL of 1. Packets observed on the wire indeed have TTL=1.
Comment 1 Arkver 2017-12-18 17:31:30 UTC
Created attachment 365711 [details] [review]
Patch re v1.12.3 which doesn't fix the problem.

I made a trial patch to move the g_object_set calls for ttl-mc value into the UDP_MCAST part of the switch in update_transport (rtsp_stream.c) but it didn't fix the problem. I didn't see the included INFO message in the log. Will trace back to see where the transport's ttl field gets set. Patch attacted fyi. It's against the 1.12.3 tag. I'll try to put together a quick testcase against master.
Comment 2 Arkver 2017-12-18 18:53:53 UTC
Not sure the behaviour will be the same with master code, since it seems to have changed to only create the sockets when a client connects, presumably to support using client info for things like ttl. I tried to build and run examples/test-multicast with a fresh master build but I get an assertion failure. This is current clean master branch code for gstreamer/plugins-[base|good|ugly]/gst-rtsp-server all as of about an hour ago. gst-rtsp-server is at 64f1a3a.

0:00:02.811077700  8968 0x55c951eaf5e0 INFO              rtspclient rtsp-client.c:3401:handle_request: client 0x55c951f38110: received a request PLAY rtsp://127.0.0.1:8554/test/ 1.0
0:00:02.811088711  8968 0x55c951eaf5e0 LOG               rtspclient rtsp-client.c:1088:default_pre_signal_handler:<GstRTSPClient@0x55c951f38110> returning GST_RTSP_STS_OK
0:00:02.811094907  8968 0x55c951eaf5e0 DEBUG              rtspmedia rtsp-media.c:4081:gst_rtsp_media_complete_pipeline:<GstRTSPMedia@0x7fc2d004a180> complete pipeline
0:00:02.811098073  8968 0x55c951eaf5e0 DEBUG             rtspstream rtsp-stream.c:4523:gst_rtsp_stream_complete_stream:<GstRTSPStream@0x7fc2d0050300> complete stream
0:00:02.811100746  8968 0x55c951eaf5e0 DEBUG             rtspstream rtsp-stream.c:2906:create_receiver_part:<GstRTSPStream@0x7fc2d0050300> create receiver part
0:00:02.811160858  8968 0x55c951eaf5e0 DEBUG             rtspstream rtsp-stream.c:2972:create_receiver_part:<GstRTSPStream@0x7fc2d0050300> mcast IPv4, create and configure udpsources
**
ERROR:rtsp-stream.c:1261:create_and_configure_udpsource: assertion failed: (socket != NULL)

Client is VLC in this case.
Comment 3 Sebastian Dröge (slomo) 2017-12-19 08:49:40 UTC
The code in GIT master is still broken. update_transport() has one case of mcast and one of unicast, but only in the unicast case does it set "ttl-mc" which makes no sense.

However that code should definitely run, otherwise neither multicast nor unicast would work at all. Are your INFO messages maybe just not run because ttl is <= 0?


For the examples, I can confirm that test-multicast asserts like that but test-multicast2 seems to work. Nonetheless, test-multicast should be fixed of course :)
Comment 4 Sebastian Dröge (slomo) 2017-12-19 09:17:13 UTC
commit 4d86f99449839f74d0ade7f86451ed70bebdbb60 (HEAD -> master, origin/master, origin/HEAD)
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Dec 19 11:14:48 2017 +0200

    rtsp-stream: Decide based on the sockets, not the addresses if we already allocated a socket
    
    In the multicast case (as in test-multicast, not test-multicast2), the
    address could be allocated/reserved (and thus set) already without
    allocating the actual socket. We need to allocate the socket here still
    instead of just claiming that it was already allocated.
    
    See https://bugzilla.gnome.org/show_bug.cgi?id=791743#c2
Comment 5 Sebastian Dröge (slomo) 2017-12-19 09:37:01 UTC
This should fix it, please reopen with a new debug log (with GST_DEBUG=rtsp*:6) otherwise

commit 4ec17b1975d38c3a87d593751197b9503ae14fba (HEAD -> master, origin/master, origin/HEAD)
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Tue Dec 19 11:34:37 2017 +0200

    rtsp-stream: Set multicast TTL on the multicast sockets
    
    And not if we do unicast UDP.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=791743
Comment 6 Arkver 2017-12-19 11:40:03 UTC
Thanks slomo, that does the job nicely, both on the TTL (though not tested with Wireshark yet) and the assert fail.

Any chance of a backport to 1.12 branch for the TTL fix? I tried to cherry-pick it but the conflicts are non-trivial (for me anyway).
Comment 7 Sebastian Dröge (slomo) 2017-12-19 13:47:51 UTC
The conflicts are non-trivial for everybody :)
Comment 8 Arkver 2018-01-04 13:06:51 UTC
Created attachment 366279 [details] [review]
Patch to set ttl-mc property on udpsink objects (on 1.12.4)

This seems to fix the TTL for me on the 1.12.4 branch. It just uses the TTL from the address allocated from the pool.