[BUG FIX] Fix 'gs.tools.animate' dropping last frame by migrating from MoviePy to PyAV.#2705
Open
vlordier wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
Open
[BUG FIX] Fix 'gs.tools.animate' dropping last frame by migrating from MoviePy to PyAV.#2705vlordier wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
vlordier wants to merge 3 commits intoGenesis-Embodied-AI:mainfrom
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
3 tasks
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b569b3229
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| container = av.open(filename, mode="w") | ||
| try: | ||
| stream = container.add_stream("libx264", rate=fps) |
There was a problem hiding this comment.
animate() now enters the PyAV path whenever import av succeeds, but it hard-codes container.add_stream("libx264", ...) and only falls back on ImportError. In environments where PyAV is installed without an available libx264 encoder (a common FFmpeg build difference), add_stream raises an FFmpeg/ValueError and the function aborts instead of using the intended moviepy fallback. This turns a recoverable configuration mismatch into a runtime failure for recording/export workflows.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Updates gs.tools.animate() to avoid the moviepy≥2.x “last frame dropped” behavior by using PyAV when available, while retaining moviepy as a fallback for environments without av.
Changes:
- Implement PyAV-based MP4 encoding in
animate()with explicit encoder flush (stream.encode(None)). - Add RGBA→RGB handling to avoid libx264/yuv420p failures on 4-channel inputs.
- Ensure the output container is closed via
try/finally, with a moviepy fallback whenavis missing.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+33
to
+48
| try: | ||
| import av | ||
|
|
||
| first = imgs[0] | ||
| if not isinstance(first, np.ndarray): | ||
| first = np.array(first) | ||
| # Strip alpha channel if present; libx264 only accepts RGB or grayscale. | ||
| if first.ndim == 3 and first.shape[2] == 4: | ||
| first = first[..., :3] | ||
| height, width = first.shape[:2] | ||
| is_color = first.ndim == 3 and first.shape[2] == 3 | ||
|
|
||
| container = av.open(filename, mode="w") | ||
| try: | ||
| stream = container.add_stream("libx264", rate=fps) | ||
| stream.width = width |
Comment on lines
+62
to
+68
| if not isinstance(img, np.ndarray): | ||
| img = np.array(img) | ||
| img = img.astype(np.uint8) | ||
| # Strip alpha channel for consistency with `first`. | ||
| if img.ndim == 3 and img.shape[2] == 4: | ||
| img = img[..., :3] | ||
| buf[: img.shape[0], : img.shape[1]] = img |
Comment on lines
+45
to
+52
| container = av.open(filename, mode="w") | ||
| try: | ||
| stream = container.add_stream("libx264", rate=fps) | ||
| stream.width = width | ||
| stream.height = height | ||
| stream.pix_fmt = "yuv420p" | ||
| stream.codec_context.options = {"preset": "ultrafast"} | ||
|
|
Comment on lines
+53
to
+70
| fmt = "rgb24" if is_color else "gray8" | ||
| video_frame = av.VideoFrame(width, height, fmt) | ||
| frame_plane = video_frame.planes[0] | ||
| if is_color: | ||
| buf = np.asarray(memoryview(frame_plane)).reshape((-1, frame_plane.line_size // 3, 3)) | ||
| else: | ||
| buf = np.asarray(memoryview(frame_plane)).reshape((-1, frame_plane.line_size)) | ||
|
|
||
| for img in imgs: | ||
| if not isinstance(img, np.ndarray): | ||
| img = np.array(img) | ||
| img = img.astype(np.uint8) | ||
| # Strip alpha channel for consistency with `first`. | ||
| if img.ndim == 3 and img.shape[2] == 4: | ||
| img = img[..., :3] | ||
| buf[: img.shape[0], : img.shape[1]] = img | ||
| for packet in stream.encode(video_frame): | ||
| container.mux(packet) |
Comment on lines
+77
to
+82
| except ImportError: | ||
| from moviepy import ImageSequenceClip | ||
|
|
||
| clip = ImageSequenceClip(imgs, fps=fps) | ||
| clip.write_videofile(filename, fps=fps, logger=None, codec="libx264", preset="ultrafast") | ||
|
|
Comment on lines
+33
to
+35
| try: | ||
| import av | ||
|
|
vlordier
force-pushed
the
fix/animate-use-pyav-v2
branch
from
0b569b3 to
7085bc7
Compare
Author
Self-review findings and fixesA thorough review uncovered one additional bug in the initial implementation (now fixed in the latest push): Bug:
|
moviepy >= 2.x drops the last frame when writing videos via ImageSequenceClip.write_videofile(). Replace with PyAV which correctly flushes all buffered frames via stream.encode(None) before closing. Additional improvements over the first attempt: - Strip alpha channel (RGBA → RGB) so libx264/yuv420p encoding doesn't fail on 4-channel inputs (e.g. PNG frames from PIL.Image.open) - Wrap the encode loop in try/finally so the container file handle is always closed, even if an encode error occurs mid-stream - Fall back to moviepy when av is not installed Fixes Genesis-Embodied-AI#1635
vlordier
force-pushed
the
fix/animate-use-pyav-v2
branch
from
7085bc7 to
3598cd2
Compare
Author
Second review pass — all comments addressed, full test suite passesChanges since last push
Static analysisTest results — 22 / 22 passNote: T12 additionally confirms the moviepy fallback warning text is emitted correctly when |
- Reject non-uint8 inputs (float/bool) with a clear ValueError and conversion hint instead of silently truncating values - Validate per-frame shape/channel consistency; raise ValueError immediately on mismatch to fail fast rather than producing corrupted output - Add unit tests for: RGB frame count, grayscale frame count, float rejection, shape mismatch rejection, RGBA alpha stripping
…de errors Only ImportError was caught previously; non-import av failures (e.g. av.FFmpegError, RuntimeError from OS/codec issues) would propagate as unhandled exceptions instead of triggering the moviepy fallback. Catch all Exception from the av block, but re-raise ValueError so that our own input-validation errors (bad dtype, shape mismatch) still propagate clearly to the caller. Add two more unit tests: - av RuntimeError triggers moviepy fallback (Codex P1 concern) - ValueError from dtype validation propagates through the av block
duburcqa
requested changes
Apr 15, 2026
Collaborator
duburcqa
left a comment
duburcqa
left a comment
There was a problem hiding this comment.
- Leverage the existing
VideoFileWriterinfrastructure available ingenesis/recorders/file_writers.pyinstead of duplicating everything - Remove fallback to
moviepyand accompanying unit test - Remove tests checking failure mode that are impossible (
test_animate_rejects_mismatched_frame_shape,test_animate_rgba_strips_alpha,test_animate_value_error_propagates_through_av_block)
duburcqa
changed the title
duburcqa
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the moviepy-based implementation in
gs.tools.animate()with PyAV, which is already used byVideoFileWriterin the recorder system. Keeps moviepy as a fallback whenavis not installed.Root cause
In moviepy ≥ 2.x,
ImageSequenceClip.write_videofile()silently drops the last frame. PyAV's explicitstream.encode(None)flush guarantees all buffered frames are written before the container is closed.Changes
genesis/utils/tools.py—animate()now uses PyAV when available:stream.encode(None)to flush all buffered frames before closePIL.Image.open) don't fail theyuv420plibx264 pathtry/finallyso the file handle is always closed even on encode errorsavis not installedTest plan
cam.start_recording()/cam.stop_recording()and confirm the output video contains exactly N framesavis uninstalledCloses #1635