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

SpectraAnalyzer, comment out print statements #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mcacker
Copy link

@mcacker mcacker commented Jul 6, 2015

PortManager, make currPort static so that multiple instances don't
conflict port useage
UdpManager, cleanup selector is created
RTPDataChannel, expose JitterBuffer
Scheduler, fix thread leaks

PortManager, make currPort static so that multiple instances don't
conflict port useage
UdpManager, cleanup selector is created
RTPDataChannel, expose JitterBuffer
Scheduler, fix thread leaks
@hrosa hrosa self-assigned this Jul 7, 2015
@hrosa hrosa added the bug label Jul 7, 2015
@hrosa
Copy link
Contributor

hrosa commented Jul 8, 2015

Hi Mitchell,

After reviewing your work, all seems fine to me except one thing - your branch is deprecated which makes the merge impossible.

Last year I added support for RTCP, ICE & STUN (for WebRTC calls) and refactored the RTPDataChannel (became deprecated, replaced with RtpChannel class).

The question now is: should I do the "merge" manually or are you willing to pull from master, apply the changes and then submit your work?

Let me know what you prefer.

Many thanks once again,
Henrique

@mcacker
Copy link
Author

mcacker commented Jul 8, 2015

Glad to hear about RTCP, that is something we may want to use and I was
thinking I might have to jump to the Jitsi library.

I'll pull the latest branch and do the merge, and let you know if I run
into any issues. I'll want to check that there are no thread/memory/cpu
leaks. I take it that the API hasn't changed significantly and the switch
to RtpChannel is straightforward?

Mitchell

On Wed, Jul 8, 2015 at 4:28 AM, Henrique Rosa [email protected]
wrote:

Hi Mitchell,

After reviewing your work, all seems fine to me except one thing - your
branch is deprecated which makes the merge impossible.

Last year I added support for RTCP, ICE & STUN (for WebRTC calls) and
refactored the RTPDataChannel (became deprecated, replaced with RtpChannel
class).

The question now is: should I do the "merge" manually or are you willing
to pull from master, apply the changes and then submit your work?

Let me know what you prefer.

Many thanks once again,
Henrique


Reply to this email directly or view it on GitHub
#24 (comment).

@hrosa
Copy link
Contributor

hrosa commented Jul 8, 2015

Hi,

There may be some work involved but I don't think too much.
Overall, here are some guidelines:

  • The RtpConnection is now composed of MediaChannels (requirement to have a video channel as well in the future).
  • The MediaChannel contains an RTP channel and an RTCP channel. The RTCP channel is optional because you can do multiplexing in the RTP channel.
  • The RtpChannel contains a list of PacketHandlers that will treat suitably incoming packets (handlers for DTLS, STUN, RTP and RTCP are implemented so far). This was necessary for WebRTC calls where STUN, DTLS and SRTP packets were sent to the same channel.
  • The RTPDataChannel is equivalent to the RtpChannel, having its RxTask and TxTask replaced by the RtpHandler and RtpTransmitter for simplicity sake.

Also, it's worth noting that I am preparing to implement a Selective Forwarding Unit for Mobicents MS (issue #20 ). As pre-requirement we need to have the capability to decide what is the relay type at RTP level: mixer or translator.
Up to this moment the entire MMS design assumes that we will always use mixing... so I'm currently working on issue #23 , refactoring the RTP library once again.

My advice is that you that a look at the current master branch and evaluate how much work is needed so you can plan ahead, but wait for issue #23 to be closed so you don't have to do extra work after a while.

Also, let's have discussions like this on the mobicents public forum so other users in community can benefit and take part of discussion.

@hrosa
Copy link
Contributor

hrosa commented Jul 8, 2015

@mcacker
Copy link
Author

mcacker commented Jan 14, 2016

Henrique,

I finally found some time to try and apply my updates to the latest repo, and see that the main source of the resource leaks, org.mobicents.media.server.scheduler.Scheduler, has been completely refactored to use standard java util executors, and none of my changes are relevant any more.

I haven't performed any memory leak tests on the latest code, but is it your opinion that this refactoring has addressed the previously observed scheduler resource leaks?

thanks, Mitchell

hrosa pushed a commit that referenced this pull request Feb 9, 2018
…pull request #24)

Combinef "DTMF+speech" mode is implemented

Approved-by: Henrique Rosa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants