Repository: pion/webrtc
Pull request #1898 (2021-07-27 23:42:32)

Check for nil transceivers on get parameters
Add public methods to allow set stream and track id, this is useful
when using ORTC API


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#1898](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (d76fe8c) into [master](https://codecov.io/gh/pion/webrtc/commit/5047c6b01014ccbfb38907b6dc08294cf9ec9ebf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (5047c6b) will **decrease** coverage by `4.54%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/pion/webrtc/pull/1898/graphs/tree.svg?width=650&height=150&src=pr&token=XOOuQo07I2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master    #1898      +/-   ##
==========================================
- Coverage   75.69%   71.14%   -4.55%     
==========================================
  Files          84       61      -23     
  Lines        6039     2277    -3762     
==========================================
- Hits         4571     1620    -2951     
+ Misses       1081      549     -532     
+ Partials      387      108     -279     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `71.14% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [constants.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29uc3RhbnRzLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwY29kZWMuZ28=) | `15.00% <0.00%> (-75.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `8.33% <0.00%> (-58.34%)` | :arrow_down: |
| [icemux.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNlbXV4Lmdv) | `0.00% <0.00%> (-55.56%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| ... and [33 more](https://codecov.io/gh/pion/webrtc/pull/1898/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [5047c6b...d76fe8c](https://codecov.io/gh/pion/webrtc/pull/1898?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

Antonito: I agree this is useful – I've been wanting to do this too.

My only concern (which also applies to https://github.com/pion/webrtc/pull/1831) is we keep adding ORTC specific stuff. Couldn't this be solved by refactoring RTPReceiver stuff?
My point is by using ORTC, ideally we should be able to do the exact same stuff as the PeerConnection does – without introducing extra code.
Sean-Der: I agree with @Antonito 

I worry if we add non-standard APIs users will expect to use them with WebRTC, and could cause bugs. It also makes the GoDoc bigger.

I haven't used the ORTC Media API myself personally. What happens because we don't do this today? Might be good to add examples/tests first.
OrlandoCo: @Antonito @Sean-Der the issue is that streamID and trackID are set internally there is no standard method to modify them. Even if we refractor RTPReceiver we would need an outside method to set stream/track ID. The easy way is to modify them on track remote rather receivers where can be more confusing with simulcast.

The other way is making TrackRemote an interface and allow setting custom TrackRemotes for Receivers, which would be a little bit more complicated and confusing. 


Sean-Der: @OrlandoCo I spend some time looking through the [ORTC Draft](https://draft.ortc.org/#rtcrtpreceiver*) and I couldn't find how the id/label would get set for a browser implementation. For WebRTC this information is communicated via SDP, but I don't set an equivalent API for ORTC?

So it looks like for ORTC it is just going to be empty? I still would love to design this in a way that we are 'ORTC Compatible'. I think the right thing here is for you to embed tracks inside another struct that contains any metadata you need.
Sean-Der: @OrlandoCo that other bug fix looks great though! Would you mind adding some minor ORTC Media tests? It doesn't have to be E2E even, just something to start us off with.
OrlandoCo: @Sean-Der With ORTC is not necessary the streamID or trackID since you have more controls over the way you signal the information you can add whatever identifier you need. But let me explain you why do we need ORTC, for ion-sfu we are building an API for relay tracks from server to server, to make easy this process is done via ORTC so no need to do negotiation signaling by just forwarding a single message.

But ORTC is only for server to server, clients will still be using webrtc. Thats why we need to set correct Track and stream ID.

Fort tests, I can add them for sure ! 

Patch:
diff --git a/rtpsender.go b/rtpsender.go
index b6c4bd17..840524cd 100644
--- a/rtpsender.go
+++ b/rtpsender.go
@@ -38,7 +38,7 @@ type RTPSender struct {
 	api *API
 	id  string
 
-	tr *RTPTransceiver
+	rtpTransceiver *RTPTransceiver
 
 	mu                     sync.RWMutex
 	sendCalled, stopCalled chan struct{}
@@ -90,10 +90,10 @@ func (r *RTPSender) setNegotiated() {
 	r.negotiated = true
 }
 
-func (r *RTPSender) setRTPTransceiver(tr *RTPTransceiver) {
+func (r *RTPSender) setRTPTransceiver(rtpTransceiver *RTPTransceiver) {
 	r.mu.Lock()
 	defer r.mu.Unlock()
-	r.tr = tr
+	r.rtpTransceiver = rtpTransceiver
 }
 
 // Transport returns the currently-configured *DTLSTransport or nil
@@ -119,7 +119,11 @@ func (r *RTPSender) getParameters() RTPSendParameters {
 			},
 		},
 	}
-	sendParameters.Codecs = r.tr.getCodecs()
+	if r.rtpTransceiver != nil {
+		sendParameters.Codecs = r.rtpTransceiver.getCodecs()
+	} else {
+		sendParameters.Codecs = r.api.mediaEngine.getCodecsByKind(r.track.Kind())
+	}
 	return sendParameters
 }
 
@@ -145,7 +149,7 @@ func (r *RTPSender) ReplaceTrack(track TrackLocal) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()
 
-	if track != nil && r.tr.kind != track.Kind() {
+	if track != nil && r.rtpTransceiver.kind != track.Kind() {
 		return ErrRTPSenderNewTrackHasIncorrectKind
 	}
 


Test patch:
diff --git a/ortc_datachannel_test.go b/ortc_datachannel_test.go
new file mode 100644
index 00000000..f9be5457
--- /dev/null
+++ b/ortc_datachannel_test.go
@@ -0,0 +1,64 @@
+// +build !js
+
+package webrtc
+
+import (
+	"io"
+	"testing"
+	"time"
+
+	"github.com/pion/transport/test"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDataChannel_ORTCE2E(t *testing.T) {
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	stackA, stackB, err := newORTCPair()
+	assert.NoError(t, err)
+
+	awaitSetup := make(chan struct{})
+	awaitString := make(chan struct{})
+	awaitBinary := make(chan struct{})
+	stackB.sctp.OnDataChannel(func(d *DataChannel) {
+		close(awaitSetup)
+
+		d.OnMessage(func(msg DataChannelMessage) {
+			if msg.IsString {
+				close(awaitString)
+			} else {
+				close(awaitBinary)
+			}
+		})
+	})
+
+	assert.NoError(t, signalORTCPair(stackA, stackB))
+
+	var id uint16 = 1
+	dcParams := &DataChannelParameters{
+		Label: "Foo",
+		ID:    &id,
+	}
+	channelA, err := stackA.api.NewDataChannel(stackA.sctp, dcParams)
+	assert.NoError(t, err)
+
+	<-awaitSetup
+
+	assert.NoError(t, channelA.SendText("ABC"))
+	assert.NoError(t, channelA.Send([]byte("ABC")))
+
+	<-awaitString
+	<-awaitBinary
+
+	assert.NoError(t, stackA.close())
+	assert.NoError(t, stackB.close())
+
+	// attempt to send when channel is closed
+	assert.Error(t, channelA.Send([]byte("ABC")), io.ErrClosedPipe)
+	assert.Error(t, channelA.SendText("test"), io.ErrClosedPipe)
+	assert.Error(t, channelA.ensureOpen(), io.ErrClosedPipe)
+}
diff --git a/ortc_media_test.go b/ortc_media_test.go
new file mode 100644
index 00000000..3fdbb3d3
--- /dev/null
+++ b/ortc_media_test.go
@@ -0,0 +1,68 @@
+// +build !js
+
+package webrtc
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/pion/transport/test"
+	"github.com/pion/webrtc/v3/pkg/media"
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_ORTC_Media(t *testing.T) {
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	stackA, stackB, err := newORTCPair()
+	assert.NoError(t, err)
+
+	assert.NoError(t, stackA.api.mediaEngine.RegisterDefaultCodecs())
+	assert.NoError(t, stackB.api.mediaEngine.RegisterDefaultCodecs())
+
+	assert.NoError(t, signalORTCPair(stackA, stackB))
+
+	track, err := NewTrackLocalStaticSample(RTPCodecCapability{MimeType: MimeTypeVP8}, "video", "pion")
+	assert.NoError(t, err)
+
+	rtpSender, err := stackA.api.NewRTPSender(track, stackA.dtls)
+	assert.NoError(t, err)
+	assert.NoError(t, rtpSender.Send(rtpSender.GetParameters()))
+
+	rtpReceiver, err := stackB.api.NewRTPReceiver(RTPCodecTypeVideo, stackB.dtls)
+	assert.NoError(t, err)
+	assert.NoError(t, rtpReceiver.Receive(RTPReceiveParameters{Encodings: []RTPDecodingParameters{
+		{RTPCodingParameters: rtpSender.GetParameters().Encodings[0].RTPCodingParameters},
+	}}))
+
+	seenPacket, seenPacketCancel := context.WithCancel(context.Background())
+	go func() {
+		track := rtpReceiver.Track()
+		_, _, err := track.ReadRTP()
+		assert.NoError(t, err)
+
+		seenPacketCancel()
+	}()
+
+	func() {
+		for range time.Tick(time.Millisecond * 20) {
+			select {
+			case <-seenPacket.Done():
+				return
+			default:
+				assert.NoError(t, track.WriteSample(media.Sample{Data: []byte{0xAA}, Duration: time.Second}))
+			}
+		}
+	}()
+
+	assert.NoError(t, rtpSender.Stop())
+	assert.NoError(t, rtpReceiver.Stop())
+
+	assert.NoError(t, stackA.close())
+	assert.NoError(t, stackB.close())
+}
diff --git a/datachannel_ortc_test.go b/ortc_test.go
similarity index 63%
rename from datachannel_ortc_test.go
rename to ortc_test.go
index 1f769c09..1e416194 100644
--- a/datachannel_ortc_test.go
+++ b/ortc_test.go
@@ -3,95 +3,9 @@
 package webrtc
 
 import (
-	"io"
-	"testing"
-	"time"
-
-	"github.com/pion/transport/test"
 	"github.com/pion/webrtc/v3/internal/util"
-	"github.com/stretchr/testify/assert"
 )
 
-func TestDataChannel_ORTCE2E(t *testing.T) {
-	// Limit runtime in case of deadlocks
-	lim := test.TimeOut(time.Second * 20)
-	defer lim.Stop()
-
-	report := test.CheckRoutines(t)
-	defer report()
-
-	stackA, stackB, err := newORTCPair()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	awaitSetup := make(chan struct{})
-	awaitString := make(chan struct{})
-	awaitBinary := make(chan struct{})
-	stackB.sctp.OnDataChannel(func(d *DataChannel) {
-		close(awaitSetup)
-
-		d.OnMessage(func(msg DataChannelMessage) {
-			if msg.IsString {
-				close(awaitString)
-			} else {
-				close(awaitBinary)
-			}
-		})
-	})
-
-	err = signalORTCPair(stackA, stackB)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	var id uint16 = 1
-	dcParams := &DataChannelParameters{
-		Label: "Foo",
-		ID:    &id,
-	}
-	channelA, err := stackA.api.NewDataChannel(stackA.sctp, dcParams)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	<-awaitSetup
-
-	err = channelA.SendText("ABC")
-	if err != nil {
-		t.Fatal(err)
-	}
-	err = channelA.Send([]byte("ABC"))
-	if err != nil {
-		t.Fatal(err)
-	}
-	<-awaitString
-	<-awaitBinary
-
-	err = stackA.close()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	err = stackB.close()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// attempt to send when channel is closed
-	err = channelA.Send([]byte("ABC"))
-	assert.Error(t, err)
-	assert.Equal(t, io.ErrClosedPipe, err)
-
-	err = channelA.SendText("test")
-	assert.Error(t, err)
-	assert.Equal(t, io.ErrClosedPipe, err)
-
-	err = channelA.ensureOpen()
-	assert.Error(t, err)
-	assert.Equal(t, io.ErrClosedPipe, err)
-}
-
 type testORTCStack struct {
 	api      *API
 	gatherer *ICEGatherer
@@ -185,10 +99,10 @@ func (s *testORTCStack) close() error {
 }
 
 type testORTCSignal struct {
-	ICECandidates    []ICECandidate   `json:"iceCandidates"`
-	ICEParameters    ICEParameters    `json:"iceParameters"`
-	DTLSParameters   DTLSParameters   `json:"dtlsParameters"`
-	SCTPCapabilities SCTPCapabilities `json:"sctpCapabilities"`
+	ICECandidates    []ICECandidate
+	ICEParameters    ICEParameters
+	DTLSParameters   DTLSParameters
+	SCTPCapabilities SCTPCapabilities
 }
 
 func newORTCPair() (stackA *testORTCStack, stackB *testORTCStack, err error) {
diff --git a/rtpsender_test.go b/rtpsender_test.go
index a187a718..d75f115c 100644
--- a/rtpsender_test.go
+++ b/rtpsender_test.go
@@ -228,7 +228,7 @@ func Test_RTPSender_ReplaceTrack_InvalidCodecChange(t *testing.T) {
 	rtpSender, err := sender.AddTrack(trackA)
 	assert.NoError(t, err)
 
-	err = rtpSender.tr.SetCodecPreferences([]RTPCodecParameters{{
+	err = rtpSender.rtpTransceiver.SetCodecPreferences([]RTPCodecParameters{{
 		RTPCodecCapability: RTPCodecCapability{MimeType: MimeTypeVP8},
 		PayloadType:        96,
 	}})