Conversation
|
| * | ||
| * @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.
| }, | ||
| }); | ||
|
|
||
| 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.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
2 participants
Resolves BOT-281