Make Packet.MarshalTo() thread-safe

[aler9]

Description

A Marshal function shouldn't change any property of the object that is being marshaled. Otherwise, Marshal() is non-thread safe
and can't be called by multiple goroutines in parallel.

PR #155 makes Packet.Marshal() set the Packet.Header.Padding bit when Packet.PaddingSize is non-zero; while this is preferable from the user point of view, it breaks thread safety.

This patch fixes the issue.

[codecov]

Codecov Report

Merging #168 (0a08f0e) into master (beba640) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #168      +/-   ##
==========================================
- Coverage   86.32%   86.31%   -0.01%     
==========================================
  Files          15       15              
  Lines        1638     1637       -1     
==========================================
- Hits         1414     1413       -1     
  Misses        195      195              
  Partials       29       29              

Flag	Coverage Δ
go	86.31% <ø> (-0.01%)	⬇️
wasm	85.82% <ø> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packet.go	87.94% <ø> (-0.04%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update beba640...0a08f0e. Read the comment docs.

[aler9]

This issue was discovered through gortsplib's unit tests - in the tests, there's a dummy rtp.Packet that is shared between all tests, and is marshaled by multiple routines at the same time.

[Sean-Der]

Thanks for fixing this @aler9! I don't fully understand the impact, but this doesn't seem to cause any test fails so should be ok? I would think this branch wouldn't be used anymore and tests would fail.

@boks1971 mind reviewing/any opinion.

[boks1971]

Thank you @aler9 . This would mean call sites would have set the proper flags which is fine.

I am not totally happy about the handling of padding myself. But, I have not found a better way. Just to give a bit of history about the changes I made

• SFUs peek into the payload to determine things like spatial/temporal layer and if a packet is a key frame or not. They were not checking if the packet is padding only (padding only packets used by clients to probe for bandwidth). Some clients did not zero out the padding bytes. So, the parsing ended up doing incorrect things. The way to address that would have been to check for Padding flag in Header and then look at the last byte of payload and check it against length to determine the actual payload size. Making apps do that was a bit cumbersome. So, my first commit was to chop the payload and get rid of payload bytes. Apps could just check if len(Payload) == 0 to check for padding only.
• Then I realised that the information about how big the packet actually was after unmarshaling. So, I added the PaddingSize field. And to ensure that Marshal does the right thing if some body just provides PaddingSize, I did the change which caused this issue.

I think it is fine that call sites have to provide the proper Packet to Marshal. Thank you again!

[aler9]

@boks1971 i would have done it the same way. I also thought about dropping the padding, but i think that this choice must be left to the user. This library should just be able to unmarshal any compliant RTP packet and marshal it as it is. I like bijective functions: every output corresponds to only one input and vice-versa.

As for my user case, i use this library for handling RTP packets generated by cameras, and they add a lot of padding that can be filtered without problems.