-
Notifications
You must be signed in to change notification settings - Fork 105
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
Transcription events CLT-343 #406
Conversation
hiroshihorie
commented
Jun 18, 2024
•
edited
Loading
edited
- Events for RoomDelegate
- Events for ParticipantDelegate
- State management
guard let participant = _state.read({ | ||
$0.remoteParticipants.values.first { $0.identity == Participant.Identity(from: packet.transcribedParticipantIdentity) } | ||
}) else { | ||
log("[Transcription] Could not find participant: \(packet.transcribedParticipantIdentity)", .warning) |
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.
@hiroshihorie you'll need to include support for transcription events coming down on the LocalParticipant as well, to record the results of STT in the agent
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.
I did a quick spike and it works to just change the delegate methods to receive generic Participant and TrackPublication instead of the Remote variants, change these lines up here to the following
let participant: Participant?
let identity = Participant.Identity(from: packet.transcribedParticipantIdentity)
if localParticipant.identity == identity {
participant = localParticipant
} else {
participant = _state.read({ $0.remoteParticipants.values.first { $0.identity == identity } })
}
guard let participant else {
log("[Transcription] Could not find participant: \(packet.transcribedParticipantIdentity)", .warning)
return
}
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.
Thank you ! will update.
If looks good will merge first, then think about if any state helpers are necessary. |
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.
looks good - go ahead and file a new issue for any state management follow-ons
override public var hash: Int { | ||
var hasher = Hasher() | ||
hasher.combine(id) | ||
return hasher.finalize() |
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.
@hiroshihorie @lukasIO do you two think it's right to identify and hash these based on ID alone? It doesn't guarantee true equality, because a newer version could be received later that should be used instead.
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.
what's the intended purpose of the hash?