Consider first packet when reading Simulcast IDs

[kvasilye]

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.

[JoTurk]

Would it be possible to add a simple unit test for this?

Thank you so much.

[kvasilye]

Would it be possible to add a simple unit test for this?

Thank you so much.

Would be happy to, can you guide me?

• First, are there existing tests for peer connection which I could add to?
• Second, is it necessary to mock anything?
• Third, are there utilities for constructing RTP packets (for testing)? With extensions?

[JoTurk]

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.

[kvasilye]

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.

[kvasilye]

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 func (r *RTPReceiver) receiveForRid on the "remote" side, and there is only one track and its RID is empty.

So the remote peer connection's onTrack is not fired because we can't find a matching track (by RID).

I'm not even sure if the SDP in this test negotiates Simulcast. I guess it doesn't?

How can I fix this?

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (4c1af4c) to head (94f9209).

Files with missing lines	Patch %	Lines
peerconnection.go	72.72%	2 Missing and 1 partial :warning:
rtptransceiver.go	75.00%	1 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3144      +/-   ##
==========================================
+ Coverage   78.03%   78.12%   +0.09%     
==========================================
  Files          93       93              
  Lines       11769    11778       +9     
==========================================
+ Hits         9184     9202      +18     
+ Misses       2074     2067       -7     
+ Partials      511      509       -2     

Flag	Coverage Δ
go	79.90% <73.33%> (+0.09%)	:arrow_up:
wasm	63.16% <ø> (ø)

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[kvasilye]

There are some failing checks:

Linter

  Error: rtptransceiver.go:294:4: handleUnknownRTPPacket - result payloadType is never used (unparam)
  ) (payloadType PayloadType, paddingOnly bool, err error) {
     ^

The 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 tests

=== RUN   TestPeerConnection_Media_Sample
    peerconnection_media_test.go:206: 
        	Error Trace:	/home/runner/work/webrtc/webrtc/peerconnection_media_test.go:206
        	Error:      	Received unexpected error:
        	            	the DTLS transport has not started yet
        	Test:       	TestPeerConnection_Media_Sample
    peerconnection_media_test.go:207: 
        	Error Trace:	/home/runner/work/webrtc/webrtc/peerconnection_media_test.go:207
        	Error:      	Should be false
        	Test:       	TestPeerConnection_Media_Sample
    util.go:42: Unexpected routines on test end: 
        goroutine 17334 [chan send]:
        github.com/pion/webrtc/v4.TestPeerConnection_Media_Sample.func3()
        	/home/runner/work/webrtc/webrtc/peerconnection_media_test.go:176 +0x2fd
        created by github.com/pion/webrtc/v4.TestPeerConnection_Media_Sample in goroutine 17324
        	/home/runner/work/webrtc/webrtc/peerconnection_media_test.go:166 +0x8ab
--- FAIL: TestPeerConnection_Media_Sample (10.91s)

I 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?

[kvasilye]

@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!

[kvasilye]

@nils-ohlmeier @JoeTurki

I added a working unit test which sends just one packet with Simulcast extensions and validates that the OnTrack callback is called with the right RID.

[JoTurk]

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 :)

[kvasilye]

@JoeTurki @nils-ohlmeier any news, gentlemen?

[JoTurk]

@kvasilye Sorry the test is failing i forgot to report that when i reviewed it this weekend, my bad

[kvasilye]

@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.

[kvasilye]

@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!

[JoTurk]

@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. Thank you a lot :)

[kvasilye]

Any news to share @JoeTurki ?

This is my first time contributing to Pion and I'm a little anxious.

[JoTurk]

@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 :)