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

Bring Your Own Packets #84

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

Bring Your Own Packets #84

wants to merge 2 commits into from

Conversation

henbos
Copy link
Contributor

@henbos henbos commented Jan 13, 2025

Fixes #77.

Allows constructing and modifying RTCRtpPacket such that packets can be read or written without having to construct temporary objects for every packet that goes through the pipeline which could be several thousands/s (easily > 10K/s).

Because you can both read and write to packets in order to recycle them between read and write calls, the the RTCRtpPacket attributes are no longer read-only. The sequence<T> members are still getters because of WebIDL rules but setter methods are added.

The payload is no longer an internal detail, it's an attribute, allowing it to be modified or replaced as well. copyPayloadTo is no longer needed because you can copy with set(packet.payload)

To be discussed:

  • Does this make sense?
  • How does packet.payload buffer work? E.g. what if my buffer is of size 100 but the packet I am reading only contains 50 bytes or vice versa
  • Promises and events is out of scope of this PR but if we do batching then we should have no "thousands of objects per second" overhead there either

Preview | Diff

@henbos henbos requested review from aboba, pthatcher and youennf January 13, 2025 15:54
attribute octet payloadType;
attribute unsigned short sequenceNumber;
attribute unsigned long timestamp;
attribute unsigned long ssrc;
sequence&lt;unsigned long&gt; getCsrcs();
Copy link
Contributor Author

@henbos henbos Jan 14, 2025

Choose a reason for hiding this comment

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

Sequences are copied by value and cannot be attributes, but I think we could use ObservableArray<T> to avoid the array copies as well then nothing is a copy (you would still internally need to copy between JS layer and C++ layer in read and write methods but we would not have any temporary objects imposted on the user of the API IIUC)

@henbos
Copy link
Contributor Author

henbos commented Jan 14, 2025

Another option is if we can move more towards dictionaries, because I'm told native JS objects are more optimized for temporaries than C++ wrapped objects, but that would probably require forcing the app to use BYOB for reading (not just writing) in order to avoid the copyPayloadTo() methods since dictionaries can't have methods and interfaces are slower to GC supposedly

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.

API creates per-packet temporary objects, this is not ideal for GC
1 participant