From the debug log, I can see the server port range has been set correctly but the video data is coming from a different random port.

From the debug log, I can see the server port range has been set correctly but the video data is coming from a different random port.

Can you check if it was ok with 1.8.0 too, and 1.6.4?

Created attachment 328147 [details]
logs from my previous attached modified test-launch (1.8.1)

Hi Sebastian,

Thanks for reply.This feature works with 1.6.4 but also breaks in 1.8.0.
I have attached the modified test-launch log for your reference. 

Jake

Please check those two lines, which shows that the gst code has accepted my port range setting:
==================================================================
0:00:00.010542168 27358       0xc0bc60 DEBUG        rtspaddresspool rtsp-address-pool.c:309:gst_rtsp_address_pool_add_range:<GstRTSPAddressPool@0xaf4780> adding 0.0.0.0-0.0.0.0:18100-18109 ttl 0

key: 'Transport', value: 'RTP/AVP;unicast;client_port=28972-28973;server_port=18100-18101;ssrc=7157B7CB;mode="PLAY"'

===================================

Thanks, that's useful to know. This will then have to be debugged, together with "git bisect" and your testcase we should be able to find the offending commit in gst-rtsp-server relatively fast.

The problem is that alloc_ports_one_family() (which sets the socket on the udpsink) is called much after create_and_configure_udpsinks(), and the multiudpsink is already in PAUSED by then and doesn't accept a new socket.

It was probably introduced by

commit f62a9a7eb9fda096fb88c013d6787c28090037a2
Author: Patricia Muscalu <patricia@axis.com>
Date:   Tue Feb 23 14:59:32 2016 +0100

    rtsp-stream: postpone UDP socket allocation until SETUP
    
    Postpone the allocation of the UDP sockets until we know
    what transport has been chosen by the client.
    Both unicast and multicast UDP sources are created in one
    function.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=757488

Maybe we can delay creation of the udp sink/sources until SETUP (i.e. configure_client_transport) instead of DESCRIBE/etc (rtsp_media_prepare). They should not be needed before SETUP, and SETUP is when we know what kind of transport the client(s) want.

This seems not fixable in time for 1.8.2. It requires bigger refactoring in rtsp-stream, or to always add a queue and fakesink.

If we only add the UDP elements on SETUP (which is the right thing to do in any case), we would have no sinks before and the pipeline would have problems to preroll because of that. The transport specific sinks and sources (appsink, appsrc, udpsrc, multiudpsink) should only be added when the transport is clear (i.e. SETUP).

What should work here most likely is to not have any sinks here (for the PLAY case), and just have the payloader pad blocked (as it already is for live streams). On SETUP sinks would be added, and preroll would be detected based on the pad probe blocking (like for live streams already) instead of async-done.

For RECORD we would add no source (and RTCP sinks) until SETUP, but it might have to be solved a bit different there for the pre-SETUP media preparation.

I'm hitting this bug as well, and reverting f62a9a7eb9fda096fb88c013d6787c28090037a2 is complicated because there are lots of other changes.

Are there easy workaround for this, even if it means breaking multicast again (I'm using only unicast)?

You would have to revert all the changes around that one, I'm not aware of a workaround other than that. We'll just have to fix it properly :)

Created attachment 331445 [details] [review]
WIP: stream: delay udpsink creation until SETUP

That patch is untested, just to get feedback if it's the right direction.

Created attachment 331547 [details] [review]
stream: Keep a ref on joined bin

Created attachment 331548 [details] [review]
stream: delay udpsink creation until SETUP

I haven't made intensive testing yet, but it seems to work.

The patch breaks unit tests actually, working on it. Still would be nice to have early feedback :)

There is something I find weird, for each client connecting to a stream (in unicast case), it will alloc a new socket, create an udpsrc for it. The it calls set_sockets_for_udpsinks() which overrides the "socket" or "socket-v6" property. I understand that multiudpsink can send to multiple destinations but does that mean that the "source" ip/port of udp packets is going to change every time a new client connects? Isn't that a bug?

*** Bug 768933 has been marked as a duplicate of this bug. ***

okay.... So I think I have a better understanding of what's going on here. The commit f62a9a7eb9fda096fb88c013d6787c28090037a2 is much more broken than I though.

1) It introduce a leak of udpsrc elements that got wrongly fixed by adding an hash table in commit cba045e. We should have at most 4 udpsrc for unicast: ipv4/ipv6, rtp/rtcp. They can be reused for all unicast clients.

2) If a mcast client connects, it creates a new socket in SETUP to try to respect the destination/port given by the client in the transport, and overrides the socket already set on the udpsink element. That means that if we already had a client connected, the source address on the udp packets it receives suddenly changes, I think.

3) If a 2nd mcast client connects, the destination/port in its transport is ignored anyway.


I think the choices we have here are:

1) Always have only one socket that we create in _join_bin(), and just make that socket join mcast groups, always ignoring destination/port from mcast clients in SETUP. That's basically reverting f62a9a7 and cba045e. That's the easiest and already difficult enough.

2) Do 1) then also create an extra socket per mcast client requesting a specific destination/port in SETUP. That means creating new udpsrc AND udpsink (so we'll always have the tee+queue before udpsinks). Those elements will have to be tracked into an hashtable similar to the one we already have, but for mcast not unicast clients.

3) Do 2) then also do the complicated task of not having any udpsink elements until SETUP just in case the first client that connects is an mcast one that wants another destination/port. If we do this, and since in (2) we always have a tee anyway, I would go with a fakesink rather than blocking the stream. It turned out to be way too complicated, I still haven't succeeded in making unit tests pass with that solution.

(In reply to Xavier Claessens from comment #22)
> 3) If a 2nd mcast client connects, the destination/port in its transport is
> ignored anyway.

Actually by ignored, I mean in a broken way: gst_rtsp_stream_allocate_udp_sockets() won't create a new socket and default_configure_client_transport() won't change the destination/port/ttl in the transport to indicate it ignored client settings.

(In reply to Xavier Claessens from comment #22)

> 3) Do 2) then also do the complicated task of not having any udpsink
> elements until SETUP just in case the first client that connects is an mcast
> one that wants another destination/port. If we do this, and since in (2) we
> always have a tee anyway, I would go with a fakesink rather than blocking
> the stream. It turned out to be way too complicated, I still haven't
> succeeded in making unit tests pass with that solution.

You mean you didn't succeed just blocking and having no sinks? How does it fail and why?


In general, I think we should only allocate UDP ports when needed (that is: SETUP). This means that before we need some kind of sinks or the pad blocks.

Then during SETUP we should take into account what the clients tell us and set up things accordingly, possibly adding new sinks/sources as needed. And during teardown shut those down again depending on the still existing clients.


Having always a tee and a fakesink for the sinks is a possible option, but it will have to be optimized away at a later time. The tee requires queues, and people were complaining in the past already about the performance impact of that (which is why we only add the tee and stuff when needed currently).

My current WIP branch is here: https://git.collabora.com/cgit/user/xclaesse/gst-rtsp-server-1.0-1.8.git/log/?h=wip/stream-ports

At the current stage unit tests still fails, it's much more complicate than I though...

The big points it changes:
 - There is no need to create more than one socket and pair of udpsrc for all unicast clients.
 - We already support only one mcast group, so we should only create only one pair of udpsrc for it.
 - The mcast group also needs its own multiudpsink. I don't think it can share the one from unicast clients because they don't share the same socket. Otherwise it wouldn't send and recv from the same port. Current code is wrong here because it changes the socket on the shared multiudpsink on the fly even for existing clients.

About avoiding the tee, it's not mandatory if we have only unicast or only mcast clients. But if we have both then it cannot be avoided AFAIK. Also it would means that if we have only an unicast client and then a mcast clients connects we would have to dynamically plug a tee... That's more work that I'm not doing in this branch, yet.

Created attachment 332308 [details] [review]
Revert "rtsp-stream: Fix crash on cleanup with shared media and multiple udpsrc"

This partly reverts commit cba045e1b19fad6e689e10206f57903e15f1229a,
but keeps unit tests.

Created attachment 332309 [details] [review]
stream: small fix in error code path

Created attachment 332310 [details] [review]
stream: code cleanup

Created attachment 332311 [details] [review]
stream: Keep a ref on joined bin

Created attachment 332312 [details] [review]
stream: rename addr_v4/6 to mcast_addr_v4/6 for clarity

Created attachment 332313 [details] [review]
stream: small documentation clarification

Created attachment 332314 [details] [review]
stream: factor out plug_sink function

Created attachment 332315 [details] [review]
stream: factor our plug_src function

Created attachment 332316 [details] [review]
stream: revert back to create udpsrc/udpsink on DESCRIBE for unicast

This is basically reverting changes introduced in commit f62a9a7,
because it was introducing various regressions:

- It introduces a leak of udpsrc elements that got wrongly fixed by adding
  an hash table in commit cba045e. We should have at most 4 udpsrc for unicast:
  ipv4/ipv6, rtp/rtcp. They can be reused for all unicast clients.
- If a mcast client connects, it creates a new socket in SETUP to try to respect
  the destination/port given by the client in the transport, and overrides the
  socket already set on the udpsink element. That means that if we already had a
  client connected, the source address on the udp packets it receives suddenly
  changes.
- If a 2nd mcast client connects, the destination/port in its transport is
  ignored but its transport wasn't updated.

What this patch does:

- Revert back to create udpsrc/udpsink for unicast clients on DESCRIBE.
- Always have a tee+queue when udp is enabled. This could be optimized
  again in a later patch, but is more complicated. If no unicast clients
  connects then those elements are useless, this could be also optimized
  in a later patch.
- When mcast transport is added, it creates a new set of udpsrc/udpsink,
  seperated from those for unicast clients. Since we already support only
  one mcast address, we also create only one set of elements.

(In reply to Xavier Claessens from comment #34)
> Created attachment 332316 [details] [review] [review]
> stream: revert back to create udpsrc/udpsink on DESCRIBE for unicast
> [...]

How do you know if a unicast client connects in DESCRIBE?

(In reply to Sebastian Dröge (slomo) from comment #35)
> (In reply to Xavier Claessens from comment #34)
> > Created attachment 332316 [details] [review] [review] [review]
> > stream: revert back to create udpsrc/udpsink on DESCRIBE for unicast
> > [...]
> 
> How do you know if a unicast client connects in DESCRIBE?

I don't, if only mcast clients connects then it's just wasted, like it was before. This could be optimized later, better make it work first, then make it fast later.

This also breaks NAT hole punching: the server reports server_port=X, the client sends its data there, the server uses port=Y and the hole punching was completely unsuccessful.

You mean my patch broken NAT punching, or it is currently broken and my patch hopefully fix it? I have no idea how that part works, tbh.

It's currently broken, your patch might fix it but I didn't try it yet. That's somewhere at the top of my list :) Your patch probably fixes it if I understand correctly what you're doing.

<slomo> xclaesse: for your gst-rtsp-server patches, it's all good and working for you or were there open questions from your side? i'll put aside some time early next week to look into it, so if there's something specific you want input about before let me know :)

Replying here to keep record:

That patch is deployed in the product we are working on and works perfectly. BUT, my case is unicast ipv4 udp only, and I have done NO testing of mcast nor TCP other than making unit tests pass. I believe I understood how mcast is supposed to work in theory, so I think I did the right thing there, but I cannot be sure.

Open questions are:
1) What do we do with gst_rtsp_stream_allocate_udp_sockets()? It has been released as public API, but IMO it should have been internal API only. My patch naively replaces its implementation by a g_warn_if_reached().
2) My patch removes the optimization where we avoided the tee+queue. I'm not really sure how to add it back, and I don't think that should be a merge blocker. Is it?
3) I'm not sure unit tests are really testing the whole thing.

I've merged them all locally now and made them apply cleanly to git master. It looks good for unicast but breaks multicast in interesting ways, looking into that now.

Also going to do a deeper review of the changes once that works.

(In reply to Xavier Claessens from comment #40)
> 1) What do we do with gst_rtsp_stream_allocate_udp_sockets()? It has been
> released as public API, but IMO it should have been internal API only. My
> patch naively replaces its implementation by a g_warn_if_reached().

Like most of the "internal" API there, that's fine. Someone should clean up the exported API and then we bump the soversion.

> 2) My patch removes the optimization where we avoided the tee+queue. I'm not
> really sure how to add it back, and I don't think that should be a merge
> blocker. Is it?

I'd rather have it working at all "slower", than having it not work very fast.

> 3) I'm not sure unit tests are really testing the whole thing.

They are definitely not (they succeed although multicast is broken currently), and they are also racy and depending too much on the environment (free ports for example).

Apart from many other problems (that I fixed locally), the problem with multicast is that we now always have the unicast udpsinks after the tee... then once everything is pre-rolled we add the multicast udpsink (but the queues after the tee for the unicast udpsinks are full! we're PAUSED), causing the multicast udpsinks to never get any data and to never preroll. Making the queues infinitely large works around that, but then there's still some other race condition that causes the sinks to never pre-roll at all about 50% of the time with test-multicast2 (but not test-multicast, that seems to work always).

How did this ever work before? It should've had the same problems with full queues when adding the multicast udpsinks.

Attachment 332308 [details] pushed as 07f17c2 - Revert "rtsp-stream: Fix crash on cleanup with shared media and multiple udpsrc"
Attachment 332309 [details] pushed as 2b223af - stream: small fix in error code path
Attachment 332310 [details] pushed as 3ff4529 - stream: code cleanup
Attachment 332311 [details] pushed as 55a1df5 - stream: Keep a ref on joined bin
Attachment 332312 [details] pushed as 82a618c - stream: rename addr_v4/6 to mcast_addr_v4/6 for clarity
Attachment 332313 [details] pushed as a44f198 - stream: small documentation clarification
Attachment 332314 [details] pushed as 47a3956 - stream: factor out plug_sink function
Attachment 332315 [details] pushed as aa0e604 - stream: factor our plug_src function
Attachment 332316 [details] pushed as 8495c47 - stream: revert back to create udpsrc/udpsink on DESCRIBE for unicast

Also pushed a few more fixups on top of that, namely:

commit ca855abae1a38c4278f85b93f84bf2b0c6a8a4d9
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Sep 5 18:04:50 2016 +0300

    rtsp-stream: Always create multicast UDP elements if the protocol flag is set
    
    Adding them later will cause deadlocks due to
    1) pre-rolling and staying in PAUSED with the unicast/TCP sinks
    2) adding the multicast sink
    3) waiting for it to get data to preroll again
    
    3) never happens because the queues after the tee are full.

commit be4b9718e33ce376c691b9b0f8c61350054cd2c3
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Sep 5 16:32:57 2016 +0300

    rtsp-stream: Fix up various multicast related issues

commit 958f4a47ec09bbc95ff2c997e485e4cfde3d54d0
Author: Sebastian Dröge <sebastian@centricular.com>
Date:   Mon Sep 5 13:40:59 2016 +0300

    tests: Fix compilation

Considering this for 1.8 if it does not cause any major problems. Things are rather broken in 1.8, it can't be much worse, even with this huge change ;)

Thanks Sebastian for your merge and fixes.

I think there is still an issue with mcast when the client request a destination/port in SETUP. Since you take a random address/port from the pool in DESCRIBE and set mcast_addr_v6/v4, when default_configure_client_transport() will call gst_rtsp_stream_reserve_address() it's going to return an error because address/port won't match.

This is minor regression compared to previous situation, but maybe we want to open a new bug to address that?

Did that ever work before, probably not? :) But you're right, we only support a single mcast address/port and that is automatically selected, not client selected. But AFAIU the stream only supports a single one anyway since forever, there's only one storage for mcast_addr.

Want to open a bug?

Opened bug #770954 if someone care to work on that (I don't).

Found another potential mcast issue: I think your patch reverts what you did in commit aa9a2443a1d303727167b5b253e09e31fea6f09b. I think alloc_ports_one_family() binds on the mcast group instead of ANY.

Thanks, done that one too.

I opened bug #770969, related to binding on ANY address for mcast clients.

Reopening because your rebase on master introduced a small leak:

It seems we had the same idea by adding priv->joined_bin. The difference is my patch keeps a ref on it (for no reason actually) and that ref is dropped in the end of gst_rtsp_stream_leave_bin(), but your code already set it to NULL at the beginning.

Created attachment 334990 [details] [review]
stream: Fix leaked joined_bin

There is no need to keep a strong ref on it, and _leave_bin() was
setting it to NULL before calling g_clear_object() so it was leaked.

Created attachment 334993 [details] [review]
stream: Compare IP addresses case insensitive in more places

Created attachment 334994 [details] [review]
stream: cosmetic cleanup

Note: I don't have a gst master build atm, so didn't test those patches. Please try to build before merging ;-)

Attachment 334990 [details] pushed as f90ab92 - stream: Fix leaked joined_bin
Attachment 334993 [details] pushed as f5f3506 - stream: Compare IP addresses case insensitive in more places
Attachment 334994 [details] pushed as e882fe9 - stream: cosmetic cleanup