Conversation
🦋 Changeset detectedLatest commit: 79f5b7c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| * | ||
| * @throws {@link DataTrackPushFrameError} If the push fails. | ||
| */ | ||
| tryPush(frame: DataTrackFrame): void { |
There was a problem hiding this comment.
thought(non-blocking): I left a similar comment on the JS implementation, it seems a bit counter-intuitive that a try* method can throw.
Although I don't know if there's a good alternative here. If users aren't expected to gain additional insight from the error's message maybe returning a "success" boolean from the method would be sufficient?
There was a problem hiding this comment.
In contrast to the web implementation, the error reasons here might be more useful and worth exposing. Under the hood, this sends the frame to a dedicated task (one for each track) for processing (E2EE, compression in a future release, etc.) and packetization before sending. If the channel used for sending frames to this task fills up (i.e., in the case the user is pushing too fast), they would get an error here.
There was a problem hiding this comment.
makes sense, my slight reservation about it being called try* in that case remains
| case 'eos': { | ||
| this.dispose(); | ||
| if (event.detail.value.error) { | ||
| controller.error(new Error(event.detail.value.error)); |
There was a problem hiding this comment.
suggestion(non-blocking): maybe nice to have a more specific (tagged) error here
There was a problem hiding this comment.
That's definitely something we can do here—the Protobuf now has enums that map to the corresponding error cases from Rust. However, I wasn't sure the nicest way to expose this in Node and if we want to adopt the new error handling patterns from the JS SDK.
There was a problem hiding this comment.
I'm all for more specific (and tagged) errors! If you think it would fall in suggestion(if-minor) then I'd say go for it ;)
| }, | ||
| }); | ||
|
|
||
| return new ReadableStream<DataTrackFrame>(new DataTrackStreamSource(res.stream!), { |
There was a problem hiding this comment.
question: should we be worried about the fact that each subscribe call pushes the same frames across the FFI boundary each time?
Not sure if there's a good way to re-use an existing source.
We could also think about teeing the stream but that comes with it's own pitfalls when reading at different rates in different places.
There was a problem hiding this comment.
Definitely something to consider, and something that also applies to Python, C++, and Unity. If there are multiple subscriptions, you are right that the same frames will need to be serialized/deserialized more than once. I wonder if the performance overhead from this is enough to justify implementing a teeing solution for each FFI client—this would be more performant at the cost of adding additional complexity to each FFI client implementation. Currently, teeing is implemented on the Rust side so each FFI client gets that behavior automatically.
There was a problem hiding this comment.
I wonder if the performance overhead from this is enough
yeah, would be nice to get a feel for this with some numbers. Definitely nothing blocking right now, but would be a good follow up to check at what point this would start to become a bottleneck
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| * Publishes a data track. | ||
| * | ||
| * @returns The published data track. Use {@link LocalDataTrack.tryPush} to send data frames. | ||
| * @throws {@link PublishDataTrackError} if there is an error publishing the data track. |
There was a problem hiding this comment.
Let's add some hint in the comment for minimum OSS server version required
2 participants
Resolves BOT-281