CSRC Unmarshal / Marshal diff

[Antonito]

Your environment.

• Version: v1.6.0
• Browser: N/A

What did you do?

package rtp_test

import (
	"bytes"
	"testing"

	"github.com/stretchr/testify/assert"

	"github.com/pion/rtp"
	"github.com/pion/rtp/codecs"
)

func Test_RTP_Marshal_Unmarshal(t *testing.T) {
	multiplepayload := make([]byte, 128)
	packetizer := rtp.NewPacketizer(100, 98, 0x1234ABCD, &codecs.G722Payloader{}, rtp.NewRandomSequencer(), 90000)
	packets := packetizer.Packetize(multiplepayload, 1000)

	rawPkts := make([][]byte, 0, 1400)
	for _, pkt := range packets {
		raw, err := pkt.Marshal()
		assert.Nil(t, err)

		rawPkts = append(rawPkts, raw)
	}

	for ndx, raw := range rawPkts {
		expectedPkt := packets[ndx]
		pkt := &rtp.Packet{}

		err := pkt.Unmarshal(raw)
		assert.Nil(t, err)

		assert.EqualValues(t, len(raw), pkt.MarshalSize())
		assert.EqualValues(t, expectedPkt.MarshalSize(), pkt.MarshalSize())

		assert.EqualValues(t, expectedPkt.Version, pkt.Version)
		assert.EqualValues(t, expectedPkt.Padding, pkt.Padding)
		assert.EqualValues(t, expectedPkt.Extension, pkt.Extension)
		assert.EqualValues(t, expectedPkt.Marker, pkt.Marker)
		assert.EqualValues(t, expectedPkt.PayloadOffset, pkt.PayloadOffset)
		assert.EqualValues(t, expectedPkt.PayloadType, pkt.PayloadType)
		assert.EqualValues(t, expectedPkt.SequenceNumber, pkt.SequenceNumber)
		assert.EqualValues(t, expectedPkt.Timestamp, pkt.Timestamp)
		assert.EqualValues(t, expectedPkt.SSRC, pkt.SSRC)
		assert.EqualValues(t, expectedPkt.CSRC, pkt.CSRC)
		assert.EqualValues(t, expectedPkt.ExtensionProfile, pkt.ExtensionProfile)
		assert.EqualValues(t, expectedPkt.Extensions, pkt.Extensions)
		assert.EqualValues(t, expectedPkt.Payload, pkt.Payload)

		assert.EqualValues(t, expectedPkt, pkt)
	}
}

What did you expect?

The test should succeed, there should be no difference between the RTP packets.

What happened?

CSRC check fails:

Error: Not equal: 
  expected: []uint32(nil)
  actual  : []uint32{}

Thus the whole packet value comparison also fails:

Error: Not equal: 
  expected: &rtp.Packet{Header:rtp.Header{Version:0x2, Padding:false, Extension:false, Marker:false, PayloadOffset:12, PayloadType:0x62, SequenceNumber:0x8a44, Timestamp:0x9f9f5831, SSRC:0x1234abcd, CSRC:[]uint32(nil), ExtensionProfile:0x0, Extensions:[]rtp.Extension(nil)}, Raw:[]uint8{0x80, 0x62, 0x8a, 0x44, 0x9f, 0x9f, 0x58, 0x31, 0x12, 0x34, 0xab, 0xcd, 0x0, ..., 0x0}}
  actual  : &rtp.Packet{Header:rtp.Header{Version:0x2, Padding:false, Extension:false, Marker:false, PayloadOffset:12, PayloadType:0x62, SequenceNumber:0x8a44, Timestamp:0x9f9f5831, SSRC:0x1234abcd, CSRC:[]uint32{}, ExtensionProfile:0x0, Extensions:[]rtp.Extension(nil)}, Raw:[]uint8{0x80, 0x62, 0x8a, 0x44, 0x9f, 0x9f, 0x58, 0x31, 0x12, 0x34, 0xab, 0xcd, 0x0, ..., 0x0}}

[q191201771]

I wanna fix this, and question is, when CSRC empty, should we Unmarshal it as nil or []uint32{}?
nil maybe better?

[Antonito]

I feel like []uint32{} might be a better choice, as if someone manually builds a packet it will most likely be initialized like that (as you can see in the unit tests) and I find it clearer, but I may be wrong!

@Sean-Der do you have any input on this?

rtp/packet_test.go

Line 38 in ddea010

CSRC: []uint32{},

[Sean-Der]

I agree with []uint32{}! Anything to make it easier to unit test IMO. As a user I think the difference would be negligible.

So excited to see you contributing @q191201771. Can I add you to the Pion organization? I would love to have you contribute long term, we always need more great programmers.

I am available on Slack anytime. Also if you have any features that are important to you tell me and i can help make it happen. thanks!

[q191201771]

I agree with []uint32{}! Anything to make it easier to unit test IMO. As a user I think the difference would be negligible.

So excited to see you contributing @q191201771. Can I add you to the Pion organization? I would love to have you contribute long term, we always need more great programmers.

I am available on Slack anytime. Also if you have any features that are important to you tell me and i can help make it happen. thanks!

yeah, I'm glad to join Pion organization, is my honor.
I will take more time to learn Pion.