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

Create RTCRtpTransportProcessor and move high freq fields there #68

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tonyherre
Copy link
Contributor

Implements the proposal in the comment #33 (comment)

Very open for comments & suggestions/alternatives. The discussion on the issue is still ongoing.

api-outline.md Outdated Show resolved Hide resolved
}

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessor {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So are we saying we want to require the use of workers to use RtpTransport? Are we sure that's what we want?

And if that's what we want, why bother calling this this RTCRtpTransportProcessor and not just RTCRtpTransport?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the discussion happening in #33. There's the suggestion it should be worker-only like RTCRtpScriptTransform (vs rely on transferability like MediaStreamTrackProcessor etc). Could you join the discussion there, Peter?

Copy link

Choose a reason for hiding this comment

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

The idea is that it should be worker-first objects given the potential high volume of events and calls.
It might be fine to be able to transfer it to another worker (when it was discussed, value seems small), as well as to transfer by-products of this object (RTCRtpSendStream for instance).
It is always feasible to add this kind of functionality in the future.

api-outline.md Show resolved Hide resolved
}

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessor {
Copy link

Choose a reason for hiding this comment

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

The idea is that it should be worker-first objects given the potential high volume of events and calls.
It might be fine to be able to transfer it to another worker (when it was discussed, value seems small), as well as to transfer by-products of this object (RTCRtpSendStream for instance).
It is always feasible to add this kind of functionality in the future.

rtpSender.setParameters(parameters);
}
}, 1000);
rtpTransport.createProcessor(new Worker("worker.js"));
Copy link

Choose a reason for hiding this comment

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

createProcessor is a bit odd in terms of naming (it returns undefined for instance).
Also, it might be a bit less flexible/idiomatic than a regular attribute:

  1. We might want in the future to change processors (Worker 1 to worker 2)
  2. Web apps might want to stop using a processor
  3. Web apps might want to preflight creating processors before having RTPTransport.
  4. Having an attribute allows to easily know whether a process is active/which processor is active

I would tend to prefer having a rtpTransport.processor attribute that is set by the web application.
For instance: rtpTransport.processor = new RTCRtpTransportProcessorHandle(worker). Or maybe RTCRtpTransportHandle.

explainer-use-case-1.md Outdated Show resolved Hide resolved
onrtcrtptransportprocessor = (e) => {
setInterval(() => {
for (const [rtpSender, bitrate] of allocateBitrates(e.processor.bandwidthEstimate)) { // Custom
const parameters = rtpSender.getParameters();
Copy link

Choose a reason for hiding this comment

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

These are window objects so it is a bit of an odd example, we should post message to the window env, or keep exposing these values in both RTCRtpTransport and RTCRtpTransportProcessor.

Orphis and others added 3 commits September 19, 2024 13:31
Co-authored-by: Harald Alvestrand <hta+github@alvestrand.no>
Co-authored-by: youennf <youennf@users.noreply.github.com>
@youennf
Copy link

youennf commented Sep 19, 2024

Here is an iteration of the current API, with the handle/processor pattern & using callbacks instead of events+read:

[Exposed=DedicatedWorker]
interface RTCRtpTransportProcessor {
  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;

  attribute RTCPacketizedRTPCallback? packetizedRTPCallback;
  attribute RTCSentRTPCallback? sentRTPCallback;
  attribute RTCReceivedRTPAcksCallback? receivedRTPAcksCallback;
}

callback RTCPacketizedRTPCallback = undefined(sequence<RTCRtpPacket> packets);
callback RTCSentRTPCallback = undefined(sequence<SentRtp> sentRtp);
callback RTCReceivedRTPAcksCallback = undefined(sequence<RtpAcks> acts);

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

[Exposed=Window]
partial interface RTCRtpTransport {
  attribute RTCRtpTransportProcessorHandle? processor;
}

@Orphis
Copy link
Contributor

Orphis commented Oct 9, 2024

Is there precedent for having callbacks stored as attributes in an interface?
All I can find is callbacks passed as arguments and then probably stored in an internal slot, or just regular EventHandler.

@henbos
Copy link
Contributor

henbos commented Oct 9, 2024

Personally I think setting via attribute is fine, however if we want to also specify other parameters then a method may be more appropriate to allow more arguments. For example in #78 we may want to consider throttling the amount of callbacks based on parameters

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.

6 participants