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

Improved nack generator #1096

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Trisfald
Copy link
Contributor

Description

This PR adds a new nack generator using an algorithm very similar to the one implemented Chrome. Tthe only difference is that Chrome leverages the decoder to have a more consistent PLI generation. RtcpFeedbackGenerationHandler has been slightly modified, because there are some changes between the old and the new interface; for example it sends a PLI when it fails to recover a packet.

We have deployed this new generator few months ago and got very good results.

[X] It needs and includes Unit Tests

Changes in Client or Server public APIs

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

@lodoyun
Copy link
Contributor

lodoyun commented Dec 19, 2017

The current implementation is also based on Chrome's, with some modifications. It does lack the keyframe and PLI logic, and I think it's a very good addition. However, has a couple of things your proposal lacks has such as limiting the number of retransmissions and the time to send a particular NACK.
I think the best solution would be a combination of both. Would you consider adding the PLI and keyframe logic to RtpNackGenerator.cpp? It would probably involve less work than the opposite as it already uses the style and the helpers we use in the rest of erizo.

@Trisfald
Copy link
Contributor Author

At the beginning we changed implementation because with the default one we weren't satisfied with packet recovering. For example at 5% ingress packet loss we saw that the default generator somewhat worse.
Then we also liked Chrome's approach to limit nacks. Nacks for a packet are retransmitted as long as they are not too old (the nack is still in the queue), and the minimum interval between nack packets is nack_interval
However, I see value in limiting the number of retransmissions. I think these are two different approach to solve the same problem. I can try to merge the keyframe logic into master, but not really soon. Got myself into a bug of the original generator that made me lose quite a bit of time... I will push the fix asap for this.

@lodoyun
Copy link
Contributor

lodoyun commented Dec 20, 2017

I think they can be complementary in some cases and there's value in having both mechanisms in place.
Let me know when/if you start working on it. I can't look into it right now either but I may when I finish the things I'm currently focusing on.

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.

2 participants