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 sender report trace event #1267

Merged
merged 6 commits into from
Jan 3, 2024
Merged

Conversation

GithubUser8080
Copy link
Contributor

@GithubUser8080 GithubUser8080 commented Dec 15, 2023

This PR adds a trace event for incoming RTCP sender reports from producers, to provide timestamp information in the node API. Covers use cases of server-side stream synchronization (like recording).

Usage

producer.enableTraceEvent(['sr'])

emits

{
  type: 'sr',
  timestamp: 6348927,
  direction: 'in',
  info: {
    ssrc: 481847794,
    ntpSec: 3911629480,
    ntpFrac: 2788138149,
    rtpTs: 2712133695,
    packetCount: 928,
    octetCount: 75009
  }
}

Resolves issue #1248

Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Looks good but Rust changes are also required.

@GithubUser8080
Copy link
Contributor Author

Not familiar with rust so I am relying on the videoroom example for manual testing, hope it is enough.

@ibc
Copy link
Member

ibc commented Dec 15, 2023

Just do grep looking for similar FBS types in worker/rust folder and will be easy.

@ibc
Copy link
Member

ibc commented Dec 15, 2023

Run cargo fmt to fix lint/clippy formating issues.

Copy link
Member

@jmillan jmillan left a comment

Choose a reason for hiding this comment

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

Looks good so far 👍, just a cosmetic naming change requested.

worker/fbs/producer.fbs Outdated Show resolved Hide resolved
worker/fbs/producer.fbs Outdated Show resolved Hide resolved
worker/include/RTC/Producer.hpp Outdated Show resolved Hide resolved
worker/src/RTC/Producer.cpp Outdated Show resolved Hide resolved
@ibc
Copy link
Member

ibc commented Jan 2, 2024

Changelog entry missing and this must be documented in the website (I'll do).

@@ -1234,3 +1236,34 @@ impl BweTraceInfo {
}
}
}

/// BWE info in trace event.
Copy link
Member

Choose a reason for hiding this comment

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

This is not "BWE info".

Copy link
Member

Choose a reason for hiding this comment

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

I'll merge and make this change in v3.

@ibc ibc merged commit 87032a8 into versatica:v3 Jan 3, 2024
19 checks passed
@GithubUser8080 GithubUser8080 deleted the add-sr-trace branch January 8, 2024 07:40
@GithubUser8080
Copy link
Contributor Author

Thank you

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.

3 participants