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

Use a plugin hook to download the webrtc library #29

Closed
oNaiPs opened this issue Jul 6, 2015 · 13 comments
Closed

Use a plugin hook to download the webrtc library #29

oNaiPs opened this issue Jul 6, 2015 · 13 comments

Comments

@oNaiPs
Copy link
Contributor

oNaiPs commented Jul 6, 2015

Installing the plugin takes a while as the history of the webrtc library is kept on git, and it significantly increases the size of the initial plugin download. Would it make more sense to download the library before installing?

See
https://github.com/remotium/cordova-plugin-webrtc/blob/master/plugin.xml
https://github.com/remotium/cordova-plugin-webrtc/blob/master/scripts/pull_webrtc_library.js

@ibc
Copy link
Collaborator

ibc commented Jul 6, 2015

Interesting. However the "libwertc.so" included in the project is the static library of a customized version of the webrtc library. This is because the original Obj-C code lacks support for MediaStream events and I added it but it's not included in the upstream project:

https://webrtc-codereview.appspot.com/50109004/

Anyhow I will come back to this proposal once I come back from holidays ;)

@YasserRabee
Copy link

+1

I suggest to add the hook at pre_build

@ibc
Copy link
Collaborator

ibc commented Aug 13, 2015

I'll need some time before I can handle this issue. Please, patience.

@murillo128
Copy link
Contributor

PR are welcome!! ;)

@yocontra
Copy link

Got it all working but there is some custom code apparently in the version of WebRTC that this library uses that isn't in WebRTC upstream so it isn't able to build (missing RTCMediaStreamDelegate which was added by @ibc). Tried removing the pointers to this custom code to get it to build and hit more problems, so I'm going to try to figure out what @ibc added to this custom version of WebRTC so I can replicate it

https://github.com/contra/cordova-plugin-iosrtc

@ibc
Copy link
Collaborator

ibc commented Aug 27, 2015

We modified the Google's WebRTC ObjC code in order to implement and fix the missing "addtrack" and "removetrack" events of RTCMediaStream. The patch was proposed but Google did not accept it because the WebRTC spec is removing MediaStream tracks events:

https://webrtc-codereview.appspot.com/50109004/

@yocontra
Copy link

@ibc is that required for iosrtc to function or is it an enhancement?

@ibc
Copy link
Collaborator

ibc commented Aug 28, 2015

Please read the comments in the patch above. It fixes the MediaStream behavior so it is not just an enhancement.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jul 28, 2016

I would suggest as an intermediate solution, to use git LFS for the large file. The repository is getting really slow to clone...

@saghul
Copy link
Collaborator

saghul commented Jul 29, 2016

If you are cloning via git you can use --depth 1 to avoid getting the entire history. We now patch libWebRTC even more, so we can't use a generic build.

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Jul 30, 2016

@saghul that doesn't invalidate what I'm suggesting. Github itself warns about files bigger than 50MB. Did you see how git LFS works? This is sponsored by github.

https://git-lfs.github.com/

@saghul
Copy link
Collaborator

saghul commented Aug 1, 2016

I'm aware of LFS. This is not my project, but if it were, I wouldn't use LFS until it's absolutely needed. See the drawbacks here: https://medium.com/@megastep/github-s-large-file-storage-is-no-panacea-for-open-source-quite-the-opposite-12c0e16a9a91#.85wtfitph

I would have already run into the problem of not being able to PR the file stored on LFS (the current build was contributed by me in a PR).

@oNaiPs
Copy link
Contributor Author

oNaiPs commented Aug 1, 2016

@saghul you're right, it doesn't look like we should use it here.

@ibc ibc closed this as completed Nov 4, 2016
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

No branches or pull requests

6 participants