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/30 08:11:43 UTC
[GitHub] [openmeetings] solomax commented on a change in pull request #71: OPENMEETINGS-2315 Only StreamProcessor to hold reference of KStream
solomax commented on a change in pull request #71:
URL: https://github.com/apache/openmeetings/pull/71#discussion_r417825799
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KRoom.java
##########
@@ -98,16 +103,16 @@ public RecordingChunkDao getChunkDao() {
public KStream join(final StreamDesc sd) {
log.info("ROOM {}: join client {}, stream: {}", roomId, sd.getClient(), sd.getUid());
final KStream stream = new KStream(sd, this);
- streams.put(stream.getUid(), stream);
+ streamProcessor.addStream(stream);
return stream;
}
public Collection<KStream> getParticipants() {
- return streams.values();
+ return streamProcessor.getStreamsByRoom(this.getRoomId());
}
public void onStopBroadcast(KStream stream, final StreamProcessor processor) {
Review comment:
`StreamProcessor` method parameter should be removed here
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KRoom.java
##########
@@ -98,16 +103,16 @@ public RecordingChunkDao getChunkDao() {
public KStream join(final StreamDesc sd) {
log.info("ROOM {}: join client {}, stream: {}", roomId, sd.getClient(), sd.getUid());
final KStream stream = new KStream(sd, this);
- streams.put(stream.getUid(), stream);
+ streamProcessor.addStream(stream);
return stream;
}
public Collection<KStream> getParticipants() {
- return streams.values();
+ return streamProcessor.getStreamsByRoom(this.getRoomId());
}
public void onStopBroadcast(KStream stream, final StreamProcessor processor) {
- streams.remove(stream.getUid());
+ streamProcessor.release(stream);
Review comment:
this call should happen during `stream.release(processor);`
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KRoom.java
##########
@@ -173,36 +163,36 @@ public void startRecording(StreamProcessor processor, Client c) {
Optional<StreamDesc> osd = c.getScreenStream();
if (osd.isPresent()) {
osd.get().addActivity(Activity.RECORD);
- processor.getClientManager().update(c);
+ streamProcessor.getClientManager().update(c);
rec.setWidth(osd.get().getWidth());
rec.setHeight(osd.get().getHeight());
}
- rec = processor.getRecordingDao().update(rec);
+ rec = streamProcessor.getRecordingDao().update(rec);
// Receive recordingId
recordingId = rec.getId();
- for (final KStream stream : streams.values()) {
- stream.startRecord(processor);
- }
+ streamProcessor.getStreamsByRoom(this.getRoomId()).forEach(
+ stream -> stream.startRecord(streamProcessor)
+ );
// Send notification to all users that the recording has been started
WebSocketHelper.sendRoom(new RoomMessage(roomId, u, RoomMessage.Type.RECORDING_TOGGLED));
log.debug("##REC:: recording in room {} is started {} ::", roomId, recordingId);
}
}
- public void stopRecording(final StreamProcessor processor, Client c) {
+ public void stopRecording(Client c) {
if (recordingStarted.compareAndSet(true, false)) {
log.debug("##REC:: recording in room {} is stopping {} ::", roomId, recordingId);
- for (final KStream stream : streams.values()) {
- stream.stopRecord();
- }
- Recording rec = processor.getRecordingDao().get(recordingId);
+ streamProcessor.getStreamsByRoom(this.getRoomId()).forEach(
+ stream -> stream.stopRecord()
Review comment:
this can be replaced with `KStream::stopRecord`
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -494,20 +498,24 @@ void remove(Client c) {
}
}
if (c.getRoomId() != null) {
- KRoom room = kHandler.getRoom(c.getRoomId());
- room.leave(this, c);
checkStreams(c.getRoomId());
}
}
- void addStream(KStream stream) {
+ public void addStream(KStream stream) {
streamByUid.put(stream.getUid(), stream);
}
public Collection<KStream> getStreams() {
return streamByUid.values();
}
+ public Collection<KStream> getStreamsByRoom(Long roomId) {
Review comment:
this can be package private
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/TestStreamProcessor.java
##########
@@ -126,4 +126,5 @@ public void destroy() {
}
streamByUid.clear();
}
+
Review comment:
this change seems useless ...
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -494,20 +498,24 @@ void remove(Client c) {
}
}
if (c.getRoomId() != null) {
- KRoom room = kHandler.getRoom(c.getRoomId());
- room.leave(this, c);
checkStreams(c.getRoomId());
}
}
- void addStream(KStream stream) {
+ public void addStream(KStream stream) {
Review comment:
Why is it `public`?
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/KRoom.java
##########
@@ -53,10 +51,16 @@
import com.github.openjson.JSONObject;
+/**
+ * Bean object dynamically created representing a conference room on the MediaServer
+ *
+ */
public class KRoom {
private static final Logger log = LoggerFactory.getLogger(KRoom.class);
-
- private final Map<String, KStream> streams = new ConcurrentHashMap<>();
+ /**
+ * Not injected by annotation but by constructor.
+ */
+ private final StreamProcessor streamProcessor;
Review comment:
can be `processor`, it is shorter :))
##########
File path: openmeetings-core/src/main/java/org/apache/openmeetings/core/remote/StreamProcessor.java
##########
@@ -494,20 +498,24 @@ void remove(Client c) {
}
}
if (c.getRoomId() != null) {
- KRoom room = kHandler.getRoom(c.getRoomId());
- room.leave(this, c);
checkStreams(c.getRoomId());
}
}
- void addStream(KStream stream) {
+ public void addStream(KStream stream) {
streamByUid.put(stream.getUid(), stream);
}
public Collection<KStream> getStreams() {
return streamByUid.values();
}
+ public Collection<KStream> getStreamsByRoom(Long roomId) {
+ return streamByUid.values().stream()
+ .filter(stream -> stream.getRoom() != null && stream.getRoom().getRoomId().equals(roomId))
+ .collect(Collectors.toCollection(ArrayList::new));
Review comment:
Why not to use `Collectors.toList()` ?
Do you have any benchmarks on how much this is slower than separate list?
----------------------------------------------------------------
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