Consider first packet when reading Simulcast IDs #3144
Conversation
The code currently ignores the first packet when reading Simulcast IDs from a new SSRC, and probes only subsequent packets. This commit makes it so that we consider the first packet as well (which we already have read). Helps if the publisher only sends Simulcast IDs on the first packet.
nils-ohlmeier
left a comment
nils-ohlmeier
left a comment
There was a problem hiding this comment.
Thanks a lot for your contribution!
Only one questions/suggestion for improving it further.
|
Would it be possible to add a simple unit test for this? Thank you so much. |
Would be happy to, can you guide me?
|
|
Hello, For this PR you can add tests here https://github.com/pion/webrtc/blob/master/peerconnection_media_test.go No need to mock the actual connections. For making RTP packets, there are some helpers exposed from pion/rtp, also there are plenty of tests to copy or base off. I can try to make a unit test this weekend. Thank you again. |
Thank you for guidance @JoeTurki - I will work on a test in my PR as well. |
|
Here is my "work in progress" for the test. // Assert that we can send just one packet with Simulcast IDs (using extensions) and they will be properly received
t.Run("ExtractIDs", func(t *testing.T) {
track, err := NewTrackLocalStaticRTP(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion")
assert.NoError(t, err)
offerer, answerer, err := newPair()
assert.NoError(t, err)
_, err = offerer.AddTrack(track)
assert.NoError(t, err)
ticker := time.NewTicker(time.Millisecond * 20)
defer ticker.Stop()
testFinished := make(chan struct{})
seenOneStream, seenOneStreamCancel := context.WithCancel(context.Background())
go func() {
sentOnePacket := false
for {
select {
case <-testFinished:
return
case <-ticker.C:
answerer.dtlsTransport.lock.Lock()
if len(answerer.dtlsTransport.simulcastStreams) >= 1 {
seenOneStreamCancel()
}
answerer.dtlsTransport.lock.Unlock()
track.mu.Lock()
if len(track.bindings) == 1 && !sentOnePacket {
sentOnePacket = true
midExtensionID, _, _ := answerer.api.mediaEngine.getHeaderExtensionID(
RTPHeaderExtensionCapability{sdp.SDESMidURI},
)
assert.Greater(t, midExtensionID, 0)
streamIDExtensionID, _, _ := answerer.api.mediaEngine.getHeaderExtensionID(
RTPHeaderExtensionCapability{sdp.SDESRTPStreamIDURI},
)
assert.Greater(t, streamIDExtensionID, 0)
header := &rtp.Header{
Version: 2,
SSRC: util.RandUint32(),
}
header.Extension = true
header.ExtensionProfile = 0x1000
assert.NoError(t, header.SetExtension(uint8(midExtensionID), []byte("0")))
assert.NoError(t, header.SetExtension(uint8(streamIDExtensionID), []byte("some_layer_id")))
_, err = track.bindings[0].writeStream.WriteRTP(header, []byte{0, 1, 2, 3, 4, 5})
assert.NoError(t, err)
}
track.mu.Unlock()
}
}
}()
assert.NoError(t, signalPair(offerer, answerer))
peerConnectionConnected := untilConnectionState(PeerConnectionStateConnected, offerer, answerer)
peerConnectionConnected.Wait()
<-seenOneStream.Done()
closePairNow(t, offerer, answerer)
close(testFinished)
})Now the problem is: The test sends Simulcast extensions with MID="0" and RID="some_layer_id" and they are received by the new code added in this PR, but then we get to So the remote peer connection's I'm not even sure if the SDP in this test negotiates Simulcast. I guess it doesn't? How can I fix this? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3144 +/- ##
==========================================
- Coverage 84.06% 83.98% -0.08%
==========================================
Files 80 80
Lines 9041 9049 +8
==========================================
Hits 7600 7600
- Misses 1022 1026 +4
- Partials 419 423 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There are some failing checks: LinterThe linter is right, but I didn't touch this file, keeping things as they were. Should I fix it - or should we ignore the error since I've not made any changes there? Failing peerconnection testsI ran all peerconnection tests on my machine just now and they all passed. I haven't touched any of the tests which failed. Are they just flakey when run in CI? |
|
@JoeTurki do we have any news on the tests please? I would be happy to continue on implementing a test, but would appreciate guidance from you on how I can make my test work. I've posted my questions above. Thank you! |
|
I added a working unit test which sends just one packet with Simulcast extensions and validates that the |
|
Sorry Kostya, I was busy and I lost track of this, This looks fine to me, I'll review it again by tomorrow, and I would like to leave it open for few few days for others to look at, to make sure we're not merging a behavior change. Thank you a lot :) |
| } | ||
|
|
||
| if _, paddingOnly, err = handleUnknownRTPPacket( | ||
| if paddingOnly, err = handleUnknownRTPPacket( |
There was a problem hiding this comment.
Lint complained that handleUnknownRTPPacket had a return value which the callers did not consume. Fixed.
| } | ||
|
|
||
| return payloadType, paddingOnly, nil | ||
| return paddingOnly, nil |
There was a problem hiding this comment.
Lint complained that handleUnknownRTPPacket had a return value which the callers did not consume. Fixed.
|
@JoeTurki @nils-ohlmeier any news, gentlemen? |
|
@kvasilye Sorry the test is failing i forgot to report that when i reviewed it this weekend, my bad |
…st_ids_from_first_packet
|
@JoeTurki the tests were failing because my branch was out of date. I've synced your master branch, things should be good now. Please run the workflows again. Thank you. |
|
@JoeTurki In the latest test run, all tests passed and we were down to Lint errors. I just fixed those. Please re-run the workflows again, thank you! |
|
@kvasilye Thank you so much, don't worry about the lint issues, we'll fix those ourselves when we merge it, I added it to the 4.2.0 milestone. I'll try to test it with few real applications this weekend. |
|
Any news to share @JoeTurki ? This is my first time contributing to Pion and I'm a little anxious. |
|
@kvasilye Sorry about that, we usually merge minor improvements or bug fixes or opt-in changes fast, but this change affects the main path for the application and it affects the behavior, we can't risk regression, I approved this change, And we'll have to wait few days if others have comments on it, But it's staged to merge with 4.2.0, maybe before that. If you also have other ideas for changes or contributions, please let me know and I'll invite you to the org, Thank you so much :) |
|
@kvasilye Hello, we're releasing 4.2 soon, do you mind enabling " Maintainers are allowed to edit this pull request." I would like to update the branch, and add few tests, if you also want to do that yourself it would be cool |
That's excellent news! It seems I don't have the checkbox for this, maybe because my account is corporate? Here is the help link that shows a screenshot of where it's supposed to be, but it's not there: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork If we can't find a way to enable this, can you explain what you're looking to do and I'll try to do it? |
|
I just clicked "update from upstream" instead and looks like it worked. There were lint errors - I'll be happy to fix them too once the logs are available. |
|
@JoeTurki here is the failure, can you help me fix it please? |
|
@kvasilye thank you so much, you can just add |
Thank you for guidance. Added, and also moved the |
|
There are new failures - |
|
@kvasilye it was a flaky test on macos, I'm going to fix it soon, not related to your pr, sorry about that, and sorry for the trouble with the lint |
|
@kvasilye thank you a lot, and sorry this took too long, we're just about to release 4.2 :) |
|
No worries Joe, I understand you have your own release schedule and that's perfectly fine! Thanks again for being so helpful and guiding me through this. |
3 participants
The code currently ignores the first packet when reading Simulcast IDs from a new SSRC, and probes only subsequent packets. This commit makes it so that we consider the first packet as well (which we already have read). Helps if the publisher only sends Simulcast IDs on the first packet.