Repository: pion/webrtc (go)

Issue:
Improve cryptic "excessive retries in CreateOffer" error message
If a user doesn't attach a `MediaEngine` with at least one codec in the parameters of `NewAPI`, subsequent calls to `PeerConnection.CreateOffer` will always result in an error with the message `excessive retries in CreateOffer`. This message is a little cryptic and doesn't point to a direct way to resolve it; instead, it could suggest adding a MediaEngine with at least one codec.

# Example code that causes the error

```go
settingsEngine := webrtc.SettingEngine{}

// ... //

rtc = webrtc.NewAPI(webrtc.WithSettingEngine(settingsEngine))

peerConnection, err := rtc.NewPeerConnection(webrtc.Configuration{})

// ... //

peerConnection.CreateOffer(nil)

```

# Example code that resolves the error

```go
settingsEngine := webrtc.SettingEngine{}

// ... //

mediaEngine := webrtc.MediaEngine{}
mediaEngine.RegisterDefaultCodecs()

rtc = webrtc.NewAPI(webrtc.WithSettingEngine(settingsEngine), webrtc.WithMediaEngine(&mediaEngine))

peerConnection, err := rtc.NewPeerConnection(webrtc.Configuration{})

// ... //

peerConnection.CreateOffer(nil)

```

Pull request description:
Throw error earlier for RTPSender with no codecs
It isn't possible to send media if a MediaEngine has no codecs. This
catches a common misconfiguration issues that users find themselves in.

Resolves #1702


Patch:
diff --git a/errors.go b/errors.go
index 1dd5d38d..1a1a491f 100644
--- a/errors.go
+++ b/errors.go
@@ -135,6 +135,10 @@ var (
 	// ErrUnsupportedCodec indicates the remote peer doesn't support the requested codec
 	ErrUnsupportedCodec = errors.New("unable to start track, codec is not supported by remote")
 
+	// ErrSenderWithNoCodecs indicates that a RTPSender was created without any codecs. To send media the MediaEngine needs at
+	// least one configured codec.
+	ErrSenderWithNoCodecs = errors.New("unable to populate media section, RTPSender created with no codecs")
+
 	// ErrRTPSenderNewTrackHasIncorrectKind indicates that the new track is of a different kind than the previous/original
 	ErrRTPSenderNewTrackHasIncorrectKind = errors.New("new track must be of the same kind as previous")
 
@@ -225,4 +229,6 @@ var (
 	errCertificatePEMFormatError = errors.New("bad Certificate PEM format")
 
 	errRTPTooShort = errors.New("not long enough to be a RTP Packet")
+
+	errExcessiveRetries = errors.New("excessive retries in CreateOffer")
 )
diff --git a/peerconnection.go b/peerconnection.go
index da640e98..9382a4f1 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -6,7 +6,6 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
-	"errors"
 	"fmt"
 	"io"
 	"strconv"
@@ -586,8 +585,6 @@ func (pc *PeerConnection) hasLocalDescriptionChanged(desc *SessionDescription) b
 	return false
 }
 
-var errExcessiveRetries = errors.New("excessive retries in CreateOffer")
-
 // CreateOffer starts the PeerConnection and generates the localDescription
 // https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-createoffer
 func (pc *PeerConnection) CreateOffer(options *OfferOptions) (SessionDescription, error) { //nolint:gocognit
diff --git a/sdp.go b/sdp.go
index 2af2b2b3..aec31896 100644
--- a/sdp.go
+++ b/sdp.go
@@ -303,6 +303,11 @@ func addTransceiverSDP(d *sdp.SessionDescription, isPlanB, shouldAddCandidates b
 		}
 	}
 	if len(codecs) == 0 {
+		// If we are sender and we have no codecs throw an error early
+		if t.Sender() != nil {
+			return false, ErrSenderWithNoCodecs
+		}
+
 		// Explicitly reject track if we don't have the codec
 		d.WithMedia(&sdp.MediaDescription{
 			MediaName: sdp.MediaName{


Test patch:
diff --git a/mediaengine_test.go b/mediaengine_test.go
index 61c87d91..32f10e28 100644
--- a/mediaengine_test.go
+++ b/mediaengine_test.go
@@ -394,9 +394,9 @@ func TestMediaEngineHeaderExtensionDirection(t *testing.T) {
 		m := &MediaEngine{}
 		registerCodec(m)
 
-		assert.Error(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirectionSendrecv), ErrRegisterHeaderExtensionInvalidDirection)
-		assert.Error(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirectionInactive), ErrRegisterHeaderExtensionInvalidDirection)
-		assert.Error(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirection(0)), ErrRegisterHeaderExtensionInvalidDirection)
+		assert.ErrorIs(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirectionSendrecv), ErrRegisterHeaderExtensionInvalidDirection)
+		assert.ErrorIs(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirectionInactive), ErrRegisterHeaderExtensionInvalidDirection)
+		assert.ErrorIs(t, m.RegisterHeaderExtension(RTPHeaderExtensionCapability{"pion-header-test"}, RTPCodecTypeAudio, RTPTransceiverDirection(0)), ErrRegisterHeaderExtensionInvalidDirection)
 	})
 }
 
diff --git a/ortc_datachannel_test.go b/ortc_datachannel_test.go
index f9be5457..5a75110c 100644
--- a/ortc_datachannel_test.go
+++ b/ortc_datachannel_test.go
@@ -58,7 +58,7 @@ func TestDataChannel_ORTCE2E(t *testing.T) {
 	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)
+	assert.ErrorIs(t, channelA.Send([]byte("ABC")), io.ErrClosedPipe)
+	assert.ErrorIs(t, channelA.SendText("test"), io.ErrClosedPipe)
+	assert.ErrorIs(t, channelA.ensureOpen(), io.ErrClosedPipe)
 }
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index 86bbbcac..d197c373 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -442,7 +442,7 @@ func TestPeerConnection_AnswerWithClosedConnection(t *testing.T) {
 	assert.NoError(t, answerPeerConn.Close())
 
 	_, err = answerPeerConn.CreateAnswer(nil)
-	assert.Error(t, err, &rtcerr.InvalidStateError{Err: ErrConnectionClosed})
+	assert.Equal(t, err, &rtcerr.InvalidStateError{Err: ErrConnectionClosed})
 }
 
 func TestPeerConnection_satisfyTypeAndDirection(t *testing.T) {
diff --git a/peerconnection_media_test.go b/peerconnection_media_test.go
index 0ec4eeb0..65a41e56 100644
--- a/peerconnection_media_test.go
+++ b/peerconnection_media_test.go
@@ -633,11 +633,11 @@ func TestRtpSenderReceiver_ReadClose_Error(t *testing.T) {
 	sender, receiver := tr.Sender(), tr.Receiver()
 	assert.NoError(t, sender.Stop())
 	_, _, err = sender.Read(make([]byte, 0, 1400))
-	assert.Error(t, err, io.ErrClosedPipe)
+	assert.ErrorIs(t, err, io.ErrClosedPipe)
 
 	assert.NoError(t, receiver.Stop())
 	_, _, err = receiver.Read(make([]byte, 0, 1400))
-	assert.Error(t, err, io.ErrClosedPipe)
+	assert.ErrorIs(t, err, io.ErrClosedPipe)
 
 	assert.NoError(t, pc.Close())
 }
@@ -977,11 +977,9 @@ func TestPeerConnection_Simulcast_Probe(t *testing.T) {
 	close(testFinished)
 }
 
-// Assert that CreateOffer can't enter infinite loop
-// We attempt to generate an offer multiple times in case a user
-// has edited the PeerConnection. We can assert this broken behavior with an
-// empty MediaEngine. See pion/webrtc#1656 for full behavior
-func TestPeerConnection_CreateOffer_InfiniteLoop(t *testing.T) {
+// Assert that CreateOffer returns an error for a RTPSender with no codecs
+// pion/webrtc#1702
+func TestPeerConnection_CreateOffer_NoCodecs(t *testing.T) {
 	lim := test.TimeOut(time.Second * 30)
 	defer lim.Stop()
 
@@ -1000,7 +998,7 @@ func TestPeerConnection_CreateOffer_InfiniteLoop(t *testing.T) {
 	assert.NoError(t, err)
 
 	_, err = pc.CreateOffer(nil)
-	assert.Error(t, err, errExcessiveRetries)
+	assert.Equal(t, err, ErrSenderWithNoCodecs)
 
 	assert.NoError(t, pc.Close())
 }
diff --git a/peerconnection_renegotiation_test.go b/peerconnection_renegotiation_test.go
index 71f14e78..a493109f 100644
--- a/peerconnection_renegotiation_test.go
+++ b/peerconnection_renegotiation_test.go
@@ -382,7 +382,7 @@ func TestPeerConnection_Transceiver_Mid(t *testing.T) {
 	assert.True(t, sdpMidHasSsrc(offer, "1", sender2.ssrc), "Expected mid %q with ssrc %d, offer.SDP: %s", "1", sender2.ssrc, offer.SDP)
 
 	_, err = pcAnswer.CreateAnswer(nil)
-	assert.Error(t, err, &rtcerr.InvalidStateError{Err: ErrIncorrectSignalingState})
+	assert.Equal(t, err, &rtcerr.InvalidStateError{Err: ErrIncorrectSignalingState})
 
 	pcOffer.ops.Done()
 	pcAnswer.ops.Done()
diff --git a/peerconnection_test.go b/peerconnection_test.go
index 4c36de7b..f73ced0c 100644
--- a/peerconnection_test.go
+++ b/peerconnection_test.go
@@ -321,7 +321,7 @@ func TestCreateOfferAnswer(t *testing.T) {
 	// so CreateAnswer should return an InvalidStateError
 	assert.Equal(t, answerPeerConn.SignalingState(), SignalingStateStable)
 	_, err = answerPeerConn.CreateAnswer(nil)
-	assert.Error(t, err, &rtcerr.InvalidStateError{Err: ErrIncorrectSignalingState})
+	assert.Error(t, err)
 
 	closePairNow(t, offerPeerConn, answerPeerConn)
 }
diff --git a/rtpreceiver_test.go b/rtpreceiver_test.go
index 56bca12b..65439c52 100644
--- a/rtpreceiver_test.go
+++ b/rtpreceiver_test.go
@@ -7,7 +7,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/pion/transport/packetio"
 	"github.com/pion/transport/test"
 	"github.com/pion/webrtc/v3/pkg/media"
 	"github.com/stretchr/testify/assert"
@@ -41,10 +40,10 @@ func Test_RTPReceiver_SetReadDeadline(t *testing.T) {
 		assert.NoError(t, readErr)
 
 		_, _, readErr = trackRemote.ReadRTP()
-		assert.Error(t, readErr, packetio.ErrTimeout)
+		assert.Error(t, readErr)
 
 		_, _, readErr = r.ReadRTCP()
-		assert.Error(t, readErr, packetio.ErrTimeout)
+		assert.Error(t, readErr)
 
 		seenPacketCancel()
 	})
diff --git a/rtpsender_test.go b/rtpsender_test.go
index d75f115c..c362d874 100644
--- a/rtpsender_test.go
+++ b/rtpsender_test.go
@@ -11,7 +11,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/pion/transport/packetio"
 	"github.com/pion/transport/test"
 	"github.com/pion/webrtc/v3/pkg/media"
 	"github.com/stretchr/testify/assert"
@@ -161,7 +160,7 @@ func Test_RTPSender_SetReadDeadline(t *testing.T) {
 
 	assert.NoError(t, rtpSender.SetReadDeadline(time.Now().Add(1*time.Second)))
 	_, _, err = rtpSender.ReadRTCP()
-	assert.Error(t, err, packetio.ErrTimeout)
+	assert.Error(t, err)
 
 	assert.NoError(t, wan.Stop())
 	closePairNow(t, sender, receiver)
diff --git a/rtptransceiver_test.go b/rtptransceiver_test.go
index f5d46337..c722eed1 100644
--- a/rtptransceiver_test.go
+++ b/rtptransceiver_test.go
@@ -40,7 +40,7 @@ func Test_RTPTransceiver_SetCodecPreferences(t *testing.T) {
 	}
 
 	for _, testCase := range failTestCases {
-		assert.Error(t, tr.SetCodecPreferences(testCase), errRTPTransceiverCodecUnsupported)
+		assert.ErrorIs(t, tr.SetCodecPreferences(testCase), errRTPTransceiverCodecUnsupported)
 	}
 
 	successTestCases := [][]RTPCodecParameters{