GNOME Bugzilla – Bug 757488
gst-rtsp-server: multicast functionality is broken if using the same port ranges for multicast and unicast
Last modified: 2016-03-17 08:25:48 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.
So the would mean regression between 1.4 and 1.6. Are you working on a patch ?
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?
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.
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.
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.
Exactly. Would you accept a patch that removes the part with the unicast udpsrc elements?
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?
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!
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?
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?
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.
Ok, let's try that then? Are you going to work on this? :)
Yes, I'll try. Thanks!
Created attachment 317023 [details] [review] code refactory
Created attachment 317025 [details] [review] code refactory
Created attachment 317026 [details] [review] code refactory
Created attachment 317027 [details] [review] code refactory
Created attachment 317028 [details] [review] code refactory
Created attachment 317029 [details] [review] code refactory
Created attachment 317030 [details] [review] postpone alloc_ports() until SETUP
Created attachment 317031 [details] [review] no socket sharing
Created attachment 317032 [details] [review] unit tests fixes
Created attachment 317033 [details] [review] create multicast udp sources in alloc_ports()
I'm continuing to work on the patches now.
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.
Not a problem. I'll send you new patches, hopefully this week.
Created attachment 318949 [details] [review] code refactory
Created attachment 318950 [details] [review] code refactory
Created attachment 318951 [details] [review] code refactory
Created attachment 318952 [details] [review] code refactory
Created attachment 318953 [details] [review] code refactory
Created attachment 318955 [details] [review] code refactory
Created attachment 318956 [details] [review] code refactory
Created attachment 318957 [details] [review] postpone alloc_ports() until SETUP
Created attachment 318958 [details] [review] unit tests fixes
Created attachment 318959 [details] [review] unit tests fixes
Created attachment 318960 [details] [review] create all udp sources in one function
Created attachment 319818 [details] [review] code refactory
Created attachment 319819 [details] [review] code refactory
Created attachment 319820 [details] [review] code refactory
Created attachment 319821 [details] [review] code refactory
Created attachment 319822 [details] [review] code refactory
Created attachment 319823 [details] [review] code refactory
Created attachment 319824 [details] [review] code refactory
Created attachment 319825 [details] [review] postpone alloc_ports() until SETUP
Created attachment 319826 [details] [review] unit tests fixes
Created attachment 319827 [details] [review] unit tests fixes
Created attachment 319828 [details] [review] create all udp sources in one function
Created attachment 319829 [details] [review] handle client transport setting properly
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 on attachment 319826 [details] [review] unit tests fixes The two unit test fixes commits can be squashed together IMHO
Comment on attachment 319827 [details] [review] unit tests fixes The two unit test fixes commits can be squashed together IMHO
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
Created attachment 321963 [details] [review] helper function for creating the sender and the receiver parts of the pipeline
Created attachment 321964 [details] [review] create and configure UDP sinks in a separate function
Created attachment 321965 [details] [review] added function for RTP/RTCP socket configuration
Created attachment 321966 [details] [review] added function for creating and configuring UDP sources
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.
Created attachment 321969 [details] [review] added function for setting UDP sources to PLAYING state
Created attachment 321972 [details] [review] postpone the creation of the UDP sources
Created attachment 321974 [details] [review] postpone UDP socket allocation until SETUP
Created attachment 321975 [details] [review] unit test fixes
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
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
Maybe I'm missing something but configuring the address pool with multicast addresses in the unicast UDP case seems strange.
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)
Created attachment 323069 [details] [review] added unit tests for stream Extended the stream test suite with new tests.
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 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
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
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.
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?
gst_rtsp_stream_get_multicast_address () will only get an address from the address pool if addr_v4 or addr_v6 are null.
(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.
(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.
(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.
(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?
(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().
(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.
This still seems to fail : https://ci.gstreamer.net/job/GStreamer-master/lastCompletedBuild/testReport/%28root%29/rtspstream/test_allocate_udp_ports_multicast/
I cannot reproduce the test failures locally. Is it a socket bind() problem?
I was running "make gst/stream.forever" since a few minutes now, no problems here either.
Are the tests still failing? Do you know more about the problem?
Currently not failing anymore, no. But nobody knows why :)