New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add windows support #597

Closed
wants to merge 25 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@marcin-sielski
Copy link
Contributor

marcin-sielski commented Aug 19, 2016

This PR adds Windows support to Janus WebRTC Gateway. It supports
Linux and OS X as well. This version is based on v0.1.1 release.
This PR was created to discuss the changes introduced to the package.
It will be updated to the latest master if no major issues are found.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Aug 21, 2016

Thanks for working on this! A quick glance showed an overwhelming number of changes... I'll try to review when I get back to the office in a week or so.

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Aug 21, 2016

Thank, you...

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Aug 22, 2016

@marcin-sielski what are you targeting, MinGW-w64?

Sofia-SIP (x86, x86_64):
mkdir mingw-w64-sofia-sip-git
cd mingw-w64-sofia-sip-git

This comment has been minimized.

@saghul

saghul Aug 22, 2016

Contributor

maybe just use git clone to simplify the steps?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 22, 2016

Contributor

Hi,
Well, Yes, Actually this is the shortest and the most comfortable way to have it compiled. Once binary packages are avaialble in pacman repository it would need simply run "pacman -S --noconfirm mingw-w64-i686-sofia-sip-git"
Hope that helps.
Best Regards

This comment has been minimized.

@saghul

saghul Aug 22, 2016

Contributor

Sure, but in the meantime adding git to the required packages, cloning the repo and running makepkg in there should do, right?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 22, 2016

Contributor

makepkg-mingw together with PKGBUILD does all the magic (it clones the repos, applies patches, compiles and installs binaries). Hope that helps.

janus.c Outdated
sessions_watchdog_context = NULL;
#ifdef _WIN32
gchar buf[MAX_PATH];
GetModuleFileName(NULL, buf, MAX_PATH);

This comment has been minimized.

@saghul

saghul Aug 22, 2016

Contributor

GetModuleFileNameW ?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 22, 2016

Contributor

Good point...

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 25, 2016

Contributor

Fixed

os.c Outdated
struct ifaddrs *p, *q;

for(p = ifp; p; ) {
if (p->ifa_name)

This comment has been minimized.

@saghul

saghul Aug 22, 2016

Contributor

style: alignment

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 22, 2016

Contributor

True. I will fix this...

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 25, 2016

Contributor

Fixed

@saghul

This comment has been minimized.

Copy link
Contributor

saghul commented Aug 22, 2016

I took the liberty of making a quick review :-) Awesome work @marcin-sielski!

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Aug 23, 2016

Thank you for the review

@@ -53,15 +59,21 @@ stream_DATA = $(NULL)
recordingsdir = $(datadir)/janus/recordings
recordings_DATA = $(NULL)

if WINDOWS_OS
INSTDIR = ../..

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Why this? Shouldn't this be decided by the prefix you pass when configuring? Or does this work differently with MingW on Windows?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

Hi Lorenzo, I hope you had great holidays. Thank you for your comment. This actually entire feature that enables to use relative paths within Janus code. It enables to create a package that is path independent so that sources can be compiled it in e.g. c:\msys64\mingw64 and binaries can be then installed in selected path by the user e.g. C:\Program Files\Janus WebRTC Gateway. I hope that helps.

This comment has been minimized.

@ploxiln

ploxiln Aug 29, 2016

Contributor

That doesn't entirely make sense ...

I regularly compile janus in /home/scratch/janus-gateway and install to /opt/janus. You mean, the user can change the install dir after compile time? That's already sort of possible, by specifying the config file location on the janus command line (and then the config file specifies all the other paths). But this doesn't remove the need to do that, you only use INSTDIR for the location to install the sample config files ...

Presumably, what you would want to do for a "windows installer", is specify minimal --prefix, --confdir etc, and use the existing DESTDIR feature to install to a temporary "package root" folder, then the installer can put that anywhere, and the last necessary step is to customize the config file and launcher shortcut, from the installer, if that's what you really want to do. (You might also want to enable janus to use a relative path to the main config file, and relative paths within it, but this doesn't do that either.)

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

Well, it does entirely make sense if you want to use 3rd party installer on Windows which is more common approach in that OS. I agree that paths could be changed after compilation but this make very limited use case on Windows. I also enabled relative path to the main config. Please refer to (https://github.com/marcin-sielski/janus-gateway/releases) binaries. Windows environment is by nature slightly diffrent then Linux. Hope that helps.

@@ -338,6 +407,7 @@ janus_pp_rec_CFLAGS = \

janus_pp_rec_LDADD = \
$(POST_PROCESSING_LIBS) \
libjanus.la \

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Why? The post-processor does not have the same dependencies as the Janus executable, most of the stuff there is not needed.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I have noticed that there are many dependencies in transports/plugins/post-processor on the functions located in janus.c. I created shared object now based on that file.

To stop Janus WebRTC Windows Service execute:
sc stop "Janus WebRTC Gateway"

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Incredibly detailed documentation! Maybe too much... 😄
I'm wondering if the installation stuff should belong to a different .md file instead, so that the README is not too long. Anyway, just thinking out loud, and it is something we can worry about later.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

Ok, I will leave as it is for now.

apierror.h Outdated
@@ -68,6 +70,6 @@
/*! \brief Helper method to get a string representation of an API error code
* @param[in] error The API error code
* @returns A string representation of the error code */
const char *janus_get_api_error(int error);
shared const char *janus_get_api_error(int error);

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

I've noticed you've added this shared pretty much everywhere. What is it for? Is it just an explicit way of saying a method is not static? Why is this needed on Windows? Is it safe to have for OSX and Linux? Just asking as I'm personally not familiar with it.

This comment has been minimized.

@saghul

saghul Aug 29, 2016

Contributor

@lminiero it's defined here: https://github.com/meetecho/janus-gateway/pull/597/files#diff-b8c9b43cc2d006d6edf7b73b4a54fe10R22 but I have to admit it also threw me off the first time. Usuallt this is called something like JANUS_API or JANUS_EXTERN. IMHO something like that reads better.

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Ah thanks the clarification, I hadn't arrived at that point in my review yet, still looking at the first part. I agree that something like you suggest would avoid ambiguities, and potential issues with dependencies that define the same thing themselves.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

Hi Lorenzo, "shared" is just #define defined in os.h
#ifdef _WIN32
#ifdef SHARED
#define shared __declspec(dllexport)
#else
#define shared __declspec(dllimport)
#endif
#else
#define shared
#endif
It is empty on other OSes (no affect). It is used to mark particular object whether it should be imported to dll or exported.

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Yep but as Saul pointed out, I'd use a different name for that. shared is too generic and risks to cause conflicts. Better something that explicitly belongs to the Janus namespace, e.g., JANUS_EXTERN or JANUS_API as Saul suggested, or something like that. Open to suggestions!

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

JANUS_SHARED?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Changed to JANUS_API

@@ -20,10 +20,20 @@ darwin*)
CFLAGS="$CFLAGS -I/usr/local/opt/openssl/include"
LDFLAGS="$LDFLAGS -L/usr/local/lib -L/usr/local/opt/openssl/lib -L/opt/local/lib"
AM_CONDITIONAL([DARWIN_OS], true)
AM_CONDITIONAL([WINDOWS_OS], false)

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Note: indentation.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I will fix that

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Fixed

*)
AM_CONDITIONAL([DARWIN_OS], false)
AM_CONDITIONAL([WINDOWS_OS], false)

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Note: indentation.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I will fix that

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Fixed

AM_CONDITIONAL([DARWIN_OS], false)
AM_CONDITIONAL([WINDOWS_OS], true)
AM_CONDITIONAL([HAS_DTLS_WINDOW_SIZE], true)
;;

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Note: indentation.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I will fix that

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Fixed

ice.c Outdated
#ifdef _WIN32
u_long argp = 1;
ioctlsocket(blackhole, FIONBIO, &argp);
#else

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

This is one of the things that frighten me, I'll most definitely break those anytime I add something sockety myself 😉

This comment has been minimized.

@saghul

saghul Aug 29, 2016

Contributor

You could probably create an internal function which sets the given fd in non-blocking mode, having it take care of the internals.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I will fix that

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Fixed

janus.c Outdated
static janus_config *config = NULL;
static char *config_file = NULL;
static char *configs_folder = NULL;
shared janus_config *config;

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Now this confuses me... why are these static values defined as shared instead? In this case, this would not result in them being static anymore on Linux.

This comment has been minimized.

@marcin-sielski

marcin-sielski Oct 20, 2016

Contributor

I have fixed that


#endif

#endif

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Missing newline here?

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

I will fix that

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 30, 2016

Contributor

Fixed

@@ -78,7 +76,11 @@ int janus_pp_h264_create(char *destination) {
vStream->time_base = (AVRational){1, 90000};
vStream->codec->width = max_width;
vStream->codec->height = max_height;
#if LIBAVCODEC_VER_AT_LEAST(53, 0)
vStream->codec->pix_fmt = AV_PIX_FMT_YUV420P;
#else

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

This looks like a generic fix we can add right away, I'll add it to master myself.

This comment has been minimized.

@marcin-sielski

marcin-sielski Aug 29, 2016

Contributor

There are many generic fixes there. This is one of the exmaple. But without it I coud not compile it on Windows

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Yep, that's why I was thinking that some of the smaller generic fixes we might merge right away, either as a smaller PR or by applying them directly to the code, without waiting for this larger PR to be merged. This particular PR will need some more time, especially considering that as anticipated we have a couple of other PRs still waiting to be merged that may need a bit of refactoring in your effort too.

@@ -99,7 +100,11 @@ int janus_pp_webm_create(char *destination, int vp8) {
vStream->codec->time_base = (AVRational){1, fps};
vStream->codec->width = max_width;
vStream->codec->height = max_height;
#if LIBAVCODEC_VER_AT_LEAST(53, 0)
vStream->codec->pix_fmt = AV_PIX_FMT_YUV420P;
#else

This comment has been minimized.

@lminiero

lminiero Aug 29, 2016

Member

Same as above.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Aug 29, 2016

First quick review done, tremendous effort, thanks for this!

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Aug 29, 2016

Thanks Lorenzo!!!

@omichel

This comment has been minimized.

Copy link

omichel commented Sep 12, 2016

I am very interested by this work as I am trying to setup an open source cross-platform WebRTC server (Linux, Windows and Mac OS X) that broadcasts video from an application producing images (OpenGL rendering). So far, I investigated OpenWebRTC and Janus (both combined with gstreamer) to achieve this goal, but none of them seemed to be well supported on Windows. Thanks to this PR, it seems Janus should now work on Windows... 😄 @marcin-sielski: I saw you also tried to compile OpenWebRTC on Windows, so I was wondering why you did this effort for Janus. Is Janus a better horse for setting-up such a cross-platform server?

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Sep 12, 2016

Janus Gateway is definetly server side software. You could write Janus plugin (e.g. based on Streaming Plugin) using GStreamer Mutimedia framework to create full multimedia Server that takes as an input anything supported by GStreamer and have WebRTC output. OpenWebRTC works better for client side. Hope that helps. Anyway you may try Windows version of Janus published at https://github.com/marcin-sielski/janus-gateway/releases. Be aware that OpenWebRTC uses different Congestion Control method then Google stack. Kurento might also fit your needs but Windows version is not currently officialy supported. Hope that helps.

@radioman

This comment has been minimized.

Copy link

radioman commented Oct 16, 2016

excellent effort! I wonder if visual studio would work?

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Oct 16, 2016

Hi,

This is the first round of bringing WebRTC server software to Windows world.
The dependencies are always problematic. Some of them can be already compiled using Visual
Studio compiler. I believe that this effort would significantly simplify second round - compile Janus with Visual Studio compiler.

I hope that helps

Best Regards

Marcin Sielski

@blackpan2

This comment has been minimized.

Copy link

blackpan2 commented Nov 15, 2016

@marcin-sielski I am using the tagged release v0.2.0 (b543884), however when doing janus.exe -v I get janus 0.1.1 as the response. Is this is a bug or are we still working from v0.1.1 in the windows version? .

Thank you for bringing this to windows.

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Nov 15, 2016

@blackpan2 Thank you for catching that. I have rebuilt Gateway from scratch. Should be fine now. Issue was related to the built itself and not to the source code.

@blackpan2

This comment has been minimized.

Copy link

blackpan2 commented Nov 15, 2016

I just wanted to confirm, especially since it was saving v0.2.0 on startup. Thanks again for fixing it and integrating.

@abraiante

This comment has been minimized.

Copy link

abraiante commented Mar 7, 2017

It looks like this pull request is broken now. @marcin-sielski, any chance this will be worked on again so we can build janus in windows using master?

@marcin-sielski

This comment has been minimized.

Copy link
Contributor

marcin-sielski commented Mar 8, 2017

@abraiante Sure, I would do it but I would like to have confirmation first from @lminiero when these changes could to be merged to the main line.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Mar 8, 2017

As I said at the time, the main concern I have is maintaining all this. I don't have ways to compile the Windows code too (and I personally don't intend to set up a Windows environment), and I can't possibly ensure I don't break anything when I work on new features. Worrying about that could take me time I don't have and I'd rather dedicate to coding and fixing things.

If you can guarantee the changes will be manageable, no issue from me. You may want to wait a bit before taking on this effort, though, as in the next days I'll merge #784 which changes quite a bit in the SDP code and some plugins, and #799 will probably follow shortly after. Besides, it might be a good idea to base it on #403 instead of master, as that will be the next major merge after that (ideally by April, or May at the latest), and so you'd definitely be some steps ahead.

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Apr 4, 2018

Closing as a long time passed and way too many things changed in the meanwhile. An updated contribution as a new PR would be of course welcome. Thanks for this huge effort!

@lminiero lminiero closed this Apr 4, 2018

@lminiero

This comment has been minimized.

Copy link
Member

lminiero commented Apr 27, 2018

For those still interested in Windows support, it looks like Janus works nicely in WSL: https://groups.google.com/forum/#!topic/meetecho-janus/6txdf8cyrns

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment