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

MediaStreams now use their own worker #1107

Merged
merged 9 commits into from
Jan 17, 2018

Conversation

lodoyun
Copy link
Contributor

@lodoyun lodoyun commented Dec 29, 2017

Description
Before this PR MediaStreams shared a worker with the corresponding WebRTCConnection. This PR removes that limitation, instantiating MediaStreams with a different working and properly passing the packet path tasks between the workers.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs

[] It includes documentation for these changes in /doc.

@Trisfald
Copy link
Contributor

Trisfald commented Jan 5, 2018

Hello Pedro!
In my opinion the problem here is in the order of the pipeline components. LayerDetectorHandler sets the is_keyframe value but, during pipeline->read(), PliPacerHandler is visited before LayerDetectorHandler, so is_keyframe is always false. Maybe the solution is to move up PliPacerHandler? You surely know better if there are cons in doing that.
It has worked until now because before you didn't make a copy of the incoming packet; PliPacerHandler was reading the keyframe value of the packet received before the current one.

@lodoyun
Copy link
Contributor Author

lodoyun commented Jan 8, 2018

@Trisfald You were right. There was an issue in DtlsTransport that could cause the pipeline to work with outdated packets, that is what was hiding the pipeline order problem.
Both should be fixed with the new commit. Thanks!

@lodoyun lodoyun changed the title (WIP) MediaStreams now use their own worker MediaStreams now use their own worker Jan 8, 2018
Copy link
Contributor

@jcague jcague left a comment

Choose a reason for hiding this comment

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

LGTM! This is a big step towards Single PC. I left just a couple of minor comments.

@@ -178,9 +178,7 @@ void DtlsTransport::onIceData(packetPtr packet) {
}
return;
} else if (this->getTransportState() == TRANSPORT_READY) {
unprotect_packet_->length = len;
Copy link
Contributor

Choose a reason for hiding this comment

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

could unprotect_packet_ be local now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -68,6 +68,13 @@ WebRtcConnection::~WebRtcConnection() {
ELOG_DEBUG("%s message: Destructor ended", toLog());
}

void WebRtcConnection::close() {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need to change the location? it might conflict with an ongoing PR ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No particular reason, it liked it better that way. I switched it back.

@lodoyun lodoyun merged commit 716a942 into lynckia:master Jan 17, 2018
@lodoyun lodoyun deleted the add/MediaStreamWorker branch April 12, 2018 09:06
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