From af86ca1e0e96148279dbe31abbcaf1136d316611 Mon Sep 17 00:00:00 2001 From: Kieran Date: Tue, 4 Jun 2024 10:39:54 -0700 Subject: [PATCH] Switched thumbnail downloading to use yt-dlp (#281) --- lib/pinchflat/downloading/media_downloader.ex | 2 +- .../metadata/metadata_file_helpers.ex | 43 +++----- lib/pinchflat/yt_dlp/media.ex | 14 +++ .../media_download_worker_test.exs | 1 + .../downloading/media_downloader_test.exs | 7 +- test/pinchflat/media_test.exs | 15 ++- .../metadata/metadata_file_helpers_test.exs | 97 +++++-------------- test/pinchflat/yt_dlp/media_test.exs | 41 +++++++- 8 files changed, 98 insertions(+), 122 deletions(-) diff --git a/lib/pinchflat/downloading/media_downloader.ex b/lib/pinchflat/downloading/media_downloader.ex index 7d4ab06..eb7a09f 100644 --- a/lib/pinchflat/downloading/media_downloader.ex +++ b/lib/pinchflat/downloading/media_downloader.ex @@ -84,7 +84,7 @@ defmodule Pinchflat.Downloading.MediaDownloader do # IDEA: might be worth kicking off a job for this since thumbnail fetching # could fail and I want to handle that in isolation metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_with_preloads, parsed_json), - thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_with_preloads, parsed_json) + thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_with_preloads) } }) diff --git a/lib/pinchflat/metadata/metadata_file_helpers.ex b/lib/pinchflat/metadata/metadata_file_helpers.ex index d54c803..80ceb53 100644 --- a/lib/pinchflat/metadata/metadata_file_helpers.ex +++ b/lib/pinchflat/metadata/metadata_file_helpers.ex @@ -11,6 +11,8 @@ defmodule Pinchflat.Metadata.MetadataFileHelpers do alias Pinchflat.Utils.FilesystemUtils + alias Pinchflat.YtDlp.Media, as: YtDlpMedia + @doc """ Returns the directory where metadata for a database record should be stored. @@ -54,35 +56,19 @@ defmodule Pinchflat.Metadata.MetadataFileHelpers do @doc """ Downloads and stores a thumbnail for a media item, returning the filepath. - Chooses the highest quality thumbnail available (preferring jpg). Returns - nil if no thumbnails are available. + Chooses the highest quality thumbnail available and converts it to a JPG + + Returns nil if no thumbnail is available or if yt-dlp encounters an error Returns binary() | nil """ - def download_and_store_thumbnail_for(database_record, metadata_map) do - thumbnails = - (metadata_map["thumbnails"] || []) - # Give it a low preference if the `preference` key doesn't exist - |> Enum.map(&Map.put_new(&1, "preference", -1000)) - # Give it a low preference if image isn't a jpg - |> Enum.map(fn t -> - preference_weight = if String.ends_with?(t["url"], ".jpg"), do: t["preference"], else: t["preference"] - 1000 + def download_and_store_thumbnail_for(database_record) do + yt_dlp_filepath = generate_filepath_for(database_record, "thumbnail.%(ext)s") + real_filepath = generate_filepath_for(database_record, "thumbnail.jpg") - Map.put(t, "preference", preference_weight) - end) - - case Enum.sort_by(thumbnails, & &1["preference"], :desc) do - [thumbnail_map | _] -> - thumbnail_url = thumbnail_map["url"] - filepath = generate_filepath_for(database_record, Path.basename(thumbnail_url)) - thumbnail_blob = fetch_thumbnail_from_url(thumbnail_url) - - :ok = FilesystemUtils.write_p!(filepath, thumbnail_blob) - - filepath - - _ -> - nil + case YtDlpMedia.download_thumbnail(database_record.original_url, output: yt_dlp_filepath) do + {:ok, _} -> real_filepath + _ -> nil end end @@ -138,13 +124,6 @@ defmodule Pinchflat.Metadata.MetadataFileHelpers do end end - defp fetch_thumbnail_from_url(url) do - http_client = Application.get_env(:pinchflat, :http_client, Pinchflat.HTTP.HTTPClient) - {:ok, body} = http_client.get(url, [], body_format: :binary) - - body - end - defp generate_filepath_for(database_record, filename) do Path.join([ metadata_directory_for(database_record), diff --git a/lib/pinchflat/yt_dlp/media.ex b/lib/pinchflat/yt_dlp/media.ex index 106d6d0..d6879e3 100644 --- a/lib/pinchflat/yt_dlp/media.ex +++ b/lib/pinchflat/yt_dlp/media.ex @@ -46,6 +46,20 @@ defmodule Pinchflat.YtDlp.Media do end end + @doc """ + Downloads a thumbnail for a single piece of media. Usually used for + downloading thumbnails for internal use + + Returns {:ok, ""} | {:error, any, ...}. + """ + def download_thumbnail(url, command_opts \\ []) do + opts = [:no_simulate, :skip_download, :write_thumbnail, convert_thumbnail: "jpg"] ++ command_opts + + # NOTE: it doesn't seem like this command actually returns anything in `after_move` since + # we aren't downloading the main media file + backend_runner().run(url, opts, "after_move:%()j") + end + @doc """ Returns a map representing the media at the given URL. diff --git a/test/pinchflat/downloading/media_download_worker_test.exs b/test/pinchflat/downloading/media_download_worker_test.exs index e82b0b1..754f838 100644 --- a/test/pinchflat/downloading/media_download_worker_test.exs +++ b/test/pinchflat/downloading/media_download_worker_test.exs @@ -9,6 +9,7 @@ defmodule Pinchflat.Downloading.MediaDownloadWorkerTest do alias Pinchflat.Downloading.MediaDownloadWorker setup do + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) stub(UserScriptRunnerMock, :run, fn _event_type, _data -> :ok end) stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) diff --git a/test/pinchflat/downloading/media_downloader_test.exs b/test/pinchflat/downloading/media_downloader_test.exs index 463b7b8..0aa5a8b 100644 --- a/test/pinchflat/downloading/media_downloader_test.exs +++ b/test/pinchflat/downloading/media_downloader_test.exs @@ -14,9 +14,8 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do [:metadata, source: :media_profile] ) - stub(HTTPClientMock, :get, fn _url, _headers, _opts -> - {:ok, ""} - end) + stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) {:ok, %{media_item: media_item}} end @@ -44,7 +43,7 @@ defmodule Pinchflat.Downloading.MediaDownloaderTest do assert {:ok, updated_media_item} = MediaDownloader.download_for_media_item(media_item) assert updated_media_item.metadata.metadata_filepath =~ "media_items/#{media_item.id}/metadata.json.gz" - assert updated_media_item.metadata.thumbnail_filepath =~ "media_items/#{media_item.id}/maxresdefault.jpg" + assert updated_media_item.metadata.thumbnail_filepath =~ "media_items/#{media_item.id}/thumbnail.jpg" end test "non-recoverable errors are passed through", %{media_item: media_item} do diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index b3c9aa0..d23fe6e 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -740,14 +740,13 @@ defmodule Pinchflat.MediaTest do end test "does delete the media item's metadata files" do - stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) media_item = Repo.preload(media_item_with_attachments(), :metadata) update_attrs = %{ metadata: %{ metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, %{}), - thumbnail_filepath: - MetadataFileHelpers.download_and_store_thumbnail_for(media_item, render_parsed_metadata(:media_metadata)) + thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_item) } } @@ -773,14 +772,13 @@ defmodule Pinchflat.MediaTest do end test "deletes the media item's metadata files" do - stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) media_item = Repo.preload(media_item_with_attachments(), :metadata) update_attrs = %{ metadata: %{ metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, %{}), - thumbnail_filepath: - MetadataFileHelpers.download_and_store_thumbnail_for(media_item, render_parsed_metadata(:media_metadata)) + thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_item) } } @@ -860,14 +858,13 @@ defmodule Pinchflat.MediaTest do end test "does not delete the media item's metadata files" do - stub(HTTPClientMock, :get, fn _url, _headers, _opts -> {:ok, ""} end) + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) media_item = Repo.preload(media_item_with_attachments(), :metadata) update_attrs = %{ metadata: %{ metadata_filepath: MetadataFileHelpers.compress_and_store_metadata_for(media_item, %{}), - thumbnail_filepath: - MetadataFileHelpers.download_and_store_thumbnail_for(media_item, render_parsed_metadata(:media_metadata)) + thumbnail_filepath: MetadataFileHelpers.download_and_store_thumbnail_for(media_item) } } diff --git a/test/pinchflat/metadata/metadata_file_helpers_test.exs b/test/pinchflat/metadata/metadata_file_helpers_test.exs index 4052703..3d5b615 100644 --- a/test/pinchflat/metadata/metadata_file_helpers_test.exs +++ b/test/pinchflat/metadata/metadata_file_helpers_test.exs @@ -50,82 +50,37 @@ defmodule Pinchflat.Metadata.MetadataFileHelpersTest do end describe "download_and_store_thumbnail_for/2" do - setup do - # This tests that the HTTP endpoint is being called with every test - expect(HTTPClientMock, :get, fn _url, _headers, _opts -> - {:ok, "thumbnail data"} + test "returns the filepath", %{media_item: media_item} do + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:ok, ""} end) + + filepath = Helpers.download_and_store_thumbnail_for(media_item) + + assert filepath =~ ~r{/media_items/#{media_item.id}/thumbnail.jpg} + end + + test "calls yt-dlp with the expected options", %{media_item: media_item} do + expect(YtDlpRunnerMock, :run, fn url, opts, ot -> + assert url == media_item.original_url + assert ot == "after_move:%()j" + + assert opts == [ + :no_simulate, + :skip_download, + :write_thumbnail, + convert_thumbnail: "jpg", + output: "/tmp/test/metadata/media_items/1/thumbnail.%(ext)s" + ] + + {:ok, ""} end) - metadata = render_parsed_metadata(:media_metadata) - - {:ok, %{metadata: metadata}} + Helpers.download_and_store_thumbnail_for(media_item) end - test "returns the filepath", %{media_item: media_item, metadata: metadata} do - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) + test "returns nil if yt-dlp fails", %{media_item: media_item} do + stub(YtDlpRunnerMock, :run, fn _url, _opts, _ot -> {:error, "error"} end) - assert filepath =~ ~r{/media_items/#{media_item.id}/maxresdefault.jpg} - end - - test "creates folder structure based on passed record", %{media_item: media_item, metadata: metadata} do - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) - - assert File.exists?(Path.dirname(filepath)) - end - - test "chooses the highest preference jpg thumbnail available", %{media_item: media_item} do - metadata = %{ - "thumbnails" => [ - %{"url" => "https://i.ytimg.com/vi/ABC123/img_1.jpg", "preference" => -1}, - %{"url" => "https://i.ytimg.com/vi/ABC123/img_2.jpg", "preference" => 1}, - %{"url" => "https://i.ytimg.com/vi/ABC123/img_3.jpg", "preference" => -10}, - %{"url" => "https://i.ytimg.com/vi/ABC123/img_4.webp", "preference" => 10} - ] - } - - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) - - assert filepath =~ ~r{/media_items/#{media_item.id}/img_2.jpg} - end - - test "will fall back to a non-jpg if it has to", %{media_item: media_item} do - metadata = %{ - "thumbnails" => [ - %{"url" => "https://i.ytimg.com/vi/ABC123/img_1.webp", "preference" => -1} - ] - } - - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) - - assert filepath =~ ~r{/media_items/#{media_item.id}/img_1.webp} - end - - test "does not require a preference field", %{media_item: media_item} do - metadata = %{ - "thumbnails" => [ - %{"url" => "https://i.ytimg.com/vi/ABC123/img_1.webp"} - ] - } - - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) - - assert filepath =~ ~r{/media_items/#{media_item.id}/img_1.webp} - end - end - - describe "download_and_store_thumbnail_for/2 when not downloading thumbnails" do - test "returns nil if there are no thumbnails", %{media_item: media_item} do - metadata = %{"thumbnails" => []} - - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) - - assert filepath == nil - end - - test "returns nil if there is no thumbnail field", %{media_item: media_item} do - metadata = %{} - - filepath = Helpers.download_and_store_thumbnail_for(media_item, metadata) + filepath = Helpers.download_and_store_thumbnail_for(media_item) assert filepath == nil end diff --git a/test/pinchflat/yt_dlp/media_test.exs b/test/pinchflat/yt_dlp/media_test.exs index ee22974..937d2ca 100644 --- a/test/pinchflat/yt_dlp/media_test.exs +++ b/test/pinchflat/yt_dlp/media_test.exs @@ -7,8 +7,8 @@ defmodule Pinchflat.YtDlp.MediaTest do @media_url "https://www.youtube.com/watch?v=TiZPUDkDYbk" - describe "download/2" do - test "it calls the backend runner with the expected arguments" do + describe "download/3" do + test "calls the backend runner with the expected arguments" do expect(YtDlpRunnerMock, :run, fn @media_url, opts, ot, addl -> assert [:no_simulate] = opts assert "after_move:%()j" = ot @@ -20,7 +20,7 @@ defmodule Pinchflat.YtDlp.MediaTest do assert {:ok, _} = Media.download(@media_url) end - test "it passes along additional options" do + test "passes along additional options" do expect(YtDlpRunnerMock, :run, fn _url, opts, _ot, addl -> assert [:no_simulate, :custom_arg] = opts assert [addl_arg: true] = addl @@ -31,7 +31,7 @@ defmodule Pinchflat.YtDlp.MediaTest do assert {:ok, _} = Media.download(@media_url, [:custom_arg], addl_arg: true) end - test "it parses and returns the generated file as JSON" do + test "parses and returns the generated file as JSON" do expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot, _addl -> {:ok, render_metadata(:media_metadata)} end) @@ -40,7 +40,7 @@ defmodule Pinchflat.YtDlp.MediaTest do Media.download(@media_url) end - test "it returns errors" do + test "returns errors" do expect(YtDlpRunnerMock, :run, fn _url, _opt, _ot, _addl -> {:error, "something"} end) @@ -49,6 +49,37 @@ defmodule Pinchflat.YtDlp.MediaTest do end end + describe "download_thumbnail/2" do + test "calls the backend runner with the expected arguments" do + expect(YtDlpRunnerMock, :run, fn @media_url, opts, ot -> + assert opts == [:no_simulate, :skip_download, :write_thumbnail, {:convert_thumbnail, "jpg"}] + assert ot == "after_move:%()j" + + {:ok, ""} + end) + + assert {:ok, _} = Media.download_thumbnail(@media_url) + end + + test "passes along additional options" do + expect(YtDlpRunnerMock, :run, fn _url, opts, _ot -> + assert :custom_arg in opts + + {:ok, "{}"} + end) + + assert {:ok, _} = Media.download_thumbnail(@media_url, [:custom_arg]) + end + + test "returns errors" do + expect(YtDlpRunnerMock, :run, fn _url, _opt, _ot -> + {:error, "something"} + end) + + assert {:error, "something"} = Media.download_thumbnail(@media_url) + end + end + describe "get_media_attributes/1" do test "returns a list of video attributes" do expect(YtDlpRunnerMock, :run, fn _url, _opts, _ot ->