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.
|
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3144 +/- ##
==========================================
- Coverage 78.65% 78.57% -0.08%
==========================================
Files 91 91
Lines 11491 11501 +10
==========================================
- Hits 9038 9037 -1
- Misses 1959 1969 +10
- Partials 494 495 +1
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! |
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.