Make new_from_enum and write_to_stream accept keyword opts

Author: akash-akya

c_src/vips_foreign.c

@@ -135,7 +135,6 @@ ERL_NIF_TERM nif_foreign_find_save(ErlNifEnv *env, int argc,
   return ret;
 }
 
-
 ERL_NIF_TERM nif_foreign_find_load_source(ErlNifEnv *env, int argc,
                                           const ERL_NIF_TERM argv[]) {
   ASSERT_ARGC(argc, 1);
@@ -155,7 +154,8 @@ ERL_NIF_TERM nif_foreign_find_load_source(ErlNifEnv *env, int argc,
   name = vips_foreign_find_load_source(source);
 
   if (!name) {
-    error("Failed to find the loader for the source. error: %s", vips_error_buffer());
+    error("Failed to find the loader for the source. error: %s",
+          vips_error_buffer());
     vips_error_clear();
     ret = make_error(env, "Failed to find loader for the source");
     goto exit;
@@ -187,7 +187,8 @@ ERL_NIF_TERM nif_foreign_find_save_target(ErlNifEnv *env, int argc,
   name = vips_foreign_find_save_target(suffix);
 
   if (!name) {
-    error("Failed to find saver for the target. error: %s", vips_error_buffer());
+    error("Failed to find saver for the target. error: %s",
+          vips_error_buffer());
     vips_error_clear();
     ret = make_error(env, "Failed to find saver for the target");
     goto exit;

c_src/vips_foreign.h

@@ -16,10 +16,10 @@ ERL_NIF_TERM nif_foreign_find_save(ErlNifEnv *env, int argc,
                                    const ERL_NIF_TERM argv[]);
 
 ERL_NIF_TERM nif_foreign_find_load_source(ErlNifEnv *env, int argc,
-                                   const ERL_NIF_TERM argv[]);
+                                          const ERL_NIF_TERM argv[]);
 
 ERL_NIF_TERM nif_foreign_find_save_target(ErlNifEnv *env, int argc,
-                                   const ERL_NIF_TERM argv[]);
+                                          const ERL_NIF_TERM argv[]);
 
 ERL_NIF_TERM nif_foreign_get_suffixes(ErlNifEnv *env, int argc,
                                       const ERL_NIF_TERM argv[]);

c_src/vix.c

@@ -153,11 +153,13 @@ static ErlNifFunc nif_funcs[] = {
      0},
 
     /* VipsForeign */
-    {"nif_foreign_find_load", 1, nif_foreign_find_load, ERL_NIF_DIRTY_JOB_IO_BOUND}, // it might read bytes form the file
+    {"nif_foreign_find_load", 1, nif_foreign_find_load,
+     ERL_NIF_DIRTY_JOB_IO_BOUND}, // it might read bytes form the file
     {"nif_foreign_find_save", 1, nif_foreign_find_save, 0},
     {"nif_foreign_find_load_buffer", 1, nif_foreign_find_load_buffer, 0},
     {"nif_foreign_find_save_buffer", 1, nif_foreign_find_save_buffer, 0},
-    {"nif_foreign_find_load_source", 1, nif_foreign_find_load_source, ERL_NIF_DIRTY_JOB_IO_BOUND}, // it might read bytes from source
+    {"nif_foreign_find_load_source", 1, nif_foreign_find_load_source,
+     ERL_NIF_DIRTY_JOB_IO_BOUND}, // it might read bytes from source
     {"nif_foreign_find_save_target", 1, nif_foreign_find_save_target, 0},
     {"nif_foreign_get_suffixes", 0, nif_foreign_get_suffixes, 0},
     {"nif_foreign_get_loader_suffixes", 0, nif_foreign_get_loader_suffixes, 0},

lib/vix/source_pipe.ex

@@ -14,6 +14,7 @@ defmodule Vix.SourcePipe do
     defstruct bin: [], client_pid: nil
   end
 
+  @spec new() :: {pid, Vix.Vips.Source.t()}
   def new do
     {:ok, pipe} = GenServer.start_link(__MODULE__, nil)
     source = GenServer.call(pipe, :source, :infinity)
@@ -37,7 +38,13 @@ defmodule Vix.SourcePipe do
   def handle_continue(nil, _) do
     case Nif.nif_source_new() do
       {:ok, {fd, source}} ->
-        {:noreply, %SourcePipe{fd: fd, pending: %Pending{}, source: source}}
+        source_pipe = %SourcePipe{
+          fd: fd,
+          pending: %Pending{},
+          source: %Vix.Vips.Source{ref: source}
+        }
+
+        {:noreply, source_pipe}
 
       {:error, reason} ->
         {:stop, reason, nil}

lib/vix/target_pipe.ex

@@ -7,17 +7,20 @@ defmodule Vix.TargetPipe do
 
   @moduledoc false
 
+  @type t() :: struct
+
   defstruct [:fd, :pending, :task_result, :task_pid]
 
   defmodule Pending do
     @moduledoc false
-    defstruct size: nil, client_pid: nil
+    defstruct size: nil, client_pid: nil, opts: []
   end
 
   @default_buffer_size 65_535
 
-  def new(vips_image, suffix) do
-    GenServer.start_link(__MODULE__, %{image: vips_image, suffix: suffix})
+  @spec new(Vix.Vips.Image.t(), String.t(), keyword) :: GenServer.on_start()
+  def new(image, suffix, opts) do
+    GenServer.start_link(__MODULE__, %{image: image, suffix: suffix, opts: opts})
   end
 
   def read(process, max_size \\ @default_buffer_size)
@@ -31,15 +34,15 @@ defmodule Vix.TargetPipe do
 
   # Server
 
-  def init(%{image: image, suffix: suffix}) do
+  def init(%{image: image, suffix: suffix, opts: opts}) do
     Process.flag(:trap_exit, true)
-    {:ok, nil, {:continue, %{image: image, suffix: suffix}}}
+    {:ok, nil, {:continue, %{image: image, suffix: suffix, opts: opts}}}
   end
 
-  def handle_continue(%{image: image, suffix: suffix}, _) do
+  def handle_continue(%{image: image, suffix: suffix, opts: opts}, _) do
     case Nif.nif_target_new() do
       {:ok, {fd, target}} ->
-        pid = start_task(image, target, suffix)
+        pid = start_task(image, %Vix.Vips.Target{ref: target}, suffix, opts)
         {:noreply, %TargetPipe{fd: fd, task_pid: pid, pending: %Pending{}}}
 
       {:error, reason} ->
@@ -98,9 +101,21 @@ defmodule Vix.TargetPipe do
     {:noreply, state}
   end
 
-  defp start_task(image, target, suffix) do
+  @spec start_task(Vix.Vips.Image.t(), Vix.Vips.Target.t(), String.t(), keyword) :: pid
+  defp start_task(%Vix.Vips.Image{} = image, target, suffix, []) do
     spawn_link(fn ->
-      result = Nif.nif_image_to_target(image, target, suffix)
+      result = Nif.nif_image_to_target(image.ref, target.ref, suffix)
+      Process.exit(self(), result)
+    end)
+  end
+
+  defp start_task(image, target, suffix, opts) do
+    spawn_link(fn ->
+      result =
+        with {:ok, saver} <- Vix.Vips.Foreign.find_save_target(suffix) do
+          Vix.Vips.Operation.Helper.operation_call(saver, [image, target], opts)
+        end
+
       Process.exit(self(), result)
     end)
   end

lib/vix/vips/foreign.ex

@@ -32,8 +32,8 @@ defmodule Vix.Vips.Foreign do
   end
 
   @spec find_load_source(Vix.Vips.Source.t()) :: {:ok, operation_name} | {:error, String.t()}
-  def find_load_source(source) do
-    Nif.nif_foreign_find_load_source(source)
+  def find_load_source(%Vix.Vips.Source{ref: vips_source}) do
+    Nif.nif_foreign_find_load_source(vips_source)
   end
 
   @spec find_save_target(String.t()) :: {:ok, operation_name} | {:error, String.t()}

lib/vix/vips/image.ex

@@ -447,6 +447,7 @@ defmodule Vix.Vips.Image do
   @doc since: "0.31.0"
   def new_from_file(path, opts) do
     with {:ok, path} <- normalize_path(path),
+         :ok <- validate_options(opts),
          {:ok, loader} <- Vix.Vips.Foreign.find_load(path),
          {:ok, {ref, _optional}} <- Operation.Helper.operation_call(loader, [path], opts) do
       {:ok, wrap_type(ref)}
@@ -504,7 +505,8 @@ defmodule Vix.Vips.Image do
   """
   @spec new_from_buffer(binary(), keyword()) :: {:ok, t()} | {:error, term()}
   def new_from_buffer(bin, opts \\ []) do
-    with {:ok, loader} <- Vix.Vips.Foreign.find_load_buffer(bin),
+    with :ok <- validate_options(opts),
+         {:ok, loader} <- Vix.Vips.Foreign.find_load_buffer(bin),
          {:ok, {ref, _optional}} <- Operation.Helper.operation_call(loader, [bin], opts) do
       {:ok, wrap_type(ref)}
     end
@@ -580,26 +582,18 @@ defmodule Vix.Vips.Image do
   :ok = Image.write_to_file(image, "puppies.png")
   ```
 
-  Optional param `opts` string is passed to the image loader. It is a string
-  of the format "[name=value,...]".
+  Optional param `opts` is passed to the image loader. Available
+  options depends on the format.
 
   ```elixir
-  Image.new_from_enum(stream, "[shrink=2]")
-  ```
-
-  Will read the stream with downsampling by a factor of two.
-
-  The full set of options available depend upon the image format. You
-  can find all options available at the command-line. To see a summary
-  of the available options for the JPEG loader:
-
-  ```shell
-  $ vips jpegload_source
+  Image.new_from_enum(stream, [shrink: 2])
   ```
 
+  You can find all options available for a format under operation function in
+  [Operation](./search.html?q=load+-buffer+-filename+-profile) module.
   """
-  @spec new_from_enum(Enumerable.t(), String.t()) :: {:ok, t()} | {:error, term()}
-  def new_from_enum(enum, opts \\ "") do
+  @spec new_from_enum(Enumerable.t(), String.t() | keyword) :: {:ok, t()} | {:error, term()}
+  def new_from_enum(enum, opts \\ []) do
     parent = self()
 
     pid =
@@ -625,9 +619,17 @@ defmodule Vix.Vips.Image do
       end)
 
     receive do
-      {^pid, source} ->
-        Nif.nif_image_new_from_source(source, opts)
+      # for backward compatibility
+      {^pid, source} when is_binary(opts) ->
+        Nif.nif_image_new_from_source(source.ref, opts)
         |> wrap_type()
+
+      {^pid, source} ->
+        with :ok <- validate_options(opts),
+             {:ok, loader} <- Vix.Vips.Foreign.find_load_source(source),
+             {:ok, {ref, _optional}} <- Operation.Helper.operation_call(loader, [source], opts) do
+          {:ok, wrap_type(ref)}
+        end
     end
   end
 
@@ -649,29 +651,22 @@ defmodule Vix.Vips.Image do
     |> Stream.run()
   ```
 
-  Second param `suffix` determines the format of the output
-  stream. Save options may be appended to the suffix as
-  "[name=value,...]".
+  Optional param `opts` is passed to the image saver. Available
+  options depends on the file format. You can find all options
+  available for a format under operation function in
+  [Operation](./search.html?q=save+buffer+-filename+-profile) module.
 
   ```elixir
-  Image.write_to_stream(vips_image, ".jpg[Q=90]")
+  Image.write_to_stream(vips_image, ".jpg", Q: 90)
   ```
-
-  Options are specific to save operation. You can find out all
-  available options for the save operation at command line. For
-  example:
-
-  ```shell
-  $ vips jpegsave_target
-  ```
-
   """
-  @spec write_to_stream(t(), String.t()) :: Enumerable.t()
-  def write_to_stream(%Image{ref: vips_image}, suffix) do
+  @spec write_to_stream(t(), String.t(), keyword) :: Enumerable.t()
+
+  @doc since: "0.32.0"
+  def write_to_stream(%Image{ref: _} = image, suffix, opts) do
     Stream.resource(
       fn ->
-        {:ok, pipe} = Vix.TargetPipe.new(vips_image, suffix)
-        pipe
+        init_write_stream(image, suffix, opts)
       end,
       fn pipe ->
         ret = Vix.TargetPipe.read(pipe)
@@ -693,6 +688,10 @@ defmodule Vix.Vips.Image do
     )
   end
 
+  def write_to_stream(%Image{ref: _} = image, suffix) do
+    write_to_stream(%Image{ref: _} = image, suffix, [])
+  end
+
   @doc """
   Converts an Image to a nested list.
 
@@ -804,12 +803,9 @@ defmodule Vix.Vips.Image do
   def write_to_file(%Image{ref: _} = image, path, opts) do
     path = normalize_string(Path.expand(path))
 
-    case Vix.Vips.Foreign.find_save(path) do
-      {:ok, saver} ->
-        Operation.Helper.operation_call(saver, [image, path], opts)
-
-      error ->
-        error
+    with :ok <- validate_options(opts),
+         {:ok, saver} <- Vix.Vips.Foreign.find_save(path) do
+      Operation.Helper.operation_call(saver, [image, path], opts)
     end
   end
 
@@ -834,7 +830,7 @@ defmodule Vix.Vips.Image do
 
   ```elixir
   # returns image in JPEG format as binary with Q factor set 90
-  Image.write_to_buffer(img, ".jpg", [Q: 90])
+  Image.write_to_buffer(img, ".jpg", Q: 90)
   ```
 
   If you want more control over the saver, Use specific format saver
@@ -845,12 +841,9 @@ defmodule Vix.Vips.Image do
 
   @doc since: "0.32.0"
   def write_to_buffer(%Image{ref: _} = image, suffix, opts) do
-    case Vix.Vips.Foreign.find_save_buffer(normalize_string(suffix)) do
-      {:ok, saver} ->
-        Operation.Helper.operation_call(saver, [image], opts)
-
-      error ->
-        error
+    with :ok <- validate_options(opts),
+         {:ok, saver} <- Vix.Vips.Foreign.find_save_buffer(normalize_string(suffix)) do
+      Operation.Helper.operation_call(saver, [image], opts)
     end
   end
 
@@ -1650,4 +1643,27 @@ defmodule Vix.Vips.Image do
       true -> raise ArgumentError, "integer size must be <= 32bit"
     end
   end
+
+  @spec validate_options(keyword) :: :ok | {:error, String.t()}
+  defp validate_options(opts) do
+    if Keyword.keyword?(opts) do
+      :ok
+    else
+      {:error, "Opts must be a keyword list"}
+    end
+  end
+
+  @spec init_write_stream(Image.t(), String.t(), keyword) :: term | no_return
+  defp init_write_stream(image, suffix, opts) do
+    with :ok <- validate_options(opts),
+         {:ok, pipe} <- Vix.TargetPipe.new(image, suffix, opts) do
+      pipe
+    else
+      {:error, reason} when is_binary(reason) ->
+        raise Error, reason
+
+      {:error, reason} ->
+        raise Error, inspect(reason)
+    end
+  end
 end

lib/vix/vips/source.ex

@@ -2,9 +2,13 @@ defmodule Vix.Vips.Source do
   @moduledoc false
 
   alias Vix.Type
+  alias __MODULE__
 
   @behaviour Type
-  @opaque t() :: reference()
+
+  @type t() :: %Source{ref: reference}
+
+  defstruct [:ref]
 
   @impl Type
   def typespec do
@@ -17,8 +21,16 @@ defmodule Vix.Vips.Source do
   def default(nil), do: :unsupported
 
   @impl Type
-  def to_nif_term(_value, _data), do: raise("VipsSource is not implemented yet")
+  def to_nif_term(source, _data) do
+    case source do
+      %Source{ref: ref} ->
+        ref
+
+      value ->
+        raise ArgumentError, message: "expected Vix.Vips.Source given: #{inspect(value)}"
+    end
+  end
 
   @impl Type
-  def to_erl_term(_value), do: raise("VipsSource is not implemented yet")
+  def to_erl_term(ref), do: %Source{ref: ref}
 end