Make a copy for packet.Payload instead of slicing

[kixelated]

When Unmarshalling, the current code slices the input to set packet.Payload. This is fast, but we really should make a copy to avoid nasty bugs. For example:

p1 := rtp.Packet{}
p2 := rtp.Packet{}

p1.Unmarshal(raw)
p2.Unmarshal(raw)

// Write in a new payload for p1
// ERROR this will also replace p2.Payload
copy(p1.Payload, newPayload)

You know how performance crazy I can be, but we should copy instead of keeping a reference to the data provided to Unmarshal.

[wdouglass]

I wonder if we can unexpose p1.Payload, and have some kind of copy-on-write semantics (just because i think writing into the payload of an unmarshalled packet like this is kind of a rare edge-case)

[kixelated]

I don't think copy-on-write is possible in Go because there's no such thing as a read-only slice. If you call .Payload(), then you can still muck with the returned memory.

I definitely want the default behavior to be safe. Kind of like how when you pass a buffer to json.Unmarshal, you expect it to make a copy instead of pointing to the memory in your buffer.

To that end, I would propose having Unmarshal make a copy. If you want to avoid the copy for hard-core performance reasons (and know that there are no other references to the payload), you could construct the packet with the syntax: rtp.Packet{header, payload}.

[Sean-Der]

👍 on this from me, I am running into this (I can fix it, but I can see why people would be surprised)

I added ReadRTP and want to use a scratch buffer locally (via sync.Pool) but I can't because Unmarshal causes the payload to be retained by default.

[jech]

In Galène, we use Unmarshal to access the header bits without copying the packet. We essentiallydo this :

packet.Unmarshal(buf)
kf := isKeyframe(&packet)  // reads packet.Header and packet.Payload
if(packet.Extension) {
    packet.Extensions = removeSomeExtensions(packet.Extensions)
    packet.MarshalTo(buf);
}

Note two things about this code:

• we count on packet.Unmarshal not allocating a copy of the packet;
• we could on packet.MarshalTo working when the destination buffer overlaps packet.Raw.

The second point is less important, since I could potentially use a second buffer in this case.

If you change the semantics of Unmarshal, please provide a new API that allows me to do the above without additional allocations.