stavro / arc

:paperclip: Flexible file upload and attachment library for Elixir
1.16k stars 210 forks source link

Issues with tranform conversions #161

Open Voronchuk opened 7 years ago

Voronchuk commented 7 years ago

I'm trying the following conversion: -thumbnail 200x200 -gravity center -extent 200x200 -fuzz 10% -transparent white -format png

The resulted file still has white background, because temporary output file doesn't have any extension, conversion looks like the following:

["/var/folders/bd/52zmmlsd1yg3g8ctp9gft6z40000gn/T//plug-1487/multipart-537811-990737-4",
 "-thumbnail", "200x200", "-gravity", "center", "-extent", "200x200", "-fuzz",
 "10%", "-transparent", "white", "-format", "png",
 "/var/folders/bd/52zmmlsd1yg3g8ctp9gft6z40000gn/T/Z5LSFDC34AFDOEB2W6IB6UBSO3ST5D2M"]

I have fixed my issue with the following changes:

processor.ex

defmodule Arc.Processor do
  def process(definition, version, {file, scope}) do
    transform = definition.transform(version, {file, scope})
    apply_transformation(file, transform)
  end

  defp apply_transformation(file, :noaction), do: {:ok, file}
  defp apply_transformation(file, {:noaction}), do: {:ok, file} # Deprecated
  defp apply_transformation(file, {cmd, conversion}) do
    Arc.Transformations.Convert.apply(cmd, Arc.File.ensure_path(file), conversion)
  end
  defp apply_transformation(file, {cmd, conversion, output_ext}) do
    Arc.Transformations.Convert.apply(cmd, Arc.File.ensure_path(file), conversion, output_ext)
  end
end

convert.ex

defmodule Arc.Transformations.Convert do
  def apply(cmd, file, args) do
    new_path = Arc.File.generate_temporary_path(file)
    apply(cmd, file, args, new_path)
  end
  def apply(cmd, file, args, :png) do
    new_path = Arc.File.generate_temporary_path(file) <> ".png"
    apply(cmd, file, args, new_path)
  end
  def apply(cmd, file, args, new_path) do
    args     = if is_function(args), do: args.(file.path, new_path), else: [file.path | (String.split(args, " ") ++ [new_path])]
    program  = to_string(cmd)

    ensure_executable_exists!(program)

    case System.cmd(program, args_list(args), stderr_to_stdout: true) do
      {_, 0} ->
        {:ok, %Arc.File{file | path: new_path}}
      {error_message, _exit_code} ->
        {:error, error_message}
    end
  end

  defp args_list(args) when is_list(args), do: args
  defp args_list(args), do: ~w(#{args})

  defp ensure_executable_exists!(program) do
    unless System.find_executable(program) do
      raise Arc.MissingExecutableError, message: program
    end
  end
end

Does it make any sense?

Also I think that transformation customisations are the common case, mb we should put Arc.Transformations.Convert as a configurable module?

stavro commented 7 years ago

The temporary file shouldn't need an extension, because when it is moved to your S3 bucket it will have an extension. Can I see where you're defining your transformation in the uploader definition?

Voronchuk commented 7 years ago

In that case we use local storage, not S3.

Generally we take any kind of images (mostly jpeg) and transform them to transparent png.

    @command_character "-thumbnail 200x200 -gravity center -extent 200x200 -fuzz 10% -transparent white -format png"

    def transform(:thumb, scope) do
        with {%{file_name: file_name}, object} when object != nil <- scope,
            ".svg" <- file_name |> Path.extname |> String.downcase
        do
            :noaction
        else
            {_, nil} ->
                :noaction
            _ ->
                {:convert, &lazy_transform/2, :png}
        end
    end

    defp lazy_transform(input, output) do
        "#{input} #{@command_character} #{output}"
    end
pedep commented 6 years ago

Pretty sure this issue is related to #198