Repository: Kraigie/nostrum
Pull request #371 (2021-12-21 00:23:31)

Add option to configure number of audio frames to send for each burst before sleeping.
This config option will allow for proper playback of extremely short audio files.


Patch:
diff --git a/docs/static/Intro.md b/docs/static/Intro.md
index 2cb10859..123b960e 100644
--- a/docs/static/Intro.md
+++ b/docs/static/Intro.md
@@ -79,6 +79,7 @@ The following fields are also supported:
  - `streamlink` - Specifies the path to the `streamlink` executable for playing livestream audio with streamlink support. Defaults to `"streamlink"`.
  - `gateway_intents` - This field takes a list of atoms representing gateway intents for Nostrum to subscribe to from the Discord API. More information can be found in the [gateway intents](gateway-intents.html) documentation page.
  - `audio_timeout` - Milliseconds that input must begin generating audio by upon invoking `play`. More information about this option can be found in the [voice](voice.html) documentation page. Defaults to `20_000` (20s).
+ - `audio_frames_per_burst` - Number of opus frames to send at a time while playing audio. More information about this option can be found in the [voice](voice.html) documentation page. Defaults to `10`.
  - `voice_auto_connect` - This will determine if Nostrum automatically connects to voice websockets gateways upon joining voice channels. If set to `false` but you still wish to connect to the voice gateway, you can do so manually by calling `Nostrum.Voice.connect_to_gateway/1` after joining a voice channel. Defaults to `true`.
  - `dev` - This is added to enable Nostrum to be run completely stand alone for
  development purposes. `true` will cause Nostrum to spawn its own event consumers.
diff --git a/docs/static/Voice.md b/docs/static/Voice.md
index 968a4b91..7b94ef72 100644
--- a/docs/static/Voice.md
+++ b/docs/static/Voice.md
@@ -56,6 +56,22 @@ If your use case involves playing large files at a timestamp several hours in li
 `play(guild_id, url, :ytdl, start_time: "2:37:56")`, you may consider setting the timeout to a higher value, as downloading a large youtube video and having `ffmpeg` seek through several hours
 of audio may take 15-20 seconds, even with a fast network connection.
 
+## Audio Frames Per Burst
+The value `:audio_frames_per_burst` represents the number of consecutive packets to send before resting.
+When using `Nostrum.Voice.play/4` to play audio, Nostrum collects a number of opus frames from the 
+audio input source before sending them all to Discord as a "burst" of ordered frames. 
+This is done to reduce the overhead of process-sleeping and setup.
+For reference, a single opus frame is 20 milliseconds of audio (at least for the format that Discord uses).
+By default, the `:audio_frames_per_burst` is set to `10`, equivalent to 200 milliseconds of audio.
+
+Under normal circumstances, there's no reason to change this value. However, if you attempt to play a very short
+piece of audio that's less than `10` frames (200ms) in length, it will time out (after the configured 
+`:audio_timeout` duration has passed) as it waits to collect `10` frames to send. For those cases, configure the
+value to *at most* the minumum frame length of the audio you intend to play, or simply `1`. Setting the value to `1`
+means that each opus frame from your audio source will be taken individually and be sent in its own "burst" with the 
+player process sleeping between each; you likely won't notice a difference in audio playback quality compared to the 
+default value of `10` other than that your sub-200ms audio files will play as expected.
+
 ## Voice Events
 There are a few voice related events that bots can consume with a `Nostrum.Consumer` process:
   - `t:Nostrum.Consumer.voice_state_update/0`
diff --git a/lib/nostrum/application.ex b/lib/nostrum/application.ex
index 7d1dfec5..a186a665 100644
--- a/lib/nostrum/application.ex
+++ b/lib/nostrum/application.ex
@@ -28,6 +28,7 @@ defmodule Nostrum.Application do
   def setup_ets_tables do
     :ets.new(:gateway_url, [:set, :public, :named_table])
     :ets.new(:unavailable_guilds, [:set, :public, :named_table])
+    :ets.new(:users, [:set, :public, :named_table])
     :ets.new(:guild_shard_map, [:set, :public, :named_table])
     :ets.new(:channel_guild_map, [:set, :public, :named_table])
   end
diff --git a/lib/nostrum/cache/cache_supervisor.ex b/lib/nostrum/cache/cache_supervisor.ex
index 87ed896a..ecb279f4 100644
--- a/lib/nostrum/cache/cache_supervisor.ex
+++ b/lib/nostrum/cache/cache_supervisor.ex
@@ -22,7 +22,6 @@ defmodule Nostrum.Cache.CacheSupervisor do
       Nostrum.Cache.Me,
       # Uses the configured cache implementations.
       Nostrum.Cache.GuildCache,
-      Nostrum.Cache.UserCache,
       Nostrum.Cache.ChannelCache,
       Nostrum.Cache.PresenceCache
     ]
diff --git a/lib/nostrum/cache/guild_cache.ex b/lib/nostrum/cache/guild_cache.ex
index 8ca0abbd..b4ee4a76 100644
--- a/lib/nostrum/cache/guild_cache.ex
+++ b/lib/nostrum/cache/guild_cache.ex
@@ -53,7 +53,8 @@ defmodule Nostrum.Cache.GuildCache do
   alias Nostrum.Util
 
   @configured_cache :nostrum
-                    |> Application.compile_env([:caches, :guilds], @default_cache_implementation)
+                    |> Application.compile_env(:caches, %{})
+                    |> Map.get(:guilds, @default_cache_implementation)
 
   @typedoc "Specifies the reason for why a lookup operation has failed."
   @type reason ::
diff --git a/lib/nostrum/cache/user_cache.ex b/lib/nostrum/cache/user_cache.ex
index 059bcd79..630e66a7 100644
--- a/lib/nostrum/cache/user_cache.ex
+++ b/lib/nostrum/cache/user_cache.ex
@@ -2,36 +2,22 @@ defmodule Nostrum.Cache.UserCache do
   @default_cache_implementation Nostrum.Cache.UserCache.ETS
   @moduledoc """
   Cache behaviour & dispatcher for users.
-
-  You can call the functions provided by this module independent of which cache
-  is configured, and it will dispatch to the configured cache implementation.
-
-  By default, #{@default_cache_implementation} will be used for caching users.
-  You can override this in the `:caches` option of the `:nostrum` application
-  by setting the `:users` field to a different module implementing the behaviour
-  defined by this module.
-
-  See the documentation for the `Nostrum.Cache.GuildCache` module for more details.
-
   """
 
   alias Nostrum.Struct.User
   alias Nostrum.Util
   import Nostrum.Snowflake, only: [is_snowflake: 1]
 
-  @configured_cache :nostrum
-                    |> Application.compile_env([:caches, :users], @default_cache_implementation)
-
-  ## Supervisor callbacks
-
-  @doc false
-  defdelegate init(init_arg), to: @configured_cache
-
-  @doc false
-  defdelegate start_link(init_arg), to: @configured_cache
-
-  @doc false
-  defdelegate child_spec(opts), to: @configured_cache
+  if function_exported?(Application, :compile_env, 3) do
+    @configured_cache :nostrum
+                      |> Application.compile_env(:caches, %{})
+                      |> Map.get(:users, @default_cache_implementation)
+  else
+    # credo:disable-for-next-line Credo.Check.Warning.ApplicationConfigInModuleAttribute
+    @configured_cache :nostrum
+                      |> Application.get_env(:caches, %{})
+                      |> Map.get(:users, @default_cache_implementation)
+  end
 
   ## Behaviour specification
 
@@ -83,15 +69,40 @@ defmodule Nostrum.Cache.UserCache do
   @callback delete(snowflake :: User.id()) :: :noop | User.t()
 
   ## Dispatching
-  defdelegate get(id), to: @configured_cache
-  defdelegate create(payload), to: @configured_cache
-  defdelegate bulk_create(users), to: @configured_cache
-  defdelegate update(payload), to: @configured_cache
-  defdelegate delete(snowflake), to: @configured_cache
+
+  @doc "Retrieve a user using the selected cache implementation."
+  @spec get(User.id()) :: {:error, atom} | {:ok, User.t()}
+  def get(id) when is_snowflake(id) do
+    @configured_cache.get(id)
+  end
 
   @doc """
-  Same as `c:get/1`, but raises `Nostrum.Error.CacheError` in case of a failure.
+  Same as `get/1`, but raises `Nostrum.Error.CacheError` in case of a failure.
   """
   @spec get!(User.id()) :: no_return | User.t()
   def get!(id) when is_snowflake(id), do: id |> get |> Util.bangify_find(id, __MODULE__)
+
+  @doc "Create a user using the selected cache implementation."
+  @spec create(map()) :: User.t()
+  def create(payload) do
+    @configured_cache.create(payload)
+  end
+
+  @doc "Bulk create multiple users using the selected cache implementation."
+  @spec bulk_create(Enum.t()) :: :ok
+  def bulk_create(users) do
+    @configured_cache.bulk_create(users)
+  end
+
+  @doc "Update the given user using the selected cache implementation."
+  @spec update(map()) :: :noop | {User.t(), User.t()}
+  def update(payload) do
+    @configured_cache.update(payload)
+  end
+
+  @doc "Delete a user by ID using the selected cache implementation."
+  @spec delete(User.id()) :: :noop | User.t()
+  def delete(id) when is_snowflake(id) do
+    @configured_cache.delete(id)
+  end
 end
diff --git a/lib/nostrum/cache/user_cache/ets.ex b/lib/nostrum/cache/user_cache/ets.ex
index 5694ad2b..24d27e5c 100644
--- a/lib/nostrum/cache/user_cache/ets.ex
+++ b/lib/nostrum/cache/user_cache/ets.ex
@@ -1,5 +1,5 @@
 defmodule Nostrum.Cache.UserCache.ETS do
-  @table_name :nostrum_users
+  @table_name :users
   @moduledoc """
   An ETS-based cache for users.
 
@@ -12,18 +12,10 @@ defmodule Nostrum.Cache.UserCache.ETS do
   @behaviour Nostrum.Cache.UserCache
 
   alias Nostrum.Struct.User
-  use Supervisor
 
-  @doc "Start the supervisor."
-  def start_link(init_arg) do
-    Supervisor.start_link(__MODULE__, init_arg, name: __MODULE__)
-  end
-
-  @doc "Set up the ETS table."
-  @impl Supervisor
-  def init(_init_arg) do
+  @doc "Set up the cache for tests."
+  def setup do
     :ets.new(@table_name, [:set, :public, :named_table])
-    Supervisor.init([], strategy: :one_for_one)
   end
 
   @impl Nostrum.Cache.UserCache
diff --git a/lib/nostrum/voice/audio.ex b/lib/nostrum/voice/audio.ex
index b9bfe444..35873d54 100644
--- a/lib/nostrum/voice/audio.ex
+++ b/lib/nostrum/voice/audio.ex
@@ -18,11 +18,15 @@ defmodule Nostrum.Voice.Audio do
   @encryption_mode "xsalsa20_poly1305"
   @samples_per_frame 960
   @usec_per_frame 20_000
-  # How many consecutive packets to send before resting
+  # Default value
   @frames_per_burst 10
 
   def encryption_mode, do: @encryption_mode
 
+  # How many consecutive packets to send before resting
+  def frames_per_burst,
+    do: Application.get_env(:nostrum, :audio_frames_per_burst, @frames_per_burst)
+
   def rtp_header(%VoiceState{} = voice) do
     <<
       0x80::8,
@@ -97,7 +101,7 @@ defmodule Nostrum.Voice.Audio do
   end
 
   def take_nap(diff \\ 0) do
-    ((@usec_per_frame * @frames_per_burst - diff) / 1000)
+    ((@usec_per_frame * frames_per_burst() - diff) / 1000)
     |> trunc()
     |> max(0)
     |> Process.sleep()
@@ -114,7 +118,7 @@ defmodule Nostrum.Voice.Audio do
     {voice, done} =
       voice
       |> get_source()
-      |> Enum.take(@frames_per_burst)
+      |> Enum.take(frames_per_burst())
       |> send_frames(voice)
 
     :timer.cancel(watchdog)
@@ -149,9 +153,9 @@ defmodule Nostrum.Voice.Audio do
        # If using raw audio and it isn't stateful, update its state manually
        raw_audio:
          unless(is_nil(voice.raw_audio) or voice.raw_stateful,
-           do: Enum.slice(voice.raw_audio, @frames_per_burst..-1)
+           do: Enum.slice(voice.raw_audio, frames_per_burst()..-1)
          )
-     ), length(frames) < @frames_per_burst}
+     ), length(frames) < frames_per_burst()}
   end
 
   def get_stream_url(url) do
@@ -308,7 +312,7 @@ defmodule Nostrum.Voice.Audio do
   end
 
   def on_stall(%VoiceState{} = voice) do
-    if VoiceState.playing?(voice) and not is_nil(voice.ffmpeg_proc) do
+    unless is_nil(voice.ffmpeg_proc) do
       Proc.stop(voice.ffmpeg_proc)
     end
   end


Test patch:
diff --git a/test/nostrum/cache/user_cache_test.exs b/test/nostrum/cache/user_cache_test.exs
index e9c3f119..c7b800c8 100644
--- a/test/nostrum/cache/user_cache_test.exs
+++ b/test/nostrum/cache/user_cache_test.exs
@@ -34,7 +34,8 @@ defmodule Nostrum.Cache.UserCacheTest do
       }
 
       setup do
-        [pid: start_supervised!(@cache)]
+        @cache.setup()
+        :ok
       end
 
       describe "bulk_create/1" do