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

fix srtp_steam leak when closing producer and consumers #427

Closed
wants to merge 2 commits into from

Conversation

penguinol
Copy link
Contributor

@ibc
Copy link
Member

ibc commented Jun 24, 2020

Good catch. Let me take a look since I think the implementation could be a bit different (more simpler). On it. Will ping here soon.

@penguinol
Copy link
Contributor Author

Good catch. Let me take a look since I think the implementation could be a bit different (more simpler). On it. Will ping here soon.

Yes, the code is a little bit duplicate because srtp session is in the sub class.

@ibc
Copy link
Member

ibc commented Jun 24, 2020

Codacy Here is an overview of what got changed by this pull request:

Clones added
============
- worker/src/RTC/WebRtcTransport.cpp  2
- worker/src/RTC/PlainTransport.cpp  4
- worker/src/RTC/PipeTransport.cpp  4
         

Clones removed
==============
+ worker/src/RTC/Transport.cpp  -2
+ worker/include/RTC/WebRtcTransport.hpp  -1
         

See the complete overview on Codacy

@ibc
Copy link
Member

ibc commented Jun 24, 2020

@penguinol can you please take a look to #428? It's a simpler approach to achieve the same. I've tested it by also printing SRTP streams handled by libsrtp and it works as expected.

@jmillan
Copy link
Member

jmillan commented Jun 24, 2020

Very nice catch @penguinol

@ibc
Copy link
Member

ibc commented Jun 24, 2020

Closing in favour of #428. thanks!

@ibc ibc closed this Jun 24, 2020
@penguinol
Copy link
Contributor Author

@penguinol can you please take a look to #428? It's a simpler approach to achieve the same. I've tested it by also printing SRTP streams handled by libsrtp and it works as expected.

Ok, i will test it next week. we have a festival next days.

@ibc
Copy link
Member

ibc commented Jun 24, 2020

Don't worry, it's tested, merged and released :)

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

Successfully merging this pull request may close these issues.

3 participants