search

otalk / getUserMedia

Cross-browser getUserMedia shim with a node.js style error-first API
233 stars 43 forks source link

why use webrtc-adapter-test? #18

Closed yocontra closed 8 years ago

yocontra commented 9 years ago

We had to switch to a different getusermedia library because webrtc-adapter-test is blowing up on anything that isn't chrome or firefox + mutating a bunch of globals. What's the reason for this library using webrtc-adapter-test?

calebboyd commented 9 years ago

I have experienced the same issues but am able to run a fork using jspm...

xdumaine commented 9 years ago

Can you elaborate on what "blowing up" means?

KaptenJansson commented 9 years ago

I assume this https://github.com/webrtc/adapter/issues/122

fippo commented 9 years ago

the reasons for blowing up should be gone with webrtc-adapter-test 0.2.3 (thanks @KaptenJansson)

The reasons for using this is that I am not interested in writing or maintaining my own shim (in particular stuff like @jan-ivar's constraints transform). The road has been a little more bumpy than I expected but...

fippo commented 9 years ago

oh wait, this still pins adapter to 0.1.9 to avoid the double-include bug in 0.2.1. (now bumped to 0.2.3)

fippo commented 9 years ago

uhm well.. I think the comment about the reasons being gone was too early -- another reason for blowing up is done at least.

yocontra commented 9 years ago

This is what's problematic:

https://github.com/webrtc/adapter/blob/master/adapter.js#L100

Why is a module that should only wrap getUserMedia defining a bunch of stuff on the window object and mutating globals that have nothing to do with getUserMedia? This makes it totally incompatible with any other webrtc tooling (temasys, iosrtc, etc.)

There's absolutely no reason that a module called getUserMedia should be messing with RTCPeerConnection, browser user agent detection for data channels, etc.

yocontra commented 9 years ago

I've opened a related ticket here https://github.com/webrtc/adapter/issues/125 but even if it quits mutating the globals, I don't understand why this getusermedia module needs to be using the adapter. Shouldn't it be the other way around?

fippo commented 9 years ago

go ahead and fix the issue that adapter started as a monolithic non-module three years ago. Good luck. Mind you that in modern browsers, 90% of the code in this module is no longer needed.

nfriedly commented 8 years ago

@contra you might be interested in https://www.npmjs.com/package/get-user-media-promise - It's a wrapper for the newer Promise-based navigator.mediaDevices.getUserMedia() API, and it's a much smaller, simpler library with no dependencies.

fippo commented 8 years ago

The problem solved by adapter is that Chrome still (half a year later) does not support the getusermedia constraints from the w3c spec and I have no desire to reinvent Jan-Ivar's code for this. And I have not seen anyone doing this in a way that is not a license violation.

yocontra commented 8 years ago

Maybe you should put a note in the readme that says this module only supports chrome and firefox so people don't get their hopes up.

KaptenJansson commented 8 years ago

To be clear this for ADAPTER.js.

That info can be found in the interop link on the README page but I do agree we can be more clear. I will add supported browsers to the README. Thanks for the suggestion!

It would also be useful if people could file an issue or submit a PR with a fix for the browsers they want it to work on.

fippo commented 8 years ago

@contra why don't you go play on your own lame copy of adapter? ktnxbye

yocontra commented 8 years ago

@KaptenJansson 100% agree on all points - I was open to submitting a PR fixing this but the response from the maintainer made it clear they were not interested in resolving it.

@fippo Lame copy of adapter? Not sure what you're talking about.

xdumaine commented 8 years ago

https://github.com/contra/rtc-everywhere/

Generally using a wrapper on a shim next to another shim is not going to go well. Suggest using one or the other, and if this one causes you issues, then just use your own.

fippo commented 8 years ago

@contra if commenting "not cool" is the way you offer a PR...

This module is a thing of the past. Both Firefox and Edge support navigator.mediaDevices.getUserMedia natively now, Chrome soon will. This removes one reason to use the module. The other reason was the callback-based API but navigator.mediaDevices.getUserMedia uses promises.

yocontra commented 8 years ago

@xdumaine How is rtc-everywhere a "lame copy" of the adapter? The projects aren't even comparable.

@fippo Why not note this in the README? All I'm asking for is either fix the library or mark it as deprecated. I don't understand why this issue is going back and forth, the fix is extremely simple and would end this in less than a minute or two.

There's no reason to get heated with me and start throwing personal insults calling my projects "lame copies" like a five year old - extremely unprofessional.

fippo commented 8 years ago

@contra: I am still looking for the concrete suggestion for a fix that fulfills constraints like being able to use spec gum constraints in browsers that don't support them.

README is not mentioning support for anything that is not a browser anywhere I can see, nor is package.json in the tags.

re lame: will file an issue over the issues I have with your project's README...

fippo commented 8 years ago

@contra I'd recommend not 'advising' people who maintain projects which are even remotely similar to your own unless invited to. In particular not making false pretences (Edge is supported here).

And check the timeline. You're making more false assertions.