-
Notifications
You must be signed in to change notification settings - Fork 351
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
feat: Enable transcription based on MUC config form #1135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
/* | ||
* Jicofo, the Jitsi Conference Focus. | ||
* | ||
* Copyright @ 2024-Present 8x8, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.jitsi.jicofo.xmpp.muc | ||
|
||
import com.fasterxml.jackson.annotation.JsonIgnoreProperties | ||
import com.fasterxml.jackson.core.JsonParser | ||
import com.fasterxml.jackson.core.JsonProcessingException | ||
import com.fasterxml.jackson.databind.JsonMappingException | ||
import com.fasterxml.jackson.databind.MapperFeature | ||
import com.fasterxml.jackson.module.kotlin.jacksonObjectMapper | ||
import com.fasterxml.jackson.module.kotlin.readValue | ||
|
||
/** | ||
* The JSON structure included in the MUC config form from the room_metadata prosody module in jitsi-meet. Includes | ||
* only the fields that we need here in jicofo. | ||
*/ | ||
@JsonIgnoreProperties(ignoreUnknown = true) | ||
data class RoomMetadata( | ||
val type: String, | ||
val metadata: Metadata? | ||
) { | ||
@JsonIgnoreProperties(ignoreUnknown = true) | ||
data class Metadata(val recording: Recording?) { | ||
@JsonIgnoreProperties(ignoreUnknown = true) | ||
data class Recording(val isTranscribingEnabled: Boolean?) | ||
} | ||
|
||
companion object { | ||
private val mapper = jacksonObjectMapper().apply { | ||
enable(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS) | ||
enable(JsonParser.Feature.STRICT_DUPLICATE_DETECTION) | ||
} | ||
|
||
@Throws(JsonProcessingException::class, JsonMappingException::class) | ||
fun parse(string: String): RoomMetadata { | ||
return mapper.readValue(string) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/* | ||
* Jicofo, the Jitsi Conference Focus. | ||
* | ||
* Copyright @ 2024-Present 8x8, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.jitsi.jicofo.xmpp.muc | ||
|
||
import io.kotest.assertions.throwables.shouldThrow | ||
import io.kotest.core.spec.style.ShouldSpec | ||
import io.kotest.matchers.nulls.shouldNotBeNull | ||
import io.kotest.matchers.shouldBe | ||
import io.kotest.matchers.types.shouldBeInstanceOf | ||
|
||
class RoomMetadataTest : ShouldSpec() { | ||
init { | ||
context("Valid") { | ||
context("With isTranscribingEnabled set") { | ||
val parsed = RoomMetadata.parse( | ||
""" | ||
{ | ||
"type": "room_metadata", | ||
"metadata": { | ||
"recording": { | ||
"isTranscribingEnabled": true, | ||
"anotherField": 123 | ||
}, | ||
"anotherField": {} | ||
} | ||
} | ||
""".trimIndent() | ||
) | ||
parsed.shouldBeInstanceOf<RoomMetadata>() | ||
parsed.metadata!!.recording!!.isTranscribingEnabled shouldBe true | ||
} | ||
context("With no recording included") { | ||
|
||
val parsed = RoomMetadata.parse( | ||
""" | ||
{ | ||
"type": "room_metadata", | ||
"metadata": { | ||
"key": { | ||
"key2": "value2" | ||
}, | ||
"anotherField": {} | ||
} | ||
} | ||
""".trimIndent() | ||
) | ||
parsed.shouldBeInstanceOf<RoomMetadata>() | ||
parsed.metadata.shouldNotBeNull() | ||
parsed.metadata?.recording shouldBe null | ||
} | ||
} | ||
context("Invalid") { | ||
context("Missing type") { | ||
shouldThrow<Exception> { | ||
RoomMetadata.parse( | ||
""" | ||
{ "key": 123 } | ||
""".trimIndent() | ||
) | ||
} | ||
} | ||
context("Invalid JSON") { | ||
shouldThrow<Exception> { | ||
RoomMetadata.parse("{") | ||
} | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,8 @@ public class TranscriberManager | |
|
||
/** | ||
* A single-threaded {@link ExecutorService} to offload inviting the | ||
* Transcriber from the smack thread updating presence. | ||
* Transcriber from the smack thread updating presence. It's important that requests are handled sequentially to | ||
* prevent multiple jigasis being invited. | ||
*/ | ||
private final ExecutorService executorService = Executors.newSingleThreadExecutor(); | ||
|
||
|
@@ -125,20 +126,37 @@ private void memberPresenceChanged(@NotNull ChatRoomMember member) | |
if (transcriptionStatusExtension != null | ||
&& TranscriptionStatusExtension.Status.OFF.equals(transcriptionStatusExtension.getStatus())) | ||
{ | ||
// puts the stopping in the single threaded executor | ||
// so we can order the events and avoid indicating active = false | ||
// while we are starting due to concurrent presences processed | ||
executorService.execute(this::stopTranscribing); | ||
active = false; | ||
logger.info("detected transcription status being turned off."); | ||
} | ||
if (isRequestingTranscriber(presence) && !active) | ||
{ | ||
if (jigasiDetector == null) | ||
tryToStart(); | ||
} | ||
} | ||
|
||
private void tryToStart() | ||
{ | ||
if (jigasiDetector == null) | ||
{ | ||
logger.warn("Transcription requested, but jigasiDetector is not configured."); | ||
return; | ||
} | ||
|
||
if (active) | ||
{ | ||
return; | ||
} | ||
|
||
executorService.execute(() -> { | ||
if (active) | ||
{ | ||
logger.warn("Transcription requested, but jigasiDetector is not configured."); | ||
return; | ||
} | ||
executorService.execute(() -> this.startTranscribing(conference.getBridgeRegions())); | ||
} | ||
|
||
// We need a modifiable list for the "exclude" parameter. | ||
selectTranscriber(2, new ArrayList<>(), conference.getBridgeRegions()); | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -159,13 +177,6 @@ private TranscriptionStatusExtension getTranscriptionStatus(Presence p) | |
*/ | ||
private void startTranscribing(@NotNull Collection<String> preferredRegions) | ||
{ | ||
if (active) | ||
{ | ||
return; | ||
} | ||
|
||
// We need a modifiable list for the "exclude" parameter. | ||
selectTranscriber(2, new ArrayList<>(), preferredRegions); | ||
} | ||
|
||
/** | ||
|
@@ -234,15 +245,6 @@ private void selectTranscriber( | |
} | ||
} | ||
|
||
/** | ||
* Indicate transcription has stopped and sets {@link this#active} to false. | ||
*/ | ||
private void stopTranscribing() | ||
{ | ||
active = false; | ||
logger.info("detected transcription status being turned off."); | ||
} | ||
|
||
/** | ||
* Checks whether the given {@link Presence} indicates a conference | ||
* participant is requesting transcription | ||
|
@@ -275,5 +277,15 @@ public void memberPresenceChanged(@NotNull ChatRoomMember member) | |
{ | ||
TranscriberManager.this.memberPresenceChanged(member); | ||
} | ||
|
||
@Override | ||
public void transcriptionRequestedChanged(boolean transcriptionRequested) | ||
{ | ||
if (transcriptionRequested) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this check for the active flag too? Note that right now there will be 2 ways to trigger this and both will happen roughly at once:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's why the tasks are put on a single threaded executor. I can check the flag here, but I'll have to re-check it in the other thread too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I'm not familiar with the code so I thought I'd ask. SGTM! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a good question |
||
{ | ||
logger.info("Transcription requested from the room."); | ||
tryToStart(); | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for the future: it would be useful to somehow propagate this all the way to the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed