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 config electron #850

Merged
merged 21 commits into from Jun 1, 2017
Merged

webrtc config electron #850

merged 21 commits into from Jun 1, 2017

Conversation

@t3chguy
Copy link
Member

@t3chguy t3chguy commented Apr 28, 2017

needs testing with webcam

allows configurabilty of input devices used for webrtc calls
depends on matrix-org/matrix-js-sdk#427

init on LoggedInView mounting
configurable via UserSettings
new class: CallMediaHandler

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@matrixbot
Copy link
Member

@matrixbot matrixbot commented Apr 28, 2017

Can one of the admins verify this patch?

Loading

*/

import UserSettingsStore from './UserSettingsStore';
import * as Matrix from 'matrix-js-sdk';
Copy link
Member

@ara4n ara4n Apr 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this feels a bit weird - shouldn't this be operating on a MatrixClient rather than importing the whole js-sdk as Matrix?

Loading

Copy link
Member Author

@t3chguy t3chguy Apr 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ara4n it's in the same class as createNewMatrixCall is in, which looking at VectorConferenceHandler is used in the same manner. Just following the patterns I see. The other way to do it would be to pass the devices when initiating/answering a call, would you prefer this?

Loading

Copy link
Member

@ara4n ara4n May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, ok. i guess this is ok

Loading

@@ -71,6 +72,10 @@ export default React.createClass({
// RoomView.getScrollState()
this._scrollStateMap = {};

// Only run these in electron, at least until a better mechanism for perms exists
// https://w3c.github.io/permissions/#dom-permissionname-device-info
if (window && window.process && window.process.type) CallMediaHandler.loadDevices();
Copy link
Member

@ara4n ara4n Apr 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, so the picker only works when in electron?

Loading

Copy link
Member Author

@t3chguy t3chguy Apr 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is to fix vector-im/element-web#2822 primarily so that's what its designed for, I did no testing in a browser but should work given microphone+camera permissions at browser level

Loading

@ara4n
Copy link
Member

@ara4n ara4n commented Apr 29, 2017

this generally looks good to me, other than questions, although it's a bit of a shame it only works for electron. (plus i need to test it; i can do so against an external UVC webcam).

any idea what's needed on the permissions front to make it work on the browser?

Loading

@t3chguy
Copy link
Member Author

@t3chguy t3chguy commented Apr 29, 2017

we could run it on the browser but if the device-info permission is missing then all the devices have an empty label which makes it pointless having it as its impossible to know which device is which.
the W3C spec seems to be including a device-info prop within the Permissions API so once that hits we could base it on that but it didn't work in Electron && Chrome when I tested it

I guess I could remove that check and instead see whether the returned labels != ''

Loading

@t3chguy t3chguy assigned ara4n and t3chguy and unassigned t3chguy May 5, 2017
@ara4n
Copy link
Member

@ara4n ara4n commented May 15, 2017

yeah, i think this would be very useful (and easier to test, critically) for non-electron too. I suggest doing the returned labels != '' check, and then i can test & merge.

Loading

@ara4n ara4n assigned t3chguy and unassigned ara4n May 15, 2017
t3chguy added 3 commits May 24, 2017
loadDevices not only in electron

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@t3chguy
Copy link
Member Author

@t3chguy t3chguy commented May 25, 2017

NEEDS TESTING

/me has no webcam </3

Loading

let webcamDropdown = <p>No Webcams detected</p>;

const audioInputs = this.state.mediaDevices.audioinput;
if ('default' in audioInputs) {
Copy link
Member

@ara4n ara4n Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs if (Object.keys(audioInputs).length > 0) {

Loading

}

const videoInputs = this.state.mediaDevices.videoinput;
if ('default' in videoInputs) {
Copy link
Member

@ara4n ara4n Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs if (Object.keys(videoInputs).length > 0) {

Loading

}

return <div>
<h3>WebRTC</h3>
Copy link
Member

@ara4n ara4n Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call it VoIP, and i18n please

Loading

_renderWebRtcSettings: function() {
if (this.state.mediaDevices === false) {
return <div>
<h3>WebRTC</h3>
Copy link
Member

@ara4n ara4n Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's call it VoIP. needs i18n

Loading

@ara4n
Copy link
Member

@ara4n ara4n commented Jun 1, 2017

@t3chguy if you can i18nize, fix the merge conflict & and fix 'defaults' bug then i'll merge this immediately. thanks

Loading

@ara4n ara4n assigned t3chguy and unassigned ara4n Jun 1, 2017
t3chguy added 2 commits Jun 1, 2017
… webrtc_settings

and i18nize webrtc stufffs

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

# Conflicts:
#	src/components/structures/UserSettings.js
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
t3chguy added 2 commits Jun 1, 2017
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
this.setState({
mediaDevices,
activeAudioInput: this._localSettings['webrtc_audioinput'] || 'default',
activeVideoInput: this._localSettings['webrtc_videoinput'] || 'default',
Copy link
Member Author

@t3chguy t3chguy Jun 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do these need changing also?

Loading

t3chguy added 11 commits Jun 1, 2017
                               so that we can set falsey values, for unsetting device
                               most dolphinately needs testing

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>
@ara4n
Copy link
Member

@ara4n ara4n commented Jun 1, 2017

lgtm after epic :)

Loading

@ara4n ara4n merged commit ec43390 into matrix-org:develop Jun 1, 2017
1 check passed
Loading
@t3chguy
Copy link
Member Author

@t3chguy t3chguy commented Jun 1, 2017

I HATE WEBRTC

Loading

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