Make RtpTransportProcessor transferable

[pthatcher]

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]

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]

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]

This issue was mentioned in WebRTC Interim, May 21st – 21 May 2024 (RtpTransport (Peter Thatcher))

[alvestrand]

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]

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]

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]

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]

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]

RTCRtpTransportProcessor in https://github.com/w3c/webrtc-rtptransport/issues/33#issuecomment-2191647460 looks good.

Would we want to support clearing/assigning a new hander 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]

@tonyherre This looks good.

[tonyherre]

@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]

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.