[REFACTOR] Move chapter timestamps support into chapter plugin, remove from youtube.video (#193)

Introduces breaking changes to youtube.video.chapter_timestamps. Please use chapters.embed_chapter_timestamps instead to embed timestamps into a video

.github/workflows/ci.yaml

@@ -21,6 +21,7 @@ jobs:
         python-version: ${{ matrix.python-version }}
     - name: Install dependencies
       run: |
+        sudo apt-get update
         sudo apt-get install -y ffmpeg
         python -m pip install --upgrade pip
         pip install -e .[lint,test]

src/ytdl_sub/downloaders/youtube/video.py

@@ -1,16 +1,10 @@
 from typing import Dict
 from typing import List
-from typing import Optional
-from typing import Tuple
 
 from ytdl_sub.downloaders.youtube.abc import YoutubeDownloader
 from ytdl_sub.downloaders.youtube.abc import YoutubeDownloaderOptions
 from ytdl_sub.entries.youtube import YoutubeVideo
-from ytdl_sub.utils.chapters import Chapters
-from ytdl_sub.utils.ffmpeg import set_ffmpeg_metadata_chapters
-from ytdl_sub.utils.file_handler import FileMetadata
 from ytdl_sub.validators.url_validator import YoutubeVideoUrlValidator
-from ytdl_sub.validators.validators import StringValidator
 
 
 class YoutubeVideoDownloaderOptions(YoutubeDownloaderOptions):
@@ -28,8 +22,6 @@ class YoutubeVideoDownloaderOptions(YoutubeDownloaderOptions):
             # required
             download_strategy: "video"
             video_url: "youtube.com/watch?v=VMAPTo7RVDo"
-            # optional
-            chapter_timestamps: path/to/timestamps.txt
 
     CLI usage:
 
@@ -39,14 +31,10 @@ class YoutubeVideoDownloaderOptions(YoutubeDownloaderOptions):
     """
 
     _required_keys = {"video_url"}
-    _optional_keys = {"chapter_timestamps"}
 
     def __init__(self, name, value):
         super().__init__(name, value)
         self._video_url = self._validate_key("video_url", YoutubeVideoUrlValidator).video_url
-        self._chapter_timestamps = self._validate_key_if_present(
-            "chapter_timestamps", StringValidator
-        )
 
     @property
     def video_url(self) -> str:
@@ -55,24 +43,6 @@ class YoutubeVideoDownloaderOptions(YoutubeDownloaderOptions):
         """
         return self._video_url
 
-    @property
-    def chapter_timestamps(self) -> Optional[str]:
-        """
-        Optional. The path to the file containing the timestamps to embed into the video as
-        chapters. Should be formatted as:
-
-        .. code-block:: markdown
-
-           0:00 Intro
-           0:24 Blackwater Park
-           10:23 Bleak
-           16:39 Jokes
-           1:02:23 Ending
-        """
-        if self._chapter_timestamps:
-            return self._chapter_timestamps.value
-        return None
-
 
 class YoutubeVideoDownloader(YoutubeDownloader[YoutubeVideoDownloaderOptions, YoutubeVideo]):
     downloader_options_type = YoutubeVideoDownloaderOptions
@@ -93,26 +63,8 @@ class YoutubeVideoDownloader(YoutubeDownloader[YoutubeVideoDownloaderOptions, Yo
             **{"break_on_existing": True},
         )
 
-    def download(self) -> List[YoutubeVideo] | List[Tuple[YoutubeVideo, FileMetadata]]:
+    def download(self) -> List[YoutubeVideo]:
         """Download a single Youtube video"""
         entry_dict = self.extract_info(url=self.download_options.video_url)
         video = YoutubeVideo(entry_dict=entry_dict, working_directory=self.working_directory)
-
+        return [video]
-        # If no chapters, just return the video
-        if not self.download_options.chapter_timestamps:
-            return [video]
-
-        # Otherwise, add the chapters and return the video + chapter metadata
-        chapters = Chapters.from_timestamps_file(
-            chapters_file_path=self.download_options.chapter_timestamps
-        )
-        if not self.is_dry_run:
-            set_ffmpeg_metadata_chapters(
-                file_path=video.get_download_file_path(),
-                chapters=chapters,
-                file_duration_sec=video.kwargs("duration"),
-            )
-
-        file_metadata = chapters.to_file_metadata(title="Chapters embedded into the video:")
-
-        return [(video, file_metadata)]

src/ytdl_sub/plugins/chapters.py

@@ -9,11 +9,14 @@ from ytdl_sub.downloaders.ytdl_options_builder import YTDLOptionsBuilder
 from ytdl_sub.entries.entry import Entry
 from ytdl_sub.plugins.plugin import Plugin
 from ytdl_sub.plugins.plugin import PluginOptions
+from ytdl_sub.utils.chapters import Chapters
+from ytdl_sub.utils.ffmpeg import set_ffmpeg_metadata_chapters
 from ytdl_sub.utils.file_handler import FileMetadata
 from ytdl_sub.validators.regex_validator import RegexListValidator
 from ytdl_sub.validators.string_select_validator import StringSelectValidator
 from ytdl_sub.validators.validators import BoolValidator
 from ytdl_sub.validators.validators import ListValidator
+from ytdl_sub.validators.validators import StringValidator
 
 SPONSORBLOCK_HIGHLIGHT_CATEGORIES: Set[str] = {"poi_highlight"}
 SPONSORBLOCK_CATEGORIES: Set[str] = SPONSORBLOCK_HIGHLIGHT_CATEGORIES | {
@@ -87,6 +90,7 @@ class ChaptersOptions(PluginOptions):
 
     _optional_keys = {
         "embed_chapters",
+        "embed_chapter_timestamps",
         "sponsorblock_categories",
         "remove_sponsorblock_categories",
         "remove_chapters_regex",
@@ -110,12 +114,20 @@ class ChaptersOptions(PluginOptions):
         self._force_key_frames = self._validate_key_if_present(
             key="force_key_frames", validator=BoolValidator, default=False
         ).value
+        self._embed_chapter_timestamps = self._validate_key_if_present(
+            "embed_chapter_timestamps", StringValidator
+        )
 
         if self._remove_sponsorblock_categories and not self._sponsorblock_categories:
             raise self._validation_exception(
                 "Must specify sponsorblock_categories if you are going to remove any of them"
             )
 
+        if self._embed_chapters and self._embed_chapter_timestamps:
+            raise self._validation_exception(
+                "Cannot embed chapters from the source and from a timestamp file"
+            )
+
     @property
     def embed_chapters(self) -> Optional[bool]:
         """
@@ -172,6 +184,27 @@ class ChaptersOptions(PluginOptions):
         """
         return self._force_key_frames
 
+    @property
+    def embed_chapter_timestamps(self) -> Optional[str]:
+        """
+        Optional. The path to the file containing the timestamps to embed into the file as
+        chapters. Should be formatted as:
+
+        .. code-block:: markdown
+
+           0:00 Intro
+           0:24 Blackwater Park
+           10:23 Bleak
+           16:39 Jokes
+           1:02:23 Ending
+
+        This should only be used with single entity download strategies. Otherwise, an entire
+        playlist or channel would all the same embedded chapters.
+        """
+        if self._embed_chapter_timestamps:
+            return self._embed_chapter_timestamps.value
+        return None
+
 
 class ChaptersPlugin(Plugin[ChaptersOptions]):
     plugin_options_type = ChaptersOptions
@@ -263,6 +296,29 @@ class ChaptersPlugin(Plugin[ChaptersOptions]):
             )
         )
 
+    def modify_entry(self, entry: Entry) -> Entry:
+        """
+        Parameters
+        ----------
+        entry
+            Entry to add custom chapters using timestamps if present
+
+        Returns
+        -------
+        entry
+        """
+        if self.plugin_options.embed_chapter_timestamps and not self.is_dry_run:
+            chapters = Chapters.from_timestamps_file(
+                chapters_file_path=self.plugin_options.embed_chapter_timestamps
+            )
+            set_ffmpeg_metadata_chapters(
+                file_path=entry.get_download_file_path(),
+                chapters=chapters,
+                file_duration_sec=entry.kwargs("duration"),
+            )
+
+        return entry
+
     def post_process_entry(self, entry: Entry) -> Optional[FileMetadata]:
         """
         Parameters
@@ -274,20 +330,29 @@ class ChaptersPlugin(Plugin[ChaptersOptions]):
         -------
         FileMetadata outlining which chapters/SponsorBlock segments got removed
         """
-        metadata_dict = {}
+        if self.plugin_options.embed_chapter_timestamps:
-        removed_chapters = self._get_removed_chapters(entry)
+            chapters = Chapters.from_timestamps_file(
-        removed_sponsorblock = self._get_removed_sponsorblock_category_counts(entry)
+                chapters_file_path=self.plugin_options.embed_chapter_timestamps
+            )
+            return chapters.to_file_metadata(title="Chapters embedded from timestamp file:")
 
-        # If no chapters are on the entry, do not report any embedded chapters
+        if self.plugin_options.embed_chapters:
-        if not _contains_any_chapters(entry):
+            metadata_dict = {}
-            return None
+            removed_chapters = self._get_removed_chapters(entry)
+            removed_sponsorblock = self._get_removed_sponsorblock_category_counts(entry)
 
-        if removed_chapters:
+            # If no chapters are on the entry, do not report any embedded chapters
-            metadata_dict["Removed Chapter(s)"] = ", ".join(removed_chapters)
+            if not _contains_any_chapters(entry):
-        if removed_sponsorblock:
+                return None
-            metadata_dict["Removed SponsorBlock Category Count(s)"] = removed_sponsorblock
 
-        # TODO: check if file actually has embedded chapters
+            if removed_chapters:
-        return FileMetadata.from_dict(
+                metadata_dict["Removed Chapter(s)"] = ", ".join(removed_chapters)
-            value_dict=metadata_dict, title="Embedded Chapters", sort_dict=False
+            if removed_sponsorblock:
-        )
+                metadata_dict["Removed SponsorBlock Category Count(s)"] = removed_sponsorblock
+
+            # TODO: check if file actually has embedded chapters
+            return FileMetadata.from_dict(
+                value_dict=metadata_dict, title="Embedded Chapters", sort_dict=False
+            )
+
+        return None

src/ytdl_sub/plugins/plugin.py

@@ -125,7 +125,7 @@ class Plugin(DownloadArchiver, Generic[PluginOptionsT], ABC):
         """
         return []
 
-    def modify_entry(self, entry: Entry) -> Optional[Entry]:
+    def modify_entry(self, entry: Entry) -> Optional[Entry | Tuple[Entry, FileMetadata]]:
         """
         For each entry downloaded, modify the entry in some way before sending it to
         post-processing.

tests/e2e/plugins/test_chapters.py

@@ -71,3 +71,42 @@ class TestChapters:
                 "JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.mp4"
             ],
         )
+
+    @pytest.mark.parametrize("dry_run", [True, False])
+    def test_chapters_from_timestamp_file_with_subs(
+        self,
+        music_video_config,
+        single_video_sponsorblock_and_embedded_subs_preset_dict,
+        timestamps_file_path,
+        output_directory,
+        dry_run,
+    ):
+        # Test chapters and video tags, throw in a video tag with special chars while we are at it
+        single_video_sponsorblock_and_embedded_subs_preset_dict["chapters"] = {
+            "embed_chapters": False,
+            "embed_chapter_timestamps": timestamps_file_path,
+        }
+        single_video_sponsorblock_and_embedded_subs_preset_dict["video_tags"] = {
+            "tags": {"description": "🎸 / ' \" \n newline?"}
+        }
+
+        subscription = Subscription.from_dict(
+            config=music_video_config,
+            preset_name="chapters_from_timestamps_with_subs",
+            preset_dict=single_video_sponsorblock_and_embedded_subs_preset_dict,
+        )
+
+        transaction_log = subscription.download(dry_run=dry_run)
+        assert_transaction_log_matches(
+            output_directory=output_directory,
+            transaction_log=transaction_log,
+            transaction_log_summary_file_name="plugins/test_chapters_from_ts_with_subs.txt",
+        )
+        assert_expected_downloads(
+            output_directory=output_directory,
+            dry_run=dry_run,
+            expected_download_summary_file_name="plugins/test_chapters_from_ts_with_subs.json",
+            ignore_md5_hashes_for=[
+                "JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.mp4"
+            ],
+        )

tests/e2e/plugins/test_subtitles.py

@@ -86,39 +86,3 @@ class TestSubtitles:
             dry_run=dry_run,
             expected_download_summary_file_name="plugins/test_subtitles_embedded_and_file.json",
         )
-
-    @pytest.mark.parametrize("dry_run", [True, False])
-    def test_subtitles_chapters_tags_embedded(
-        self,
-        music_video_config,
-        timestamps_file_path,
-        single_video_subs_embed_preset_dict,
-        output_directory,
-        dry_run,
-    ):
-        # Test chapters and video tags in addition to subtitles
-        mergedeep.merge(
-            single_video_subs_embed_preset_dict,
-            {
-                "youtube": {"chapter_timestamps": timestamps_file_path},
-                "video_tags": {"tags": {"title": "{title}"}},
-            },
-        )
-
-        subscription = Subscription.from_dict(
-            config=music_video_config,
-            preset_name="subtitles_embedded_test",
-            preset_dict=single_video_subs_embed_preset_dict,
-        )
-
-        transaction_log = subscription.download(dry_run=dry_run)
-        assert_transaction_log_matches(
-            output_directory=output_directory,
-            transaction_log=transaction_log,
-            transaction_log_summary_file_name="plugins/test_subtitles_tags_chapters.txt",
-        )
-        assert_expected_downloads(
-            output_directory=output_directory,
-            dry_run=dry_run,
-            expected_download_summary_file_name="plugins/test_subtitles_tags_chapters.json",
-        )

tests/e2e/resources/expected_downloads_summaries/plugins/test_chapters_from_ts_with_subs.json

@@ -0,0 +1,5 @@
+{
+  "JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case-thumb.jpg": "a81457393418b5abed785a82122f3352",
+  "JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.mp4": "2d40822bf4c0527f9080f00357b26ce0",
+  "JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.nfo": "0c06fe6874588209fccbd9276a446750"
+}

tests/e2e/resources/expected_downloads_summaries/plugins/test_subtitles_tags_chapters.json

@@ -1,5 +0,0 @@
-{
-  "JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind-thumb.jpg": "704246dd78074e8a0ec001dd8d03fd60",
-  "JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind.mp4": "e2e60d3e3ff7739d071aa953642980af",
-  "JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind.nfo": "ffa10f1cbc098ace7b1c7a8fbe3097a8"
-}

tests/e2e/resources/expected_downloads_summaries/youtube/test_video_with_chapter_timestamps.json

@@ -1,5 +0,0 @@
-{
-  "JMC - Oblivion Mod ＂Falcor＂ p.1-thumb.jpg": "fb95b510681676e81c321171fc23143e",
-  "JMC - Oblivion Mod ＂Falcor＂ p.1.mp4": "567631875d95fee5899e2ff407b74fd8",
-  "JMC - Oblivion Mod ＂Falcor＂ p.1.nfo": "89f509a8a3d9003e22a9091abeeae5dc"
-}

tests/e2e/resources/transaction_log_summaries/plugins/test_chapters_from_ts_with_subs.txt

@@ -0,0 +1,23 @@
+Files created in '{output_directory}'
+----------------------------------------
+JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case-thumb.jpg
+JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.mp4
+  Chapters embedded from timestamp file:
+    0:00: Intro
+    0:10: Part 1
+    0:20: Part 2
+    0:30: Part 3
+    0:40: Part 4
+    1:01: Part 5
+  Embedded subtitles with lang(s) en, de
+  Video Tags:
+    description:
+      🎸 / ' "
+      newline?
+JMC - This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case.nfo
+  NFO tags:
+    musicvideo:
+      album: Music Videos
+      artist: JMC
+      title: This GPU SLIDES into this Case! - Silverstone SUGO 16 ITX Case
+      year: 2021

tests/e2e/resources/transaction_log_summaries/plugins/test_subtitles_tags_chapters.txt

@@ -1,21 +0,0 @@
-Files created in '{output_directory}'
-----------------------------------------
-JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind-thumb.jpg
-JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind.mp4
-  Chapters embedded into the video:
-    0:00: Intro
-    0:10: Part 1
-    0:20: Part 2
-    0:30: Part 3
-    0:40: Part 4
-    1:01: Part 5
-  Embedded subtitles with lang(s) en, de
-  Video Tags:
-    title: YouTube Rewind 2019: For the Record | #YouTubeRewind
-JMC - YouTube Rewind 2019： For the Record ｜ #YouTubeRewind.nfo
-  NFO tags:
-    musicvideo:
-      album: Music Videos
-      artist: JMC
-      title: YouTube Rewind 2019: For the Record | #YouTubeRewind
-      year: 2019

tests/e2e/resources/transaction_log_summaries/youtube/test_video_with_chapter_timestamps.txt

@@ -1,23 +0,0 @@
-Files created in '{output_directory}'
-----------------------------------------
-JMC - Oblivion Mod ＂Falcor＂ p.1-thumb.jpg
-JMC - Oblivion Mod ＂Falcor＂ p.1.mp4
-  Chapters embedded into the video:
-    0:00: Intro
-    0:10: Part 1
-    0:20: Part 2
-    0:30: Part 3
-    0:40: Part 4
-    1:01: Part 5
-  Video Tags:
-    description:
-      🎸 / ' "
-      newline?
-    title: Oblivion Mod "Falcor" p.1
-JMC - Oblivion Mod ＂Falcor＂ p.1.nfo
-  NFO tags:
-    musicvideo:
-      album: Music Videos
-      artist: JMC
-      title: Oblivion Mod "Falcor" p.1
-      year: 2010

tests/e2e/youtube/test_video.py

@@ -86,34 +86,3 @@ class TestYoutubeVideo:
             dry_run=dry_run,
             expected_download_summary_file_name="youtube/test_video.json",
         )
-
-    @pytest.mark.parametrize("dry_run", [True, False])
-    def test_single_video_with_timestamp_chapters_download(
-        self,
-        timestamps_file_path,
-        music_video_config,
-        single_video_preset_dict,
-        output_directory,
-        dry_run,
-    ):
-        # Test chapters and video tags, throw in a video tag with special chars while we are at it
-        single_video_preset_dict["youtube"]["chapter_timestamps"] = timestamps_file_path
-        single_video_preset_dict["video_tags"]["tags"]["description"] = "🎸 / ' \" \n newline?"
-
-        single_video_subscription = Subscription.from_dict(
-            config=music_video_config,
-            preset_name="music_video_single_video_test",
-            preset_dict=single_video_preset_dict,
-        )
-
-        transaction_log = single_video_subscription.download(dry_run=dry_run)
-        assert_transaction_log_matches(
-            output_directory=output_directory,
-            transaction_log=transaction_log,
-            transaction_log_summary_file_name="youtube/test_video_with_chapter_timestamps.txt",
-        )
-        assert_expected_downloads(
-            output_directory=output_directory,
-            dry_run=dry_run,
-            expected_download_summary_file_name="youtube/test_video_with_chapter_timestamps.json",
-        )