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

EncryptRTP API change #9

Open
kixelated opened this issue Feb 17, 2019 · 2 comments
Open

EncryptRTP API change #9

kixelated opened this issue Feb 17, 2019 · 2 comments

Comments

@kixelated
Copy link
Contributor

kixelated commented Feb 17, 2019

current:

EncryptRTP(dst []byte, plaintext []byte, header *rtp.Header) ([]byte, error)

proposed:

EncryptRTP(dst []byte, packet *rtp.Packet) ([]byte, error)

pros:

  1. Simpler API when callers use *rtp.Packet.
  2. Avoids an extra Unmarshal for callers when use rtp.Packet. For example: WriteRTP.
  3. Avoids a memcpy of the payload prior to encryption.
  4. Removes the implicit assumption that cap(dst) == cap(plaintext) will avoid an allocation.

cons:

  1. Worse API when callers use byte slices. Requires Unmarshal, but net performance is still the same/better.
  2. Removes the option to fill the packet headers after sending. I can't think of a case when this would be practical.
@kixelated
Copy link
Contributor Author

ditto for the WriteRTP and ReadRTP methods. Should combine Header, []byte into Packet everywhere.

@sirzooro
Copy link
Contributor

I am against this. In my case RTP/RTCP packets are generated by other parts of the system, and I use pion/srtp to encrypt generated packets. The same case is for receiving, I need to decrypt them to []byte and pass elsewhere for processing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants