Conversation
|
size-limit report 📦
|
2f31fbc to
6a5960d
Compare
6a5960d to
2e6f8cf
Compare
src/room/events.ts
Outdated
| /** | ||
| * Emits when a new data track has been published by a downstream participant. | ||
| */ | ||
| RemoteDataTrackPublished = 'remoteDataTrackPublished', |
There was a problem hiding this comment.
| RemoteDataTrackPublished = 'remoteDataTrackPublished', | |
| DataTrackPublished = 'dataTrackPublished', |
conforms with existing TrackPublished and TrackUnpublished APIs which are remote if not explicitly prefixed with Local
There was a problem hiding this comment.
usually we would also proxy the events as a ParticipantEvent and emit it directly on the participant.
If we want it to feel a bit closer to how existing APIs work, that might be a good idea here, too
There was a problem hiding this comment.
conforms with existing TrackPublished and TrackUnpublished APIs which are remote if not explicitly prefixed with Local
Ah, good point - updated.
usually we would also proxy the events as a ParticipantEvent and emit it directly on the participant.
If we want it to feel a bit closer to how existing APIs work, that might be a good idea here, too
Looking at the current participant events, it looks like they are segmented by locality rather than type. ie:
trackPublished: (publication: RemoteTrackPublication) => void;
// ...
trackUnpublished: (publication: RemoteTrackPublication) => void;
// ...
localTrackPublished: (publication: LocalTrackPublication) => void;
localTrackUnpublished: (publication: LocalTrackPublication) => void;Because of this, I'm sort of of the opinion to hold off on proxying these events to the participant until the larger track changes get rolled out across everything, and then at that point we can get these all going through the same interfaces rather than tacking on some additional data track specific events to this list. Any concerns with that approach?
|
|
||
| /** Publish the track to the SFU. This must be done before calling {@link tryPush} for the first time. */ | ||
| async publish(signal?: AbortSignal) { | ||
| try { |
There was a problem hiding this comment.
I guess this should error early (or warn and return) if isPublished is true?
There was a problem hiding this comment.
There is already an error that gets thrown within publishRequest if the track is already published, but I could probably make the error message a little nicer (right now it is one of those "panic" bare errors).
IMO in this case, I think an explicit throw makes sense because you really should not be publishing / unpublishing data tracks without some care. But looking at the existing tracks it looks like it just warns and that's it. So maybe it makes sense to convert this throw into a warning, not sure.
EDIT: more thoughts on this here!
| * */ | ||
| async unpublish() { | ||
| if (!this.handle) { | ||
| throw DataTrackPushFrameError.trackUnpublished(); |
There was a problem hiding this comment.
suggestion: might be enough to log a warning here and return early?
There was a problem hiding this comment.
Yea I am not sure - see my previous comment about the throw vs warning here. I like the hard error in that it is something that can be caught and explicitly handled though since this is checking an invariant. The existing track publication logic for audio / video tracks logs a warning so maybe it makes sense to update it to be consistent.
I guess one downside to the hard error is it opens this code up to a TOC-TOU issue where something like the below could happen:
if (track.isPublished) {
// Imagine here the track is unpublished at the SFU level for some unrelated reason ...
await track.unpublish(); // ... so this would then throw an error
}I am definitely curious what others think here!
2619816 to
8e7a138
Compare
Lukas has some reservations: #1820 (comment)
After doing this I was able to get it to work end to end!
Example of where this can be triggered is specifying an empty name when creating a new data track.
Temporarily replace the hexdump with a chart for a demo. I think this probably should be reverted though, the hexdump is more useful longer term.
…ks data channel Adds a "sendLossyBytes" method which hooks into the existing data channel logging / buffer status / etc infrastructure.
…urn DataTrackSubscriptionReader instead of ReadableStream" This reverts commit ee2d5ed.
The api now looks like:
const readableStream = track.subscribe({ signal });
for await (const frame of readableStream) {
// Do something with `frame`
}
Manual cancellation can be done with `signal` OR `readableStream` - they
should act identically.
…sts can determine sfu subscription completion
This is a pretty rare case but could occur in theory
This was updated at the SFU level.
| error = DataTrackPublishError.limitReached(response.message); | ||
| break; | ||
| default: | ||
| this.log.error( |
There was a problem hiding this comment.
question: If later new error reasons are added for data track operations, would we want to log an error on older client or introduce an unknown case?
There was a problem hiding this comment.
Good point, I hadn't considered this. It looks like the rust implementation is raising an error in this case currently - a PublishError::Internal.
I will do something similar here (2d58fee) but in regards to the name: What do you think about ::Unknown for this case in both js / rust, or maybe something else that is specific for this exact case? Internal is being used as a catch all in a few other places so it isn't unique.
| /** | ||
| * Emits when a new data track has been published by a downstream participant. | ||
| */ | ||
| DataTrackPublished = 'dataTrackPublished', |
There was a problem hiding this comment.
comment: In Rust, this event is called RemoteDataTrackPublished.
There was a problem hiding this comment.
This was a change lukas requested, more info here: #1820 (comment)
src/room/Room.ts
Outdated
| .on('sfuUpdateSubscription', (event) => { | ||
| this.engine.client.sendUpdateDataSubscription(event.sid, event.subscribe); | ||
| }) | ||
| .on('trackAvailable', (event) => { |
There was a problem hiding this comment.
question: Would it make sense to rename the manager level events for consistency with the room level ones (e.g., published instead of available)? Will do this in Rust too if you end up changing it.
… are active The message from the SFU doesn't contain all participant identities, only the ones which change.
This allows multiple different versions of the app to run and all join the same room
Integrates the data track managers (both incoming and outgoing) into the existing sdk
RoomandRemoteParticipantinterfaces. Once this is merged, data tracks is fully implemented!Interface - Publishing
Interface - Subscribing
Todo: