-
Notifications
You must be signed in to change notification settings - Fork 463
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
Pass stream ID arrays when calling peerConnection.addTrack and addTransceiver #548
Conversation
…tch C++ versions -addTrack modified from taking a MediaStream pointer to a taking in a vector of strings / media stream ids -addTransceiver modified from taking a RtpTransceiverInit property that contains a vector of media streams, to one that takes a vector of strings / media stream ids -attempted to modify the required test and javascript files -including the marking some potential additional work with TODOs in rtcrtpsender.js
…ia stream id via new RTCMediaStreamInit struct -added a unit test for this new constructor to test/mediastream.js -modified PeerConnection AddTrack to accept a vector of MediaStream's -reverted previous changes to RtpTransceiverInit so we go back to accepting a vector of MediaStreams -enabled rtcrtpsender.js test in all.js test suite -adjusted test/rtcrtpsender.js for new addTrack signature that now accepts a vector of MediaStreams -corrected test/rtcrtpsender.js expected test results for removeTrack
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Thanks for y'all's help here. There is one set of changes we still need related to rest parameter syntax in the JavaScript layer. Once that is completed, I believe we can merge.
|
||
#define DICT(X) RTC_MEDIA_STREAM_INIT ## X | ||
#include "src/dictionaries/macros/impls.h" | ||
#undef DICT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
#define DICT(X) RTC_MEDIA_STREAM_INIT ## X | ||
#include "src/dictionaries/macros/def.h" | ||
#include "src/dictionaries/macros/decls.h" | ||
#undef DICT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
test/destructor/index.js
Outdated
@@ -109,9 +109,9 @@ test('Destructors fire in MediaStreamTrack use-case', async t => { | |||
|
|||
const [pc1, pc2] = await negotiateRTCPeerConnections({ | |||
withPc1(pc1) { | |||
stream1.getTracks().forEach(track => pc1.addTrack(track, stream1)); | |||
stream1.getTracks().forEach(track => pc1.addTrack(track, [stream1])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per comment above, using rest parameter syntax, there should be no change necessary here.
-reverted some changes to test/destructor/index.js and rtcrtpsender.js since array is no longer needed with rest syntax -added test case to rtcrtpsender.js for multiple streams in addTrack and duplicate stream id's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much!!!
closes #546
This pull request contains our proposal to supply media stream IDs from the javascript peer connection addTrack() and addTransceiver() methods. IDs are passed in as an array, which is similar to the underlying WebRTC API.