-
Notifications
You must be signed in to change notification settings - Fork 8
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
Sending RTCP Reports #81
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #81 +/- ##
==========================================
- Coverage 88.80% 88.06% -0.74%
==========================================
Files 31 31
Lines 1411 1492 +81
==========================================
+ Hits 1253 1314 +61
- Misses 158 178 +20
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! No blocking changes except this comment: #81 (comment)
@spec new(MediaStreamTrack.t(), RTPCodecParameters.t() | nil) :: t() | ||
def new(track, codec) do | ||
report_recorder = %ReportRecorder{ | ||
clock_rate: codec && codec.clock_rate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smart! Looks like optional 🤯
@@ -1,5 +1,5 @@ | |||
defmodule ExWebRTC.RTPReceiver.ReportRecorder do | |||
@moduledoc false | |||
@moduledoc nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moduledoc nil | |
@moduledoc false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish, ReportRecorder
is a part of a RTPReceiver.t()
and RTPSender.t()
typespecs, we can either make it public (like here) or remove the fields from sender's and receiver's typespecs.
@@ -132,14 +146,18 @@ defmodule ExWebRTC.RTPTransceiver do | |||
|
|||
codecs = get_codecs(mline, config) | |||
rtp_hdr_exts = get_rtp_hdr_extensions(mline, config) | |||
sender = RTPSender.update(transceiver.sender, mid, List.first(codecs), rtp_hdr_exts) | |||
codec = List.first(codecs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, the other side may send data using any payload format negotiated in SDP offer/answer. IIRC, they can even switch between codecs during the session without renegotiation. I don't want to handle that behaviour but maybe we should somewhere ensure that a packet we receive has expected payload type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was considering this, but we only really care about the clock rate here (if codec changes, but the clock rate remains the same, it's all good).
AFAIR if clock rate changes, the source should generate a new RTP stream (and SSRC) for that, so we don't really have to care (I cannot find the RFC that specified that atm).
Even if it did, we only risk one invalid report, so it's not very critical I'd say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR utilises previously added
RTPSender.ReportRecorder
andRTPReceiver.ReportRecorder
modules to actually track incoming packets and send corresponding RTCP Sender and Receiver Reports.