Repository navigation
CSRC Unmarshal / Marshal diff #79
Description
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}}
Activity
q191201771 commentedon Sep 10, 2020
I wanna fix this, and question is, when CSRC empty, should we Unmarshal it as nil or []uint32{}?
nil maybe better?
Antonito commentedon Sep 10, 2020
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
Sean-Der commentedon Sep 11, 2020
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 commentedon Sep 11, 2020
yeah, I'm glad to join Pion organization, is my honor.
I will take more time to learn Pion.
Allow packetizer marshal / unmarshal to be consistent
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default
Set CSRC as a empty (not nil) slice by default