rtp forward: create both a ipv4 and ipv6 udp socket. fixes #2051

[kazzmir]

For issue, #2051, macos (and probably other non-linux systems) cannot send data with a udp AF_INET6 packet to an ipv4 AF_INET address.

int udp = socket(AF_INET6, SOCK_DGRAM, IPPROTO_UDP);
struct sockaddr_in address;
address.sin_family = AF_INET;
sendto(udp, ..., &address);

Currently in plugins/janus_videoroom.c a single udp_sock socket is created with AF_INET6 and use to send to both ipv4 and ipv6 addresses. The solution this PR has is to create two udp sockets, udp4_socket and udp6_socket, where if the address being sent to is ipv4 then use the udp4 socket, and if the address is ipv6 then use udp6 socket.

The implementation is straight forward, although it could be cleaned up due to having two sockets. In particular, the lines of code that set up the udp socket and address to invoke sendto() on could be abstracted out into its own function.

[lminiero]

I don't think that's the way to go. One socket should be enough, we just need to know in advance which one we'll need. That's why I suggested moving the host resolution sooner than where it currently is.

[lminiero]

PS: Sorry, forgot to say thanks for this! 😊

[lminiero]

(isolation is killing my manners)

[kazzmir]

Ok I see what you mean. I will do the host resolution part earlier and then create the udp sock based on that. I am thinking the simplest way is just to call all the janus_videoroom_rtp_forwarder_add_helper() functions first, then set an is_ipv4 flag in the publisher, and then use the is_ipv4 flag to create the udp_sock, and also to help select the right parameters when calling sendto().

[kazzmir]

is_ipv4 is set outside of janus_videoroom_rtp_forwarder_add_helper, and passed in as a flag now.

[lminiero]

Mh that still feels a bit too convoluted and verbose to me, and adds a property to the signature that is unneeded IMO. I'll prepare a parallel PR that anticipates the host resolution.

[lminiero]

As a side note, I think we have the same exact code in the AudioBridge plugin too (which supports RTP forwarders as well), so I'll make sure it's also applied there.

[lminiero]

Ah, but another thing dawned on me, now... we're actually using a shared socket for publishers, that we use for ALL forwarders belonging to that publisher. If we force it to be just IPv4 or IPv6, it won't work if we want the same publisher to be sent to multiple addresses of different families. The socket that is created when setting up the forwarder itself is only the RTCP one, and that can actually still be IPv6 to receive either (it's how the Streaming plugin works too), so what needs fixing is just the RTP one.

My guess is that we should get rid of the global udp_sock in the publisher structure, and move that one to the forwarder struct as well. It will mean more sockets if we have multiple forwarders for the same publisher (so two rather than one if we're sending audio and video), but that's probably not a big deal. As an alternative, your original proposal of having two sockets (one v4 and one v6) in the publisher now makes more sense, even though each should then only be created when really needed (and so it should still be aware of what we're going to send to).

I'll think about this a bit more and then share something in a new PR.

[lminiero]

Apparently it should be possible to detect this at runtime by checking if IPV6_V6ONLY is defined. This way I can add a different behaviour for systems that don't support it, and keep what we have now for Linux (which I'd rather not touch). I'll try to do that, so that you can check if it works for you (I don't have access to non-Linux machines so I wouldn't be able to test it).

[lminiero]

FYI, apologies for this very late comment: I started working on that at the time, and then got swamped in a ton of other things. At the current stage, it's unlikely I'll get back to it any time soon, especially considering we're going to work on the new, revamped, multistream branch: since that will be a considerable refactoring of its own, I'd reject any big change to the code base until that's merged. That said, I don't think the PR as it is works as explained in the comments above, so it unfortunately cannot be merged: as such, I think we'll close this for now, and we can come back to this once the multistream work has been done. Thanks anyway for the contribution!

[lminiero]

I'll keep the original issue open and add the enhancement label just as a reminder.