HenrikJoreteg / webrtcsupport

Browser module to detect support for WebRTC and extract proper constructors.
55 stars 33 forks source link

Add tests for VP8 and getUserMedia support #6

Closed dagingaa closed 9 years ago

dagingaa commented 9 years ago

This change adds tests for VP8 and getUserMedia support to the testing suite.

The rationale for doing this is that the basic test for WebRTC support today only tests for the presence of RTCPeerConnection. However, basic interoperable support for WebRTC today requires VP8 and getUserMedia too, so I figured I should add some code to test for that as well. Not included are tests for NAT traversal which we have over at http://iswebrtcready.appear.in, because this triggers a call to Google's STUN server, which you may not want. I can add this too if you want. Otherwise, refer to each commit message separately for more information.

I also tried to establish a convention of using support* for booleans that denote support for a specific feature. On request from @fippo I have made breaking changes to the API for this, so the version should be bumped.

The bundle is not updated, but I ran the build script manually locally and the tests passed/failed as expected in respective browsers. If desired, I can update the bundle and package number as well before you merge this.

fippo commented 9 years ago

The test for VP8 support is neat, I had expected that you'd create a peerconnection, call create offer and look at the codecs but... @juberti @samdutton maybe steal this for testrtc? :-)

I'll update the bundle after merging (some time next week probably), I have some other changes pending anyway. Some of those changes might be a breaking anyway so +1 on establishing a support* convention.

I'm not sure if I really want getUserMedia here... the purpose of this module is to check support, not be a shim like adapter. But then, MediaStream is in here too. So future versions might get rid of that with another major version bump :-)

Thanks!

dagingaa commented 9 years ago

The test for VP8 support is neat, I had expected that you'd create a peerconnection, call create offer and look at the codecs but... @juberti @samdutton maybe steal this for testrtc? :-)

That seems more robust. I'll try looking into doing that instead. It' a good point that VP8 support in the browser for the video element is not necessarily connected to support in WebRTC, which this library is originally meant to test.

I'm not sure if I really want getUserMedia here... the purpose of this module is to check support, not be a shim like adapter.

I agree with that, it looks a lot like a shim at the moment exposing RTCPeerConnection and others. I'll leave it up to you to decide if you want it or not, I'm open to removing it. I would like the supportGetUserMedia field though, or rather, I feel that it should be included in WebRTC support testing as the usage of WebRTC is today.

Do you want me to add the non-vendor prefixed version of RTCPeerConnection as well to the test? As far as I know nothing currently uses that, but eventually most browsers will hopefully remove the vendor prefix.

Also, keep up the good work! I was originally looking at creating and publishing something similar, but I would much rather contribute here.

fippo commented 9 years ago

That seems more robust.

It is, but requires an async call which would make the information available only after ~100ms (since createOffer takes some time, mostly for generating DTLS keys/certificates). In the future, RtpSender.getCapabilities will provide that information so i'd leave it as is for now.

Do you want me to add the non-vendor prefixed version of RTCPeerConnection as well to the test?

@stefhak maybe that's the reason talky fails in bowser? (re https://github.com/webrtcftw/iswebrtcreadyyet.com/pull/38) Unfortunately bowser crashes on all ios devices I have access to so I can't test that :-/

fippo commented 9 years ago

note to self: add something that checks for MediaStreamTrack.getSources and/or whatever the new name of that thing is.

dagingaa commented 9 years ago

Alright, I've cleaned up and made support* the syntax for boolean tests. I think this is ready for merge unless you have anything else? :)