Incorporate suggestions from code review

Review done by Max Hawkins on November 27, 2018
against commit 874b648ba5
Relates to #119

internal/network/manager.go

@@ -16,11 +16,11 @@ import (
 	"github.com/pkg/errors"
 )
 
-// Transportpair allows the application to be notified about both Rtp
+// TransportPair allows the application to be notified about both Rtp
 // and Rtcp messages incoming from the remote host
-type Transportpair struct {
-	Rtp  chan<- *rtp.Packet
-	Rtcp chan<- *rtcp.PacketWithHeader
+type TransportPair struct {
+	RTP  chan<- *rtp.Packet
+	RTCP chan<- *rtcp.PacketWithHeader
 }
 
 // Manager contains all network state (DTLS, SRTP) that is shared between ports
@@ -38,7 +38,8 @@ type Manager struct {
 	dataChannelEventHandler DataChannelEventHandler
 
 	bufferTransportGenerator BufferTransportGenerator
-	bufferTransportPairs     map[uint32]*Transportpair
+	pairsLock                sync.RWMutex
+	bufferTransportPairs     map[uint32]*TransportPair
 
 	srtpInboundContextLock sync.RWMutex
 	srtpInboundContext     *srtp.Context
@@ -55,9 +56,11 @@ type Manager struct {
 //AddTransportPair notifies the network manager that an RTCTrack has
 //been created externally, and packets may be incoming with this ssrc
 func (m *Manager) AddTransportPair(ssrc uint32, Rtp chan<- *rtp.Packet, Rtcp chan<- *rtcp.PacketWithHeader) {
+	m.pairsLock.Lock()
+	defer m.pairsLock.Unlock()
 	bufferTransport := m.bufferTransportPairs[ssrc]
 	if bufferTransport == nil {
-		bufferTransport = &Transportpair{Rtp, Rtcp}
+		bufferTransport = &TransportPair{Rtp, Rtcp}
 		m.bufferTransportPairs[ssrc] = bufferTransport
 	}
 }
@@ -66,7 +69,7 @@ func (m *Manager) AddTransportPair(ssrc uint32, Rtp chan<- *rtp.Packet, Rtcp cha
 func NewManager(btg BufferTransportGenerator, dcet DataChannelEventHandler, ntf ICENotifier) (m *Manager, err error) {
 	m = &Manager{
 		iceNotifier:              ntf,
-		bufferTransportPairs:     make(map[uint32]*Transportpair),
+		bufferTransportPairs:     make(map[uint32]*TransportPair),
 		bufferTransportGenerator: btg,
 		dataChannelEventHandler:  dcet,
 	}
@@ -98,12 +101,15 @@ func NewManager(btg BufferTransportGenerator, dcet DataChannelEventHandler, ntf
 	return m, err
 }
 
-func (m *Manager) getBufferTransports(ssrc uint32) *Transportpair {
-	fmt.Println(m.bufferTransportPairs)
+func (m *Manager) getBufferTransports(ssrc uint32) *TransportPair {
+	m.pairsLock.RLock()
+	defer m.pairsLock.RUnlock()
 	return m.bufferTransportPairs[ssrc]
 }
 
-func (m *Manager) getOrCreateBufferTransports(ssrc uint32, payloadtype uint8) *Transportpair {
+func (m *Manager) getOrCreateBufferTransports(ssrc uint32, payloadtype uint8) *TransportPair {
+	m.pairsLock.Lock()
+	defer m.pairsLock.Unlock()
 	bufferTransport := m.bufferTransportPairs[ssrc]
 	if bufferTransport == nil {
 		bufferTransport = m.bufferTransportGenerator(ssrc, payloadtype)

internal/network/network.go

@@ -6,8 +6,8 @@ import (
 )
 
 // BufferTransportGenerator generates a new channel for the associated SSRC
-// This channel is used to send RTP packets to users of pion-WebRTC
-type BufferTransportGenerator func(uint32, uint8) *Transportpair
+// This channel is used to send RTP and RTCP packets to users of pion-WebRTC
+type BufferTransportGenerator func(uint32, uint8) *TransportPair
 
 // ICENotifier notifies the RTCPeerConnection if ICE state has changed
 type ICENotifier func(ice.ConnectionState)

internal/network/port-receive.go

@@ -53,9 +53,9 @@ func (p *port) handleSRTP(buffer []byte) {
 
 				f := func(ssrc uint32) {
 					bufferTransport := p.m.getBufferTransports(ssrc)
-					if bufferTransport != nil && bufferTransport.Rtcp != nil {
+					if bufferTransport != nil && bufferTransport.RTCP != nil {
 						select {
-						case bufferTransport.Rtcp <- &report:
+						case bufferTransport.RTCP <- &report:
 						default:
 						}
 					}
@@ -100,9 +100,9 @@ func (p *port) handleSRTP(buffer []byte) {
 	}
 
 	bufferTransport := p.m.getOrCreateBufferTransports(packet.SSRC, packet.PayloadType)
-	if bufferTransport != nil && bufferTransport.Rtp != nil {
+	if bufferTransport != nil && bufferTransport.RTP != nil {
 		select {
-		case bufferTransport.Rtp <- packet:
+		case bufferTransport.RTP <- packet:
 		default:
 		}
 	}

media.go

@@ -15,7 +15,7 @@ type RTCTrack struct {
 	Ssrc        uint32
 	Codec       *RTCRtpCodec
 	Packets     <-chan *rtp.Packet
-	RtcpPackets <-chan *rtcp.PacketWithHeader
+	RTCPPackets <-chan *rtcp.PacketWithHeader
 	Samples     chan<- media.RTCSample
 	RawRTP      chan<- *rtp.Packet
 }

pkg/rtcp/packet.go

@@ -6,7 +6,7 @@ type Packet interface {
 	Unmarshal(rawPacket []byte) error
 }
 
-// PacketWithHeader is a pair to represent an RTCP header and it's
+// PacketWithHeader is a pair to represent an RTCP header and its
 // packet's polymorphic parsed and unparsed forms.
 type PacketWithHeader struct {
 	Header
@@ -32,51 +32,51 @@ func (p *PacketWithHeader) Unmarshal(rawPacket []byte) error {
 	case TypeSenderReport:
 		sr := new(SenderReport)
 		err := sr.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = sr
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = sr
+
 	case TypeReceiverReport:
 		rr := new(ReceiverReport)
 		err := rr.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = rr
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = rr
+
 	case TypeSourceDescription:
 		sdes := new(SourceDescription)
 		err := sdes.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = sdes
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = sdes
+
 	case TypeGoodbye:
 		bye := new(Goodbye)
 		err := bye.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = bye
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = bye
+
 	case TypeTransportSpecificFeedback:
 		rrr := new(RapidResynchronizationRequest)
 		err := rrr.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = rrr
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = rrr
+
 	case TypePayloadSpecificFeedback:
 		psfb := new(PictureLossIndication)
 		err := psfb.Unmarshal(rawPacket)
-		if err == nil {
-			p.Packet = psfb
-		} else {
+		if err != nil {
 			return err
 		}
+		p.Packet = psfb
+
 	default:
 		return errWrongType
 	}

pkg/rtcp/rapid_resynchronization_request.go~

@@ -1,64 +0,0 @@
-package rtcp
-
-import (
-	"encoding/binary"
-)
-
-// The PictureLossIndication packet informs the encoder about the loss of an undefined amount of coded video data belonging to one or more pictures
-type PictureLossIndication struct {
-	// SSRC of sender
-	SenderSSRC uint32
-
-	// SSRC where the loss was experienced
-	MediaSSRC uint32
-}
-
-const (
-	pliFMT    = 1
-	pliLength = 2
-)
-
-// Marshal encodes the PictureLossIndication in binary
-func (p PictureLossIndication) Marshal() ([]byte, error) {
-	/*
-	 * PLI does not require parameters.  Therefore, the length field MUST be
-	 * 2, and there MUST NOT be any Feedback Control Information.
-	 *
-	 * The semantics of this FB message is independent of the payload type.
-	 */
-	rawPacket := make([]byte, 8)
-	binary.BigEndian.PutUint32(rawPacket, p.SenderSSRC)
-	binary.BigEndian.PutUint32(rawPacket[4:], p.MediaSSRC)
-
-	h := Header{
-		Count:  pliFMT,
-		Type:   TypePayloadSpecificFeedback,
-		Length: pliLength,
-	}
-	hData, err := h.Marshal()
-	if err != nil {
-		return nil, err
-	}
-
-	return append(hData, rawPacket...), nil
-}
-
-// Unmarshal decodes the PictureLossIndication from binary
-func (p *PictureLossIndication) Unmarshal(rawPacket []byte) error {
-	if len(rawPacket) < (headerLength + (ssrcLength * 2)) {
-		return errPacketTooShort
-	}
-
-	var h Header
-	if err := h.Unmarshal(rawPacket); err != nil {
-		return err
-	}
-
-	if h.Type != TypePayloadSpecificFeedback || h.Count != pliFMT {
-		return errWrongType
-	}
-
-	p.SenderSSRC = binary.BigEndian.Uint32(rawPacket[headerLength:])
-	p.MediaSSRC = binary.BigEndian.Uint32(rawPacket[headerLength+ssrcLength:])
-	return nil
-}

rtcpeerconnection.go

@@ -1008,7 +1008,7 @@ func (pc *RTCPeerConnection) Close() error {
 }
 
 /* Everything below is private */
-func (pc *RTCPeerConnection) generateChannel(ssrc uint32, payloadType uint8) (tpair *network.Transportpair) {
+func (pc *RTCPeerConnection) generateChannel(ssrc uint32, payloadType uint8) (tpair *network.TransportPair) {
 	pc.RLock()
 	if pc.onTrackHandler == nil {
 		pc.RUnlock()
@@ -1039,13 +1039,13 @@ func (pc *RTCPeerConnection) generateChannel(ssrc uint32, payloadType uint8) (tp
 		Ssrc:        ssrc,
 		Codec:       codec,
 		Packets:     rtpTransport,
-		RtcpPackets: rtcpTransport,
+		RTCPPackets: rtcpTransport,
 	}
 
 	// TODO: Register the receiving Track
 
 	pc.onTrack(track)
-	return &network.Transportpair{Rtp: rtpTransport, Rtcp: rtcpTransport}
+	return &network.TransportPair{RTP: rtpTransport, RTCP: rtcpTransport}
 }
 
 func (pc *RTCPeerConnection) iceStateChange(newState ice.ConnectionState) {
@@ -1249,7 +1249,7 @@ func (pc *RTCPeerConnection) newRTCTrack(payloadType uint8, ssrc uint32, id, lab
 		Label:       label,
 		Ssrc:        ssrc,
 		Codec:       codec,
-		RtcpPackets: rtcpPackets,
+		RTCPPackets: rtcpPackets,
 		Samples:     trackInput,
 		RawRTP:      rawPackets,
 	}