Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Clone to rtp.Packet #88

Closed
jech opened this issue May 23, 2020 · 1 comment · Fixed by #118
Closed

Add Clone to rtp.Packet #88

jech opened this issue May 23, 2020 · 1 comment · Fixed by #118

Comments

@jech
Copy link
Member

jech commented May 23, 2020

In my SFU, packets go through a number of data structures, and finally end up in a static buffer that is passed to Track.WriteRTP. This works fine with ordinary tracks, but fails when trying to write data to disk: SampleBuilder.Push requires the packet to never be overwritten. This does not appear to be documented. (My personal opinion is that Push should clone the packet, but as a second choice I'd be okay if the requirement that the Packet and its underlying buffer never be reusied were clearly documented.)

What is more, there is no easy way to clone a packet. I'm currently using the following code:

func clonePacket(packet *rtp.Packet) *rtp.Packet {
	buf, err := packet.Marshal()
	if err != nil {
		return nil
	}
	var p rtp.Packet
	err = p.Unmarshal(buf)
	if err != nil {
		return nil
	}
	return &p
}

My suggestions are therefore:

  • document that SampleBuilder.Push requires its parameter to never be modified;
  • provide a function Packet.Clone() that clones a packet efficiently;
  • in v3, avoid the complication by having SampleBuilder.Push copy the packet internally.
@Sean-Der Sean-Der transferred this issue from pion/webrtc Sep 16, 2020
@Sean-Der Sean-Der changed the title No easy way to clone a packet Add Clone to rtp.Packet Sep 16, 2020
@Sean-Der
Copy link
Member

Hey @jech

I am all for Clone! I don't think it would be that hard to add either.

SampleBuilder.Push copying might be unpopular. Users who are implementing clients will have to do an extra copy. If we don't force a clone we support both uses cases.

We should for sure add a comment to SampleBuilder.Push I will do that now and tag this commit.

Sean-Der added a commit to pion/webrtc that referenced this issue Sep 17, 2020
The SampleBuilder doesn't copy. If you wish to modify the memory that
was passed you should copy it first.

Relates to pion/rtp#88
ffmiruz added a commit to ffmiruz/rtp that referenced this issue Feb 3, 2021
This method performs deep copy on a packet to produce equivalent but independently mutable values.

Fixes pion#88
ffmiruz added a commit to ffmiruz/rtp that referenced this issue Feb 3, 2021
Clone performs deep copy on a packet to produce equivalent but independently mutable packets.

Fixes pion#88
ffmiruz added a commit to ffmiruz/rtp that referenced this issue Feb 3, 2021
Clone performs deep copy on a packet to produce equivalent\nbut independently mutable packets.

Fixes pion#88
ffmiruz added a commit to ffmiruz/rtp that referenced this issue Feb 3, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to ffmiruz/rtp that referenced this issue May 16, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
Sean-Der pushed a commit to ffmiruz/rtp that referenced this issue Aug 5, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
Sean-Der pushed a commit to ffmiruz/rtp that referenced this issue Aug 5, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
Sean-Der pushed a commit that referenced this issue Aug 5, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes #88
Sean-Der pushed a commit that referenced this issue Aug 6, 2021
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes #88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 12, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
aler9 pushed a commit to aler9/rtp that referenced this issue Jun 13, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes pion#88
Sean-Der pushed a commit that referenced this issue Jun 14, 2023
Clone performs deep copy on a packet to produce equivalent
but independently mutable packet.

Fixes #88
ourwarmhouse added a commit to ourwarmhouse/Smart-Contract---Go that referenced this issue May 1, 2024
The SampleBuilder doesn't copy. If you wish to modify the memory that
was passed you should copy it first.

Relates to pion/rtp#88
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants