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

feat: initial polyfill functionality #173

Merged
merged 5 commits into from
Aug 8, 2023

Conversation

ThaUnknown
Copy link
Contributor

No description provided.

@ThaUnknown ThaUnknown mentioned this pull request Aug 1, 2023
not required as we use node 18 as baseline
get iceConnectionState() {
return this.#iceConnectionState;
return this.#peerConnection.state();
Copy link
Contributor

@achingbrain achingbrain Aug 1, 2023

Choose a reason for hiding this comment

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

This returns the same underlying prop for .connectionState and .iceConnectionState - I wasn't sure which one to use on the underlying PeerConnection but we don't use this in @libp2p/webrtc so it wasn't a big deal.

It would need fixing here though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, the type checks error on this, so it's visible to us that it needs to be changed

Copy link
Contributor

Choose a reason for hiding this comment

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

I can add iceConnectionState to libdatachannel, is there something else missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ice restart, Receivers, Senders, Transceivers, and maybe stats?

Copy link
Contributor

@paullouisageneau paullouisageneau Aug 7, 2023

Choose a reason for hiding this comment

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

About Receivers, Senders, and Transceivers, do you plan to polyfill tracks? I don't think this is possible currently, given tracks are lower level in libdatachannel and basically transceivers themselves (libdatachannel doesn't handle media encoding/decoding). For data channels however it can be done 1:1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know? I haven't even had time to test this as libdatachannel segfaults on my os and I haven't had time to debug it, we'd need to implement stuff from https://devdocs.io/dom/rtcstats/type
and I don't know what we'd need to track ourselves and what exists in libdatachannel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

polyfilling tracks means polyfilling mediasource, so pretty much faking it like mediasource.fromFilePath or some other non standard stuff, and a transceiver needs to exist to add a track, so then that might need to be faked too, idk dont have the time to check

this pr is just for the bare minimum simple functionality, so for now it's not something we need to worry about, but generating certificates and getting ICE data would be nice

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I added the ICE connection state to libdatachannel in paullouisageneau/libdatachannel#944

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks @paullouisageneau
I have integrated this feature now.
Is it possible to add restartIce function also or not possible?

@ThaUnknown
Copy link
Contributor Author

NDC also needs to expose remote description which exist in libdatachannel

@ThaUnknown ThaUnknown changed the title feat: initial functionality feat: initial polyfill functionality Aug 1, 2023
@murat-dogan
Copy link
Owner

NDC also needs to expose remote description which exist in libdatachannel

I have added this

@murat-dogan
Copy link
Owner

I am merging as it is. I could make changes though.

@murat-dogan murat-dogan merged commit 0cacec7 into murat-dogan:polyfill Aug 8, 2023
@ThaUnknown
Copy link
Contributor Author

@murat-dogan please tell me you at least tested this, because I didn't

also paul is gonna add ice connection state, maybe ice reset and we already have remote description in libdatachannel, all of those need to be exposed in node-datachannel

@ThaUnknown ThaUnknown deleted the polyfill2 branch August 8, 2023 19:03
@murat-dogan
Copy link
Owner

@ThaUnknown thank you for your help.
But at this point I need to merge and clean all of these things, otherwise it will be a mess.
For sure it will take some time.

I already added remotedesc method.

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.

4 participants