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

feat: Enable transcription based on MUC config form #1135

Merged
merged 1 commit into from
Feb 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions jicofo-common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@
<groupId>org.glassfish.jersey.media</groupId>
<artifactId>jersey-media-json-jackson</artifactId>
</dependency>
<dependency>
<groupId>com.fasterxml.jackson.module</groupId>
<artifactId>jackson-module-kotlin</artifactId>
</dependency>

<!-- https://mvnrepository.com/artifact/com.github.spotbugs/spotbugs-annotations -->
<dependency>
<groupId>com.github.spotbugs</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ interface ChatRoom {
* Read from the MUC config form. */
val participantsSoftLimit: Int?

/** Whether the room is configured to require transcription. */
val transcriptionRequested: Boolean

val debugState: OrderedJsonObject

/** Returns the number of members that currently have their audio sources unmuted. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,15 @@ class ChatRoomImpl(
}
}

override var transcriptionRequested: Boolean = false
private set(value) {
if (value != field) {
logger.info("transcriptionRequested is now $value.")
field = value
eventEmitter.fireEvent { transcriptionRequestedChanged(value) }
}
}

private val avModerationByMediaType = ConcurrentHashMap<MediaType, AvModerationForMediaType>()

/** The emitter used to fire events. */
Expand Down Expand Up @@ -278,10 +287,25 @@ class ChatRoomImpl(
private fun parseConfigForm(configForm: Form) {
lobbyEnabled =
configForm.getField(MucConfigFormManager.MUC_ROOMCONFIG_MEMBERSONLY)?.firstValue?.toBoolean() ?: false
visitorsEnabled =
configForm.getField(MucConfigFields.VISITORS_ENABLED)?.firstValue?.toBoolean()
participantsSoftLimit =
configForm.getField(MucConfigFields.PARTICIPANTS_SOFT_LIMIT)?.firstValue?.toInt()
visitorsEnabled = configForm.getField(MucConfigFields.VISITORS_ENABLED)?.firstValue?.toBoolean()
participantsSoftLimit = configForm.getField(MucConfigFields.PARTICIPANTS_SOFT_LIMIT)?.firstValue?.toInt()
// Default to false unless specified.
val roomMetadata = configForm.getRoomMetadata()
if (roomMetadata != null) {
transcriptionRequested = roomMetadata.recording?.isTranscribingEnabled == true
}
}

private fun Form.getRoomMetadata(): RoomMetadata.Metadata? {
getField("muc#roominfo_jitsimetadata")?.firstValue?.let {
try {
return RoomMetadata.parse(it).metadata
} catch (e: Exception) {
logger.warn("Invalid room metadata content", e)
return null
}
}
return null
}

override fun leave() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ interface ChatRoomListener {
fun localRoleChanged(newRole: MemberRole) {}
fun numAudioSendersChanged(numAudioSenders: Int) {}
fun numVideoSendersChanged(numVideoSenders: Int) {}
fun transcriptionRequestedChanged(transcriptionRequested: Boolean) {}
}

/** A class with the default kotlin method implementations (to avoid using @JvmDefault) **/
Expand Down
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
Expand Up @@ -2336,6 +2336,11 @@ public void memberPresenceChanged(@NotNull ChatRoomMember member)
{
}

@Override
public void transcriptionRequestedChanged(boolean transcriptionRequested)
{
}

@Override
public void numAudioSendersChanged(int numAudioSenders)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down Expand Up @@ -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.");
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed

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());
});
}

/**
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -275,5 +277,15 @@ public void memberPresenceChanged(@NotNull ChatRoomMember member)
{
TranscriberManager.this.memberPresenceChanged(member);
}

@Override
public void transcriptionRequestedChanged(boolean transcriptionRequested)
{
if (transcriptionRequested)
Copy link
Member

Choose a reason for hiding this comment

The 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:

  • presence
  • room metadata

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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!

Copy link
Member Author

Choose a reason for hiding this comment

The 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();
}
}
}
}
Loading