Skip to content

Fix video I/O: context manager, no-audio guard, temp cleanup#483

Merged
satra merged 3 commits into
alphafrom
fix/video-io-review
Apr 25, 2026
Merged

Fix video I/O: context manager, no-audio guard, temp cleanup#483
satra merged 3 commits into
alphafrom
fix/video-io-review

Conversation

@satra

@satra satra commented Apr 25, 2026

Copy link
Copy Markdown
Collaborator

Addresses PR #482 review comments:

  • with av.open() context manager for proper resource cleanup
  • Guard against videos with no audio track (was IndexError on streams.audio[0])
  • Temp directory cleaned up with shutil.rmtree in finally block
  • Videos without audio are skipped with a warning instead of crashing

🤖 Generated with Claude Code

Version

Published prerelease version: 1.3.1-alpha.19

Changelog

🐛 Bug Fix

  • Fix video I/O: context manager, no-audio guard, temp cleanup #483 (@satra)

Authors: 1

Addresses PR #482 review comments:
- Use `with av.open()` context manager (proper resource cleanup)
- Guard against videos with no audio track (was IndexError)
- Clean up temp directory with shutil.rmtree in finally block
- Skip videos without audio instead of crashing

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request improves the audio extraction process from video files by implementing context managers for resource handling, adding checks for missing audio tracks, and ensuring temporary directory cleanup using a try-finally block. The review feedback identifies two significant issues: a fragile path calculation logic that could lead to permission errors by attempting to write to the root directory, and a potential crash in the file reading utility when no audio files are successfully extracted. Both issues include specific code suggestions for resolution.

I am having trouble creating individual review comments. Click here to see my feedback.

src/senselab/video/tasks/input_output.py (84-85)

high

The current logic for calculating base_file_name using replace is fragile and can lead to incorrect paths. If common_path does not end with a separator (which is common for path utilities), the resulting base_file_name will have a leading slash (e.g., /video). When passed to os.path.join, any argument starting with a slash is treated as an absolute path, causing temp_dir to be ignored. This could result in attempts to write to the root directory, likely causing a PermissionError. Using os.path.relpath is a much safer and more robust approach.

            rel_path = os.path.relpath(file.filepath, common_path)
            base_file_name = os.path.splitext(rel_path)[0]
            output_audio_path = os.path.join(temp_dir, f"{base_file_name}.{audio_format}")

src/senselab/video/tasks/input_output.py (90)

high

If no audio tracks are found in any of the provided videos, or if the input list is empty, audio_files_paths will be an empty list. Calling read_files_from_disk([]) currently triggers an IndexError in the utility function (which attempts to access files[0].type). To fulfill the PR's goal of skipping videos without audio instead of crashing, you should handle the case where no audio files were successfully extracted.

        result = read_files_from_disk(audio_files_paths) if audio_files_paths else {}

- Use os.path.relpath instead of str.replace for safe relative paths
  (leading slash caused writes to root directory)
- Return empty dict when no audio tracks found instead of crashing
  read_files_from_disk with empty list

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@satra
satra merged commit 02fbaaf into alpha Apr 25, 2026
9 checks passed
@satra
satra deleted the fix/video-io-review branch April 25, 2026 14:30
github-actions Bot added a commit that referenced this pull request Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant