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

Make RtpTransportProcessor transferable #33

Open
pthatcher opened this issue May 14, 2024 · 12 comments · May be fixed by #80
Open

Make RtpTransportProcessor transferable #33

pthatcher opened this issue May 14, 2024 · 12 comments · May be fixed by #80
Labels
Ready for PR Issue discussion has converged

Comments

@pthatcher
Copy link
Collaborator

pthatcher commented May 14, 2024

Once it's transferred, PeerConnection.RtpTransport no longer allows you to attach an event handler, because only the RtpTransport that was transferred can attach an event handler.

@pthatcher
Copy link
Collaborator Author

pthatcher commented May 14, 2024

And we need to make sure we can't transfer the RtpTransport if you've already attached an event handler when on the main thread.

@youennf
Copy link

youennf commented May 14, 2024

With the current proposal, you can get the RTCRtpTransport from RTCRtpSender/RTCRtpReceiver.
Getting a neutered RTCRtpTransport from these is not really appealing.
I would tend to not go there.

@dontcallmedom-bot
Copy link

@alvestrand
Copy link

Should there be an RTCRtpTransportProcessor embedded within the RTCRtpTransport that is the object that can be moved to (or instantiated in) a different context than where the PeerConnection lives?

@youennf
Copy link

youennf commented Jun 11, 2024

Some of the functionality added in RTPTransport, like custom pacing, makes much more sense in a worker than in a window environment.
We do not actually need to move the whole RTPtransport object to a worker. But at least some of its funcionality.

@tonyherre
Copy link
Contributor

Some kind of transferable RTCRtpTransportProcessor subset of RTCRtpTransport containing the hooks for getting RtpAcks and all other high frequency stuff sounds great to me, and seems to be the consensus from the last few comments. Are we ready for a PR to get into details?

@youennf
Copy link

youennf commented Jun 25, 2024

Right, we could move the current attributes, event handlers and readPacketizedRtp to a separate object that would be exposed in a worker.
We would then have a few options:

  • Get a transferable handle from RTCRtpTransport and a RTCRtpTransportProcessor constructor that takes the handle as parameter
  • Add a getProcessor(Worker) method on RTCRtpTransport and a DedicatedWorkerGlobalScope.onrtpTransportProcessor event handler.

@youennf
Copy link

youennf commented Jun 26, 2024

I would tend to go with something similar to WebRTC encoded transform.
This removes the need to deal with out of process dedicated workers for instance.
That would mean something like:

interface RTCRtpTransport {
  Promise addRtpSendStream(RTCRtpSendStreamInit);
  Promise addRtpReceiveStream(RTCRtpReceiveStreamInit);
  attribute RTCRtpTransportHandler? handler;
}

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessor : EventTarget {
  attribute EventHandler onrtpsent;
  attribute EventHandler onrtpacksreceived;
  attribute EventHandler onpacketizedrtpavailable;
  sequence<RTCRtpPacket> readPacketizedRtp(maxNumberOfPackets);

  readonly attribute any options;
  readonly attribute unsigned long bandwidthEstimate;
  readonly attribute unsigned long allocatedBandwidth;
  attribute unsigned long customAllocatedBandwidth;
  attribute unsigned long customMaxBandwidth;
  attribute unsigned long customPerPacketOverhead;
};

[Exposed=Window]
interface RTCRtpTransportHandler {
    constructor(Worker worker,  optional any options, optional sequence<object> transfer);
};

[Exposed=DedicatedWorker]
interface RTCRtpTransportHandlerEvent : Event {
    readonly attribute RTCRtpTransportProcessor processor;
};

partial interface DedicatedWorkerGlobalScope {
    attribute EventHandler onrtcrtptransport;
};

@tonyherre
Copy link
Contributor

tonyherre commented Jul 15, 2024

RTCRtpTransportProcessor in #33 (comment) looks good.

Would we want to support clearing/assigning a new handler mid-stream, like we do in encoded transforms? I don't see a usecase for that level of complexity. Synchronizing that cross-thread is going to be a pain to implement without hidden surprises/dropped packets!

For argument's sake, an alternative would be making processor itself transferrable, provided as a direct attribute on RTCRtpTransport:

interface RTCRtpTransport {
  Promise addRtpSendStream(RTCRtpSendStreamInit);
  Promise addRtpReceiveStream(RTCRtpReceiveStreamInit);
  attribute RTCRtpTransportProcessor processor;
}

[Exposed=Worker, DedicatedWorker] // Transferable
interface RTCRtpTransportProcessor {
// as above.
}

This is the same debate as that taking place in #36 for RtpSendStream/RtpReceiveStream.

@aboba aboba changed the title Make RtpTransport transferable Make RtpTransportProcessor transferable Jul 15, 2024
@aboba
Copy link
Contributor

aboba commented Jul 15, 2024

@tonyherre This looks good.

@aboba aboba added the Ready for PR Issue discussion has converged label Jul 15, 2024
@tonyherre
Copy link
Contributor

tonyherre commented Aug 1, 2024

@youennf what's the purpose of the RTCRtpTransportHandler object in your suggestion?
I can't think of any usecase where an app wouldn't just always do rtpTransport.handler = new RTCRtpTransportHandler(worker, message, transfer);.

If we took the approach of triggering an event in the worker instead of general Transferability, wouldn't it work as well to just have a method directly on RTCRtpTransport:

interface RTCRtpTransport {
  Promise addRtpSendStream(RTCRtpSendStreamInit);
  Promise addRtpReceiveStream(RTCRtpReceiveStreamInit);

  // Causes RTCRtpTransportProcessorEvent to be fired on |worker|.
  createProcessor(Worker worker,  optional any options, optional sequence<object> transfer);
}

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessor {
 ...
};

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessorEvent : Event {
    readonly attribute RTCRtpTransportProcessor processor;
};

partial interface DedicatedWorkerGlobalScope {
    attribute EventHandler onrtcrtptransportprocessor;
};

thus saving us an interface and a lot of theoretical corner cases around Handler lifecycles.

@tonyherre
Copy link
Contributor

tonyherre commented Aug 16, 2024

Btw, I've tried out the event-based transfer (using a createProcessor() method) in Chromium (crrev.com/c/5759780) and an extension to my sample page (code).

Seems to work quite nicely. I still mildly prefer Transferability but would be fine with this.

Discovered that a big benefit of using a createProcessor() method on RTCRtpTransport rather than a separate RTCRtpTransportHandler constructor is that the processor is ready to be used for the Transport as soon as the RTCRtpTransportProcessorEvent is fired in the worker. Otherwise, the worker JS needs to wait some undefined amount of time for the assignment to happen on the main thread and everything to be asynchronously set up by the browser before it can start using the Processor.

@youennf youennf linked a pull request Nov 12, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for PR Issue discussion has converged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants