From 52e260fd57b01bebb698c783db9a887942549536 Mon Sep 17 00:00:00 2001 From: Kieran Date: Thu, 14 Mar 2024 09:10:49 -0700 Subject: [PATCH] Append `-thumb` to thumbnails when downloading (#87) * Appended -thumb to thumbnails when downloading * Added code to compensate for yt-dlp bug --- .../downloading/download_option_builder.ex | 35 ++++++++++++--- lib/pinchflat/metadata/metadata_parser.ex | 19 ++++++-- lib/pinchflat/profiles/media_profile.ex | 6 +++ .../download_option_builder_test.exs | 10 ++++- .../metadata/metadata_parser_test.exs | 9 ++++ test/pinchflat/profiles_test.exs | 44 +++++++++++++++++-- .../media_profile_controller_test.exs | 4 +- 7 files changed, 110 insertions(+), 17 deletions(-) diff --git a/lib/pinchflat/downloading/download_option_builder.ex b/lib/pinchflat/downloading/download_option_builder.ex index dabf407..2c4a77e 100644 --- a/lib/pinchflat/downloading/download_option_builder.ex +++ b/lib/pinchflat/downloading/download_option_builder.ex @@ -18,7 +18,7 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do built_options = default_options() ++ subtitle_options(media_profile) ++ - thumbnail_options(media_profile) ++ + thumbnail_options(media_item_with_preloads) ++ metadata_options(media_profile) ++ quality_options(media_profile) ++ output_options(media_item_with_preloads) @@ -57,13 +57,16 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do end) end - defp thumbnail_options(media_profile) do + defp thumbnail_options(media_item_with_preloads) do + media_profile = media_item_with_preloads.source.media_profile mapped_struct = Map.from_struct(media_profile) Enum.reduce(mapped_struct, [], fn attr, acc -> case attr do {:download_thumbnail, true} -> - acc ++ [:write_thumbnail, convert_thumbnail: "jpg"] + thumbnail_save_location = determine_thumbnail_location(media_item_with_preloads) + + acc ++ [:write_thumbnail, convert_thumbnail: "jpg", output: "thumbnail:#{thumbnail_save_location}"] {:embed_thumbnail, true} -> acc ++ [:embed_thumbnail, convert_thumbnail: "jpg"] @@ -102,15 +105,20 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do end defp output_options(media_item_with_preloads) do - media_profile = media_item_with_preloads.source.media_profile - additional_options_map = output_options_map(media_item_with_preloads) - {:ok, output_path} = OutputPathBuilder.build(media_profile.output_path_template, additional_options_map) + output_path_template = media_item_with_preloads.source.media_profile.output_path_template [ - output: Path.join(base_directory(), output_path) + output: build_output_path(output_path_template, media_item_with_preloads) ] end + defp build_output_path(string, media_item_with_preloads) do + additional_options_map = output_options_map(media_item_with_preloads) + {:ok, output_path} = OutputPathBuilder.build(string, additional_options_map) + + Path.join(base_directory(), output_path) + end + defp output_options_map(media_item_with_preloads) do source = media_item_with_preloads.source @@ -120,6 +128,19 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilder do } end + # I don't love the string manipulation here, but what can ya' do. + # It's dependent on the output_path_template being a string ending `.{{ ext }}` + # (or equivalent), but that's validated by the MediaProfile schema. + defp determine_thumbnail_location(media_item_with_preloads) do + output_path_template = media_item_with_preloads.source.media_profile.output_path_template + + output_path_template + |> String.split(~r{\.}, include_captures: true) + |> List.insert_at(-3, "-thumb") + |> Enum.join() + |> build_output_path(media_item_with_preloads) + end + defp base_directory do Application.get_env(:pinchflat, :media_directory) end diff --git a/lib/pinchflat/metadata/metadata_parser.ex b/lib/pinchflat/metadata/metadata_parser.ex index 7aa2ed0..ee6e8c5 100644 --- a/lib/pinchflat/metadata/metadata_parser.ex +++ b/lib/pinchflat/metadata/metadata_parser.ex @@ -54,9 +54,22 @@ defmodule Pinchflat.Metadata.MetadataParser do |> Enum.reverse() |> Enum.find_value(fn attrs -> attrs["filepath"] end) - %{ - thumbnail_filepath: thumbnail_filepath - } + if thumbnail_filepath do + # NOTE: whole ordeal needed due to a bug I found in yt-dlp + # https://github.com/yt-dlp/yt-dlp/issues/9445 + # Can be reverted to remove this entire conditional once fixed + %{ + thumbnail_filepath: + thumbnail_filepath + |> String.split(~r{\.}, include_captures: true) + |> List.insert_at(-3, "-thumb") + |> Enum.join() + } + else + %{ + thumbnail_filepath: thumbnail_filepath + } + end end defp parse_infojson_metadata(metadata) do diff --git a/lib/pinchflat/profiles/media_profile.ex b/lib/pinchflat/profiles/media_profile.ex index 7ecddb7..ef60b02 100644 --- a/lib/pinchflat/profiles/media_profile.ex +++ b/lib/pinchflat/profiles/media_profile.ex @@ -67,6 +67,12 @@ defmodule Pinchflat.Profiles.MediaProfile do media_profile |> cast(attrs, @allowed_fields) |> validate_required(@required_fields) + # Ensures it ends with `.{{ ext }}` or `.%(ext)s` or similar (with a little wiggle room) + |> validate_format(:output_path_template, ext_regex(), message: "must end with .{{ ext }}") |> unique_constraint(:name) end + + defp ext_regex do + ~r/\.({{ ?ext ?}}|%\( ?ext ?\)[sS])$/ + end end diff --git a/test/pinchflat/downloading/download_option_builder_test.exs b/test/pinchflat/downloading/download_option_builder_test.exs index 22fba78..bd88236 100644 --- a/test/pinchflat/downloading/download_option_builder_test.exs +++ b/test/pinchflat/downloading/download_option_builder_test.exs @@ -125,7 +125,15 @@ defmodule Pinchflat.Downloading.DownloadOptionBuilderTest do assert :write_thumbnail in res end - test "convertes thumbnail to jpg when download_thumbnail is true", %{media_item: media_item} do + test "appends -thumb to the thumbnail name when download_thumbnail is true", %{media_item: media_item} do + media_item = update_media_profile_attribute(media_item, %{download_thumbnail: true}) + + assert {:ok, res} = DownloadOptionBuilder.build(media_item) + + assert {:output, "thumbnail:/tmp/test/media/%(title)S-thumb.%(ext)s"} in res + end + + test "converts thumbnail to jpg when download_thumbnail is true", %{media_item: media_item} do media_item = update_media_profile_attribute(media_item, %{download_thumbnail: true}) assert {:ok, res} = DownloadOptionBuilder.build(media_item) diff --git a/test/pinchflat/metadata/metadata_parser_test.exs b/test/pinchflat/metadata/metadata_parser_test.exs index a3ef8e6..0f2184c 100644 --- a/test/pinchflat/metadata/metadata_parser_test.exs +++ b/test/pinchflat/metadata/metadata_parser_test.exs @@ -94,6 +94,15 @@ defmodule Pinchflat.Metadata.MetadataParserTest do assert String.ends_with?(result.thumbnail_filepath, ".webp") end + # NOTE: this can be removed once this bug is fixed + # https://github.com/yt-dlp/yt-dlp/issues/9445 + # and the associated conditional in the parser is removed + test "automatically appends `-thumb` to the thumbnail filename", %{metadata: metadata} do + result = Parser.parse_for_media_item(metadata) + + assert String.contains?(result.thumbnail_filepath, "-thumb.webp") + end + test "doesn't freak out if the media has no thumbnails", %{metadata: metadata} do metadata = Map.put(metadata, "thumbnails", %{}) diff --git a/test/pinchflat/profiles_test.exs b/test/pinchflat/profiles_test.exs index ea127c6..6bb597e 100644 --- a/test/pinchflat/profiles_test.exs +++ b/test/pinchflat/profiles_test.exs @@ -26,11 +26,11 @@ defmodule Pinchflat.ProfilesTest do describe "create_media_profile/1" do test "creation with valid data creates a media_profile" do - valid_attrs = %{name: "some name", output_path_template: "some output_path_template"} + valid_attrs = %{name: "some name", output_path_template: "output_template.{{ ext }}"} assert {:ok, %MediaProfile{} = media_profile} = Profiles.create_media_profile(valid_attrs) assert media_profile.name == "some name" - assert media_profile.output_path_template == "some output_path_template" + assert media_profile.output_path_template == "output_template.{{ ext }}" end test "creation with invalid data returns error changeset" do @@ -44,14 +44,14 @@ defmodule Pinchflat.ProfilesTest do update_attrs = %{ name: "some updated name", - output_path_template: "some updated output_path_template" + output_path_template: "new_output_template.{{ ext }}" } assert {:ok, %MediaProfile{} = media_profile} = Profiles.update_media_profile(media_profile, update_attrs) assert media_profile.name == "some updated name" - assert media_profile.output_path_template == "some updated output_path_template" + assert media_profile.output_path_template == "new_output_template.{{ ext }}" end test "updating with invalid data returns error changeset" do @@ -132,5 +132,41 @@ defmodule Pinchflat.ProfilesTest do media_profile = media_profile_fixture() assert %Ecto.Changeset{} = Profiles.change_media_profile(media_profile) end + + test "it ensures the media profile's output template ends with an extension" do + valid_templates = [ + "output_template.{{ ext }}", + "output_template.{{ext}}", + "output_template.%(ext)s", + "output_template.%(ext)S", + "output_template.%( ext )s", + "output_template.%( ext )S" + ] + + for template <- valid_templates do + cs = Profiles.change_media_profile(%MediaProfile{}, %{name: "a", output_path_template: template}) + + assert cs.valid? + end + end + + test "it does not allow invalid output templates" do + invalid_templates = [ + "output_template.{{ ext }}.something", + "output_template.{{ ext }}", + "output_template{{ ext }}", + "output_template.%(ext)s.something", + "output_template.txt", + "output_template%(ext)s", + "output_template.%(nope)s", + "output_template" + ] + + for template <- invalid_templates do + cs = Profiles.change_media_profile(%MediaProfile{}, %{name: "a", output_path_template: template}) + + refute cs.valid? + end + end end end diff --git a/test/pinchflat_web/controllers/media_profile_controller_test.exs b/test/pinchflat_web/controllers/media_profile_controller_test.exs index 1a2387b..8a707eb 100644 --- a/test/pinchflat_web/controllers/media_profile_controller_test.exs +++ b/test/pinchflat_web/controllers/media_profile_controller_test.exs @@ -8,10 +8,10 @@ defmodule PinchflatWeb.MediaProfileControllerTest do alias Pinchflat.Repo alias Pinchflat.Settings - @create_attrs %{name: "some name", output_path_template: "some output_path_template"} + @create_attrs %{name: "some name", output_path_template: "output_template.{{ ext }}"} @update_attrs %{ name: "some updated name", - output_path_template: "some updated output_path_template" + output_path_template: "new_output_template.{{ ext }}" } @invalid_attrs %{name: nil, output_path_template: nil}