diff --git a/lib/pinchflat/media.ex b/lib/pinchflat/media.ex index 04d1379..a3127f6 100644 --- a/lib/pinchflat/media.ex +++ b/lib/pinchflat/media.ex @@ -188,17 +188,10 @@ defmodule Pinchflat.Media do Returns {:ok, %MediaItem{}} | {:error, %Ecto.Changeset{}} """ - def create_media_item_from_backend_attrs(source, media_attrs) do - attrs = %{ - source_id: source.id, - title: media_attrs["title"], - media_id: media_attrs["id"], - original_url: media_attrs["original_url"], - livestream: media_attrs["was_live"], - description: media_attrs["description"] - } - - create_media_item(attrs) + def create_media_item_from_backend_attrs(source, media_attrs_struct) do + %{source_id: source.id} + |> Map.merge(Map.from_struct(media_attrs_struct)) + |> create_media_item() end @doc """ @@ -264,7 +257,7 @@ defmodule Pinchflat.Media do {{:shorts_behaviour, :only}, %{livestream_behaviour: :only}} -> dynamic( [mi], - ^dynamic and (mi.livestream == true or fragment("LOWER(?) LIKE LOWER(?)", mi.original_url, "%/shorts/%")) + ^dynamic and (mi.livestream == true or mi.short_form_content == true) ) # Technically redundant, but makes the other clauses easier to parse @@ -273,16 +266,13 @@ defmodule Pinchflat.Media do dynamic {{:shorts_behaviour, :only}, _} -> - # return records with /shorts/ in the original_url - dynamic([mi], ^dynamic and fragment("LOWER(?) LIKE LOWER(?)", mi.original_url, "%/shorts/%")) + dynamic([mi], ^dynamic and mi.short_form_content == true) {{:livestream_behaviour, :only}, _} -> - # return records with livestream: true dynamic([mi], ^dynamic and mi.livestream == true) {{:shorts_behaviour, :exclude}, %{livestream_behaviour: lb}} when lb != :only -> - # return records without /shorts/ in the original_url - dynamic([mi], ^dynamic and fragment("LOWER(?) NOT LIKE LOWER(?)", mi.original_url, "%/shorts/%")) + dynamic([mi], ^dynamic and mi.short_form_content == false) {{:livestream_behaviour, :exclude}, %{shorts_behaviour: sb}} when sb != :only -> # return records with livestream: false diff --git a/lib/pinchflat/media/media_item.ex b/lib/pinchflat/media/media_item.ex index 99c9fbc..cd91812 100644 --- a/lib/pinchflat/media/media_item.ex +++ b/lib/pinchflat/media/media_item.ex @@ -19,6 +19,7 @@ defmodule Pinchflat.Media.MediaItem do :original_url, :livestream, :source_id, + :short_form_content, # these fields are captured only on download :media_downloaded_at, :media_filepath, @@ -27,7 +28,7 @@ defmodule Pinchflat.Media.MediaItem do :thumbnail_filepath, :metadata_filepath ] - @required_fields ~w(title original_url livestream media_id source_id)a + @required_fields ~w(title original_url livestream media_id source_id short_form_content)a schema "media_items" do field :title, :string @@ -35,6 +36,7 @@ defmodule Pinchflat.Media.MediaItem do field :description, :string field :original_url, :string field :livestream, :boolean, default: false + field :short_form_content, :boolean, default: false field :media_downloaded_at, :utc_datetime field :media_filepath, :string diff --git a/lib/pinchflat/tasks/source_tasks.ex b/lib/pinchflat/tasks/source_tasks.ex index fa8f973..8fab791 100644 --- a/lib/pinchflat/tasks/source_tasks.ex +++ b/lib/pinchflat/tasks/source_tasks.ex @@ -20,6 +20,8 @@ defmodule Pinchflat.Tasks.SourceTasks do alias Pinchflat.Workers.MediaCollectionIndexingWorker alias Pinchflat.Utils.FilesystemUtils.FileFollowerServer + alias Pinchflat.YtDlp.Backend.Media, as: YtDlpMedia + @doc """ Starts tasks for indexing a source's media regardless of the source's indexing frequency. It's assumed the caller will check for that. @@ -162,7 +164,8 @@ defmodule Pinchflat.Tasks.SourceTasks do {:ok, media_attrs} -> Logger.debug("FileFollowerServer Handler: Got media attributes: #{inspect(media_attrs)}") - create_media_item_and_enqueue_download(source, media_attrs) + media_struct = YtDlpMedia.response_to_struct(media_attrs) + create_media_item_and_enqueue_download(source, media_struct) err -> Logger.debug("FileFollowerServer Handler: Error decoding JSON: #{inspect(err)}") diff --git a/lib/pinchflat/yt_dlp/backend/media.ex b/lib/pinchflat/yt_dlp/backend/media.ex index ae1dbb0..1c96237 100644 --- a/lib/pinchflat/yt_dlp/backend/media.ex +++ b/lib/pinchflat/yt_dlp/backend/media.ex @@ -3,6 +3,27 @@ defmodule Pinchflat.YtDlp.Backend.Media do Contains utilities for working with singular pieces of media """ + @enforce_keys [ + :media_id, + :title, + :description, + :original_url, + :livestream, + :short_form_content + ] + + defstruct [ + :media_id, + :title, + :description, + :original_url, + :livestream, + :short_form_content + ] + + alias __MODULE__ + alias Pinchflat.Utils.FunctionUtils + @doc """ Downloads a single piece of media (and possibly its metadata) directly to its final destination. Returns the parsed JSON output from yt-dlp. @@ -23,11 +44,6 @@ defmodule Pinchflat.YtDlp.Backend.Media do @doc """ Returns a map representing the media at the given URL. - IDEA: should I return a struct here? I want these methods to be agnostic - to implementation, but maybe it can specify its own contract by - returning a struct with well-known fields. Would make refactoring - turbo easy if a field needs to exist but its behavior changes slightly. - Returns {:ok, [map()]} | {:error, any, ...}. """ def get_media_attributes(url) do @@ -36,8 +52,14 @@ defmodule Pinchflat.YtDlp.Backend.Media do output_template = indexing_output_template() case runner.run(url, command_opts, output_template) do - {:ok, output} -> Phoenix.json_library().decode(output) - res -> res + {:ok, output} -> + output + |> Phoenix.json_library().decode!() + |> response_to_struct() + |> FunctionUtils.wrap_ok() + + res -> + res end end @@ -45,7 +67,37 @@ defmodule Pinchflat.YtDlp.Backend.Media do Returns the output template for yt-dlp's indexing command. """ def indexing_output_template do - "%(.{id,title,was_live,original_url,description})j" + "%(.{id,title,was_live,webpage_url,description,aspect_ratio,duration})j" + end + + @doc """ + Transforms a response from yt-dlp into a struct. Interprets the response to + determine if the media is short-form content. + + Returns %Media{}. + """ + def response_to_struct(response) do + %Media{ + media_id: response["id"], + title: response["title"], + description: response["description"], + original_url: response["webpage_url"], + livestream: response["was_live"], + short_form_content: short_form_content?(response) + } + end + + defp short_form_content?(response) do + if String.contains?(response["webpage_url"], "/shorts/") do + true + else + # Sometimes shorts are returned without /shorts/ in the URL, + # so we need to do our best to determine if it's a short. This + # WILL returns false positives, but it's a best-effort approach + # that should work for most cases. The aspect_ratio check is + # based on a gut feeling and may need to be tweaked. + response["duration"] <= 60 && response["aspect_ratio"] < 0.8 + end end defp backend_runner do diff --git a/lib/pinchflat/yt_dlp/backend/media_collection.ex b/lib/pinchflat/yt_dlp/backend/media_collection.ex index 13c0317..ff8e044 100644 --- a/lib/pinchflat/yt_dlp/backend/media_collection.ex +++ b/lib/pinchflat/yt_dlp/backend/media_collection.ex @@ -36,6 +36,7 @@ defmodule Pinchflat.YtDlp.Backend.MediaCollection do output |> String.split("\n", trim: true) |> Enum.map(&Phoenix.json_library().decode!/1) + |> Enum.map(&YtDlpMedia.response_to_struct/1) |> FunctionUtils.wrap_ok() res -> diff --git a/priv/repo/migrations/20240309052602_add_short_form_to_media_items.exs b/priv/repo/migrations/20240309052602_add_short_form_to_media_items.exs new file mode 100644 index 0000000..0b77459 --- /dev/null +++ b/priv/repo/migrations/20240309052602_add_short_form_to_media_items.exs @@ -0,0 +1,9 @@ +defmodule Pinchflat.Repo.Migrations.AddShortFormToMediaItems do + use Ecto.Migration + + def change do + alter table(:media_items) do + add :short_form_content, :boolean, null: false, default: false + end + end +end diff --git a/test/pinchflat/media_test.exs b/test/pinchflat/media_test.exs index a7a992f..870cef7 100644 --- a/test/pinchflat/media_test.exs +++ b/test/pinchflat/media_test.exs @@ -11,6 +11,8 @@ defmodule Pinchflat.MediaTest do alias Pinchflat.Media.MediaItem alias Pinchflat.Metadata.MetadataFileHelpers + alias Pinchflat.YtDlp.Backend.Media, as: YtDlpMedia + setup :verify_on_exit! @invalid_attrs %{title: nil, media_id: nil, media_filepath: nil} @@ -95,7 +97,7 @@ defmodule Pinchflat.MediaTest do test "returns shorts and normal media when shorts_behaviour is :include" do source = source_fixture(%{media_profile_id: media_profile_fixture(%{shorts_behaviour: :include}).id}) normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [normal, short] end @@ -103,7 +105,7 @@ defmodule Pinchflat.MediaTest do test "returns only shorts when shorts_behaviour is :only" do source = source_fixture(%{media_profile_id: media_profile_fixture(%{shorts_behaviour: :only}).id}) _normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [short] end @@ -111,7 +113,7 @@ defmodule Pinchflat.MediaTest do test "returns only normal media when shorts_behaviour is :exclude" do source = source_fixture(%{media_profile_id: media_profile_fixture(%{shorts_behaviour: :exclude}).id}) normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) - _short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + _short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [normal] end @@ -156,7 +158,7 @@ defmodule Pinchflat.MediaTest do normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) livestream = media_item_fixture(%{source_id: source.id, media_filepath: nil, livestream: true}) - short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [normal, livestream, short] end @@ -173,7 +175,7 @@ defmodule Pinchflat.MediaTest do _normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) livestream = media_item_fixture(%{source_id: source.id, media_filepath: nil, livestream: true}) - short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [livestream, short] end @@ -190,7 +192,7 @@ defmodule Pinchflat.MediaTest do normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) _livestream = media_item_fixture(%{source_id: source.id, media_filepath: nil, livestream: true}) - _short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + _short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [normal] end @@ -207,7 +209,7 @@ defmodule Pinchflat.MediaTest do _normal = media_item_fixture(%{source_id: source.id, media_filepath: nil}) _livestream = media_item_fixture(%{source_id: source.id, media_filepath: nil, livestream: true}) - short = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + short = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) assert Media.list_pending_media_items_for(source) == [short] end @@ -247,7 +249,7 @@ defmodule Pinchflat.MediaTest do test "returns false if the media hasn't been downloaded but the profile doesn't DL shorts" do source = source_fixture(%{media_profile_id: media_profile_fixture(%{shorts_behaviour: :exclude}).id}) - media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, original_url: "/shorts/"}) + media_item = media_item_fixture(%{source_id: source.id, media_filepath: nil, short_form_content: true}) refute Media.pending_download?(media_item) end @@ -388,14 +390,18 @@ defmodule Pinchflat.MediaTest do describe "create_media_item_from_backend_attrs/2" do test "creates a media item for a given source and attributes" do source = source_fixture() - media_attrs = Phoenix.json_library().decode!(media_attributes_return_fixture()) + + media_attrs = + media_attributes_return_fixture() + |> Phoenix.json_library().decode!() + |> YtDlpMedia.response_to_struct() assert {:ok, %MediaItem{} = media_item} = Media.create_media_item_from_backend_attrs(source, media_attrs) assert media_item.source_id == source.id - assert media_item.title == media_attrs["title"] - assert media_item.media_id == media_attrs["id"] - assert media_item.original_url == media_attrs["original_url"] - assert media_item.description == media_attrs["description"] + assert media_item.title == media_attrs.title + assert media_item.media_id == media_attrs.media_id + assert media_item.original_url == media_attrs.original_url + assert media_item.description == media_attrs.description end end diff --git a/test/pinchflat/tasks/media_items_tasks_test.exs b/test/pinchflat/tasks/media_items_tasks_test.exs index 8e93f2f..7cf18e8 100644 --- a/test/pinchflat/tasks/media_items_tasks_test.exs +++ b/test/pinchflat/tasks/media_items_tasks_test.exs @@ -84,9 +84,11 @@ defmodule Pinchflat.Tasks.MediaItemTasksTest do Phoenix.json_library().encode!(%{ id: "video2", title: "Video 2", - original_url: "https://example.com/shorts/video2", + webpage_url: "https://example.com/shorts/video2", was_live: true, - description: "desc2" + description: "desc2", + aspect_ratio: 1.67, + duration: 345.67 }) {:ok, output} diff --git a/test/pinchflat/tasks/source_tasks_test.exs b/test/pinchflat/tasks/source_tasks_test.exs index 3d688ee..74bec27 100644 --- a/test/pinchflat/tasks/source_tasks_test.exs +++ b/test/pinchflat/tasks/source_tasks_test.exs @@ -229,9 +229,11 @@ defmodule Pinchflat.Tasks.SourceTasksTest do Phoenix.json_library().encode!(%{ id: "video2", title: "Video 2", - original_url: "https://example.com/shorts/video2", + webpage_url: "https://example.com/shorts/video2", was_live: true, - description: "desc2" + description: "desc2", + aspect_ratio: 1.67, + duration: 345.67 }) File.write(filepath, contents) diff --git a/test/pinchflat/yt_dlp/backend/media_collection_test.exs b/test/pinchflat/yt_dlp/backend/media_collection_test.exs index 01c1ac0..1e5805d 100644 --- a/test/pinchflat/yt_dlp/backend/media_collection_test.exs +++ b/test/pinchflat/yt_dlp/backend/media_collection_test.exs @@ -16,7 +16,7 @@ defmodule Pinchflat.YtDlp.Backend.MediaCollectionTest do {:ok, source_attributes_return_fixture() <> "\n\n"} end) - assert {:ok, [%{"id" => "video1"}, %{"id" => "video2"}, %{"id" => "video3"}]} = + assert {:ok, [%Media{media_id: "video1"}, %Media{media_id: "video2"}, %Media{media_id: "video3"}]} = MediaCollection.get_media_attributes_for_collection(@channel_url) end diff --git a/test/pinchflat/yt_dlp/backend/media_test.exs b/test/pinchflat/yt_dlp/backend/media_test.exs index f9bf74c..94c031d 100644 --- a/test/pinchflat/yt_dlp/backend/media_test.exs +++ b/test/pinchflat/yt_dlp/backend/media_test.exs @@ -55,7 +55,7 @@ defmodule Pinchflat.YtDlp.Backend.MediaTest do {:ok, media_attributes_return_fixture()} end) - assert {:ok, %{"description" => _, "id" => _, "original_url" => _, "title" => _, "was_live" => _}} = + assert {:ok, %{description: _, media_id: _, original_url: _, title: _, livestream: _}} = Media.get_media_attributes(@media_url) end @@ -64,7 +64,7 @@ defmodule Pinchflat.YtDlp.Backend.MediaTest do assert opts == [:simulate, :skip_download] assert ot == Media.indexing_output_template() - {:ok, "{}"} + {:ok, media_attributes_return_fixture()} end) assert {:ok, _} = Media.get_media_attributes(@media_url) @@ -79,8 +79,61 @@ defmodule Pinchflat.YtDlp.Backend.MediaTest do describe "indexing_output_template/0" do test "contains all the greatest hits" do - assert "%(.{id,title,was_live,original_url,description})j" == + assert "%(.{id,title,was_live,webpage_url,description,aspect_ratio,duration})j" == Media.indexing_output_template() end end + + describe "response_to_struct/1" do + test "transforms a response into a struct" do + response = %{ + "id" => "TiZPUDkDYbk", + "title" => "Trying to Wheelie Without the Rear Brake", + "description" => "I'm not sure what I expected.", + "webpage_url" => "https://www.youtube.com/watch?v=TiZPUDkDYbk", + "was_live" => false, + "aspect_ratio" => 1.0, + "duration" => 60 + } + + assert %Media{ + media_id: "TiZPUDkDYbk", + title: "Trying to Wheelie Without the Rear Brake", + description: "I'm not sure what I expected.", + original_url: "https://www.youtube.com/watch?v=TiZPUDkDYbk", + livestream: false, + short_form_content: false + } = Media.response_to_struct(response) + end + + test "sets short_form_content to true if the URL contains /shorts/" do + response = %{ + "webpage_url" => "https://www.youtube.com/shorts/TiZPUDkDYbk", + "aspect_ratio" => 1.0, + "duration" => 61 + } + + assert %Media{short_form_content: true} = Media.response_to_struct(response) + end + + test "sets short_form_content to true if the aspect ratio are duration are right" do + response = %{ + "webpage_url" => "https://www.youtube.com/watch?v=TiZPUDkDYbk", + "aspect_ratio" => 0.5, + "duration" => 59 + } + + assert %Media{short_form_content: true} = Media.response_to_struct(response) + end + + test "sets short_form_content to false otherwise" do + response = %{ + "webpage_url" => "https://www.youtube.com/watch?v=TiZPUDkDYbk", + "aspect_ratio" => 1.0, + "duration" => 61 + } + + assert %Media{short_form_content: false} = Media.response_to_struct(response) + end + end end diff --git a/test/support/fixtures/media_fixtures.ex b/test/support/fixtures/media_fixtures.ex index 245e3af..e4f3ffe 100644 --- a/test/support/fixtures/media_fixtures.ex +++ b/test/support/fixtures/media_fixtures.ex @@ -19,6 +19,7 @@ defmodule Pinchflat.MediaFixtures do title: Faker.Commerce.product_name(), original_url: "https://www.youtube.com/watch?v=#{media_id}", livestream: false, + short_form_content: false, media_filepath: "/video/#{Faker.File.file_name(:video)}", source_id: SourcesFixtures.source_fixture().id }) @@ -70,9 +71,11 @@ defmodule Pinchflat.MediaFixtures do media_attributes = %{ id: "video1", title: "Video 1", - original_url: "https://example.com/video1", + webpage_url: "https://example.com/video1", was_live: false, - description: "desc1" + description: "desc1", + aspect_ratio: 1.67, + duration: 123.45 } Phoenix.json_library().encode!(media_attributes) diff --git a/test/support/fixtures/sources_fixtures.ex b/test/support/fixtures/sources_fixtures.ex index 5c25ffc..89c113c 100644 --- a/test/support/fixtures/sources_fixtures.ex +++ b/test/support/fixtures/sources_fixtures.ex @@ -35,23 +35,29 @@ defmodule Pinchflat.SourcesFixtures do %{ id: "video1", title: "Video 1", - original_url: "https://example.com/video1", + webpage_url: "https://example.com/video1", was_live: false, - description: "desc1" + description: "desc1", + aspect_ratio: 1.67, + duration: 12.34 }, %{ id: "video2", title: "Video 2", - original_url: "https://example.com/video2", + webpage_url: "https://example.com/video2", was_live: true, - description: "desc2" + description: "desc2", + aspect_ratio: 1.67, + duration: 345.67 }, %{ id: "video3", title: "Video 3", - original_url: "https://example.com/video3", + webpage_url: "https://example.com/video3", was_live: false, - description: "desc3" + description: "desc3", + aspect_ratio: 1.0, + duration: 678.90 } ]