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

Avoid dropping packets for simulcast probes #2777

Open
wants to merge 1 commit into
base: v3
Choose a base branch
from

Conversation

cptpcrd
Copy link

@cptpcrd cptpcrd commented May 29, 2024

Description

Currently, when Pion is receiving simulcast, PeerConnection.handleIncomingSSRC reads (at least) 2 packets: one packet is used to determine the payload type, and one (or more) packets are used to determine the MID, RID, and repair stream ID. This means that the first video frame is always partially lost, which is undesirable.

To fix this:

  1. Get the initial payload type from the SRTP read stream to avoid having to consume the first packet.
  2. Peek (instead of reading) from the SRTP read stream to avoid having to consume the second packet (assuming that the MID/RID/repair stream ID come in right away).

(This patch relies on corresponding changes to pion/srtp which I will PR shortly and link back here.)

Reference issue

N/A

Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.09%. Comparing base (eb6e395) to head (fd2aab2).
Report is 10 commits behind head on v3.

Additional details and impacted files
@@             Coverage Diff             @@
##               v3    #2777       +/-   ##
===========================================
- Coverage   76.32%   65.09%   -11.23%     
===========================================
  Files          87       64       -23     
  Lines        9846     3100     -6746     
===========================================
- Hits         7515     2018     -5497     
+ Misses       1866      954      -912     
+ Partials      465      128      -337     
Flag Coverage Δ
go ?
wasm 65.09% <ø> (+0.64%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adriancable
Copy link
Contributor

@cptpcrd - does your issue #1 (consuming the first packet) also impact non-simulcast?

@cptpcrd
Copy link
Author

cptpcrd commented Jun 6, 2024

@cptpcrd - does your issue #1 (consuming the first packet) also impact non-simulcast?

As far as I'm aware, no.

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.

2 participants