Updating Small WebRTC transport to Pipecat Client v1.1.0 #27
Conversation
|
CI expected to fail until core client is released. |
| @Serializable | ||
| internal data class SmallWebRTCStartBotResult( | ||
| val sessionId: String, | ||
| val iceConfig: Value = Value.Null |
There was a problem hiding this comment.
This iceConfig maybe it would be nice to create a specific type for it. For example this is the one for Web:
export type IceConfig = {
iceServers?: RTCIceServer[];
};
Where RTCIceServer is this: https://udn.realityripple.com/docs/Web/API/RTCIceServer
| json: String, | ||
| startBotRequest: APIRequest | ||
| ): SmallWebRTCTransportConnectParams { | ||
| val startBotResult = JSON_INSTANCE.decodeFromString<SmallWebRTCStartBotResult>(json) |
There was a problem hiding this comment.
I believe we should also add a check to make sure the json isn't already of type SmallWebRTCTransportConnectParams.
| ): SmallWebRTCTransportConnectParams { | ||
| val startBotResult = JSON_INSTANCE.decodeFromString<SmallWebRTCStartBotResult>(json) | ||
|
|
||
| return SmallWebRTCTransportConnectParams( |
There was a problem hiding this comment.
The SmallWebRTCTransportConnectionParams should also be able to receive the iceConfig from the startBotResult. So we can use it when connecting.
|
|
||
| import ai.pipecat.client.types.APIRequest | ||
|
|
||
| data class SmallWebRTCTransportConnectParams( |
There was a problem hiding this comment.
We should also add the iceConfig here.
|
A couple of other missing features that I’ve noticed:
Not sure if you’ve already tested connecting with a deployed agent on Pipecat Cloud, but that would probably be a good test as well, to make sure we haven’t missed anything. |
|
I've added @mattieruth as a reviewer since Filipi is away at the moment. All comments should now be addressed, we now support |
| } | ||
|
|
||
| "renegotiate" -> negotiate() | ||
| "renegotiate" -> negotiate(transportParams) |
There was a problem hiding this comment.
is there any reason this should use connectParams instead? i.e. is it possible for the connectParams to change between now and when a renegotiation occurs and therefore should use the updated params?
There was a problem hiding this comment.
It doesn't look like there's any way for this to change in the meantime -- the type is immutable and we don't change the connectParams field. If nothing else using transportParams avoids an unnecessary "non-null" assertion.
mattieruth
left a comment
mattieruth
left a comment
There was a problem hiding this comment.
Mostly leaning on Filipi's review for this one. So just the one question.
No description provided.