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

🧠 Substantial refactor of connection and routing logic #60

Merged
merged 29 commits into from
Feb 9, 2021

Conversation

haydenmc
Copy link
Member

@haydenmc haydenmc commented Jan 19, 2021

This payload is a substantial refactor of several parts of the project to address longstanding technical debt and pave the way for automated testing.

  • Network transports have been abstracted into their own interfaces and implementations, allowing for re-use and trivial mocking for classes that depend on transmitting and receiving network data.
  • FTL server implementation has been decoupled from JanusFtl and refactored into several classes with clearer separation of responsibilities: FtlServer, FtlControlConnection, and FtlStream.
  • FtlStream class now works to maintain the order of incoming RTP packets, more intelligently sends NACKs for groups of missing packets instead of one-at-a-time, and calculates additional stats such as running average bitrate.
  • Fewer copies of RTP packets are made as they are received and transmitted to viewers.
  • Only one stream metadata update thread runs instead of one for each stream.
  • Clearer locking patterns (and shared/exclusive locks instead of lock guards) for enforcing thread safety.
  • Simplified JanusFtl handling of FtlStream objects and associated JanusSession viewers, removing FtlStreamStore.
  • Removed RelayThreadPool, as the performance benefits were not yet made clear, and it often resulted in packets being delivered out-of-order
  • Ownership patterns made more clear through replacement of many shared_ptr instances with unique_ptr and plain reference passing.
  • Removed dependency on Janus helper functions JANUS_LOG and rtp.h utility functions.
  • Result<T> type introduced to better indicate success/failure of an operation.
  • Introduced Catch2 for automated testing

@danstiner
Copy link
Collaborator

danstiner commented Jan 26, 2021

Been testing this branch, it's definitely more reliable and at 5mbps I see somewhat decreased packet loss and grey frames.

Using callgrind I see no obvious places in code that waste lots of CPU, previously thumbnail generation alone was ~50% of CPU usage and relay threads another ~25% (seemingly due to constructing a byte vector in an inefficient way)

@haydenmc why not just merge this as is and then add back thumbnail generation? I mean it works great :)

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