Repository: Rapptz/discord.py
Pull request: Add encoder params to VoiceClient.play
## Summary
Adds kwarg parameters to allow access to setting `VoiceClient`'s underlying `Encoder` initial parameters.  In addition, this also changes the APPLICATION_ constants to be like the other CTL TypedDicts.  Also fixes an error message typo.  I'm not really sure about using stringly typed constants but its what the `Encoder` constructor uses. 

Closes #7130 

## Checklist

- [X] If code changes were made then they have been tested.
    - [X] I have updated the documentation to reflect the changes.
- [X] This PR fixes an issue.
- [X] 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, ...)


Problem statement:
Audio bitrate limiting
### Summary

Being able to limit the bitrate of outgoing audio streams

### What is the feature request for?

The core library

### The Problem

Hosting bots on connections with low bandwidth can make the played audio lag, this could be solved by giving the option to throttle the outgoing audio from the bot to a lower bandwidth to sacrifice audio quality for stability

### The Ideal Solution

Being able to simply give `voice_channel.play` an option for bitrate (in kilobytes, i guess) - perhaps like the PCMVolumeTransformer is implemented.


```
vc.source = discord.PCMVolumeTransformer(vc.source)
vc.source.volume = float(volume)
```
->

```
vc.source = discord.BitrateTransformer(vc.source)
vc.source.bitrate = 32
```

### The Current Solution

None

### Additional Context

_No response_

Hints from issue discussion:
you can set the bitrate in kilobytes in [discord.FFmpegOpusAudio](https://discordpy.readthedocs.io/en/latest/api.html#discord.FFmpegOpusAudio)
Oh thanks! A dev of this repo said this wasnt possible and told me to create an issue, must have missunderstood him. Nice!

Commit message: Fix Encoder class references in docstring

Patch to review:
diff --git a/discord/opus.py b/discord/opus.py
index 966a8ccc68b4..4c6ae0122284 100644
--- a/discord/opus.py
+++ b/discord/opus.py
@@ -39,10 +39,17 @@
 
 if TYPE_CHECKING:
     T = TypeVar('T')
+    APPLICATION_CTL = Literal['audio', 'voip', 'lowdelay']
     BAND_CTL = Literal['narrow', 'medium', 'wide', 'superwide', 'full']
     SIGNAL_CTL = Literal['auto', 'voice', 'music']
 
 
+class ApplicationCtl(TypedDict):
+    audio: int
+    voip: int
+    lowdelay: int
+
+
 class BandCtl(TypedDict):
     narrow: int
     medium: int
@@ -57,6 +64,15 @@ class SignalCtl(TypedDict):
     music: int
 
 
+class EncoderKwargs(TypedDict, total=False):
+    application: APPLICATION_CTL
+    bitrate: int
+    fec: bool
+    packet_loss_pct: float
+    bandwidth: BAND_CTL
+    signal_type: SIGNAL_CTL
+
+
 __all__ = (
     'Encoder',
     'OpusError',
@@ -90,9 +106,10 @@ class DecoderStruct(ctypes.Structure):
 BAD_ARG = -1
 
 # Encoder CTLs
-APPLICATION_AUDIO    = 2049
-APPLICATION_VOIP     = 2048
-APPLICATION_LOWDELAY = 2051
+APPLICATION_AUDIO    = 'audio'
+APPLICATION_VOIP     = 'voip'
+APPLICATION_LOWDELAY = 'lowdelay'
+# These remain as strings for backwards compat
 
 CTL_SET_BITRATE      = 4002
 CTL_SET_BANDWIDTH    = 4008
@@ -105,6 +122,12 @@ class DecoderStruct(ctypes.Structure):
 CTL_LAST_PACKET_DURATION = 4039
 # fmt: on
 
+application_ctl: ApplicationCtl = {
+    'audio': 2049,
+    'voip': 2048,
+    'lowdelay': 2051,
+}
+
 band_ctl: BandCtl = {
     'narrow': 1101,
     'medium': 1102,
@@ -319,16 +342,36 @@ def get_opus_version() -> str:
 
 
 class Encoder(_OpusStruct):
-    def __init__(self, application: int = APPLICATION_AUDIO):
-        _OpusStruct.get_opus_version()
-
-        self.application: int = application
+    def __init__(
+        self,
+        *,
+        application: APPLICATION_CTL = 'audio',
+        bitrate: int = 128,
+        fec: bool = True,
+        packet_loss_pct: float = 0.15,
+        bandwidth: BAND_CTL = 'full',
+        signal_type: SIGNAL_CTL = 'auto',
+    ):
+        if application not in application_ctl:
+            raise KeyError(f'{application} is not a valid application setting. Try one of: {"".join(application_ctl)}')
+
+        if not 16 <= bitrate <= 512:
+            raise ValueError(f'bitrate must be between 16 and 512, not {bitrate}')
+
+        if not 0 < packet_loss_pct <= 1.0:
+            raise ValueError(f'packet_loss_pct must be a positive number less than or equal to 1, not {packet_loss_pct}')
+
+        _OpusStruct.get_opus_version()  # lazy loads the opus library
+
+        self.application: int = application_ctl[application]
         self._state: EncoderStruct = self._create_state()
-        self.set_bitrate(128)
-        self.set_fec(True)
-        self.set_expected_packet_loss_percent(0.15)
-        self.set_bandwidth('full')
-        self.set_signal_type('auto')
+
+        self.set_bitrate(bitrate)
+        self.set_fec(fec)
+        if fec:
+            self.set_expected_packet_loss_percent(packet_loss_pct)
+        self.set_bandwidth(bandwidth)
+        self.set_signal_type(signal_type)
 
     def __del__(self) -> None:
         if hasattr(self, '_state'):
@@ -355,7 +398,7 @@ def set_bandwidth(self, req: BAND_CTL) -> None:
 
     def set_signal_type(self, req: SIGNAL_CTL) -> None:
         if req not in signal_ctl:
-            raise KeyError(f'{req!r} is not a valid bandwidth setting. Try one of: {",".join(signal_ctl)}')
+            raise KeyError(f'{req!r} is not a valid signal type setting. Try one of: {",".join(signal_ctl)}')
 
         k = signal_ctl[req]
         _lib.opus_encoder_ctl(self._state, CTL_SET_SIGNAL, k)
diff --git a/discord/voice_client.py b/discord/voice_client.py
index 66fa1d2afb73..0bdfd6379ebb 100644
--- a/discord/voice_client.py
+++ b/discord/voice_client.py
@@ -45,6 +45,7 @@
 import struct
 import threading
 from typing import Any, Callable, List, Optional, TYPE_CHECKING, Tuple, Union
+from typing_extensions import Unpack
 
 from . import opus, utils
 from .backoff import ExponentialBackoff
@@ -58,7 +59,7 @@
     from .guild import Guild
     from .state import ConnectionState
     from .user import ClientUser
-    from .opus import Encoder
+    from .opus import Encoder, EncoderKwargs
     from .channel import StageChannel, VoiceChannel
     from . import abc
 
@@ -569,7 +570,13 @@ def _encrypt_xsalsa20_poly1305_lite(self, header: bytes, data) -> bytes:
 
         return header + box.encrypt(bytes(data), bytes(nonce)).ciphertext + nonce[:4]
 
-    def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Exception]], Any]] = None) -> None:
+    def play(
+        self,
+        source: AudioSource,
+        *,
+        after: Optional[Callable[[Optional[Exception]], Any]] = None,
+        **encoder_kwargs: Unpack[EncoderKwargs],
+    ) -> None:
         """Plays an :class:`AudioSource`.
 
         The finalizer, ``after`` is called after the source has been exhausted
@@ -579,9 +586,15 @@ def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Except
         caught and the audio player is then stopped.  If no after callback is
         passed, any caught exception will be logged using the library logger.
 
+        Extra parameters may be passed to the internal opus encoder if a PCM based
+        source is used.  Otherwise, they are ignored.
+
         .. versionchanged:: 2.0
             Instead of writing to ``sys.stderr``, the library's logger is used.
 
+        .. versionchanged:: 2.4
+            Added encoder parameters as keyword arguments in ``encoder_kwargs``.
+
         Parameters
         -----------
         source: :class:`AudioSource`
@@ -590,6 +603,27 @@ def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Except
             The finalizer that is called after the stream is exhausted.
             This function must have a single parameter, ``error``, that
             denotes an optional exception that was raised during playing.
+        application: :class:`str`
+            Configures the encoder's intended application.  Can be one of:
+            ``'audio'``, ``'voip'``, ``'lowdelay'``.
+            Defaults to ``'audio'``.
+        bitrate: :class:`int`
+            Configures the bitrate in the encoder.  Can be between ``16`` and ``512``.
+            Defaults to ``128``.
+        fec: :class:`bool`
+            Configures the encoder's use of inband forward error correction.
+            Defaults to ``True``.
+        packet_loss_pct: :class:`float`
+            Configures the encoder's expected packet loss percentage.  Requires FEC.
+            Defaults to ``0.15``.
+        bandwidth: :class:`str`
+            Configures the encoder's bandpass.  Can be one of:
+            ``'narrow'``, ``'medium'``, ``'wide'``, ``'superwide'``, ``'full'``.
+            Defaults to ``'full'``.
+        signal_type: :class:`str`
+            Configures the type of signal being encoded.  Can be one of:
+            ``'auto'``, ``'voice'``, ``'music'``.
+            Defaults to ``'auto'``.
 
         Raises
         -------
@@ -599,6 +633,10 @@ def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Except
             Source is not a :class:`AudioSource` or after is not a callable.
         OpusNotLoaded
             Source is not opus encoded and opus is not loaded.
+        ValueError
+            An improper value was passed as an encoder parameter.
+        KeyError
+            An improper value was passed as an encoder parameter.
         """
 
         if not self.is_connected():
@@ -610,8 +648,8 @@ def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Except
         if not isinstance(source, AudioSource):
             raise TypeError(f'source must be an AudioSource not {source.__class__.__name__}')
 
-        if not self.encoder and not source.is_opus():
-            self.encoder = opus.Encoder()
+        if (not self.encoder or encoder_kwargs) and not source.is_opus():
+            self.encoder = opus.Encoder(**encoder_kwargs)
 
         self._player = AudioPlayer(source, self, after=after)
         self._player.start()


Code review comments:
diff_hunk: @@ -45,6 +45,7 @@
 import struct
 import threading
 from typing import Any, Callable, List, Optional, TYPE_CHECKING, Tuple, Union
+from typing_extensions import Unpack
path: discord/voice_client.py
text: @user1:
This is not a runtime requirement so it will fail for most people. We usually import from `typing_extensions` purely at type checking time inside `TYPE_CHECKING`. That being said, this style of unpacking kwargs is *not* used in the repository at the moment. Originally the reason was because it was not formally accepted into Python but it was added during [PEP-692](https://peps.python.org/pep-0692/) for Python 3.12, which has yet to release. However, last time we attempted to incorporate this the ergonomics were not ideal since it had a lot of bugs in Pyright and did not have good auto complete experience.

I am unsure if this has changed since then.

diff_hunk: @@ -57,6 +64,15 @@ class SignalCtl(TypedDict):
     music: int
 
 
+class EncoderKwargs(TypedDict, total=False):
+    application: APPLICATION_CTL
+    bitrate: int
+    fec: bool
+    packet_loss_pct: float
path: discord/opus.py
text: @user1:
```suggestion
    expected_packet_loss: float
```

Shortcuts in typing are usually not done unless necessary and this one's better off longer.

diff_hunk: @@ -319,16 +342,36 @@ def get_opus_version() -> str:
 
 
 class Encoder(_OpusStruct):
-    def __init__(self, application: int = APPLICATION_AUDIO):
-        _OpusStruct.get_opus_version()
-
-        self.application: int = application
+    def __init__(
+        self,
+        *,
+        application: APPLICATION_CTL = 'audio',
+        bitrate: int = 128,
+        fec: bool = True,
+        packet_loss_pct: float = 0.15,
+        bandwidth: BAND_CTL = 'full',
+        signal_type: SIGNAL_CTL = 'auto',
+    ):
+        if application not in application_ctl:
+            raise KeyError(f'{application} is not a valid application setting. Try one of: {"".join(application_ctl)}')
path: discord/opus.py
text: @user1:
```suggestion
            raise ValueError(f'{application} is not a valid application setting. Try one of: {"".join(application_ctl)}')
```

diff_hunk: @@ -599,6 +633,10 @@ def play(self, source: AudioSource, *, after: Optional[Callable[[Optional[Except
             Source is not a :class:`AudioSource` or after is not a callable.
         OpusNotLoaded
             Source is not opus encoded and opus is not loaded.
+        ValueError
+            An improper value was passed as an encoder parameter.
+        KeyError
+            An improper value was passed as an encoder parameter.
path: discord/voice_client.py
text: @user1:
```suggestion
```

diff_hunk: @@ -57,6 +64,15 @@ class SignalCtl(TypedDict):
     music: int
 
 
+class EncoderKwargs(TypedDict, total=False):
+    application: APPLICATION_CTL
path: discord/opus.py
text: @user1:
I don't really see much point in allowing this in particular to be set?

@author:
Without `total=False` pyright complains if you don't have all the parameters passed in the function, if that's what you were talking about.

@user1:
No I was talking about the application parameter.

@author:
That was originally the only constructor parameter that Encoder had so it didn't occur to me to either remove it or arbitrarily restrict access to only that setting.