Skip to content
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

Webrtc win #69

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Webrtc win #69

wants to merge 13 commits into from

Conversation

asicerik
Copy link

@asicerik asicerik commented Oct 2, 2017

Hey there! We needed a version of this to run under Windows, so I put in all the work to get DLL linkage in. Sooner or later, hopefully Google or MSFT will get their act together and get Go / CGO to work properly in Windows using Visual Studio.

The DLL approach is not the cleanest since we have to do: Go -> CGO -> Go -> DLL. I did not want to change your Go code to be able to access the DLL directly, so there is an extra layer of CGO there that we don't need. If we put an abstraction layer in the Go code, we could get rid of the CGO layer for Windows, but it introduces another layer in the POSIX code. So, I don't recommend that.

We are going to be working on Video and Audio with this library, so I hope to be able to continue to contribute to the project. If you are not happy with something here, let me know how you would like to see it done.

I also made some changes to the test programs. It seems the enums were backwards w.r.t. expected vs. actual.

The DLL build in Visual Studio right now, but we could move that to a command line system if you wish.
Thanks!
Erik

@arlolra
Copy link
Collaborator

arlolra commented Oct 11, 2017

Thanks for the pull. Haven't had a chance to look at it yet, but a few quick things.

We needed a version of this to run under Windows, so I put in all the work to get DLL linkage in.

Have you seen golang/go#11058

We are going to be working on Video and Audio with this library, so I hope to be able to continue to contribute to the project.

See #65, which also needs review / merge.

@asicerik
Copy link
Author

asicerik commented Oct 12, 2017 via email

@arlolra
Copy link
Collaborator

arlolra commented Oct 12, 2017

I suppose that path could work too, but it turns everything upside down from the Linux/etc builds. I will have to ponder that.

Sorry, I was confused with another project. It's not really relevant. Please ignore.

Are you guys planning on merging that in? It was submitted in May.

Yes, I was planning on reviewing it shortly. As apparent, we don't always have a ton of cycles available.

@blizzardplus
Copy link

@asicerik: In the latest commit, build_win.bat is removed, so how can I reproduce the build for webrtc.dll?
Was it removed because it breaks the Linux build?
Also, we should not have the headers in the "include" folder in git. It should be generated as part of the build process.

@asicerik
Copy link
Author

@blizzardplus - thanks for getting back to me. That file should not have disappeared, I will look into it. On the include folder, I thought that was already there? The cgo files have to reference the include files if you use the pre-build binaries (for Linix or Win).
I pulled in the audio changes already that were merged in yesterday, and I am adding video. Let me get all that done, and I will put in a new pull request. Does that make sense?

@blizzardplus
Copy link

blizzardplus commented Oct 19, 2017

@asicerik We would like to have everything reproducible, so it means that the windows build script should generate the DLL/LIB and include headers from the source code. Take a look at build.sh:

echo "Copying headers ..."
pushd $WEBRTC_SRC || exit 1
rm -rf "$INCLUDE_DIR"
for h in $(find webrtc/ -type f -name '*.h')
do
mkdir -p "$INCLUDE_DIR/$(dirname $h)"
cp $h "$INCLUDE_DIR/$h"
done
popd

@asicerik
Copy link
Author

go ahead and reject this pull request. I will another one in a few days that includes audio and video support. Is there anyone on your side that can assist with the build script? I am not very good script writer :)

@blizzardplus
Copy link

blizzardplus commented Oct 20, 2017

@asicerik As long as you have clear instructions on how to build the DLL, I should be able to help you write the script, or can probably write it myself. And by instructions, I mean what steps that have to be taken to create the DLL and what process looks like. Also, we have to make sure not to break the Linux build. Thanks for your help.

@asicerik
Copy link
Author

The dll currently has a Visual Studio Solution for it. You can't run it from the command line, but it is easy to build. Do you need a command line version for the dll? It can be treated like the checked in binaries for the Linux/Mac builds where you don't have to build the dll to run the Windows version.
Linux support is fully maintained. I have not tried building Mac since I don't have a Mac. I have no reason to expect Mac should fail though.

@arlolra
Copy link
Collaborator

arlolra commented Oct 20, 2017

The checked in binaries are only for convenience. In practice, we build from source when including them as part of a product.

See https://gitweb.torproject.org/builders/tor-browser-build.git/tree/projects/webrtc

@asicerik
Copy link
Author

Is project being used as part of Tor? That would be cool.
I understand about the binaries. Are you going to have a Windows build server?

@arlolra
Copy link
Collaborator

arlolra commented Oct 20, 2017

Is project being used as part of Tor? That would be cool.

Yes, it's used in a pluggable transport,
https://trac.torproject.org/projects/tor/wiki/doc/Snowflake

I understand about the binaries. Are you going to have a Windows build server?

We cross-compile with Mingw-w64, http://mingw-w64.org/doku.php

@Zentendo
Copy link

Zentendo commented Mar 2, 2018

Any plans to continue working on this? @asicerik

Currently using this fork in production.

@asicerik
Copy link
Author

asicerik commented Mar 2, 2018

We are currently using it in our application. We are working on getting Mac support to work right now. Once that is done, I want to pull in all the changes from here, and re-push

@arlolra
Copy link
Collaborator

arlolra commented Mar 3, 2018

From our perspective, @blizzardplus has gotten our app to build with the visual studio project here, but we're still working on cross-compilation.

Before merging though, it'd be nice to automate generating some of the redundant parts of this patch so that there's less of a maintenance burden as the code evolves.

@blizzardplus
Copy link

@asicerik any idea on how to maintain the code moving forward, there seems to be a lot of dependencies between the DLL wrapper and the actual code, so wondering if there's a way to automate or at least reduce the maintenance overhead.

@Zentendo
Copy link

Zentendo commented Apr 1, 2018

@asicerik Is there any way to pass options into CreateDataChannel in this fork?

Currently I can't seem to set unreliable/UDP options on the data channel (which is the primary reason to use WebRTC) on windows like the latest branch of keroserene/go-webrtc on linux. It just creates a reliable channel by default regardless of the webrtc.Init{} options I pass into it.

@asicerik
Copy link
Author

asicerik commented Apr 1, 2018

Hey all,
It seems like the original project has a way to cross-compile into Windows now. You might want to check that out. If that does not work for you, let me know.
#79

@Zentendo
Copy link

Zentendo commented Apr 27, 2018

@asicerik Windows support appears not to be coming for a while: #83

It would be great if you could update these bindings to allow unreliable / UDP messaging as a temporary solution in the meanwhile.

@asicerik
Copy link
Author

I will see what I can do to get the project synced up, etc. I am pretty busy with work, so we'll see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants