From 029ee0d0f306c7ce896a3a7f73ddddfd8c37903d Mon Sep 17 00:00:00 2001
From: Billal Ghilas <84322223+gBillal@users.noreply.github.com>
Date: Mon, 27 Jan 2025 23:31:37 +0100
Subject: [PATCH] Use nimble options for converter and reader (#30)

---
 lib/encoder.ex                |   3 +-
 lib/reader.ex                 | 151 +++++++++++++++++++---------------
 lib/video_converter.ex        |  71 ++++++----------
 test/video_converter_test.exs |   6 +-
 4 files changed, 118 insertions(+), 113 deletions(-)

diff --git a/lib/encoder.ex b/lib/encoder.ex
index 59a34f3..dbb710a 100644
--- a/lib/encoder.ex
+++ b/lib/encoder.ex
@@ -63,7 +63,8 @@ defmodule Xav.Encoder do
       """
     ],
     profile: [
-      type: {:in, [:constrained_baseline, :baseline, :main, :high, :main_10, :main_still_picture]},
+      type:
+        {:in, [:constrained_baseline, :baseline, :main, :high, :main_10, :main_still_picture]},
       type_doc: "`t:atom/0`",
       doc: """
       The encoder's profile.
diff --git a/lib/reader.ex b/lib/reader.ex
index f825b10..95122e3 100644
--- a/lib/reader.ex
+++ b/lib/reader.ex
@@ -3,20 +3,37 @@ defmodule Xav.Reader do
   Audio/video file reader.
   """
 
-  @typedoc """
-  Reader options.
-
-  * `read` - determines which stream to read from a file.
-  Defaults to `:video`.
-  * `device?` - determines whether path points to the camera. Defaults to `false`.
-  """
-  @type opts :: [
-          read: :audio | :video,
-          device?: boolean,
-          out_format: Xav.Frame.format(),
-          out_sample_rate: integer(),
-          out_channels: integer()
-        ]
+  @audio_out_formats [:u8, :s16, :s32, :s64, :f32, :f64]
+
+  @reader_options_schema [
+    read: [
+      type: {:in, [:audio, :video]},
+      default: :video,
+      doc: "The type of the stream to read from the input, either `video` or `audio`"
+    ],
+    device?: [
+      type: :boolean,
+      default: false,
+      doc: "Whether the path points to the camera"
+    ],
+    out_format: [
+      type: {:in, @audio_out_formats},
+      doc: """
+      The output format of the audio samples. It should be one of
+      the following values: `#{Enum.join(@audio_out_formats, ", ")}`.
+
+      For video samples, it is always `:rgb24`.
+      """
+    ],
+    out_sample_rate: [
+      type: :pos_integer,
+      doc: "The output sample rate of the audio samples"
+    ],
+    out_channels: [
+      type: :pos_integer,
+      doc: "The output number of channels of the audio samples"
+    ]
+  ]
 
   @type t() :: %__MODULE__{
           reader: reference(),
@@ -37,9 +54,9 @@ defmodule Xav.Reader do
               [:in_sample_rate, :out_sample_rate, :in_channels, :out_channels, :framerate]
 
   @doc """
-  The same as new/1 but raises on error.
+  The same as `new/1` but raises on error.
   """
-  @spec new!(String.t(), opts()) :: t()
+  @spec new!(String.t(), Keyword.t()) :: t()
   def new!(path, opts \\ []) do
     case new(path, opts) do
       {:ok, reader} -> reader
@@ -56,57 +73,12 @@ defmodule Xav.Reader do
 
   Microphone input is not supported.
 
-  `opts` can be used to specify desired output parameters.
-  Video frames are always returned in RGB format. This setting cannot be changed.
-  Audio samples are always in the packed form.
-  See `Xav.Decoder.new/2` for more information.
+  The following options can be provided:\n#{NimbleOptions.docs(@reader_options_schema)}
   """
-  @spec new(String.t(), opts()) :: {:ok, t()} | {:error, term()}
+  @spec new(String.t(), Keyword.t()) :: {:ok, t()} | {:error, term()}
   def new(path, opts \\ []) do
-    read = opts[:read] || :video
-    device? = opts[:device?] || false
-    out_format = opts[:out_format]
-    out_sample_rate = opts[:out_sample_rate] || 0
-    out_channels = opts[:out_channels] || 0
-
-    case Xav.Reader.NIF.new(
-           path,
-           to_int(device?),
-           to_int(read),
-           out_format,
-           out_sample_rate,
-           out_channels
-         ) do
-      {:ok, reader, in_format, out_format, in_sample_rate, out_sample_rate, in_channels,
-       out_channels, bit_rate, duration, codec} ->
-        {:ok,
-         %__MODULE__{
-           reader: reader,
-           in_format: in_format,
-           out_format: out_format,
-           in_sample_rate: in_sample_rate,
-           out_sample_rate: out_sample_rate,
-           in_channels: in_channels,
-           out_channels: out_channels,
-           bit_rate: bit_rate,
-           duration: duration,
-           codec: to_human_readable(codec)
-         }}
-
-      {:ok, reader, in_format, out_format, bit_rate, duration, codec, framerate} ->
-        {:ok,
-         %__MODULE__{
-           reader: reader,
-           in_format: in_format,
-           out_format: out_format,
-           bit_rate: bit_rate,
-           duration: duration,
-           codec: to_human_readable(codec),
-           framerate: framerate
-         }}
-
-      {:error, _reason} = err ->
-        err
+    with {:ok, opts} <- NimbleOptions.validate(opts, @reader_options_schema) do
+      do_create_reader(path, opts)
     end
   end
 
@@ -144,8 +116,10 @@ defmodule Xav.Reader do
 
   @doc """
   Creates a new reader stream.
+
+  Check `new/1` for the available options.
   """
-  @spec stream!(String.t(), opts()) :: Enumerable.t()
+  @spec stream!(String.t(), Keyword.t()) :: Enumerable.t()
   def stream!(path, opts \\ []) do
     Stream.resource(
       fn ->
@@ -167,6 +141,51 @@ defmodule Xav.Reader do
     )
   end
 
+  defp do_create_reader(path, opts) do
+    out_sample_rate = opts[:out_sample_rate] || 0
+    out_channels = opts[:out_channels] || 0
+
+    case Xav.Reader.NIF.new(
+           path,
+           to_int(opts[:device?]),
+           to_int(opts[:read]),
+           opts[:out_format],
+           out_sample_rate,
+           out_channels
+         ) do
+      {:ok, reader, in_format, out_format, in_sample_rate, out_sample_rate, in_channels,
+       out_channels, bit_rate, duration, codec} ->
+        {:ok,
+         %__MODULE__{
+           reader: reader,
+           in_format: in_format,
+           out_format: out_format,
+           in_sample_rate: in_sample_rate,
+           out_sample_rate: out_sample_rate,
+           in_channels: in_channels,
+           out_channels: out_channels,
+           bit_rate: bit_rate,
+           duration: duration,
+           codec: to_human_readable(codec)
+         }}
+
+      {:ok, reader, in_format, out_format, bit_rate, duration, codec, framerate} ->
+        {:ok,
+         %__MODULE__{
+           reader: reader,
+           in_format: in_format,
+           out_format: out_format,
+           bit_rate: bit_rate,
+           duration: duration,
+           codec: to_human_readable(codec),
+           framerate: framerate
+         }}
+
+      {:error, _reason} = err ->
+        err
+    end
+  end
+
   defp to_human_readable(:libdav1d), do: :av1
   defp to_human_readable(:mp3float), do: :mp3
   defp to_human_readable(other), do: other
diff --git a/lib/video_converter.ex b/lib/video_converter.ex
index ec6a9b8..1f41031 100644
--- a/lib/video_converter.ex
+++ b/lib/video_converter.ex
@@ -15,38 +15,45 @@ defmodule Xav.VideoConverter do
           out_height: Frame.height()
         }
 
-  @typedoc """
-  Type definition for converter options.
-
-  * `out_format` - video format to convert to (`e.g. :rgb24`).
-  * `out_width` - scale the video frame to this width.
-  * `out_height` - scale the video frame to this height.
-
-  If `out_width` and `out_height` are both not provided, scaling is not performed. If one of the
-  dimensions is `nil`, the other will be calculated based on the input dimensions as
-  to keep the aspect ratio.
-  """
-  @type converter_opts() :: [
-          out_format: Frame.video_format(),
-          out_width: Frame.width(),
-          out_height: Frame.height()
-        ]
+  @converter_schema [
+    out_width: [
+      type: :pos_integer,
+      required: false,
+      doc: """
+      scale the video frame to this width
+
+      If `out_width` and `out_height` are both not provided, scaling is not performed. If one of the
+      dimensions is `nil`, the other will be calculated based on the input dimensions as
+      to keep the aspect ratio.
+      """
+    ],
+    out_height: [
+      type: :pos_integer,
+      required: false,
+      doc: "scale the video frame to this height"
+    ],
+    out_format: [
+      type: :atom,
+      required: false,
+      doc: "video format to convert to (e.g. `:rgb24`)"
+    ]
+  ]
 
   defstruct [:converter, :out_format, :out_width, :out_height]
 
   @doc """
   Creates a new video converter.
+
+  The following options can be passed:\n#{NimbleOptions.docs(@converter_schema)}
   """
-  @spec new(converter_opts()) :: t()
+  @spec new(Keyword.t()) :: t()
   def new(converter_opts) do
-    opts = Keyword.validate!(converter_opts, [:out_format, :out_width, :out_height])
+    opts = NimbleOptions.validate!(converter_opts, @converter_schema)
 
     if is_nil(opts[:out_format]) and is_nil(opts[:out_width]) and is_nil(opts[:out_height]) do
       raise "At least one of `out_format`, `out_width` or `out_height` must be provided"
     end
 
-    :ok = validate_converter_options(opts)
-
     converter = NIF.new(opts[:out_format], opts[:out_width] || -1, opts[:out_height] || -1)
 
     %__MODULE__{
@@ -80,28 +87,4 @@ defmodule Xav.VideoConverter do
       pts: frame.pts
     }
   end
-
-  defp validate_converter_options([]), do: :ok
-
-  defp validate_converter_options([{_key, nil} | opts]) do
-    validate_converter_options(opts)
-  end
-
-  defp validate_converter_options([{key, value} | _opts])
-       when key in [:out_width, :out_height] and not is_integer(value) do
-    raise %ArgumentError{
-      message: "Expected an integer value for #{inspect(key)}, received: #{inspect(value)}"
-    }
-  end
-
-  defp validate_converter_options([{key, value} | _opts])
-       when key in [:out_width, :out_height] and value < 1 do
-    raise %ArgumentError{
-      message: "Invalid value for #{inspect(key)}, expected a value to be >= 1"
-    }
-  end
-
-  defp validate_converter_options([{_key, _value} | opts]) do
-    validate_converter_options(opts)
-  end
 end
diff --git a/test/video_converter_test.exs b/test/video_converter_test.exs
index 21d0066..23c6e82 100644
--- a/test/video_converter_test.exs
+++ b/test/video_converter_test.exs
@@ -1,6 +1,8 @@
 defmodule Xav.VideoConverterTest do
   use ExUnit.Case, async: true
 
+  alias NimbleOptions.ValidationError
+
   describe "new/1" do
     test "new converter" do
       assert %Xav.VideoConverter{out_format: :rgb24, converter: converter} =
@@ -14,8 +16,8 @@ defmodule Xav.VideoConverterTest do
     end
 
     test "fails on invalid options" do
-      assert_raise ArgumentError, fn -> Xav.VideoConverter.new(out_width: 0) end
-      assert_raise ArgumentError, fn -> Xav.VideoConverter.new(out_height: "15") end
+      assert_raise ValidationError, fn -> Xav.VideoConverter.new(out_width: 0) end
+      assert_raise ValidationError, fn -> Xav.VideoConverter.new(out_height: "15") end
     end
   end