Add ICE candidate event handlers #546
Conversation
|
This PR doesn't add all methods related to ICE candidate gathering. Before we merge this PR, I plan to make another pass to see if there are any other easy changes to make the Go and JavaScript/Wasm APIs more similar. |
Pull Request Test Coverage Report for Build 2657
💛 - Coveralls |
|
There are still potentially more event handlers or methods to add in the future, but I've done all I want to do for this PR. The Go implementation and the JavaScript/Wasm bindings now have matching function signatures and similar enough behavior for |
peerconnection.go
Outdated
| if pc.onICEGatheringStateChangeHandler != nil { | ||
| // Note: Gathering is already done at this point, but some clients might | ||
| // still expect the state change handler to be triggered. | ||
| pc.onICEGatheringStateChangeHandler() |
There was a problem hiding this comment.
I think we need to call this in a goroutine (copying how onTrack does it) otherwise someone via the API could do something blocking (and then pc.mu.Lock() could be held for a long time)
Sean-Der
force-pushed
the
ice-gathering-events
branch
from
18fd5e3
to
26a3616
Compare
albrow
force-pushed
the
ice-gathering-events
branch
from
26a3616
to
2df5644
Compare
|
@Sean-Der I applied changes based on your feedback. Ready for another look. |
There was a problem hiding this comment.
LGTM!
Do you feel good about it testing wise? It might be nice to assert we get candidates or we got a nil, also another useful test is the blocking in the callbacks doesn't break anything.
Also mind squashing down into single commit? Just nice in case we have to revert things, but if you think it makes history reading harder in this case feel free to disregard!
Testing could definitely be improved, but I think it's good enough for now. The updated |
|
And yes, I'll squash before merging. |
Add OnICECandidate and OnICEGatheringStateChange methods to PeerConnection. The main goal of this change is to improve API compatibility with the JavaScript/Wasm bindings. It does not actually add trickle ICE support or change the ICE candidate gathering process, which is still synchronous in the Go implementation. Rather, it fires the appropriate events similar to they way they would be fired in a true trickle ICE process. Remove unused OnNegotiationNeeded event handler. This handler is not required for most applications and would be difficult to implement in Go. This commit removes the handler from the JavaScript/Wasm bindings, which leads to a more similar API for Go and JavaScript/Wasm. Add OnICEGatheringStateChange to the JavaScript/Wasm bindings. Also changes the Go implementation so that the function signatures match.
albrow
force-pushed
the
ice-gathering-events
branch
2 times, most recently
from
53c0629
to
b24bea3
Compare
Description
Add OnICECandidate and OnICEGatheringStateChange methods to PeerConnection. The main goal of this change is to improve API compatibility with the JavaScript/Wasm bindings. It does not actually add trickle ICE support or change the ICE candidate gathering process, which is still synchronous in the Go implementation. Rather, it fires the appropriate events similar to they way they would be fired in a true trickle ICE process.