From 2a8670b7c7cdfc920f4d1b2ee7e5f36b55eb4927 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Z=C3=BCbeyde=20Civelek?= Date: Thu, 28 Aug 2025 11:45:08 +0200 Subject: [PATCH] chapters: improve ExtractChapterFramesTask --- cds/modules/flows/tasks.py | 115 +++++++++++------ cds/modules/previewer/api.py | 12 ++ cds/modules/previewer/extensions/video.py | 6 - .../cds_previewer/macros/player.html | 88 ++++++------- .../templates/cds_records/video/detail.html | 6 +- .../bootstrap3/js/cds_records/cdsRecord.js | 6 +- tests/unit/test_flows_tasks.py | 120 +++++++++++++++++- 7 files changed, 247 insertions(+), 106 deletions(-) diff --git a/cds/modules/flows/tasks.py b/cds/modules/flows/tasks.py index 8dc0068d5..f9df6918a 100644 --- a/cds/modules/flows/tasks.py +++ b/cds/modules/flows/tasks.py @@ -28,6 +28,7 @@ import shutil import signal import tempfile +from io import BytesIO import jsonpatch import requests @@ -57,7 +58,7 @@ from cds.modules.flows.models import FlowTaskMetadata from cds.modules.flows.models import FlowTaskStatus as FlowTaskStatus - +from cds.modules.records.api import CDSVideosFilesIterator from ..ffmpeg import ff_frames, ff_probe_all from ..opencast.api import OpenCast from ..opencast.error import RequestError @@ -746,8 +747,13 @@ class ExtractChapterFramesTask(AVCTask): name = "file_video_extract_chapter_frames" @staticmethod - def clean(version_id, *args, **kwargs): - """Delete generated chapter frame ObjectVersion slaves.""" + def clean(version_id, valid_chapter_seconds=None, *args, **kwargs): + """Delete generated chapter frame ObjectVersion slaves. + + - If valid_chapter_seconds is given, keep them. + - If not, remove all chapter frames. + """ + valid_chapter_seconds = valid_chapter_seconds or [] # remove all objects version "slave" with type "frame" that are chapter frames tag_alias_1 = aliased(ObjectVersionTag) tag_alias_2 = aliased(ObjectVersionTag) @@ -764,8 +770,18 @@ def clean(version_id, *args, **kwargs): ) for slave in slaves: + ts_val = next(t.value for t in slave.tags if t.key == "timestamp") + if ts_val in valid_chapter_seconds: + continue dispose_object_version(slave) + # If no valid chapter seconds, remove the chapters.vtt file + if not valid_chapter_seconds: + master_obj = ObjectVersion.query.get(version_id) + vtt_objs = ObjectVersion.get_versions(master_obj.bucket_id, "chapters.vtt") + for vtt_obj in vtt_objs: + dispose_object_version(vtt_obj) + def run(self, *args, **kwargs): """Extract frames only at chapter timestamps from video description. @@ -808,10 +824,6 @@ def run(self, *args, **kwargs): # Parse chapters from description chapters = parse_video_chapters(description) - - if not chapters: - self.log("No chapters found in description - task completed") - return {"chapter_frames_extracted": 0, "status": "no_chapters"} self.log("Found {0} chapters in description".format(len(chapters))) @@ -822,7 +834,7 @@ def run(self, *args, **kwargs): raise ValueError("Video duration is 0 - cannot extract frames") # Check which timestamps already have frames - existing_timestamps = self._get_existing_frame_timestamps() + existing_timestamps = self._get_existing_chapter_frame_timestamps(deposit_video) def progress_updater(current_chapter): """Progress reporter.""" @@ -835,7 +847,7 @@ def progress_updater(current_chapter): ) self.log(meta["message"]) - frames = self._create_chapter_frames( + frames, chapter_seconds = self._create_chapter_frames( chapters=chapters, duration=duration, object_=self.object_version, @@ -844,6 +856,12 @@ def progress_updater(current_chapter): progress_updater=progress_updater, ) + # Clean unused chapters + self.clean(version_id=self.object_version_id, valid_chapter_seconds=chapter_seconds) + + # Create or update WebVTT file for chapters + self._build_chapter_vtt(chapters, duration) + # Sync deposit and record files sync_records_with_deposit_files(self.deposit_id) @@ -865,32 +883,17 @@ def progress_updater(current_chapter): self.log("Finished task {0}".format(kwargs["task_id"])) return "Created {0} chapter frames.".format(total_frames) - def _get_existing_frame_timestamps(self): - """Get set of existing frame timestamps to avoid duplicates.""" - tag_alias_1 = aliased(ObjectVersionTag) - tag_alias_2 = aliased(ObjectVersionTag) - tag_alias_3 = aliased(ObjectVersionTag) + def _get_existing_chapter_frame_timestamps(self, deposit): + """Get timestamps of existing chapter frames.""" + master_file = CDSVideosFilesIterator.get_master_video_file(deposit) + frames = CDSVideosFilesIterator.get_video_frames(master_file) - existing = ( - ObjectVersion.query.join(tag_alias_1, ObjectVersion.tags) - .join(tag_alias_2, ObjectVersion.tags) - .join(tag_alias_3, ObjectVersion.tags) - .filter(tag_alias_1.key == "master", tag_alias_1.value == self.object_version_id) - .filter(tag_alias_2.key == "context_type", tag_alias_2.value == "frame") - .filter(tag_alias_3.key == "timestamp") - .all() - ) - - existing_timestamps = set() - for obj in existing: - for tag in obj.tags: - if tag.key == "timestamp": - try: - existing_timestamps.add(float(tag.value)) - except ValueError: - continue - - return existing_timestamps + existing = set() + for f in frames: + tags = f.get("tags", {}) + if tags.get("is_chapter_frame") == "true": + existing.add(float(tags.get("timestamp"))) + return existing @classmethod def _create_chapter_frames( @@ -904,6 +907,7 @@ def _create_chapter_frames( ): """Create frames for chapters that don't already exist at those timestamps.""" created_frames = [] + valid_chapter_seconds = [] current_chapter = 0 with move_file_into_local(object_, delete=True) as url: @@ -920,6 +924,10 @@ def _create_chapter_frames( if chapter_seconds > duration: continue + # For 0:00 chapters, use a small offset to avoid extraction issues + chapter_seconds = max(chapter_seconds, 0.1) if chapter_seconds == 0 else chapter_seconds + valid_chapter_seconds.append(to_string(chapter_seconds)) + # Skip if frame already exists at this timestamp (with some tolerance) timestamp_exists = any( abs(existing_ts - chapter_seconds) < 0.1 @@ -931,9 +939,6 @@ def _create_chapter_frames( frame_filename = "chapter-{0}.jpg".format(int(chapter_seconds)) frame_path = os.path.join(output_dir, frame_filename) - # For 0:00 chapters, use a small offset to avoid extraction issues - chapter_seconds = max(chapter_seconds, 0.1) if chapter_seconds == 0 else chapter_seconds - try: # Extract single frame at chapter timestamp using ff_frames ff_frames( @@ -970,7 +975,41 @@ def _create_chapter_frames( ) continue - return created_frames + return created_frames, valid_chapter_seconds + + def _build_chapter_vtt(self, chapters, duration): + """Build WebVTT content string from chapters list.""" + if not chapters: + return + vtt = "WEBVTT\n\n" + for i, c in enumerate(sorted(chapters, key=lambda x: x["seconds"])): + start = c["seconds"] + end = chapters[i+1]["seconds"] if i+1 < len(chapters) else duration + start_str = "{:02}:{:02}:{:02}.000".format( + int(start // 3600), + int((start % 3600) // 60), + int(start % 60) + ) + end_str = "{:02}:{:02}:{:02}.000".format( + int(end // 3600), + int((end % 3600) // 60), + int(end % 60) + ) + vtt += f"{i+1}\n{start_str} --> {end_str}\n{c['title']}\n\n" + + vtt_bytes = vtt.encode("utf-8") + vtt_key = "chapters.vtt" + + obj = ObjectVersion.create( + bucket=self.object_version.bucket, + key=vtt_key, + stream=BytesIO(vtt_bytes), + size=len(vtt_bytes), + ) + ObjectVersionTag.create(obj, "media_type", "chapters") + ObjectVersionTag.create(obj, "context_type", "chapters") + ObjectVersionTag.create(obj, "content_type", "vtt") + self.log("Created chapters.vtt") class TranscodeVideoTask(AVCTask): diff --git a/cds/modules/previewer/api.py b/cds/modules/previewer/api.py index d226f2b7e..8c94f3b4b 100644 --- a/cds/modules/previewer/api.py +++ b/cds/modules/previewer/api.py @@ -128,6 +128,18 @@ def vr(self): """Get video's VR flag.""" return self.record.get("vr") + @property + def chapters_uri(self): + """Get the chapters.vtt file link if available.""" + try: + return [ + f["links"]["self"] + for f in self.record["_files"] + if f.get("context_type") == "chapters" and f.get("content_type") == "vtt" + ][0] + except IndexError: + return None + class CDSPreviewDepositFile(PreviewFile): """Preview deposit files implementation.""" diff --git a/cds/modules/previewer/extensions/video.py b/cds/modules/previewer/extensions/video.py index 3f01305fb..6bbc754e7 100644 --- a/cds/modules/previewer/extensions/video.py +++ b/cds/modules/previewer/extensions/video.py @@ -65,12 +65,6 @@ def preview(self, file, embed_config=None): else "" ) - description = record.get('description', '') - if description: - record['chapters'] = parse_video_chapters(description) - else: - record['chapters'] = [] - return render_template( self.template, file=file, diff --git a/cds/modules/previewer/templates/cds_previewer/macros/player.html b/cds/modules/previewer/templates/cds_previewer/macros/player.html index 8144c430f..bf45ca7fe 100644 --- a/cds/modules/previewer/templates/cds_previewer/macros/player.html +++ b/cds/modules/previewer/templates/cds_previewer/macros/player.html @@ -54,7 +54,7 @@ window.top.player = player; - // --- Chapters helpers --- + // --- helpers --- function durationToSeconds(durationStr) { if (!durationStr) return null; const parts = durationStr.split(':').map(Number); // [HH, MM, SS] or [MM, SS] @@ -66,55 +66,8 @@ } return null; } - function formatVttTime(total) { - const h = Math.floor(total / 3600); - const m = Math.floor((total % 3600) / 60); - const s = Math.floor(total % 60); - return String(h).padStart(2,'0') + ':' + String(m).padStart(2,'0') + ':' + String(s).padStart(2,'0') + '.000'; - } - function buildChaptersVttFromArray(chaptersArr, videoDurationStr) { - if (!chaptersArr || !chaptersArr.length) return null; - - chaptersArr.sort((a,b) => a.seconds - b.seconds); - const videoDuration = durationToSeconds(videoDurationStr); - if (videoDuration !== null && videoDuration <= 0) return null; - let vtt = 'WEBVTT\n\n'; - for (let i = 0; i < chaptersArr.length; i++) { - const c = chaptersArr[i]; - const start = c.seconds; - const end = (i + 1 < chaptersArr.length) - ? chaptersArr[i+1].seconds - : (videoDuration || start + 1); - - vtt += `${i+1}\n${formatVttTime(start)} --> ${formatVttTime(end)}\n${c.title}\n\n`; - } - return vtt; - } - // Build textTracks - var textTracksArr = []; - // Chapters - var chaptersArr = {{ (record.chapters if record and record.chapters else []) | tojson }}; - var videoDurationStr = {{ (record.duration if record and record.duration else "") | tojson }}; - var vttStr = buildChaptersVttFromArray(chaptersArr, videoDurationStr); - if (vttStr) { - var chapUrl = URL.createObjectURL(new Blob([vttStr], { type: 'text/vtt' })); - textTracksArr.push({ kind: 'chapters', src: chapUrl, label: 'Chapters' }); - } - {% if not embed_config.subtitlesOff %} - textTracksArr.push({ kind: 'metadata', src: '{{ obj.thumbnails_uri }}', label: 'thumbnails', default: true }); - {% for uri, lang in obj.subtitles %} - textTracksArr.push({ - kind: 'subtitles', - src: '{{ uri }}', - label: '{{ lang }}', - srclang: '{{ lang }}' - {% if embed_config.subtitles and embed_config.subtitles == lang %}, default: true{% endif %} - }); - {% endfor %} - {% endif %} - - // Set source with textTracks + // Preload player.source = { sources: [ { @@ -128,11 +81,42 @@ src: '{{ obj.uri }}', type: 'video/mp4' {% endif %} - } + }, + ], + textTracks: [ + { + kind: 'metadata', + src: '{{ obj.thumbnails_uri }}', + label: 'thumbnails', + default: true, + }, + + // Add chapters.vtt if available + {% if obj.chapters_uri %} + { + kind: 'chapters', + src: '{{ obj.chapters_uri }}', + label: 'Chapters', + }, + {% endif %} + + // Add subtitles + {% if not embed_config.subtitlesOff %} + {% for uri, lang in obj.subtitles %} + { + kind: 'subtitles', + src: '{{ uri }}', + label: '{{ lang }}', + srclang: '{{ lang }}', + {% if embed_config.subtitles and embed_config.subtitles == lang %} + default: true, + {% endif %} + }, + {% endfor %} + {% endif %} ], - textTracks: textTracksArr, poster: '{{ obj.poster_uri }}', - {% if obj.vr %} + {% if obj.vr %} vr: { 360: true, }, diff --git a/cds/modules/records/static/templates/cds_records/video/detail.html b/cds/modules/records/static/templates/cds_records/video/detail.html index 17bcaf145..d079dade0 100644 --- a/cds/modules/records/static/templates/cds_records/video/detail.html +++ b/cds/modules/records/static/templates/cds_records/video/detail.html @@ -26,7 +26,7 @@

In this video -
+

@@ -312,7 +312,7 @@

{{translation.title.title}}

Chapters

@@ -369,7 +369,7 @@

Transcriptions

diff --git a/cds/modules/theme/assets/bootstrap3/js/cds_records/cdsRecord.js b/cds/modules/theme/assets/bootstrap3/js/cds_records/cdsRecord.js index fce62a650..2760868a3 100644 --- a/cds/modules/theme/assets/bootstrap3/js/cds_records/cdsRecord.js +++ b/cds/modules/theme/assets/bootstrap3/js/cds_records/cdsRecord.js @@ -117,8 +117,12 @@ function cdsRecordController($scope, $sce, $http, $timeout, $filter) { $scope.seekTo(timecode); }; + $scope.closeInThisVideoSection = function () { + $scope.showInThisVideoSection = false; + }; + $scope.toggleInThisVideo = function (tab) { - $scope.showInThisVideoSection = !$scope.showInThisVideoSection; + $scope.showInThisVideoSection = true; $scope.activeTab = tab; // Jump to Transcriptions section diff --git a/tests/unit/test_flows_tasks.py b/tests/unit/test_flows_tasks.py index 57bd0e3ed..71f2b1ffe 100644 --- a/tests/unit/test_flows_tasks.py +++ b/tests/unit/test_flows_tasks.py @@ -580,7 +580,7 @@ def test_sync_records_with_deposits( def test_extract_chapter_frames_task(app, db, bucket, video, users): - """Test that chapter frames are extracted for each chapter timestamp in the description.""" + """Test that chapter frames and chapters.vtt are created from description.""" # Create a video object version obj = ObjectVersion.create(bucket=bucket, key="video.mp4", stream=open(video, "rb")) add_video_tags(obj) @@ -592,8 +592,7 @@ def test_extract_chapter_frames_task(app, db, bucket, video, users): "type": "VIDEO", } project = Project.create(project_data) - - # All chapters < 60 seconds so they are in range of test video + video_data = { "_project_id": project["_deposit"]["id"], "title": {"title": "Test Video with Chapters"}, @@ -629,7 +628,7 @@ def test_extract_chapter_frames_task(app, db, bucket, video, users): # Expected chapter timestamps in seconds (0 becomes 0.1 offset) expected_timestamps = [0.1, 10, 20, 30, 40] - # Mock file operations, ffmpeg, and duration + # Mocks with mock.patch("cds.modules.flows.tasks.move_file_into_local") as mock_move, \ mock.patch("cds.modules.flows.tasks.ff_frames") as mock_ff_frames, \ mock.patch("cds.modules.flows.tasks.file_opener_xrootd") as mock_file_opener, \ @@ -637,11 +636,16 @@ def test_extract_chapter_frames_task(app, db, bucket, video, users): mock.patch("os.path.getsize", return_value=1024), \ mock.patch("cds.modules.flows.tasks.sync_records_with_deposit_files"), \ mock.patch.object(ExtractChapterFramesTask, "_base_payload", {"tags": {"duration": 500}}), \ - mock.patch("cds.modules.flows.tasks.ExtractFramesTask._create_object") as mock_create_object: + mock.patch("cds.modules.flows.tasks.ExtractFramesTask._create_object") as mock_create_object, \ + mock.patch("cds.modules.flows.tasks.ObjectVersion.create") as mock_obj_create, \ + mock.patch("cds.modules.flows.tasks.ObjectVersionTag.create") as mock_tag_create: mock_move.return_value.__enter__.return_value = "/tmp/test_video.mp4" mock_file_opener.return_value = BytesIO(b"fake_frame_data") - + + fake_obj = mock.Mock() + mock_obj_create.return_value = fake_obj + # Run task ExtractChapterFramesTask().s(**payload.copy()).apply_async() @@ -663,3 +667,107 @@ def test_extract_chapter_frames_task(app, db, bucket, video, users): assert kwargs["is_chapter_frame"] is True assert kwargs["context_type"] == "frame" assert kwargs["media_type"] == "image" + + # ---- Verify chapters.vtt creation ---- + mock_obj_create.assert_called_once() + vtt_call_args = mock_obj_create.call_args + assert vtt_call_args.kwargs["bucket"] == obj.bucket + assert vtt_call_args.kwargs["key"] == "chapters.vtt" + + # Tags applied to chapters.vtt + tag_keys = [c.args[1] for c in mock_tag_create.call_args_list] + assert "context_type" in tag_keys + assert "content_type" in tag_keys + assert "media_type" in tag_keys + + +def test_extract_chapter_frames_task_cleanup(app, db, bucket, video, users): + """Test that chapter frames are updated/cleaned when description changes.""" + + # Create master ObjectVersion + obj = ObjectVersion.create(bucket=bucket, key="video.mp4", stream=open(video, "rb")) + add_video_tags(obj) + db.session.commit() + master_version_id = str(obj.version_id) + + # Create project + video deposit + project = Project.create({"category": "OPEN", "type": "VIDEO"}) + video_deposit = Video.create({ + "_project_id": project["_deposit"]["id"], + "title": {"title": "Video with chapters"}, + "description": """0:00 Intro + 0:10 Chapter 1 + 0:20 Chapter 2""", + }) + deposit_id = str(video_deposit["_deposit"]["id"]) + + # Flow metadata + payload = dict( + version_id=master_version_id, + key=obj.key, + bucket_id=str(obj.bucket_id), + deposit_id=deposit_id, + ) + flow_metadata = FlowMetadata.create( + deposit_id=deposit_id, user_id=users[0], payload=payload + ) + payload["flow_id"] = str(flow_metadata.id) + flow_metadata.payload = payload + FlowService(flow_metadata) + db.session.commit() + + with mock.patch("cds.modules.flows.tasks.move_file_into_local"), \ + mock.patch("cds.modules.flows.tasks.ff_frames"), \ + mock.patch("cds.modules.flows.tasks.file_opener_xrootd", return_value=BytesIO(b"fake_frame_data")), \ + mock.patch("os.path.exists", return_value=True), \ + mock.patch("os.path.getsize", return_value=1024), \ + mock.patch("cds.modules.flows.tasks.sync_records_with_deposit_files"), \ + mock.patch.object(ExtractChapterFramesTask, "_base_payload", {"tags": {"duration": 100}}), \ + mock.patch("cds.modules.flows.tasks.ExtractFramesTask._create_object") as mock_create_object, \ + mock.patch("cds.modules.flows.tasks.ExtractChapterFramesTask._build_chapter_vtt"), \ + mock.patch("cds.modules.flows.tasks.ExtractChapterFramesTask._get_existing_chapter_frame_timestamps") as mock_existing: + + # Track created & disposed timestamps (floats) + created_timestamps = set() + disposed_timestamps = [] + + def fake_create_object(*args, **kwargs): + if "timestamp" in kwargs: + created_timestamps.add(float(kwargs["timestamp"])) + return mock.Mock() + + def fake_clean(version_id, valid_chapter_seconds=None, *args, **kwargs): + """Simulate cleaning: if 20.0 isn't in valid seconds, mark it disposed.""" + valid_floats = {float(s) for s in (valid_chapter_seconds or [])} + if 20.0 not in valid_floats: + disposed_timestamps.append(20.0) + + mock_create_object.side_effect = fake_create_object + + # First run → should create frames at 0.1, 10.0, 20.0 + mock_existing.return_value = set() + with mock.patch("cds.modules.flows.tasks.ExtractChapterFramesTask.clean", side_effect=fake_clean): + ExtractChapterFramesTask().s(**payload.copy()).apply_async() + assert created_timestamps == {0.1, 10.0, 20.0} + assert disposed_timestamps == [] # nothing disposed on first run + + # Update description → now chapters at 0.1, 10.0, 30.0 + video_deposit["description"] = """0:00 Intro + 0:10 Chapter 1 + 0:30 Chapter 3""" + video_deposit.commit() + db.session.commit() + + # Reset state + created_timestamps.clear() + disposed_timestamps.clear() + + # Second run → should create 30.0, dispose 20.0 + mock_existing.return_value = {0.1, 10.0, 20.0} + with mock.patch("cds.modules.flows.tasks.ExtractChapterFramesTask.clean", side_effect=fake_clean): + ExtractChapterFramesTask().s(**payload.copy()).apply_async() + + assert 30.0 in created_timestamps + assert 20.0 in disposed_timestamps + +