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

Packet sender receiver worker api #81

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

youennf
Copy link

@youennf youennf commented Nov 12, 2024

Fixes #36.

@youennf youennf marked this pull request as draft November 12, 2024 14:50
@youennf youennf force-pushed the packet-sender-receiver-worker-api branch from 398830a to ba06697 Compare November 12, 2024 17:53
}

[Exposed=Window]
Copy link
Collaborator

Choose a reason for hiding this comment

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

At the last RtpTransport meeting, we had an idea of how to simplify this.

  1. Instead of having both an RTCRtpTransport and RTCRtpTransportProcessorHandle, we simplify down to just RTCRtpTransport.
  2. Instead constructing an RTCRtpTransportProcessorHandle passing in an RTCRtpTransport from RTCPeerConnection.rtpTransport, you would call RTCRtpPeerConnection.sendRtpTransportToWorker(worker, ...).
  3. RTCRtpReceiver.replacePacketReceiver and RTCRtpSender.replacePacketSender would still trigger .onpacketsender and .onpacketreceiver in the Worker, but these would happen on an RTCRtpTransport instead of an RTCRtpTransportProcessorHandle.
  4. Incoming packets would be buffered between when RTCPeerConnection.ontrack is fired and when RTCRtpReceiver.replacePacketReceiver is called, and those buffered packets would be provided to the RtpPacketReciev given in RTCRtpTransport.onpacketreceiver.

Copy link
Author

Choose a reason for hiding this comment

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

I am hesitant to go with the sendRtpTransportToWorker approach, some thoughts:

1 All RTP***Transport objects are window objects currently.
If we ever want to create in the future a RTCRtpTransport from a RTCDtlsTransport say, we would no longer be able to use a constructor based API.

  1. RTCRtpTransport is the object that represents a bundle group.
    This allows grouping some senders/receivers together, these object living as window objects.
    If we want to expose some API about this bundle group, it might be handy to have it as a window object, like the list of receivers/senders of a given bundle group.

1 & 2 makes me think that having an object in window for that construct is more future proof, even if it does not do a lot of things today.

Or maybe we could use RTCDtlsTransport instead.

  1. It is convenient to be able from a ontrack event handler to do something like:
const receiver = event.receiver;
if (receiver.rtpTransport && !receiver.rtpTransport.handle)
    receiver.rtpTransport.handle = new RTCRtpTransportProcessorHandle(...)

I am not exactly sure how that would work with sendRtpTransportToWorker .
Does it mean all rtp transports would go to the same worker?
Using RTCDtlsTransport might be somehow better in that respect.

  1. sendRtpTransportToWorker would not allow to nullify the rtp transport to go back to default.
    At least, in its current form, it does not seem capable of doing so.

Maybe it is the name send... which gives me that impression.

Copy link
Author

Choose a reason for hiding this comment

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

Another slight difference, in one case, the setup of the processor can start before having an actual transport. Not sure whether it matters much in practice though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

From meeting: Let's try putting it on DtlsTransport, something like DtlsTransport.createRtpTransport.

We would need to change the examples to include createOffer+setLocal/setRemote to make sure the DtlsTransport exists. Or we would could make DtlsTransport always exist if "max-bundle" is used in RTCConfiguration.

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.

Should RtpSendStream/RtpReceiveStream be transferable?
2 participants