FreeBSD support #2508

jsm222 · 2020-12-28T15:40:18Z

I mostly corrected endianness includes and defines.
Most notable change is in transports/janus_websockets.c
There I set LWS_SERVER_OPTION_DISABLE_IPV6, because of
warmcat/libwebsockets#1947.
This is probably a FreeBSD only issue, and perhaps it should be in preprocessor macros.
I also did comment ip = iface out, since ws_ip is supposed to only bind to the ws_ip set.
This might be a bug common to all systems though. I have only tested build on FreeBSD.
Also see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=219444
See also #2232

januscla · 2020-12-28T15:40:20Z

Thanks for your contribution, @jsm222! Please make sure you sign our CLA, as it's a required step before we can merge this.

atoppi

thanks for the effort @jsm222 !
I don't have a FreeBSD machine and I don't know much about the OS, so I can't comment about the endiannes and macro stuff.
Anyway I noticed some minor points I'd be glad to discuss.

configure.ac

janus.c

transports/janus_websockets.c

Add LWS_SERVER_OPTION_DISABLE_IPV6 only for FreeBSD.

atoppi

Thanks, I've added some more notes.

configure.ac

transports/janus_websockets.c

atoppi

we're almost there, just some more empty lines to fix here and there!

configure.ac

transports/janus_websockets.c

atoppi · 2021-01-05T20:33:11Z

transports/janus_websockets.c

@@ -706,6 +726,13 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 #else
 				info.options = 0;
 #endif
+


forgot this one

atoppi · 2021-01-05T20:33:23Z

transports/janus_websockets.c

@@ -818,10 +865,18 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 				info.ssl_cipher_list = ciphers;
 				info.gid = -1;
 				info.uid = -1;
+
+


forgot these

jsm222 · 2021-01-06T13:31:35Z

The empty lines should be fixed now..

atoppi · 2021-01-06T15:34:17Z

lgtm 👍
Thanks again!

lminiero · 2021-01-06T15:40:06Z

I'll need a few days to review, as I'm still on my holiday break. Thanks for the effort!

lminiero

Thanks for this effort, and apologies for the late review: I've only recently come back to work after the holidays. I see Alessandro already provided good notes, so I've just added a few myself that are mostly related to form.

As a side note, is the ipv4/ipv6 libwebsockets issue on FreeBSD going to be an issue in Streaming plugin as well, as I think we always binding using an IPv6 struct by default, unless explicitly specifying IPv4 there? There may be issues with RTP forwarders too (VideoRoom and AudioBridge), as IIRC we do the same.

lminiero · 2021-01-13T11:21:28Z

README.md

@@ -207,6 +207,30 @@ If Doxygen and graphviz are available, the process can also build the documentat

 You can also selectively enable/disable other features (e.g., specific plugins you don't care about, or whether or not you want to build the recordings post-processor). Use the --help option when configuring for more info.

+### Building on FreeBSD


Please add some more descriptive description here too, as we do for MacOs. The pkg line should be "compacted" a bit too (like what we do above for yum and apt-get), as there's too many lines now I believe.

lminiero · 2021-01-13T11:22:34Z

README.md

+	sh autogen.sh
+	./configure
+	gmake
+	gmake install


Why is there a gmake here? I don't think the lines from autogen.sh on are needed at all, as they're shared for all systems (they're not there for the MacOS instructions for instance).

gmake is for GNU make. BSDs has its own make

lminiero · 2021-01-13T11:25:34Z

transports/janus_websockets.c

@@ -618,6 +621,12 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 			item = janus_config_get(config, config_general, janus_config_type_item, "ws_ip");
 			if(item && item->value) {
 				ip = (char *)item->value;
+#ifdef __FreeBSD__
+				struct in_addr addr;
+				if(inet_net_pton(AF_INET, ip, &addr, sizeof(addr))>0) {


Code style: there should be spaces before and after the > comparator.
Besides, since the action is simple, I don't think it's needed to wrap it in curly brackets.

lminiero · 2021-01-13T11:26:21Z

transports/janus_websockets.c

@@ -664,6 +679,12 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 			item = janus_config_get(config, config_general, janus_config_type_item, "wss_ip");
 			if(item && item->value) {
 				ip = (char *)item->value;
+#ifdef __FreeBSD__
+				struct in_addr addr;
+				if(inet_net_pton(AF_INET, ip, &addr, sizeof(addr))>0) {


lminiero · 2021-01-13T11:26:35Z

transports/janus_websockets.c

@@ -735,6 +762,12 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 			item = janus_config_get(config, config_admin, janus_config_type_item, "admin_ws_ip");
 			if(item && item->value) {
 				ip = (char *)item->value;
+#ifdef __FreeBSD__
+				struct in_addr addr;
+				if(inet_net_pton(AF_INET, ip, &addr, sizeof(addr))>0) {


lminiero · 2021-01-13T11:26:44Z

transports/janus_websockets.c

@@ -781,6 +820,12 @@ int janus_websockets_init(janus_transport_callbacks *callback, const char *confi
 			item = janus_config_get(config, config_admin, janus_config_type_item, "admin_wss_ip");
 			if(item && item->value) {
 				ip = (char *)item->value;
+#ifdef __FreeBSD__
+				struct in_addr addr;
+				if(inet_net_pton(AF_INET, ip, &addr, sizeof(addr))>0) {


jsm222 · 2021-01-14T18:27:01Z

As a side note, is the ipv4/ipv6 libwebsockets issue on FreeBSD going to be an issue in Streaming plugin as well, as I think we always binding using an IPv6 struct by default, unless explicitly specifying IPv4 there? There may be issues with RTP forwarders too (VideoRoom and AudioBridge), as IIRC we do the same.

Videoroom seem to work. I did not test audiobridge.. The issue with listening (lib)websockets is that it does not listen on both Ipv4 and IPv6 when ipv6 is enabled it is ipv6 only.. Can you provide more details of what you think might not work, and how to test it?

lminiero · 2021-01-14T18:42:41Z

Can you provide more details of what you think might not work, and how to test it?

What I'm thinking about is what is described in #2051. I just noticed you actually already commented there, at the time.

jsm222 · 2021-01-14T20:50:51Z

What I'm thinking about is what is described in #2051. I just noticed you actually already commented there, at the time.

I get the following errors trying to rtp_forward from videoroom:
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c7 --> v=1, level=71
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c8 --> v=1, level=72
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c9 --> v=1, level=73
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
But that might be an endian issue, I will look into it..

jsm222 · 2021-01-14T21:44:44Z

But that might be an endian issue, I will look into it..

specifyi

What I'm thinking about is what is described in #2051. I just noticed you actually already commented there, at the time.

I get the following errors trying to rtp_forward from videoroom:
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c7 --> v=1, level=71
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c8 --> v=1, level=72
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
[rtp.c:janus_rtp_header_extension_parse_audio_level:184] c9 --> v=1, level=73
Error forwarding RTP audio packet for laptop... Invalid argument (len=101)...
But that might be an endian issue, I will look into it..

    struct sockaddr *address = (rtp_forward->serv_addr.sin_family == AF_INET ?
                                        (struct sockaddr *)&rtp_forward->serv_addr : (struct sockaddr *)&rtp_forward->serv_addr6);
                                size_t addrlen = (rtp_forward->serv_addr.sin_family == AF_INET ? sizeof(rtp_forward->serv_addr) : sizeof(rtp_forward->serv_addr6));
                                if(sendto(participant->udp_sock, buf, len, 0, address, addrlen) < 0) {
                                        JANUS_LOG(LOG_HUGE, "Error forwarding RTP %s packet for %s... %s (len=%d)...\n",
                                                (video ? "video" : "audio"), participant->display, strerror(errno), len);
                                }

I can send to ipv6 but i do not manage to read from it..

jsm222 · 2021-01-14T22:24:04Z

I can send to ipv6 but i do not manage to read from it..

ffmeg can read it my vlc cannot.. well it works over ipv6 so issue #2051 applies.

lminiero · 2021-01-20T10:24:30Z

I get the following errors trying to rtp_forward from videoroom:

This may be because we always open an AF_INET6 socket when creating forwarders, even when we're only going to send to IPv4. This is because we only create a single socket per publisher, and we may have to send to different family address (e.g., RTP forward to both an IPv4 and an IPv6 address). An IPv6 socket with IPV6_V6ONLY disabled can send to both, on Linux, but apparently caused issues on MacOS, and maybe on FreeBSD too?

jsm222 · 2021-01-20T13:00:06Z

and maybe on FreeBSD too?

man inet6
Interaction between IPv4/v6 sockets
By default, FreeBSD does not route IPv4 traffic to AF_INET6 sockets. The
default behavior intentionally violates RFC2553 for security reasons.
Listen to two sockets if you want to accept both IPv4 and IPv6 traffic.

So yes.

lminiero · 2021-01-20T14:33:01Z

For the time being this feature should be considered unsupported in FreeBSD then. We don't plan to revisit the implementation until the multistream branch is merged.

jsm222 · 2021-01-20T16:17:41Z

I agree. Should it be added in README?

lminiero · 2021-01-20T16:19:15Z

That's probably a good idea, thanks!

lminiero · 2021-01-21T11:53:51Z

rtp.h

@@ -14,7 +14,7 @@
 #define JANUS_RTP_H

 #include <arpa/inet.h>
-#ifdef __MACH__
+#if defined (__MACH__) || defined(__FreeBSD__)


We have the same definition in pp-rtp.h, which is in the post-processor code. Have you checked if postprocessing MJR recordings does indeed work as expected on FreeBSD?

Sorry for the delay, I could not find the docs to acutally do a test, but I did change pp-rtp.h

jsm222 · 2021-02-14T18:57:54Z

I tested with /usr/local/bin/janus-pp-rec /usr/local/share/janus/recordings/rec-sample-audio.mjr test.opus

jsm222 · 2021-03-21T16:42:40Z

Hi, any reason not to merge this? Thanks :-)

lminiero · 2021-03-22T10:11:25Z

@jsm222 no reason beyond the fact it's a lot of changes that I need to review again, and test to make sure there's no regression, so entirely my fault, sorry. I've been very busy these past few weeks, I'll try to do that in the next few days. Thanks!

lminiero

Added some more small notes on the PR: nothing major, just small nits. I'll make some tests as well to see the parts that have changed the most still work (which I expect to do, since it's all happening in #ifdefs)

lminiero · 2021-03-22T16:58:28Z

README.md

@@ -207,6 +207,19 @@ If Doxygen and graphviz are available, the process can also build the documentat

 You can also selectively enable/disable other features (e.g., specific plugins you don't care about, or whether or not you want to build the recordings post-processor). Use the --help option when configuring for more info.

+### Building on FreeBSD
+* *Note*: rtp_forward of streams only works streaming to IPv6,
+because of [2051](https://github.com/meetecho/janus-gateway/issues/2051) and thus the feature


I think you can just put #2051 as text, and it should create a link to the issue automatically.

lminiero · 2021-03-22T16:59:07Z

README.md

+* *Note*: rtp_forward of streams only works streaming to IPv6,
+because of [2051](https://github.com/meetecho/janus-gateway/issues/2051) and thus the feature
+is not supported on FreeBSD at the moment
+


Small note: missing full stop at the end of the sentence? Also, extra empty line can be removed (I see two).

lminiero · 2021-03-22T17:01:36Z

README.md

+
+When building on FreeBSD you can install the depencencies from ports or packages, here only pkg method is used. You also need to use gmake instead of make,
+since it is a GNU makefile. ./configure can be run without arguments since the default prefix is /usr/local which is your default LOCALBASE.
+Note that the configure.ac is coded to use openssl in base. If you wish to use openssl from ports or any other ssl you must change configure.ac accordingly.


The ./configure, make, gmake, /usr/local, LOCALBASE and configure.ac texts should be in inverted quotes, to follow the style we use in the README.

lminiero · 2021-03-22T17:04:47Z

transports/janus_websockets.c

+			if (ipv4_only) {
+                            info.options |= LWS_SERVER_OPTION_DISABLE_IPV6;
+                            ipv4_only = 0;
+                        }


Indentation seems broken? Spaces are used, but we use tabs.

Add missing gengetopt to dependencies pkg install

lminiero · 2021-03-25T11:54:33Z

Thanks for the quick fixes! I'll make a couple of tests after lunch, and if all looks fine, I'll finally merge 👍

lminiero · 2021-03-25T15:59:10Z

I made a few checks and all seems to be working. Of course the actual checks should be made on FreeBSD, to ensure Janus does work there now, but I assume you've made all these tests already. This looks good to me, so I'll merge, thanks again!

lminiero · 2022-06-20T09:37:16Z

@jsm222 FYI, I got rid of the inet_net_pton check you added, to use inet_pton instead. Not sure why you chose the former in your patch initially, so in case it was for a specific reason that FreeBSD needed (and for which inet_pton is not enough) you may want to be aware of it.

FreeBSD support

d603b18

Try to correct configure.ac for all systems

57e365f

atoppi reviewed Jan 4, 2021

View reviewed changes

configure.ac Outdated Show resolved Hide resolved

janus.c Outdated Show resolved Hide resolved

transports/janus_websockets.c Outdated Show resolved Hide resolved

transports/janus_websockets.c Outdated Show resolved Hide resolved

transports/janus_websockets.c Outdated Show resolved Hide resolved

jsm222 added 2 commits January 4, 2021 22:25

Do not add libnice explictly

383fb2a

Add LWS_SERVER_OPTION_DISABLE_IPV6 only for FreeBSD.

Remove undefined ipv4_only

90b43c1

atoppi reviewed Jan 5, 2021

View reviewed changes

indentation style and removal of dlerror()

0ff09a2

atoppi reviewed Jan 5, 2021

View reviewed changes

Style empty lines

eb8d908

Code style add spaces

14a4673

lminiero reviewed Jan 13, 2021

View reviewed changes

Update README and fix code style

054eabc

Add note of unsupported rtp_forward on FreeBSD

23a7751

lminiero reviewed Jan 21, 2021

View reviewed changes

jsm222 added 2 commits January 28, 2021 14:03

Add __FreeBSD__ in pp-rtp.h

b5925e3

Merge branch 'master' into freebsd-support

46ad70b

Add missing defines for --enable-post-processing

4a24859

lminiero reviewed Mar 22, 2021

View reviewed changes

Fix style

401f621

Add missing gengetopt to dependencies pkg install

lminiero merged commit 7c9d522 into meetecho:master Mar 25, 2021

lminiero mentioned this pull request Nov 26, 2021

Cannot create socket for audio ... 97 (Address family not supported by protocol) #2821

Closed

		@@ -207,6 +207,30 @@ If Doxygen and graphviz are available, the process can also build the documentat

		You can also selectively enable/disable other features (e.g., specific plugins you don't care about, or whether or not you want to build the recordings post-processor). Use the --help option when configuring for more info.

		### Building on FreeBSD

FreeBSD support #2508

FreeBSD support #2508

Conversation

jsm222 commented Dec 28, 2020

januscla commented Dec 28, 2020

atoppi left a comment

Choose a reason for hiding this comment

atoppi left a comment • edited Loading

Choose a reason for hiding this comment

atoppi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsm222 commented Jan 6, 2021

atoppi commented Jan 6, 2021

lminiero commented Jan 6, 2021

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsm222 commented Jan 14, 2021

lminiero commented Jan 14, 2021

jsm222 commented Jan 14, 2021

jsm222 commented Jan 14, 2021

jsm222 commented Jan 14, 2021 • edited Loading

lminiero commented Jan 20, 2021 • edited Loading

jsm222 commented Jan 20, 2021

lminiero commented Jan 20, 2021

jsm222 commented Jan 20, 2021

lminiero commented Jan 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsm222 commented Feb 14, 2021

jsm222 commented Mar 21, 2021

lminiero commented Mar 22, 2021

lminiero left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lminiero commented Mar 25, 2021

lminiero commented Mar 25, 2021

lminiero commented Jun 20, 2022

atoppi left a comment •

edited

Loading

jsm222 commented Jan 14, 2021 •

edited

Loading

lminiero commented Jan 20, 2021 •

edited

Loading