Skip to content

fix: resolve video save path without os.chdir (fixes #378)#448

Open
Haris-bin-shakeel wants to merge 1 commit into
brainglobe:mainfrom
Haris-bin-shakeel:fix/378-video-save-path
Open

fix: resolve video save path without os.chdir (fixes #378)#448
Haris-bin-shakeel wants to merge 1 commit into
brainglobe:mainfrom
Haris-bin-shakeel:fix/378-video-save-path

Conversation

@Haris-bin-shakeel
Copy link
Copy Markdown

@Haris-bin-shakeel Haris-bin-shakeel commented May 18, 2026

Summary

Fixes #378 by removing os.chdir() from VideoMaker.make_video() and switching to explicit absolute paths throughout the video pipeline.

Root Cause

make_video() called os.chdir(self.save_fld) so ffmpeg could locate the output file using only a bare filename. The CWD was restored at the end — but only on the happy path. Any exception during rendering left the process CWD permanently mutated, causing subsequent path operations in the caller's script to silently resolve to the wrong location. This explains the intermittent nature of the bug.

_video.py compounded this by passing only a filename to ffmpeg, making output placement entirely dependent on global CWD state.

Changes

  • brainrender/video.py: removed curdir = os.getcwd() and both os.chdir() calls; Video() now receives a fully-resolved absolute path via self.save_fld.resolve(); compress() updated to match
  • brainrender/_video.py: ffmpeg input dir and output file both quoted to handle paths with spaces
  • tests/test_video.py: assertions updated to validate resolved absolute paths
  • tests/test_video.py::test_compress PASSED

Verification

tests/test_video.py::test_video PASSED
tests/test_video.py::test_video_custom PASSED
tests/test_video.py::test_animation PASSED

@adamltyson adamltyson requested a review from a team May 19, 2026 10:50
@codecov
Copy link
Copy Markdown

codecov Bot commented May 22, 2026

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.90%. Comparing base (2322e0f) to head (f6f63a9).

Files with missing lines Patch % Lines
brainrender/video.py 20.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
- Coverage   89.07%   88.90%   -0.17%     
==========================================
  Files          27       27              
  Lines        1281     1280       -1     
==========================================
- Hits         1141     1138       -3     
- Misses        140      142       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Haris-bin-shakeel Haris-bin-shakeel force-pushed the fix/378-video-save-path branch 2 times, most recently from 743dafb to 588daad Compare May 24, 2026 17:21
@Haris-bin-shakeel
Copy link
Copy Markdown
Author

Added test_compress to cover the 4 previously uncovered lines in
compress() mocks os.system and validates that ffmpeg receives
fully-resolved absolute paths for both input and output.
test_compress passes locally.

@Haris-bin-shakeel Haris-bin-shakeel force-pushed the fix/378-video-save-path branch from e240ee8 to 39d4813 Compare May 24, 2026 17:25
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.

animation.make_video is not saving videos to the right place

1 participant