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

Support SIP DTMF data messages #1130

Merged
merged 2 commits into from
May 17, 2024
Merged

Support SIP DTMF data messages #1130

merged 2 commits into from
May 17, 2024

Conversation

dennwc
Copy link
Contributor

@dennwc dennwc commented May 7, 2024

Added support for SIP DTMF data messages on rooms and participants.

For consistency with other SDKs, the RTCEngine data packet handler signature was changed to receive all message types, which is then switched by type in the Room.

Also compatibility code was added to properly select participant from a shared participant_identity on the DataPacket (see livekit/protocol#706).

@dennwc dennwc requested a review from lukasIO May 7, 2024 11:06
@dennwc dennwc self-assigned this May 7, 2024
Copy link

changeset-bot bot commented May 7, 2024

🦋 Changeset detected

Latest commit: f577316

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented May 7, 2024

size-limit report 📦

Path Size
dist/livekit-client.esm.mjs 80.05 KB (+0.15% 🔺)
dist/livekit-client.umd.js 85.77 KB (+0.17% 🔺)

src/room/Room.ts Outdated Show resolved Hide resolved
} else {
if (dp.value?.case === 'user') {
// compatibility
if (dp.participantIdentity === '' && dp.value.value.participantIdentity !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!
just to make it a bit easier to maintain, can we extract the compatibility stuff into a function?
it seems the pattern is always dp[myProperty] || dp.value.value[myProperty] (empty strings are falsy in JS). Not sure if it's still elegant with typescript stuff, but having it extracted into a helper of some sort (not necessarily the one I proposed) would be nice!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like we might remove the compatibility part for the transcriptions (see livekit/protocol#706).

But in general, sure! I assume empty arrays are falsy too, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

But in general, sure! I assume empty arrays are falsy too, right?

no, empty arrays are truthy :D my naive proposal won't work for both strings and arrays...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the helper functions you mentioned, but I get some TS error about DataPacket type not supporting indexing. I might need your help on this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a look and my initial idea might be totally flawed. I don't really see a way to get around the typescript issues without either being overly specific about the allowed strings or converting the protobuf to JSON first and then parsing it back. Which seems pointless and not at all simplifying things.
While verbose, your original implementation of handling these separately looks a lot preferable to me than back and forth parsing.
I think the main reason why I pushed for that was because we had logic duplication for transcription and user packet, but it seems that is no longer the case, because the field got renamed for transcriptions to transcribed_participant. So I don't see a huge incentive anymore to change what you had originally.

I pushed to a branch what would work, but that's basically just extracting what you had previously already into a function 🤦 42b8381
Feel free to ignore it entirely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that looks very specific. OK, lets settle on the middle ground - I'll revert the "generic" part of the change, but will keep compat logic in a separate function, so that it's easier to read the main handler function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, pushed the updated function. Also simplified logic a bit.

Copy link
Contributor

@lukasIO lukasIO left a comment

Choose a reason for hiding this comment

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

nice!

@dennwc dennwc merged commit 1e8465a into main May 17, 2024
3 checks passed
@dennwc dennwc deleted the sip-dtmf branch May 17, 2024 18:58
@github-actions github-actions bot mentioned this pull request May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants