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

Add auto subscriptions API #1361

Merged
merged 23 commits into from
Mar 8, 2019
Merged

Conversation

jcague
Copy link
Contributor

@jcague jcague commented Feb 15, 2019

Description

This is a work in progress project. The API is being discussed here

  • It needs and includes Unit Tests

Changes in Client or Server public APIs

There will be changes. TBD.

  • It includes documentation for these changes in /doc.

Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

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

In general it looks good to me. Since it's a draft I'll probably do more passes this is just a couple of things I've seen so far

erizo/src/erizo/WebRtcConnection.cpp Show resolved Hide resolved
erizo/src/erizo/NicerConnection.cpp Show resolved Hide resolved
@jcague jcague marked this pull request as ready for review March 6, 2019 13:03
Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

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

LGTM! I really like the implementation of the filters for autosubscriptions.
Just some comments and suggestions inside but nothing big.

@@ -0,0 +1,2529 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to start version controlling this?

stream_found = true;
}

if (!stream_found && isPublisher()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we already know isPublisher() is true here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch!

return;
}

const erizoIds = [...new Set(streamIds.map(streamId => getErizoQueue(streamId)))];
Copy link
Contributor

Choose a reason for hiding this comment

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

Ugh I don't love spread operations ... I'd prefer Array.from here because it makes it very explicit what we want to get as a result.
It's just a comment though, no need to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm.... I like that solution too, will change it


connection.init(knownStreamIds[0], constraints);
promises.push(connection.createOfferPromise);
if (options.singlePC) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're creating a single Connection and we're not supporting singlePC for this at this moment. I'd suggest using this parameter (singlePC) to log an error

@jcague jcague merged commit 980b0d6 into lynckia:master Mar 8, 2019
@jcague jcague deleted the add/auto_subscriptions_api branch March 8, 2019 10:54
@chaitanyaprem
Copy link

I am running into the following error while doing fresh install in ubuntu-14.04 LTS desktop. My installUbuntuDeps.sh and installNuve.sh is successful without any errors. installErizo.sh is failing with the error.
erizoinstall.txt

@chaitanyaprem
Copy link

I am running into the following error while doing fresh install in ubuntu-14.04 LTS desktop. My installUbuntuDeps.sh and installNuve.sh is successful without any errors. installErizo.sh is failing with the error.
erizoinstall.txt

This error got fixed after I installed boost version 1.65.0. Looks like install script needs to be updated to handle this.

Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
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.

3 participants