From 4994e70652ea1cbc47cbe489a841c8464846e215 Mon Sep 17 00:00:00 2001 From: Kieran Date: Tue, 4 Jun 2024 09:19:37 -0700 Subject: [PATCH] [Bugfix] Partially revert custom codec selection (#279) * Removed and re-added codec preference columns * Removed custom codec work from download options builder * Updated settings UI * Made codec preferences non-optional fields --- lib/pinchflat/downloading/codec_parser.ex | 82 ------------------- .../downloading/download_option_builder.ex | 11 ++- lib/pinchflat/settings/setting.ex | 39 +-------- .../controllers/settings/setting_html.ex | 2 - .../codec_settings_help.html.heex | 26 +++--- .../setting_html/setting_form.html.heex | 16 ++-- ...40603230512_refactor_codec_preferences.exs | 15 ++++ .../downloading/codec_parser_test.exs | 70 ---------------- .../download_option_builder_test.exs | 12 ++- test/pinchflat/settings_test.exs | 60 -------------- 10 files changed, 48 insertions(+), 285 deletions(-) delete mode 100644 lib/pinchflat/downloading/codec_parser.ex create mode 100644 priv/repo/migrations/20240603230512_refactor_codec_preferences.exs delete mode 100644 test/pinchflat/downloading/codec_parser_test.exs diff --git a/lib/pinchflat/downloading/codec_parser.ex b/lib/pinchflat/downloading/codec_parser.ex deleted file mode 100644 index 6953cf4..0000000 --- a/lib/pinchflat/downloading/codec_parser.ex +++ /dev/null @@ -1,82 +0,0 @@ -defmodule Pinchflat.Downloading.CodecParser do - @moduledoc """ - Functions for generating yt-dlp codec strings - """ - - alias Pinchflat.Settings - - @doc """ - Generate a video codec string based on the value of the video_codec_preference setting. - - Returns binary() - """ - def generate_vcodec_string_from_settings do - generate_vcodec_string(Settings.get!(:video_codec_preference)) - end - - @doc """ - Generate an audio codec string based on the value of the audio_codec_preference setting. - - Returns binary() - """ - def generate_acodec_string_from_settings do - generate_acodec_string(Settings.get!(:audio_codec_preference)) - end - - @doc """ - Generate a video codec string from a list of video codecs. - - If the list is nil or empty, the default video codec is AVC. - - Returns binary() - """ - def generate_vcodec_string(nil), do: "bestvideo[vcodec~='^avc']/bestvideo" - def generate_vcodec_string([]), do: generate_vcodec_string(nil) - - def generate_vcodec_string(video_codecs) do - video_codecs - |> Enum.map(&video_codec_map()[&1]) - |> Enum.reject(&is_nil/1) - |> Enum.map(&"bestvideo[vcodec~='^#{&1}']") - |> Enum.concat(["bestvideo"]) - |> Enum.join("/") - end - - @doc """ - Generate an audio codec string from a list of audio codecs. - - If the list is nil or empty, the default audio codec is MP4A. - - Returns binary() - """ - def generate_acodec_string(nil), do: "bestaudio[acodec~='^mp4a']/bestaudio" - def generate_acodec_string([]), do: generate_acodec_string(nil) - - def generate_acodec_string(audio_codecs) do - audio_codecs - |> Enum.map(&audio_codec_map()[&1]) - |> Enum.reject(&is_nil/1) - |> Enum.map(&"bestaudio[acodec~='^#{&1}']") - |> Enum.concat(["bestaudio"]) - |> Enum.join("/") - end - - @doc false - def video_codec_map do - %{ - "av01" => "av01", - "avc" => "avc", - "vp9" => "vp0?9" - } - end - - @doc false - def audio_codec_map do - %{ - "aac" => "aac", - "mp4a" => "mp4a", - "mp3" => "mp3", - "opus" => "opus" - } - end -end diff --git a/lib/pinchflat/downloading/download_option_builder.ex b/lib/pinchflat/downloading/download_option_builder.ex index d508b26..f1817f4 100644 --- a/lib/pinchflat/downloading/download_option_builder.ex +++ b/lib/pinchflat/downloading/download_option_builder.ex @@ -4,9 +4,9 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do """ alias Pinchflat.Sources + alias Pinchflat.Settings alias Pinchflat.Sources.Source alias Pinchflat.Media.MediaItem - alias Pinchflat.Downloading.CodecParser alias Pinchflat.Downloading.OutputPathBuilder alias Pinchflat.Utils.FilesystemUtils, as: FSUtils @@ -122,13 +122,13 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do end defp quality_options(media_profile) do - vcodec_string = CodecParser.generate_vcodec_string_from_settings() - acodec_string = CodecParser.generate_acodec_string_from_settings() + vcodec = Settings.get!(:video_codec_preference) + acodec = Settings.get!(:audio_codec_preference) case media_profile.preferred_resolution do # Also be aware that :audio disabled all embedding options for subtitles :audio -> - [:extract_audio, format: "#{acodec_string}/best"] + [:extract_audio, format_sort: "+acodec:#{acodec}"] resolution_atom -> {resolution_string, _} = @@ -137,10 +137,9 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do |> Integer.parse() [ - format_sort: "res:#{resolution_string}", # Since Plex doesn't support reading metadata from MKV remux_video: "mp4", - format: "((#{vcodec_string})+(#{acodec_string}))/best" + format_sort: "res:#{resolution_string},+codec:#{vcodec}:#{acodec}" ] end end diff --git a/lib/pinchflat/settings/setting.ex b/lib/pinchflat/settings/setting.ex index 8c393ff..d8d0560 100644 --- a/lib/pinchflat/settings/setting.ex +++ b/lib/pinchflat/settings/setting.ex @@ -16,14 +16,11 @@ defmodule Pinchflat.Settings.Setting do :audio_codec_preference ] - @virtual_fields [ - :video_codec_preference_string, - :audio_codec_preference_string - ] - @required_fields ~w( onboarding pro_enabled + video_codec_preference + audio_codec_preference )a schema "settings" do @@ -33,42 +30,14 @@ defmodule Pinchflat.Settings.Setting do field :apprise_version, :string field :apprise_server, :string - field :video_codec_preference, {:array, :string}, default: [] - field :audio_codec_preference, {:array, :string}, default: [] - field :video_codec_preference_string, :string, default: nil, virtual: true - field :audio_codec_preference_string, :string, default: nil, virtual: true + field :video_codec_preference, :string + field :audio_codec_preference, :string end @doc false def changeset(setting, attrs) do setting |> cast(attrs, @allowed_fields) - |> cast(attrs, @virtual_fields, empty_values: []) - |> convert_codec_preference_strings() |> validate_required(@required_fields) end - - defp convert_codec_preference_strings(changeset) do - fields = [ - video_codec_preference_string: :video_codec_preference, - audio_codec_preference_string: :audio_codec_preference - ] - - Enum.reduce(fields, changeset, fn {virtual_field, actual_field}, changeset -> - case get_change(changeset, virtual_field) do - nil -> - changeset - - value -> - new_value = - value - |> String.split(">") - |> Enum.map(&String.trim/1) - |> Enum.reject(&(String.trim(&1) == "")) - |> Enum.map(&String.downcase/1) - - put_change(changeset, actual_field, new_value) - end - end) - end end diff --git a/lib/pinchflat_web/controllers/settings/setting_html.ex b/lib/pinchflat_web/controllers/settings/setting_html.ex index d6f549e..dcb549a 100644 --- a/lib/pinchflat_web/controllers/settings/setting_html.ex +++ b/lib/pinchflat_web/controllers/settings/setting_html.ex @@ -1,8 +1,6 @@ defmodule PinchflatWeb.Settings.SettingHTML do use PinchflatWeb, :html - alias Pinchflat.Downloading.CodecParser - embed_templates "setting_html/*" @doc """ diff --git a/lib/pinchflat_web/controllers/settings/setting_html/codec_settings_help.html.heex b/lib/pinchflat_web/controllers/settings/setting_html/codec_settings_help.html.heex index d6b6354..4d90703 100644 --- a/lib/pinchflat_web/controllers/settings/setting_html/codec_settings_help.html.heex +++ b/lib/pinchflat_web/controllers/settings/setting_html/codec_settings_help.html.heex @@ -1,17 +1,17 @@ diff --git a/lib/pinchflat_web/controllers/settings/setting_html/setting_form.html.heex b/lib/pinchflat_web/controllers/settings/setting_html/setting_form.html.heex index 73ee7bb..0f017ab 100644 --- a/lib/pinchflat_web/controllers/settings/setting_html/setting_form.html.heex +++ b/lib/pinchflat_web/controllers/settings/setting_html/setting_form.html.heex @@ -37,24 +37,20 @@

<.input - id="video_codec_preference_string" - name="setting[video_codec_preference_string]" - value={Enum.join(f[:video_codec_preference].value, ">")} - placeholder="avc>vp9>av01" + field={f[:video_codec_preference]} + placeholder="avc" type="text" label="Video Codec Preference" - help="Order of preference for video codecs. Separate with >. Will be remuxed into an MP4 container. See below for available codecs" + help="Video codec preference. Will be remuxed into an MP4 container. See below for more details" inputclass="font-mono text-sm mr-4" /> <.input - id="audio_codec_preference_string" - name="setting[audio_codec_preference_string]" - value={Enum.join(f[:audio_codec_preference].value, ">")} - placeholder="mp4a>opus>aac" + field={f[:audio_codec_preference]} + placeholder="m4a" type="text" label="Audio Codec Preference" - help="Order of preference for audio codecs. Separate with >. See below for available codecs" + help="Audio codec preference. See below for more details" inputclass="font-mono text-sm mr-4" /> diff --git a/priv/repo/migrations/20240603230512_refactor_codec_preferences.exs b/priv/repo/migrations/20240603230512_refactor_codec_preferences.exs new file mode 100644 index 0000000..0b00830 --- /dev/null +++ b/priv/repo/migrations/20240603230512_refactor_codec_preferences.exs @@ -0,0 +1,15 @@ +defmodule Pinchflat.Repo.Migrations.RefactorCodecPreferences do + use Ecto.Migration + + def change do + alter table(:settings) do + remove :video_codec_preference, {:array, :string}, default: [] + remove :audio_codec_preference, {:array, :string}, default: [] + end + + alter table(:settings) do + add :video_codec_preference, :string, default: "avc", null: false + add :audio_codec_preference, :string, default: "m4a", null: false + end + end +end diff --git a/test/pinchflat/downloading/codec_parser_test.exs b/test/pinchflat/downloading/codec_parser_test.exs deleted file mode 100644 index c7b856a..0000000 --- a/test/pinchflat/downloading/codec_parser_test.exs +++ /dev/null @@ -1,70 +0,0 @@ -defmodule Pinchflat.Downloading.CodecParserTest do - use Pinchflat.DataCase - - alias Pinchflat.Settings - alias Pinchflat.Downloading.CodecParser - - describe "generate_vcodec_string_from_settings/1" do - test "returns a default vcodec string when setting isn't set" do - Settings.set(video_codec_preference: []) - - assert "bestvideo[vcodec~='^avc']/bestvideo" == CodecParser.generate_vcodec_string_from_settings() - end - - test "generates a vcodec string" do - Settings.set(video_codec_preference: ["av01"]) - - assert "bestvideo[vcodec~='^av01']/bestvideo" == CodecParser.generate_vcodec_string_from_settings() - end - end - - describe "generate_acodec_string_from_settings/1" do - test "returns a default acodec string when setting isn't set" do - Settings.set(audio_codec_preference: []) - - assert "bestaudio[acodec~='^mp4a']/bestaudio" == CodecParser.generate_acodec_string_from_settings() - end - - test "generates an acodec string" do - Settings.set(audio_codec_preference: ["mp3"]) - - assert "bestaudio[acodec~='^mp3']/bestaudio" == CodecParser.generate_acodec_string_from_settings() - end - end - - describe "generate_vcodec_string/1" do - test "returns a default vcodec string when nil" do - assert "bestvideo[vcodec~='^avc']/bestvideo" == CodecParser.generate_vcodec_string(nil) - end - - test "returns a default vcodec string when empty" do - assert "bestvideo[vcodec~='^avc']/bestvideo" == CodecParser.generate_vcodec_string([]) - end - - test "generates a vcodec string" do - assert "bestvideo[vcodec~='^av01']/bestvideo" == CodecParser.generate_vcodec_string(["av01"]) - end - - test "ignores options that don't exist" do - assert "bestvideo[vcodec~='^av01']/bestvideo" == CodecParser.generate_vcodec_string(["av01", "foo"]) - end - end - - describe "generate_acodec_string/1" do - test "returns a default acodec string when nil" do - assert "bestaudio[acodec~='^mp4a']/bestaudio" == CodecParser.generate_acodec_string(nil) - end - - test "returns a default acodec string when empty" do - assert "bestaudio[acodec~='^mp4a']/bestaudio" == CodecParser.generate_acodec_string([]) - end - - test "generates an acodec string" do - assert "bestaudio[acodec~='^mp3']/bestaudio" == CodecParser.generate_acodec_string(["mp3"]) - end - - test "ignores options that don't exist" do - assert "bestaudio[acodec~='^mp3']/bestaudio" == CodecParser.generate_acodec_string(["mp3", "foo"]) - end - end -end diff --git a/test/pinchflat/downloading/download_option_builder_test.exs b/test/pinchflat/downloading/download_option_builder_test.exs index a22e11c..d31c6c6 100644 --- a/test/pinchflat/downloading/download_option_builder_test.exs +++ b/test/pinchflat/downloading/download_option_builder_test.exs @@ -256,10 +256,8 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do media_item = Repo.preload(media_item_fixture(source_id: source.id), source: :media_profile) assert {:ok, res} = DownloadOptionBuilder.build(media_item) - assert {:format_sort, "res:#{resolution}"} in res - - assert {:format, "((bestvideo[vcodec~='^avc']/bestvideo)+(bestaudio[acodec~='^mp4a']/bestaudio))/best"} in res + assert {:format_sort, "res:#{resolution},+codec:avc:m4a"} in res assert {:remux_video, "mp4"} in res end) end @@ -270,20 +268,20 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do assert {:ok, res} = DownloadOptionBuilder.build(media_item) assert :extract_audio in res - assert {:format, "bestaudio[acodec~='^mp4a']/bestaudio/best"} in res + assert {:format_sort, "+acodec:m4a"} in res refute {:remux_video, "mp4"} in res end test "includes custom quality options if specified", %{media_item: media_item} do - Settings.set(video_codec_preference: ["av01"]) - Settings.set(audio_codec_preference: ["aac"]) + Settings.set(video_codec_preference: "av01") + Settings.set(audio_codec_preference: "aac") media_item = update_media_profile_attribute(media_item, %{preferred_resolution: :"1080p"}) assert {:ok, res} = DownloadOptionBuilder.build(media_item) - assert {:format, "((bestvideo[vcodec~='^av01']/bestvideo)+(bestaudio[acodec~='^aac']/bestaudio))/best"} in res + assert {:format_sort, "res:1080,+codec:av01:aac"} in res end end diff --git a/test/pinchflat/settings_test.exs b/test/pinchflat/settings_test.exs index 3c4d653..944eaf1 100644 --- a/test/pinchflat/settings_test.exs +++ b/test/pinchflat/settings_test.exs @@ -78,64 +78,4 @@ defmodule Pinchflat.SettingsTest do assert %Ecto.Changeset{} = Settings.change_setting(setting, %{onboarding: true}) end end - - describe "change_setting/2 when testing codec preferences" do - test "converts (video|audio)_codec_preference_string to an array" do - setting = Settings.record() - - new_setting = %{ - video_codec_preference_string: "avc>vp9", - audio_codec_preference_string: "aac>opus" - } - - changeset = Settings.change_setting(setting, new_setting) - - assert ["avc", "vp9"] = changeset.changes.video_codec_preference - assert ["aac", "opus"] = changeset.changes.audio_codec_preference - end - - test "removes whitespace from (video|audio)_codec_preference" do - setting = Settings.record() - - new_setting = %{ - video_codec_preference_string: " avc > > vp9 ", - audio_codec_preference_string: "aac> opus " - } - - changeset = Settings.change_setting(setting, new_setting) - - assert ["avc", "vp9"] = changeset.changes.video_codec_preference - assert ["aac", "opus"] = changeset.changes.audio_codec_preference - end - - test "downcases (video|audio)_codec_preference" do - setting = Settings.record() - - new_setting = %{ - video_codec_preference_string: "AVC>VP9", - audio_codec_preference_string: "AAC>OPUS" - } - - changeset = Settings.change_setting(setting, new_setting) - - assert ["avc", "vp9"] = changeset.changes.video_codec_preference - assert ["aac", "opus"] = changeset.changes.audio_codec_preference - end - - test "an empty value will remove the codec settings" do - Settings.set(video_codec_preference: ["avc", "vp9"]) - Settings.set(audio_codec_preference: ["aac", "opus"]) - setting = Settings.record() - - new_setting = %{ - video_codec_preference_string: "", - audio_codec_preference_string: "" - } - - changeset = Settings.change_setting(setting, new_setting) - - assert [] = changeset.changes.video_codec_preference - assert [] = changeset.changes.audio_codec_preference - end - end end