diff --git a/lib/pinchflat/metadata/source_image_parser.ex b/lib/pinchflat/metadata/source_image_parser.ex index 9a0370c..26bacd7 100644 --- a/lib/pinchflat/metadata/source_image_parser.ex +++ b/lib/pinchflat/metadata/source_image_parser.ex @@ -20,29 +20,49 @@ defmodule Pinchflat.Metadata.SourceImageParser do def store_source_images(base_directory, source_metadata) do (source_metadata["thumbnails"] || []) |> Enum.filter(&(&1["filepath"] != nil)) - |> select_useful_images() + |> select_useful_images(source_metadata) |> Enum.map(&move_image(&1, base_directory)) |> Enum.into(%{}) end - defp select_useful_images(images) do + defp select_useful_images(images, source_metadata) do labelled_images = - Enum.reduce(images, [], fn image_map, acc -> + Enum.reduce(images, %{}, fn image_map, acc -> case image_map do - %{"id" => "avatar_uncropped"} -> - acc ++ [{:poster, :poster_filepath, image_map["filepath"]}] - - %{"id" => "banner_uncropped"} -> - acc ++ [{:fanart, :fanart_filepath, image_map["filepath"]}] - - _ -> - acc + %{"id" => "avatar_uncropped"} -> put_image_key(acc, :poster, image_map["filepath"]) + %{"id" => "banner_uncropped"} -> put_image_key(acc, :fanart, image_map["filepath"]) + _ -> acc end end) labelled_images - |> Enum.concat([{:banner, :banner_filepath, determine_best_banner(images)}]) - |> Enum.filter(fn {_, _, tmp_filepath} -> tmp_filepath end) + |> add_fallback_poster(source_metadata) + |> put_image_key(:banner, determine_best_banner(images)) + |> Enum.filter(fn {_key, attrs} -> attrs.current_filepath end) + end + + # If a poster is set, short-circuit and return the images as-is + defp add_fallback_poster(%{poster: _} = images, _), do: images + + # If a poster is NOT set, see if we can find a suitable image to use as a fallback + defp add_fallback_poster(images, source_metadata) do + case source_metadata["entries"] do + nil -> images + [] -> images + [first_entry | _] -> add_poster_from_entry_thumbnail(images, first_entry) + end + end + + defp add_poster_from_entry_thumbnail(images, entry) do + thumbnail = + (entry["thumbnails"] || []) + |> Enum.reverse() + |> Enum.find(& &1["filepath"]) + + case thumbnail do + nil -> images + _ -> put_image_key(images, :poster, thumbnail["filepath"]) + end end defp determine_best_banner(images) do @@ -58,12 +78,22 @@ defmodule Pinchflat.Metadata.SourceImageParser do Map.get(best_candidate || %{}, "filepath") end - defp move_image({filename, source_attr_name, tmp_filepath}, base_directory) do - extension = Path.extname(tmp_filepath) - final_filepath = Path.join([base_directory, "#{filename}#{extension}"]) + defp move_image({_key, attrs}, base_directory) do + extension = Path.extname(attrs.current_filepath) + final_filepath = Path.join([base_directory, "#{attrs.final_filename}#{extension}"]) - FilesystemUtils.cp_p!(tmp_filepath, final_filepath) + FilesystemUtils.cp_p!(attrs.current_filepath, final_filepath) - {source_attr_name, final_filepath} + {attrs.attribute_name, final_filepath} + end + + defp put_image_key(map, key, image) do + attribute_atom = String.to_existing_atom("#{key}_filepath") + + Map.put(map, key, %{ + attribute_name: attribute_atom, + final_filename: to_string(key), + current_filepath: image + }) end end diff --git a/lib/pinchflat/metadata/source_metadata_storage_worker.ex b/lib/pinchflat/metadata/source_metadata_storage_worker.ex index 52ec810..8ccfdf9 100644 --- a/lib/pinchflat/metadata/source_metadata_storage_worker.ex +++ b/lib/pinchflat/metadata/source_metadata_storage_worker.ex @@ -77,10 +77,8 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorker do defp fetch_source_metadata_and_images(series_directory, source) do metadata_directory = MetadataFileHelpers.metadata_directory_for(source) - tmp_output_path = "#{tmp_directory()}/#{StringUtils.random_string(16)}/source_image.%(ext)S" - opts = [:write_all_thumbnails, convert_thumbnails: "jpg", output: tmp_output_path] - {:ok, metadata} = MediaCollection.get_source_metadata(source.original_url, opts) + {:ok, metadata} = fetch_metadata_for_source(source) metadata_image_attrs = SourceImageParser.store_source_images(metadata_directory, metadata) if source.media_profile.download_source_images && series_directory do @@ -110,6 +108,20 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorker do end end + defp fetch_metadata_for_source(source) do + tmp_output_path = "#{tmp_directory()}/#{StringUtils.random_string(16)}/source_image.%(ext)S" + base_opts = [convert_thumbnails: "jpg", output: tmp_output_path] + + opts = + if source.collection_type == :channel do + base_opts ++ [:write_all_thumbnails, playlist_items: 0] + else + base_opts ++ [:write_thumbnail, playlist_items: 1] + end + + MediaCollection.get_source_metadata(source.original_url, opts) + end + defp tmp_directory do Application.get_env(:pinchflat, :tmpfile_directory) end diff --git a/lib/pinchflat/yt_dlp/media_collection.ex b/lib/pinchflat/yt_dlp/media_collection.ex index 7e397ab..f657195 100644 --- a/lib/pinchflat/yt_dlp/media_collection.ex +++ b/lib/pinchflat/yt_dlp/media_collection.ex @@ -99,10 +99,21 @@ defmodule Pinchflat.YtDlp.MediaCollection do as a compressed blob for possible future use. That's why it's not getting formatted like `get_source_details/1` + ! IMPORTANT ! - you'll always want to set `playlist_items: int` in `addl_opts. + This is great if you want to also return details about the videos in the playlists, + but it should be set in all cases to not over-fetch data. + For channels you should usually set this to 0 since channels return all the + metadata we need without needing to fetch the videos. On the other hand, playlists + don't return very useful images so you can set this to 1 to get the first video's + images, for instance. + Returns {:ok, map()} | {:error, any, ...}. """ - def get_source_metadata(source_url, addl_opts \\ []) do - opts = [playlist_items: 0] ++ addl_opts + def get_source_metadata(source_url, addl_opts \\ [playlist_items: 0]) do + # This only validates that the `playlist_items` key is present. It's otherwise unused + _playlist_items = Keyword.fetch!(addl_opts, :playlist_items) + + opts = [:skip_download] ++ addl_opts output_template = "playlist:%()j" with {:ok, output} <- backend_runner().run(source_url, opts, output_template), diff --git a/test/pinchflat/metadata/source_image_parser_test.exs b/test/pinchflat/metadata/source_image_parser_test.exs index 8ec3c6c..3f815ae 100644 --- a/test/pinchflat/metadata/source_image_parser_test.exs +++ b/test/pinchflat/metadata/source_image_parser_test.exs @@ -75,4 +75,51 @@ defmodule Pinchflat.Metadata.SourceImageParserTest do assert SourceImageParser.store_source_images(@base_dir, metadata) == %{} end end + + describe "store_source_images/2 when testing fallbacks" do + test "uses the entries list for a fallback poster if needed" do + metadata = %{ + "thumbnails" => [], + "entries" => [ + %{ + "thumbnails" => [%{"filepath" => "/app/test/support/files/channel_photos/a.0.jpg"}] + } + ] + } + + expected = %{ + poster_filepath: "#{@base_dir}/poster.jpg" + } + + assert SourceImageParser.store_source_images(@base_dir, metadata) == expected + end + + test "doesn't blow up if the entries list doesn't have any suitable thumbnails" do + metadata = %{ + "thumbnails" => [], + "entries" => [ + %{"thumbnails" => [%{"id" => "1"}]} + ] + } + + assert SourceImageParser.store_source_images(@base_dir, metadata) == %{} + end + + test "doesn't use the entries list if it's empty" do + metadata = %{ + "thumbnails" => [], + "entries" => [] + } + + assert SourceImageParser.store_source_images(@base_dir, metadata) == %{} + end + + test "doesn't use the entries list if it's not present" do + metadata = %{ + "thumbnails" => [] + } + + assert SourceImageParser.store_source_images(@base_dir, metadata) == %{} + end + end end diff --git a/test/pinchflat/metadata/source_metadata_storage_worker_test.exs b/test/pinchflat/metadata/source_metadata_storage_worker_test.exs index 503d0f3..d8b0324 100644 --- a/test/pinchflat/metadata/source_metadata_storage_worker_test.exs +++ b/test/pinchflat/metadata/source_metadata_storage_worker_test.exs @@ -143,7 +143,9 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorkerTest do {:ok, source_details_return_fixture(%{filename: filename})} - _url, _opts, ot when ot == @metadata_ot -> + _url, opts, ot when ot == @metadata_ot -> + assert {:convert_thumbnails, "jpg"} in opts + {:ok, render_metadata(:channel_source_metadata)} end) @@ -164,6 +166,42 @@ defmodule Pinchflat.Metadata.SourceMetadataStorageWorkerTest do Sources.delete_source(source, delete_files: true) end + test "calls one set of yt-dlp metadata opts for channels" do + stub(YtDlpRunnerMock, :run, fn + _url, _opts, ot when ot == @source_details_ot -> + {:ok, source_details_return_fixture()} + + _url, opts, ot when ot == @metadata_ot -> + assert {:playlist_items, 0} in opts + assert :write_all_thumbnails in opts + + {:ok, render_metadata(:channel_source_metadata)} + end) + + profile = media_profile_fixture(%{download_source_images: true}) + source = source_fixture(media_profile_id: profile.id, collection_type: :channel) + + perform_job(SourceMetadataStorageWorker, %{id: source.id}) + end + + test "calls another set of yt-dlp metadata opts for playlists" do + stub(YtDlpRunnerMock, :run, fn + _url, _opts, ot when ot == @source_details_ot -> + {:ok, source_details_return_fixture()} + + _url, opts, ot when ot == @metadata_ot -> + assert {:playlist_items, 1} in opts + assert :write_thumbnail in opts + + {:ok, render_metadata(:channel_source_metadata)} + end) + + profile = media_profile_fixture(%{download_source_images: true}) + source = source_fixture(media_profile_id: profile.id, collection_type: :playlist) + + perform_job(SourceMetadataStorageWorker, %{id: source.id}) + end + test "does not store source images if the profile is not set to" do stub(YtDlpRunnerMock, :run, fn _url, _opts, ot when ot == @source_details_ot -> diff --git a/test/pinchflat/yt_dlp/media_collection_test.exs b/test/pinchflat/yt_dlp/media_collection_test.exs index fa69e58..f08c649 100644 --- a/test/pinchflat/yt_dlp/media_collection_test.exs +++ b/test/pinchflat/yt_dlp/media_collection_test.exs @@ -129,7 +129,7 @@ defmodule Pinchflat.YtDlp.MediaCollectionTest do test "it passes the expected args to the backend runner" do expect(YtDlpRunnerMock, :run, fn @channel_url, opts, ot -> - assert opts == [playlist_items: 0] + assert opts == [:skip_download, playlist_items: 0] assert ot == "playlist:%()j" {:ok, "{}"} @@ -152,12 +152,18 @@ defmodule Pinchflat.YtDlp.MediaCollectionTest do test "allows you to pass additional opts" do expect(YtDlpRunnerMock, :run, fn _url, opts, _ot -> - assert opts == [playlist_items: 0, real_opt: :yup] + assert opts == [:skip_download, playlist_items: 1, real_opt: :yup] {:ok, "{}"} end) - assert {:ok, _} = MediaCollection.get_source_metadata(@channel_url, real_opt: :yup) + assert {:ok, _} = MediaCollection.get_source_metadata(@channel_url, playlist_items: 1, real_opt: :yup) + end + + test "blows up if you pass addl opts but don't pass playlist items" do + assert_raise KeyError, fn -> + MediaCollection.get_source_metadata(@channel_url, real_opt: :yup) + end end end end