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

refactor: python implementation of audioop.mul #2176

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

Conversation

davidhozic
Copy link
Contributor

@davidhozic davidhozic commented Jul 23, 2023

Summary

This PR replaces the PCMVolumeTransformer.read() method with a Python equivalent, that does not use the audioop module, which is deprecated since Python 3.11 and will be removed in Python 3.13.

This implements #2177.

Information

  • This PR fixes an issue.
  • This PR adds something new (e.g. new method or parameters).
  • This PR is a breaking change (e.g. methods or parameters removed/renamed).
  • This PR is not a code change (e.g. documentation, README, typehinting,
    examples, ...).

Checklist

  • I have searched the open pull requests for duplicates.
  • If code changes were made then they have been tested.
    • I have updated the documentation to reflect the changes.
  • If type: ignore comments were used, a comment is also left explaining why.
  • I have updated the changelog to include these changes.

@davidhozic davidhozic changed the title feat: Replace deprecated audioop feat: Replace deprecated audioop implementation of PCMVolumeTransformer.read Jul 23, 2023
@davidhozic davidhozic marked this pull request as ready for review July 23, 2023 10:54
@davidhozic davidhozic requested a review from a team as a code owner July 23, 2023 10:54
@pullapprove4 pullapprove4 bot requested a review from BobDotCom July 23, 2023 10:54
@pullapprove4 pullapprove4 bot requested a review from FrostByte266 July 23, 2023 10:54
VincentRPS
VincentRPS previously approved these changes Jul 26, 2023
Copy link
Member

@VincentRPS VincentRPS left a comment

Choose a reason for hiding this comment

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

LGTM

@OmLanke
Copy link
Contributor

OmLanke commented Jul 27, 2023

A python equivalent will not be nearly as close as the C one in terms of performance. Live audio is a heavy load and has to be low latency.

@VincentRPS
Copy link
Member

A python equivalent will not be nearly as close as the C one in terms of performance. Live audio is a heavy load and has to be low latency.

I mean we can always rewrite it in Rust.

@VincentRPS
Copy link
Member

and also this largely uses python stdlib modules which from the seems of things are written in C in many places.

@OmLanke
Copy link
Contributor

OmLanke commented Jul 27, 2023

A python equivalent will not be nearly as close as the C one in terms of performance. Live audio is a heavy load and has to be low latency.

I mean we can always rewrite it in Rust.

The whole voice stuff could be rewritten to Rust 😬

@OmLanke
Copy link
Contributor

OmLanke commented Jul 27, 2023

and also this largely uses python stdlib modules which from the seems of things are written in C in many places.

Possible to do actual performance tests?

@VincentRPS
Copy link
Member

A python equivalent will not be nearly as close as the C one in terms of performance. Live audio is a heavy load and has to be low latency.

I mean we can always rewrite it in Rust.

The whole voice stuff could be rewritten to Rust 😬

only if :)

@davidhozic
Copy link
Contributor Author

audioop:
image

Without audioop:
image

Hardware:
OS Name Microsoft Windows 10 Pro
Processor AMD Ryzen 5 5600X 6-Core Processor, 3701 Mhz, 6 Core(s), 12 Logical Processor(s)

@OmLanke
Copy link
Contributor

OmLanke commented Jul 27, 2023

Hmm that sure is a difference.

Use time.perf_counter_ns() instead of time.time(). The result will be more accurate and will return ns instead of s

@OmLanke
Copy link
Contributor

OmLanke commented Jul 27, 2023

Can we maintain and ship our own audioop C file? Dpy plans to do something like that

Or we could try Cython or maybe Rust?

@davidhozic
Copy link
Contributor Author

davidhozic commented Jul 28, 2023

R5 5600x 4.7 GHz

Elapsed: 31819500 ns
Elapsed: 1016100 ns
Elapsed: 868800 ns
Elapsed: 739800 ns
Elapsed: 897400 ns
Elapsed: 743300 ns
Elapsed: 732200 ns
Elapsed: 784500 ns
Elapsed: 840000 ns

@OmLanke
Copy link
Contributor

OmLanke commented Jul 29, 2023

Wow that is some real improvement 👍

LGTM

discord/player.py Outdated Show resolved Hide resolved
@Dorukyum Dorukyum changed the title feat: Replace deprecated audioop implementation of PCMVolumeTransformer.read feat: native implementation of audioop.mul Dec 31, 2023
auto-merge was automatically disabled December 31, 2023 13:51

Head branch was pushed to by a user without write access

davidhozic and others added 2 commits December 31, 2023 14:51
Co-authored-by: Dorukyum <53639936+Dorukyum@users.noreply.github.com>
Signed-off-by: David Hozic <davidhozic@gmail.com>
@Dorukyum Dorukyum changed the title feat: native implementation of audioop.mul refactor: native implementation of audioop.mul Jan 2, 2024
@VincentRPS VincentRPS changed the title refactor: native implementation of audioop.mul refactor: python implementation of audioop.mul Jan 14, 2024
@Lulalaby Lulalaby added this to the v2.6 milestone Feb 28, 2024
@Dorukyum Dorukyum removed the on hold label Mar 6, 2024
Signed-off-by: Lala Sabathil <lala@pycord.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Replace deprecated audioop implementation of PCMVolumeTransformer.read
5 participants