Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

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

Can one of the admins verify this patch?

*/

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

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?

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?

Copy link
Member

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

@@ -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

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?

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 element-hq/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

@ara4n
Copy link
Member

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?

@t3chguy
Copy link
Member Author

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 != ''

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

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.

@ara4n ara4n assigned t3chguy and unassigned ara4n May 15, 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 commented May 25, 2017

NEEDS TESTING

/me has no webcam </3

@ara4n ara4n added the X-Release-Blocker This affects the current release cycle and must be solved for a release to happen label May 30, 2017
let webcamDropdown = <p>No Webcams detected</p>;

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

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) {

}

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

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) {

}

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

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

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

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

@ara4n
Copy link
Member

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

@ara4n ara4n assigned t3chguy and unassigned ara4n 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>
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

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?

t3chguy added 11 commits June 1, 2017 23:26
                               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 commented Jun 1, 2017

lgtm after epic :)

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

t3chguy commented Jun 1, 2017

I HATE WEBRTC

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Release-Blocker This affects the current release cycle and must be solved for a release to happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants