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

Split "room" from "call" when using the standalone signaling server. #492

Merged
merged 8 commits into from
Feb 2, 2018

Conversation

fancycode
Copy link
Member

This is a follow-up to #459

js/webrtc.js Outdated
@@ -146,6 +143,8 @@ var spreedPeerConnectionTable = [];
});

if (!selfInCall) {
// Own session is no longer in the call, disconnect from all others.
usersChanged([], previousUsersInRoom);
Copy link
Member

Choose a reason for hiding this comment

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

opsy

lib/Room.php Outdated
* @param string $sessionId
* @return bool
*/
public function updateUserSessionId($userId, $sessionId) {
Copy link
Member

Choose a reason for hiding this comment

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

Please check that the user id is not null (guest)

// TODO(fancycode): This will remove the user from other rooms
// he is currently in. Needs to get communicated to the signaling
// server.
$room->updateUserSessionId($userId, $sessionId);
Copy link
Member

Choose a reason for hiding this comment

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

Why all these changes? The session id does not change when joining/leaving a room

try {
$room = $this->manager->getRoomForParticipantByToken($roomId, $userId);
$room = $this->manager->getRoomByToken($roomId);
Copy link
Member

Choose a reason for hiding this comment

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

I think the only thing that needed a change was to make sure user id is null instead of empty string for guests?

@nickvergessen nickvergessen added this to the 3.0 (Nextcloud 13.0.0) milestone Nov 20, 2017
@nickvergessen nickvergessen added 2. developing bug feature: signaling 📶 Internal and external signaling backends labels Nov 20, 2017
@fancycode
Copy link
Member Author

Simplified the code: clients through the standalone signaling server are using the Nextcloud session id as with the internal signaling. The ids get "pinged" by the signaling server to make sure sessions stay active.

@fancycode
Copy link
Member Author

Nice benefit, this automatically added support for password-protected rooms with the standalone signaling server 🎉

@fancycode
Copy link
Member Author

@nickvergessen ptal

@nickvergessen nickvergessen modified the milestones: 3.0 (Nextcloud 13.0.0), 3.1 (Nextcloud 13.0.2/3) Jan 8, 2018
@nickvergessen nickvergessen changed the base branch from master to stable13 January 15, 2018 13:48
@fancycode fancycode force-pushed the split_room_call_signaling branch 4 times, most recently from 43ba0e8 to 9c0be57 Compare January 29, 2018 14:47
@fancycode
Copy link
Member Author

Rebased and squashed commits - ptal.

I will now be looking at writing some tests for the new backend code.

* @return string
*/
protected function getInputStream() {
return file_get_contents('php://input');
Copy link
Member

Choose a reason for hiding this comment

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

IRequest::getContent should do the same

Copy link
Member Author

Choose a reason for hiding this comment

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

Doesn't IRequest::getContent already parse the contents if JSON is sent? I need the raw body to verify the checksum.

@fancycode fancycode force-pushed the split_room_call_signaling branch 2 times, most recently from 8600dcb to e9121c9 Compare January 30, 2018 12:42
try {
$room = $this->manager->getRoomForParticipantByToken($roomId, $userId);
$room = $this->manager->getRoomByToken($roomId);
} catch (RoomNotFoundException $e) {
return new JSONResponse([
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a OCSController now, you should return DataResponse only.
In your client you can set the accept header or append format=json to get json:
https://docs.nextcloud.com/server/12/developer_manual/core/externalapi.html#output

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed

@fancycode fancycode force-pushed the split_room_call_signaling branch from 925054b to b0fee66 Compare February 2, 2018 09:33
@fancycode fancycode changed the base branch from stable13 to master February 2, 2018 09:34
@fancycode
Copy link
Member Author

Rebased to master.

Signed-off-by: Joachim Bauch <bauch@struktur.de>
Clients use the regular joinRoom/-Call API and get a Nextcloud session
id. No special handling for sessions from the standalone signaling
server are required.

The signaling server regularly "pings" active sessions to prevent them
from timing out (in case of guest users).

Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
@nickvergessen nickvergessen force-pushed the split_room_call_signaling branch from b0fee66 to e3ef4d2 Compare February 2, 2018 10:43
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Works now, rebased to hopefully fix tests

@nickvergessen nickvergessen merged commit 362f6f9 into master Feb 2, 2018
@nickvergessen nickvergessen deleted the split_room_call_signaling branch February 2, 2018 10:51
@nickvergessen nickvergessen modified the milestones: 3.1 (Nextcloud 13.0.2/3), 4.0 (Nextcloud 14) Feb 2, 2018
marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019
Bump @babel/preset-env from 7.5.4 to 7.5.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review bug feature: signaling 📶 Internal and external signaling backends
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants