You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@openmeetings.apache.org by GitBox <gi...@apache.org> on 2020/04/22 23:20:59 UTC
[GitHub] [openmeetings] sebawagner opened a new pull request #65: OPENMEETINGS-2278 Add functionality to stop the sharing as part of re…
sebawagner opened a new pull request #65:
URL: https://github.com/apache/openmeetings/pull/65
OPENMEETINGS-2278
- Add functionality to stop the sharing as part of recording stop as it will stop the MediaStream on stop of recording.
- This also clears the KStream that goes ghost and enables restarting the recording.
- Some further work required on the UI to prevent starting to screenshare while recording potentially required.
This is a first Draft, please allow me to explain some of the changes.
And if agreed I can try to add some unit testing. (Or at least figure out if that is possible)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413468734
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
It's not me :(
here is the example: https://issues.apache.org/jira/browse/OPENMEETINGS-2253
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413449594
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
Recording is `virtual` event, it is only flag on current stream
we need to be able to set/unset this flag any time, and the stream will not be affected
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r414872263
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -373,6 +373,8 @@ private void stopSharing(Client c, String uid) {
StreamDesc sd = doStopSharing(c.getSid(), uid);
if (sender != null && sd != null) {
sender.stopBroadcast(this);
+ } else {
Review comment:
this usually happens on `double-stop` are you sure it will produce ghosts?
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -384,17 +386,26 @@ public void connected() {
notifyRooms();
}
}
+
+ private class KWatchDogDestroy implements EventListener<ObjectDestroyedEvent> {
Review comment:
This one seems to be `LogDog` :)))
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KRoom.java
##########
@@ -237,7 +237,7 @@ public void startSharing(StreamProcessor processor, IClientManager cm, Client c,
sd = c.addStream(StreamType.SCREEN, a);
sd.setWidth(msg.getInt("width")).setHeight(msg.getInt("height"));
cm.update(c);
- log.debug("User {}: has started sharing", sd.getUid());
+ log.debug("User {}: has started sharing, with stream: {}", sd.getUid(), a);
Review comment:
I Would change this message to be `Stream {}: sharing has been started, activity: {}`
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -431,6 +433,10 @@ public void stopRecording(Client c) {
return;
}
kHandler.getRoom(c.getRoomId()).stopRecording(this, c);
+ Optional<StreamDesc> osd = c.getScreenStream();
Review comment:
I would write it using modern syntax:
```
c.getScreenStream().ifPresent(sd -> {
if (!sd.hasActivity(Activity.SCREEN)) {
pauseSharing(c, sd.getUid());
}
});
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-619254249
> If you're happy with the PR, I start thinking of modifying some of the tests in
TestRoomFlowMocked to see how to verify it.
please do not use tests from `TestRoomFlowMocked` these are for audio/video setup dialog
please create separate class (common code can be unified later, if any)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on issue #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on issue #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-618125240
I would say `If stopping the recording it will also stop the sharing of the screen` is not the best option
I would write some tests and try to avoid this
it might me extremely annoying is all participants in the room will have to re-open screen-sharing just because recording has been stopped
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 Add functionality to stop the sharing as part of re…
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413399164
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -431,6 +433,10 @@ public void stopRecording(Client c) {
return;
}
kHandler.getRoom(c.getRoomId()).stopRecording(this, c);
+ Optional<StreamDesc> osd = c.getScreenStream();
Review comment:
In face as seen by the Destroy events the stopRecord actually stops the MediaStream. So stopping the Stream makes sense.
Also this solves the problem to not have any KStream ghost connections AND you can restart the recording.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-619247237
@sebawagner sorry for the long review, I'm on vacation so trying to relax :)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner edited a comment on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner edited a comment on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-618853159
> I would say `If stopping the recording it will also stop the sharing of the screen` is not the best option
=> That is fixed now. It won't stop the sharing - except - sharing wasn't started. Then it will stop the stream.
If you start however the sharing _while_ recording then sharing will continue.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 Add functionality to stop the sharing as part of re…
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413398620
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
I think the destroy events are very useful in terms of detecting that the stream actually closed. So from a debug perspective this is very useful.
And I think we can also continue to use the destroy event to detect a KStream is gone. Rather then just rely on browser events.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r415148044
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -373,6 +373,8 @@ private void stopSharing(Client c, String uid) {
StreamDesc sd = doStopSharing(c.getSid(), uid);
if (sender != null && sd != null) {
sender.stopBroadcast(this);
+ } else {
Review comment:
The stop doesn't directly. But it does indirectly. Cause when its stopped with above JavaScript/client side logic on startingRecording again, some stream object isn't 100% cleared. Which then leads to a ghost KStream and a failed restart of the recording.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413446020
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
Well when stopping to recording tis won't trigger. And when, after stopping to record, leave the room or exit the browser, it won't seem to trigger either.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413453821
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
you are demanding 😄
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
let me search further
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413399164
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -431,6 +433,10 @@ public void stopRecording(Client c) {
return;
}
kHandler.getRoom(c.getRoomId()).stopRecording(this, c);
+ Optional<StreamDesc> osd = c.getScreenStream();
Review comment:
It seems the Destroy events the stopRecord actually stops the MediaStream. So stopping the Stream makes sense.
Also this solves the problem to not have any KStream ghost connections AND you can restart the recording.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-619251117
Thanks! Take care!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r415148044
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -373,6 +373,8 @@ private void stopSharing(Client c, String uid) {
StreamDesc sd = doStopSharing(c.getSid(), uid);
if (sender != null && sd != null) {
sender.stopBroadcast(this);
+ } else {
Review comment:
The stop doesn't directly. But it does indirectly. Cause when its stopped like that on startingRecording again, some stream object isn't 100% cleared. Which then leads to a ghost KStream and a failed restart of the recording.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on a change in pull request #65: OPENMEETINGS-2278 Add functionality to stop the sharing as part of re…
Posted by GitBox <gi...@apache.org>.
sebawagner commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413398204
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KStream.java
##########
@@ -359,4 +359,12 @@ public String getUid() {
public boolean contains(String uid) {
return this.uid.equals(uid) || listeners.containsKey(uid);
}
+
+ @Override
Review comment:
Improve logging of KStream to see in the log the details of the KStream object.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] solomax commented on a change in pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
solomax commented on a change in pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#discussion_r413443529
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KurentoHandler.java
##########
@@ -120,7 +121,8 @@ public void init() {
try {
kuid = randomUUID().toString();
client = KurentoClient.create(kurentoWsUrl, new KConnectionListener(kuid));
- client.getServerManager().addObjectCreatedListener(new KWatchDog());
+ client.getServerManager().addObjectCreatedListener(new KWatchDogCreate());
+ client.getServerManager().addObjectDestroyedListener(new KWatchDogDestroy());
Review comment:
We are not relying on browser events already
https://github.com/apache/openmeetings/blob/master/openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KStream.java#L123
The stream should both be stopped and destroyed
I would further investigate why it is not happening
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-618853159
> I would say `If stopping the recording it will also stop the sharing of the screen` is not the best option
=> That is fixed now. It won't stop the sharing - except - sharing wasn't started. Then it will stop the stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-618852104
@solomax it works now!
You can enable the recoridng and it will just start recoring, you can stop it and it will stop recording and only stop sharing if sharing was disabled.
However if you start recording, and then enable sharing while recording, and then stop recording, it will continue to share screen.
You can also share screen and start/stop recording as many times as you want.
The issue with the ghost KStream was because the video-util close closed the stream on the first try. The logic on client side was closing the stream in a way so you couldn't restart. Stopping from Server side actually works better.
I'm not quite sure what happens client side its a also a bit confusing, cause if you "just record" on server side you still have a screen-share. Even though you don't create an screen-share event. Its kind of like hidden on the server, until you send the share event.
So I think controlling the logic from the server logic seems to work better.
I also did check with a 2nd browser to make sure all screen-share events come through. And also no Zero size or ghost KStreams are created.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-619210249
@solomax r u happy with it? I can't find or think of any regression introduced by it
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner commented on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner commented on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-619240155
Please let me know on the approach.
There is really not much that changed. I should simply now work as expected.
If you're happy with the PR, I start thinking of modifying some of the tests in
TestRoomFlowMocked to see how to verify it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openmeetings] sebawagner edited a comment on pull request #65: OPENMEETINGS-2278 fix restart of recording and ghost KStream
Posted by GitBox <gi...@apache.org>.
sebawagner edited a comment on pull request #65:
URL: https://github.com/apache/openmeetings/pull/65#issuecomment-618853159
> I would say `If stopping the recording it will also stop the sharing of the screen` is not the best option
=> That is fixed now. It won't stop the sharing - except - sharing wasn't started. Then it will stop the stream.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org