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 757488 - gst-rtsp-server: multicast functionality is broken if using the same port ranges for multicast and unicast
gst-rtsp-server: multicast functionality is broken if using the same port ran...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
1.6.0
Other Linux
: Normal blocker
: 1.7.90
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-11-02 15:31 UTC by Patricia Muscalu
Modified: 2016-03-17 08:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
code refactory (11.36 KB, patch)
2015-12-09 13:40 UTC, Patricia Muscalu
none Details | Review
code refactory (7.72 KB, patch)
2015-12-09 13:42 UTC, Patricia Muscalu
none Details | Review
code refactory (2.54 KB, patch)
2015-12-09 13:43 UTC, Patricia Muscalu
none Details | Review
code refactory (5.98 KB, patch)
2015-12-09 13:43 UTC, Patricia Muscalu
none Details | Review
code refactory (4.45 KB, patch)
2015-12-09 13:44 UTC, Patricia Muscalu
none Details | Review
code refactory (4.87 KB, patch)
2015-12-09 13:44 UTC, Patricia Muscalu
none Details | Review
postpone alloc_ports() until SETUP (4.91 KB, patch)
2015-12-09 13:45 UTC, Patricia Muscalu
none Details | Review
no socket sharing (2.02 KB, patch)
2015-12-09 13:46 UTC, Patricia Muscalu
none Details | Review
unit tests fixes (5.43 KB, patch)
2015-12-09 13:47 UTC, Patricia Muscalu
none Details | Review
create multicast udp sources in alloc_ports() (23.89 KB, patch)
2015-12-09 13:48 UTC, Patricia Muscalu
none Details | Review
code refactory (11.36 KB, patch)
2016-01-13 13:29 UTC, Patricia Muscalu
none Details | Review
code refactory (7.72 KB, patch)
2016-01-13 13:30 UTC, Patricia Muscalu
none Details | Review
code refactory (2.54 KB, patch)
2016-01-13 13:30 UTC, Patricia Muscalu
none Details | Review
code refactory (5.98 KB, patch)
2016-01-13 13:31 UTC, Patricia Muscalu
none Details | Review
code refactory (5.18 KB, patch)
2016-01-13 13:32 UTC, Patricia Muscalu
none Details | Review
code refactory (4.42 KB, patch)
2016-01-13 13:34 UTC, Patricia Muscalu
none Details | Review
code refactory (4.89 KB, patch)
2016-01-13 13:35 UTC, Patricia Muscalu
none Details | Review
postpone alloc_ports() until SETUP (4.53 KB, patch)
2016-01-13 13:37 UTC, Patricia Muscalu
none Details | Review
unit tests fixes (1.63 KB, patch)
2016-01-13 13:37 UTC, Patricia Muscalu
none Details | Review
unit tests fixes (5.50 KB, patch)
2016-01-13 13:38 UTC, Patricia Muscalu
none Details | Review
create all udp sources in one function (13.62 KB, patch)
2016-01-13 13:39 UTC, Patricia Muscalu
none Details | Review
code refactory (12.29 KB, patch)
2016-01-27 12:44 UTC, Patricia Muscalu
none Details | Review
code refactory (7.82 KB, patch)
2016-01-27 12:44 UTC, Patricia Muscalu
none Details | Review
code refactory (2.54 KB, patch)
2016-01-27 12:45 UTC, Patricia Muscalu
none Details | Review
code refactory (5.99 KB, patch)
2016-01-27 12:46 UTC, Patricia Muscalu
none Details | Review
code refactory (5.18 KB, patch)
2016-01-27 12:46 UTC, Patricia Muscalu
none Details | Review
code refactory (2.92 KB, patch)
2016-01-27 12:47 UTC, Patricia Muscalu
none Details | Review
code refactory (5.21 KB, patch)
2016-01-27 12:47 UTC, Patricia Muscalu
none Details | Review
postpone alloc_ports() until SETUP (4.92 KB, patch)
2016-01-27 12:48 UTC, Patricia Muscalu
none Details | Review
unit tests fixes (1.63 KB, patch)
2016-01-27 12:49 UTC, Patricia Muscalu
none Details | Review
unit tests fixes (5.51 KB, patch)
2016-01-27 12:49 UTC, Patricia Muscalu
none Details | Review
create all udp sources in one function (17.00 KB, patch)
2016-01-27 12:50 UTC, Patricia Muscalu
none Details | Review
handle client transport setting properly (8.39 KB, patch)
2016-01-27 12:51 UTC, Patricia Muscalu
none Details | Review
helper function for creating the sender and the receiver parts of the pipeline (12.35 KB, patch)
2016-02-23 14:11 UTC, Patricia Muscalu
committed Details | Review
create and configure UDP sinks in a separate function (7.87 KB, patch)
2016-02-23 14:13 UTC, Patricia Muscalu
committed Details | Review
added function for RTP/RTCP socket configuration (2.59 KB, patch)
2016-02-23 14:15 UTC, Patricia Muscalu
committed Details | Review
added function for creating and configuring UDP sources (4.36 KB, patch)
2016-02-23 14:16 UTC, Patricia Muscalu
committed Details | Review
added function for setting UDP sources to PLAYING state (2.98 KB, patch)
2016-02-23 14:18 UTC, Patricia Muscalu
committed Details | Review
postpone the creation of the UDP sources (5.25 KB, patch)
2016-02-23 14:20 UTC, Patricia Muscalu
committed Details | Review
postpone UDP socket allocation until SETUP (22.29 KB, patch)
2016-02-23 14:21 UTC, Patricia Muscalu
committed Details | Review
unit test fixes (8.21 KB, patch)
2016-02-23 14:22 UTC, Patricia Muscalu
committed Details | Review
added unit tests for stream (8.46 KB, patch)
2016-03-04 08:43 UTC, Patricia Muscalu
committed Details | Review

Description Patricia Muscalu 2015-11-02 15:31:01 UTC
The following commit in gst-rtsp-server 3159b374b9d4b73d20667667f8fd843fa319e9e9
makes it impossible to handle multicast requests.

When the address pools for the unicast and the multicast addresses are configured to use the same port ranges, setting the source->udpsrc elements (multicast part of the receiver) into the PLAYING state (see update_transport() in rtsp-stream.c) results in a bind() error: "Error binding to address: Address already in use".
The reason for this failure is that the alloc_ports_one_family function has already created the udpsrc elements - the receiving part of the pipeline that is supposed to handle the unicast client feedback - and the requested ports by the multicast receiving part are already taken after alloc_ports_one_family() was called.

The current design of the code in not consistent. The receiver parts (multicast and unicast) of the pipeline are created in different places which makes the code hard to understand and maintain.
Comment 1 Nicolas Dufresne (ndufresne) 2015-11-02 16:16:20 UTC
So the would mean regression between 1.4 and 1.6. Are you working on a patch ?
Comment 2 Sebastian Dröge (slomo) 2015-11-03 07:42:28 UTC
Note that before 3159b374b9d4b73d20667667f8fd843fa319e9e9, multicast was broken completely with regard to RTCP and IIRC it caused session timeouts.

Patricia, thanks for reporting :) Shouldn't this be solved also by having reuse=true on the udpsrc?
Comment 3 Patricia Muscalu 2015-11-04 09:16:15 UTC
Sebastian, the "reuse" option is set to true by default, so this doesn't solve the issue.
We should allow reusing the address when calling g_socket_bind() in alloc_ports_one_family function.
Comment 4 Patricia Muscalu 2015-11-04 20:38:25 UTC
I would like to clarify, that in our unicast address pool we allow INADDR_ANY addresses in order to bind to all available IPv4 or IPv6 addresses. So in our case the receiver part created in update_transport() would be exact the same as the one created in alloc_port_one_family().
What I don't really understand is why is this unicast receiver part necessary in the multicast mode? How is the address/port information propagated to the clients? Please, guide me with a proper RFC document. Thanks in advance.
Comment 5 Sebastian Dröge (slomo) 2015-11-04 20:43:38 UTC
The clients can check from which IP and port a packet is sent. But I guess this doesn't make sense, why would they assume that the same port also works for receiving RTCP?!

So I think your suggestion is correct to just not set up the unicast sources.
Comment 6 Patricia Muscalu 2015-11-04 21:03:55 UTC
Exactly. Would you accept a patch that removes the part with the unicast udpsrc elements?
Comment 7 Sebastian Dröge (slomo) 2015-11-04 21:15:58 UTC
Actually... what if we have a shared media and some clients are connecting via UDP and others with UDP-MCAST? Wouldn't we need both then?

But wouldn't the better solution then be to have a single udpsrc and just also join the multicast group on that socket?
Comment 8 Patricia Muscalu 2015-11-04 21:37:26 UTC
What I meant by removing this part was, that this part should not be present while in the multicast mode. We don't want to have both parts at the same time.

Yes, your suggestion sounds reasonable. I think it should work.

Thank you for your advice!
Comment 9 Patricia Muscalu 2015-11-09 09:45:03 UTC
I wounder if it's ok to postpone the creation of the receiver until we know what transport the client is about to use? Sebastian, what I mean is to create the receiver part in update_transport() as you did in your commit 3159b374b9d4b73d20667667f8fd843fa319e9e9. 

Second question: why in alloc_ports_one_family() are we trying to acquire the unicast addresses?
Comment 10 Sebastian Dröge (slomo) 2015-11-11 21:47:18 UTC
I checked the code a bit now. So, in alloc_ports_one_family() we are allocating sockets and bind them to random ports. At a later time we get the multicast transport, which has fixed ports (that are different than the random ones).

So I think we have three options here: either we allocate all ports in update_transport(), or we don't share sockets between udpsrc and udpsink and only allocate the receiver side in update_transport(), or we could check in update_transport() if the existing udpsrcs are already listening on the right port, and if they do only call g_socket_join/leave_multicast_group() on those sockets instead of creating new sources.

I think I currently prefer the latter as it's least invasive


For your second question, what exactly do you mean?
Comment 11 Patricia Muscalu 2015-11-12 08:37:25 UTC
I mean that alloc_ports_one_family() should check the transport used by the client before deciding what address to use for the receiver part (in alloc_ports_one_family()):

if (unicast transport && pool that has unicast addresses)
   acquire a unicast address/port
   ..
else if (multicast transport && pool that has multicast addresses)
   acquire a multicast address
   ..
else
   try to use random port
   ..

Regarding your suggestion above:

1. I agree, I think it's a good idea to allocate all needed ports in one  function, in update_transport(). So what is currently done in alloc_ports_one_family() should be done in update_transport() with some modifications suggested by you in alternative 3.
2. I think it introduces unnecessary complexity.
3. I agree. This should be combined with alternative 1.
Comment 12 Sebastian Dröge (slomo) 2015-11-12 09:07:44 UTC
Ok, let's try that then? Are you going to work on this? :)
Comment 13 Patricia Muscalu 2015-11-12 09:50:20 UTC
Yes, I'll try. Thanks!
Comment 14 Patricia Muscalu 2015-12-09 13:40:37 UTC
Created attachment 317023 [details] [review]
code refactory
Comment 15 Patricia Muscalu 2015-12-09 13:42:45 UTC
Created attachment 317025 [details] [review]
code refactory
Comment 16 Patricia Muscalu 2015-12-09 13:43:12 UTC
Created attachment 317026 [details] [review]
code refactory
Comment 17 Patricia Muscalu 2015-12-09 13:43:44 UTC
Created attachment 317027 [details] [review]
code refactory
Comment 18 Patricia Muscalu 2015-12-09 13:44:07 UTC
Created attachment 317028 [details] [review]
code refactory
Comment 19 Patricia Muscalu 2015-12-09 13:44:37 UTC
Created attachment 317029 [details] [review]
code refactory
Comment 20 Patricia Muscalu 2015-12-09 13:45:37 UTC
Created attachment 317030 [details] [review]
postpone alloc_ports() until SETUP
Comment 21 Patricia Muscalu 2015-12-09 13:46:49 UTC
Created attachment 317031 [details] [review]
no socket sharing
Comment 22 Patricia Muscalu 2015-12-09 13:47:24 UTC
Created attachment 317032 [details] [review]
unit tests fixes
Comment 23 Patricia Muscalu 2015-12-09 13:48:19 UTC
Created attachment 317033 [details] [review]
create multicast udp sources in alloc_ports()
Comment 24 Patricia Muscalu 2016-01-12 09:50:43 UTC
I'm continuing to work on the patches now.
Comment 25 Sebastian Dröge (slomo) 2016-01-12 10:18:00 UTC
Thanks, and thanks for working on this :) I wasn't able to look at the patches yet but splitting them up like this definitely makes sense and makes it easier to review them. All the refactoring also seems to make sense and make the code more readable, but I'll have to look in detail yet.
Comment 26 Patricia Muscalu 2016-01-13 08:34:22 UTC
Not a problem. I'll send you new patches, hopefully this week.
Comment 27 Patricia Muscalu 2016-01-13 13:29:37 UTC
Created attachment 318949 [details] [review]
code refactory
Comment 28 Patricia Muscalu 2016-01-13 13:30:20 UTC
Created attachment 318950 [details] [review]
code refactory
Comment 29 Patricia Muscalu 2016-01-13 13:30:45 UTC
Created attachment 318951 [details] [review]
code refactory
Comment 30 Patricia Muscalu 2016-01-13 13:31:27 UTC
Created attachment 318952 [details] [review]
code refactory
Comment 31 Patricia Muscalu 2016-01-13 13:32:02 UTC
Created attachment 318953 [details] [review]
code refactory
Comment 32 Patricia Muscalu 2016-01-13 13:34:26 UTC
Created attachment 318955 [details] [review]
code refactory
Comment 33 Patricia Muscalu 2016-01-13 13:35:12 UTC
Created attachment 318956 [details] [review]
code refactory
Comment 34 Patricia Muscalu 2016-01-13 13:37:00 UTC
Created attachment 318957 [details] [review]
postpone alloc_ports() until SETUP
Comment 35 Patricia Muscalu 2016-01-13 13:37:44 UTC
Created attachment 318958 [details] [review]
unit tests fixes
Comment 36 Patricia Muscalu 2016-01-13 13:38:21 UTC
Created attachment 318959 [details] [review]
unit tests fixes
Comment 37 Patricia Muscalu 2016-01-13 13:39:43 UTC
Created attachment 318960 [details] [review]
create all udp sources in one function
Comment 38 Patricia Muscalu 2016-01-27 12:44:18 UTC
Created attachment 319818 [details] [review]
code refactory
Comment 39 Patricia Muscalu 2016-01-27 12:44:48 UTC
Created attachment 319819 [details] [review]
code refactory
Comment 40 Patricia Muscalu 2016-01-27 12:45:11 UTC
Created attachment 319820 [details] [review]
code refactory
Comment 41 Patricia Muscalu 2016-01-27 12:46:01 UTC
Created attachment 319821 [details] [review]
code refactory
Comment 42 Patricia Muscalu 2016-01-27 12:46:24 UTC
Created attachment 319822 [details] [review]
code refactory
Comment 43 Patricia Muscalu 2016-01-27 12:47:14 UTC
Created attachment 319823 [details] [review]
code refactory
Comment 44 Patricia Muscalu 2016-01-27 12:47:46 UTC
Created attachment 319824 [details] [review]
code refactory
Comment 45 Patricia Muscalu 2016-01-27 12:48:33 UTC
Created attachment 319825 [details] [review]
postpone alloc_ports() until SETUP
Comment 46 Patricia Muscalu 2016-01-27 12:49:16 UTC
Created attachment 319826 [details] [review]
unit tests fixes
Comment 47 Patricia Muscalu 2016-01-27 12:49:52 UTC
Created attachment 319827 [details] [review]
unit tests fixes
Comment 48 Patricia Muscalu 2016-01-27 12:50:55 UTC
Created attachment 319828 [details] [review]
create all udp sources in one function
Comment 49 Patricia Muscalu 2016-01-27 12:51:58 UTC
Created attachment 319829 [details] [review]
handle client transport setting properly
Comment 50 Sebastian Dröge (slomo) 2016-02-22 08:52:02 UTC
Looks generally good to me but it breaks two unit tests: gst/rtspclientsink and gst/rtspserver

Also please give your commits better commit messages, lots of commits that just say "code refactory" in the summary are not useful. In your case you can just move the first sentence of the longer commit message to the summary :)
Comment 51 Sebastian Dröge (slomo) 2016-02-22 08:53:19 UTC
Comment on attachment 319826 [details] [review]
unit tests fixes

The two unit test fixes commits can be squashed together IMHO
Comment 52 Sebastian Dröge (slomo) 2016-02-22 08:53:23 UTC
Comment on attachment 319827 [details] [review]
unit tests fixes

The two unit test fixes commits can be squashed together IMHO
Comment 53 Sebastian Dröge (slomo) 2016-02-22 08:56:40 UTC
Running suite(s): rtspserver
0:00:00.076670887 12466      0x213f2d0 ERROR             rtspclient rtsp-client.c:726:find_media: client 0x21418f0: no factory for path /non-existing
0:00:00.076726807 12466      0x213f2d0 ERROR             rtspclient rtsp-client.c:2286:handle_describe_request: client 0x21418f0: no media
0:00:00.081932729 12471      0x213f230 ERROR             rtspclient rtsp-client.c:2293:handle_describe_request: client 0x21418f0: media does not support DESCRIBE
0:00:00.147712703 12567      0x213f2d0 ERROR             rtspclient rtsp-client.c:2966:handle_request: client 0x21418f0: Required option is not supported (funky-feature)
0:00:00.147835175 12567      0x213f2d0 ERROR             rtspclient rtsp-client.c:2966:handle_request: client 0x21418f0: Required option is not supported (funky-feature, foo-bar, superburst)
0:00:00.160141986 12586      0x213f2d0 ERROR             rtspclient rtsp-client.c:2067:handle_setup_request: client 0x21418f0: stream 'stream=7' not found
0:00:01.984546413 12628      0x213f2d0 ERROR             rtspclient rtsp-client.c:1255:handle_play_request: client 0x21418f0: no session
0:00:01.987152611 12633      0x21f8380 ERROR             rtspserver rtsp-server.c:928:gst_rtsp_server_create_socket:<GstRTSPServer@0x2226900> failed to create socket
0:00:01.987181451 12633      0x21f8380 ERROR             rtspserver rtsp-server.c:1295:gst_rtsp_server_create_source:<GstRTSPServer@0x2226900> failed to create socket
0:00:01.987187106 12633      0x21f8380 ERROR             rtspserver rtsp-server.c:1341:gst_rtsp_server_attach:<GstRTSPServer@0x2226900> failed to create watch: Error binding to address: Address already in use
0:00:19.542189368 12734      0x213f2d0 ERROR             rtspclient rtsp-client.c:2953:handle_request: client 0x21418f0: session not found
0:00:38.247117716 12903      0x213f230 ERROR             rtspclient rtsp-client.c:2448:handle_announce_request: client 0x21418f0: can't find SDP message
0:00:38.247245498 12903      0x213f230 ERROR             rtspclient rtsp-client.c:2448:handle_announce_request: client 0x21418f0: can't find SDP message
0:00:38.247331870 12903      0x213f230 ERROR             rtspclient rtsp-client.c:2442:handle_announce_request: client 0x21418f0: unknown content type


Unexpected critical/warning: gst_object_unref: assertion '((GObject *) object)->ref_count > 0' failed
95%: Checks: 22, Failures: 1, Errors: 0
gstcheck.c:79:S:general:test_record_tcp:0: Unexpected critical/warning: gst_object_unref: assertion '((GObject *) object)->ref_count > 0' failed





Running suite(s): rtspclientsink
0:00:20.034472659 13749      0x1fbe0f0 ERROR             rtspclient rtsp-client.c:2344:handle_sdp: client 0x1f939d0: can't prepare media
0:00:20.034497150 13749      0x1fbe0f0 ERROR             rtspclient rtsp-client.c:2478:handle_announce_request: client 0x1f939d0: can't handle SDP
0%: Checks: 1, Failures: 1, Errors: 0
gst/rtspclientsink.c:181:F:general:test_record:0: Failure 'GST_MESSAGE_TYPE (msg) != GST_MESSAGE_EOS' occurred
Comment 54 Patricia Muscalu 2016-02-23 14:11:21 UTC
Created attachment 321963 [details] [review]
helper function for creating the sender and the receiver parts of the pipeline
Comment 55 Patricia Muscalu 2016-02-23 14:13:37 UTC
Created attachment 321964 [details] [review]
create and configure UDP sinks in a separate function
Comment 56 Patricia Muscalu 2016-02-23 14:15:10 UTC
Created attachment 321965 [details] [review]
added function for RTP/RTCP socket configuration
Comment 57 Patricia Muscalu 2016-02-23 14:16:13 UTC
Created attachment 321966 [details] [review]
added function for creating and configuring UDP sources
Comment 58 Jan Schmidt 2016-02-23 14:17:17 UTC
Review of attachment 319819 [details] [review]:

::: gst/rtsp-server/rtsp-stream.c
@@ +1087,3 @@
+  /* ERRORS */
+no_udp_protocol:
+  {

This needs to do cleanup to free any allocated udpsink0 that isn't going to be used or it will leak.
Comment 59 Patricia Muscalu 2016-02-23 14:18:23 UTC
Created attachment 321969 [details] [review]
added function for setting UDP sources to PLAYING state
Comment 60 Patricia Muscalu 2016-02-23 14:20:48 UTC
Created attachment 321972 [details] [review]
postpone the creation of the UDP sources
Comment 61 Patricia Muscalu 2016-02-23 14:21:52 UTC
Created attachment 321974 [details] [review]
postpone UDP socket allocation until SETUP
Comment 62 Patricia Muscalu 2016-02-23 14:22:21 UTC
Created attachment 321975 [details] [review]
unit test fixes
Comment 63 Sebastian Dröge (slomo) 2016-03-02 09:51:12 UTC
Follow up commit:

commit bcee3202d3a6c8749d9a0c7158096d7009f8e337
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Wed Mar 2 11:47:47 2016 +0200

    rtsp-stream: Don't bind the sockets to multicast addresses
    
    This works on Linux but fails completely on Windows. You're supposed
    to bind to ANY and then join the multicast group.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757488
Comment 64 Sebastian Dröge (slomo) 2016-03-03 08:45:57 UTC
commit a7ced98346c6d49c4569d1b609a773a042c46796
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Thu Mar 3 10:41:51 2016 +0200

    rtsp-stream: Only use the address pool for unicast UDP if it contains unicast addresses
    
    Otherwise we fail to allocate UDP ports if the pool only contains multicast
    addresses, which is something that used to work before. For unicast addresses
    if the pool contains none, we just allocate them as if there is no pool at
    all.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757488
Comment 65 Patricia Muscalu 2016-03-03 09:11:41 UTC
Maybe I'm missing something but configuring the address pool with multicast addresses in the unicast UDP case seems strange.
Comment 66 Sebastian Dröge (slomo) 2016-03-03 09:21:56 UTC
Discussion from IRC:

<patriciam> slomo, I've just checked your last commit in gst-rtsp-server. having only multicast adresses in the address pool in the unicast UDP case seems very strange. am I missing something?
<slomo> patriciam: it is, but that's how it worked before :)
<slomo> let me find you the code
<slomo> patriciam: https://cgit.freedesktop.org/gstreamer/gst-rtsp-server/tree/gst/rtsp-server/rtsp-stream.c?id=b6ca057c720c2528c9fd557d4b84db83fa0412ee#n1078
<slomo> this is for the case when you add a pool to your media that only has multicast addresses, but the client is requesting unicast udp
<patriciam> slomo, hmm... strange configuration :). anyway, it means, that we are missing the unit-test checking this case. 
<slomo> patriciam: i agree that it's weird, yes. but application expected this to work and broke now :) should probably fix that for 2.0 (and will probably forget to fix it because that's so far in the future)
Comment 67 Patricia Muscalu 2016-03-04 08:43:56 UTC
Created attachment 323069 [details] [review]
added unit tests for stream

Extended the stream test suite with new tests.
Comment 68 Sebastian Dröge (slomo) 2016-03-05 08:10:15 UTC
Another follow-up, a bit more to come unfortunately

commit 97948225499b37ceff773d77e345a2a0b7cb4593
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Fri Mar 4 13:51:12 2016 +0200

    rtsp-stream: Only bind multicast sockets to ANY on Windows
    
    On Linux it is still needed to bind to the multicast address
    to filter out random other packets, while on Windows binding
    to multicast addresses just fails.
Comment 69 Sebastian Dröge (slomo) 2016-03-05 08:13:31 UTC
Comment on attachment 323069 [details] [review]
added unit tests for stream

commit 422d3a300216df5388d00eb1908562a9665b992a
Author: Patricia Muscalu <patricia@axis.com>
Date:   Thu Mar 3 15:07:06 2016 +0100

    stream tests: added new tests
    
    Test a case when the address pool only contains multicast addresses
    and the client is requesting unicast udp.
    Added tests for multicast ports allocation.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757488
Comment 70 Sebastian Dröge (slomo) 2016-03-05 08:55:19 UTC
commit 206d2ded099e49d8d6e03fdc6158eb6ed96d6276
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Sat Mar 5 10:52:11 2016 +0200

    rtsp-stream: Disable multicast loopback for all our sockets
    
    On Windows this is a receiver-side setting, on Linux a sender-side setting. As
    we provide a socket ourselves to udpsrc, udpsrc is never setting the multicast
    loopback setting on the socket... while udpsink does which unfortunately has
    no effect here on Windows but on Linux.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757488
Comment 71 Sebastian Dröge (slomo) 2016-03-05 08:58:46 UTC
For 1.9 we should probably go to using the udpsrc sockets directly instead of providing our own. Or alternatively set up our own sockets completely ourselves.
Comment 72 Sebastian Dröge (slomo) 2016-03-05 09:11:56 UTC
gst_rtsp_stream_get_multicast_address() also seems a bit problematic. Shouldn't it return the one that was actually chosen during setup instead of getting a (possibly new!) one from the address pool?
Comment 73 Patricia Muscalu 2016-03-07 07:16:10 UTC
gst_rtsp_stream_get_multicast_address () will only get an address from the address pool if addr_v4 or addr_v6 are null.
Comment 74 Patricia Muscalu 2016-03-07 07:32:17 UTC
(In reply to Sebastian Dröge (slomo) from comment #70)
> commit 206d2ded099e49d8d6e03fdc6158eb6ed96d6276
> Author: Sebastian Dröge <sebastian@centricular.com>
> Date:   Sat Mar 5 10:52:11 2016 +0200
> 
>     rtsp-stream: Disable multicast loopback for all our sockets
>     
>     On Windows this is a receiver-side setting, on Linux a sender-side
> setting. As
>     we provide a socket ourselves to udpsrc, udpsrc is never setting the
> multicast
>     loopback setting on the socket... while udpsink does which unfortunately
> has
>     no effect here on Windows but on Linux.
>     
>     https://bugzilla.gnome.org/show_bug.cgi?id=757488

This is supposed to be done in create_and_configure_udpsources_one_family():
g_object_set (G_OBJECT (udpsrc_out[0]), "loop", FALSE, NULL);
g_object_set (G_OBJECT (udpsrc_out[1]), "loop", FALSE, NULL);

Compare to cdc08 commit.
Comment 75 Sebastian Dröge (slomo) 2016-03-07 10:17:31 UTC
(In reply to Patricia Muscalu from comment #74)
> (In reply to Sebastian Dröge (slomo) from comment #70)
> > commit 206d2ded099e49d8d6e03fdc6158eb6ed96d6276
> > Author: Sebastian Dröge <sebastian@centricular.com>
> > Date:   Sat Mar 5 10:52:11 2016 +0200
> > 
> >     rtsp-stream: Disable multicast loopback for all our sockets
> >     
> >     On Windows this is a receiver-side setting, on Linux a sender-side
> > setting. As
> >     we provide a socket ourselves to udpsrc, udpsrc is never setting the
> > multicast
> >     loopback setting on the socket... while udpsink does which unfortunately
> > has
> >     no effect here on Windows but on Linux.
> >     
> >     https://bugzilla.gnome.org/show_bug.cgi?id=757488
> 
> This is supposed to be done in create_and_configure_udpsources_one_family():
> g_object_set (G_OBJECT (udpsrc_out[0]), "loop", FALSE, NULL);
> g_object_set (G_OBJECT (udpsrc_out[1]), "loop", FALSE, NULL);

Just that udpsrc does not do anything with that flag if we provide it our own socket :) That's why I went with the hammer solution now and just disabled it directly on the socket from the very start.
Comment 76 Sebastian Dröge (slomo) 2016-03-07 10:19:26 UTC
(In reply to Patricia Muscalu from comment #73)
> gst_rtsp_stream_get_multicast_address () will only get an address from the
> address pool if addr_v4 or addr_v6 are null.

Ah I missed that, thanks! But if I understand this correctly this can happen:
1) get_multicast_address(), stores it in the stream then
2) alloc_ports_one_family()
3) Frees that address from 1) and gets a new one

I think instead 3) should use that address.
Comment 77 Patricia Muscalu 2016-03-07 10:47:03 UTC
(In reply to Sebastian Dröge (slomo) from comment #75)
> (In reply to Patricia Muscalu from comment #74)
> > (In reply to Sebastian Dröge (slomo) from comment #70)
> > > commit 206d2ded099e49d8d6e03fdc6158eb6ed96d6276
> > > Author: Sebastian Dröge <sebastian@centricular.com>
> > > Date:   Sat Mar 5 10:52:11 2016 +0200
> > > 
> > >     rtsp-stream: Disable multicast loopback for all our sockets
> > >     
> > >     On Windows this is a receiver-side setting, on Linux a sender-side
> > > setting. As
> > >     we provide a socket ourselves to udpsrc, udpsrc is never setting the
> > > multicast
> > >     loopback setting on the socket... while udpsink does which unfortunately
> > > has
> > >     no effect here on Windows but on Linux.
> > >     
> > >     https://bugzilla.gnome.org/show_bug.cgi?id=757488
> > 
> > This is supposed to be done in create_and_configure_udpsources_one_family():
> > g_object_set (G_OBJECT (udpsrc_out[0]), "loop", FALSE, NULL);
> > g_object_set (G_OBJECT (udpsrc_out[1]), "loop", FALSE, NULL);

> Just that udpsrc does not do anything with that flag if we provide it our
> own socket :) That's why I went with the hammer solution now and just
> disabled it directly on the socket from the very start.

Yes, I understand, but in that case we should remove these two lines above from the code. Maybe setting multicast loopback should be also done in the same function just to make sure that all configuration that is really needed is done in one place. What do you think?
Comment 78 Patricia Muscalu 2016-03-07 10:49:55 UTC
(In reply to Sebastian Dröge (slomo) from comment #76)
> (In reply to Patricia Muscalu from comment #73)
> > gst_rtsp_stream_get_multicast_address () will only get an address from the
> > address pool if addr_v4 or addr_v6 are null.
> 
> Ah I missed that, thanks! But if I understand this correctly this can happen:
> 1) get_multicast_address(), stores it in the stream then
> 2) alloc_ports_one_family()
> 3) Frees that address from 1) and gets a new one
> 
> I think instead 3) should use that address.

It's not good that we actually can get addresses in two ways. We should probably only allocate the multicast address in alloc_ports() and return this address in _get_multicast_address().
Comment 79 Sebastian Dröge (slomo) 2016-03-07 10:56:19 UTC
(In reply to Patricia Muscalu from comment #77)

> > Just that udpsrc does not do anything with that flag if we provide it our
> > own socket :) That's why I went with the hammer solution now and just
> > disabled it directly on the socket from the very start.
> 
> Yes, I understand, but in that case we should remove these two lines above
> from the code. Maybe setting multicast loopback should be also done in the
> same function just to make sure that all configuration that is really needed
> is done in one place. What do you think?

For after 1.8.0 I would like all this code to be changed. Either we use udpsrc/udpsink to set up all things for us, including providing the sockets... or we do everything with the socket setup ourselves and provide them only to the elements.

(In reply to Patricia Muscalu from comment #78)
> (In reply to Sebastian Dröge (slomo) from comment #76)
> > (In reply to Patricia Muscalu from comment #73)
> > > gst_rtsp_stream_get_multicast_address () will only get an address from the
> > > address pool if addr_v4 or addr_v6 are null.
> > 
> > Ah I missed that, thanks! But if I understand this correctly this can happen:
> > 1) get_multicast_address(), stores it in the stream then
> > 2) alloc_ports_one_family()
> > 3) Frees that address from 1) and gets a new one
> > 
> > I think instead 3) should use that address.
> 
> It's not good that we actually can get addresses in two ways. We should
> probably only allocate the multicast address in alloc_ports() and return
> this address in _get_multicast_address().

And make it an error to call get_multicast_address() before? If that works, sure.
Comment 81 Patricia Muscalu 2016-03-09 07:58:00 UTC
I cannot reproduce the test failures locally. Is it a socket bind() problem?
Comment 82 Sebastian Dröge (slomo) 2016-03-09 10:07:30 UTC
I was running "make gst/stream.forever" since a few minutes now, no problems here either.
Comment 83 Patricia Muscalu 2016-03-17 08:09:34 UTC
Are the tests still failing? Do you know more about the problem?
Comment 84 Sebastian Dröge (slomo) 2016-03-17 08:25:48 UTC
Currently not failing anymore, no. But nobody knows why :)