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

Implement AudioPacketInterceptor #425

Closed
wants to merge 2 commits into from
Closed

Implement AudioPacketInterceptor #425

wants to merge 2 commits into from

Conversation

mentlerd
Copy link

@mentlerd mentlerd commented Aug 7, 2017

Implemented a new interface for AudioPacketInterceptor.

This object can be registered just like an AudioReceiveHandler, but it provides a much more low level access to audio packets than the standard handleUserAudio callback.

Also allows discarding AudioPackets, effectively hiding them from the JDA backend.

This PR implements issue #418

@@ -258,28 +266,33 @@ else if (sendHandler == null && sendSystem != null)

private synchronized void setupReceiveSystem()
{
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation initializes the receive thread even if there is only an AudioPacketInterceptor, but no ReceiveHandler.

I have tried to cleanup this setup routine as much as I could, but I would appreciate a second pair of eyes checking on it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With your changes, if there is a "consumer" but the socket is either null or closed, nothing happens.
In the old implementation, if the socket was null/closed (no matter if "consumer" is set, there was cleanup (your else branch)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did not spot that!

I think it does not make a difference in this case tough, the combined audio thread only gets setup when the udpSocket is not null, so there is nothing to clean up.

@MinnDevelopment
Copy link
Member

This PR is on hold until voice resume has been merged. On a side note, you need to update your code style to fit into JDA, the wiki explains this.

@mentlerd
Copy link
Author

I have fixed the teardown and style issues

@MinnDevelopment
Copy link
Member

I'm assigning this to @DV8FromTheWorld so he can decide how we should implement this.

@MinnDevelopment MinnDevelopment removed their request for review September 6, 2017 17:48
@DV8FromTheWorld DV8FromTheWorld added the status: freezer this is currently put on hold label Sep 21, 2017
@MinnDevelopment
Copy link
Member

Closing this, so @DV8FromTheWorld can use the issue to decide on an implementation. This PR is not satisfying.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: freezer this is currently put on hold type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants