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

Extended consumer options to provide way to override MID. #586

Merged
merged 9 commits into from
Jun 21, 2021

Conversation

mstyura
Copy link
Contributor

@mstyura mstyura commented Jun 11, 2021

This PR is alternative approach to resolve scenario described in #584 in a way suggested by @ibc.

@ibc could you please clarify what test units are requited which you've mentioned in your comment?

@ibc
Copy link
Member

ibc commented Jun 11, 2021

I'm afraid I cannot review Rust code. Regarding tests, we have tons of test units in mediasoup/test/ folder. This feature (custom per Consumer MID) must be tested in there.

@mstyura mstyura changed the title Extended consumer options to provide way to override MID. WIP: Extended consumer options to provide way to override MID. Jun 11, 2021
@mstyura mstyura marked this pull request as draft June 11, 2021 09:18
@nazar-pc
Copy link
Collaborator

Code looks perfectly fine, for Rust another test case should be added to rust/tests/integration/consumer.rs to make sure correct MID is actually used (use one of the branches in consume_succeeds as an example) as well as it should be mirrored on TypeScript side I think.

@mstyura mstyura changed the title WIP: Extended consumer options to provide way to override MID. Extended consumer options to provide way to override MID. Jun 14, 2021
@mstyura mstyura marked this pull request as ready for review June 14, 2021 06:04
src/Transport.ts Outdated Show resolved Hide resolved
lib/Consumer.d.ts Outdated Show resolved Hide resolved
src/Consumer.ts Outdated Show resolved Hide resolved
@jmillan
Copy link
Member

jmillan commented Jun 14, 2021

LGTM, I've just made few cosmetic change requests.

@jmillan
Copy link
Member

jmillan commented Jun 14, 2021

Adding here too the need for adding the corresponding tests in TS.

@mstyura
Copy link
Contributor Author

mstyura commented Jun 14, 2021

@jmillan could you please suggest next steps to do to make this PR merged?

test/test-Consumer.js Outdated Show resolved Hide resolved
@jmillan
Copy link
Member

jmillan commented Jun 15, 2021

@jmillan could you please suggest next steps to do to make this PR merged?

I have added a new comment on consumer test.

@mstyura mstyura force-pushed the explicit-mid-for-consumers branch from 8d6d8ad to 389ab89 Compare June 15, 2021 09:45
lib/Consumer.d.ts Outdated Show resolved Hide resolved
@mstyura mstyura requested a review from ibc June 17, 2021 14:02
@ibc
Copy link
Member

ibc commented Jun 18, 2021

Thanks for the changes. Please wait until Monday for me to do a proper review and merge&release it.

test/test-Consumer.js Outdated Show resolved Hide resolved
@ibc ibc merged commit 39ef8dd into versatica:v3 Jun 21, 2021
@ibc
Copy link
Member

ibc commented Jun 21, 2021

Released in mediasoup 3.7.16. Thanks!

However... a Rust test is failing: https://github.com/mstyura/mediasoup/runs/2873138895?check_suite_focus=true

@nazar-pc would you mind taking a look?

@ibc
Copy link
Member

ibc commented Jun 21, 2021

Sorry, ignore. That's not the mediasoup mainstream branch :)

@ibc
Copy link
Member

ibc commented Jun 21, 2021

Nope, of course it also happens in mainstream: https://github.com/versatica/mediasoup/runs/2873156693?check_suite_focus=true

@nazar-pc
Copy link
Collaborator

nazar-pc commented Jun 21, 2021

New Rust version was released and added a new lint. Any warnings cause CI errors, will fix and bump crates versions for publishing.

UPD: BTW it literally told you what is wrong, why, how to fix and where to read more about it 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants