fix: scenedetect API compat, color overflow, list mutation, temp cleanup - #1
fix: scenedetect API compat, color overflow, list mutation, temp cleanup#1numbpilled2133 wants to merge 1 commit into
Conversation
- fixes scenedetect import to handle both v1 and v2 APIs gracefully - _rgb_shift and _datamosh now use np.clip to prevent uint8 overflow on shifted pixels - update_creatures uses deferred kill list instead of modifying list while iterating - gradio UI registers atexit cleanup for temp directories - properly passes str to cv2.VideoCapture (was passing Path in some paths)
|
Unable to trigger custom agent "Code Reviewer". You have run out of credits 😔 |
Reviewer's GuideUpdates scenedetect integration to support both v1 and v2 APIs with a robust fallback, fixes image processing overflow and creature list mutation bugs, and adds temporary directory cleanup plus minor API/cleanup adjustments. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Code Review by Qodo
1. Unclosed SceneDetect stream
|
|
Warning Review limit reached
More reviews will be available in 51 minutes and 6 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codereviewbot-ai[bot]](https://avatars.githubusercontent.com/in/309638?s=60&v=4){kind=small}
| @@ -22,10 +22,21 @@ | |||
| warnings.filterwarnings('ignore') | |||
There was a problem hiding this comment.
Suppressing all warnings globally with warnings.filterwarnings('ignore') can mask important runtime or dependency issues, making debugging and maintenance more difficult. It is safer to suppress only specific, known benign warnings, or to do so in a more targeted manner.
Recommendation:
Replace with selective suppression, e.g.:
warnings.filterwarnings('ignore', category=DeprecationWarning)or restrict the scope to only the libraries or code sections where warnings are expected and harmless.
| for i in range(len(self.creatures)): | ||
| for j in range(i + 1, len(self.creatures)): | ||
| c1, c2 = self.creatures[i], self.creatures[j] | ||
| if c1.get('_dead') or c2.get('_dead'): | ||
| continue | ||
| dx = c1['position'][0] - c2['position'][0] | ||
| dy = c1['position'][1] - c2['position'][1] | ||
| if (dx * dx + dy * dy) < 4.0: | ||
| self.handle_creature_interaction(c1, c2, step) | ||
|
|
||
| # apply deferred removals | ||
| self.creatures = [c for c in self.creatures if not c.get('_dead')] | ||
|
|
||
| def handle_creature_interaction(self, c1, c2, step): | ||
| color_sim = self.color_similarity( | ||
| c1['clip']['metadata']['avg_color'], |
There was a problem hiding this comment.
The nested loop over all pairs of creatures results in O(n^2) complexity per simulation step, which can significantly degrade performance as the number of creatures increases.
Recommendation:
Consider optimizing with a spatial partitioning structure (e.g., grid, quadtree) to reduce the number of pairwise checks, or limit the number of interactions per step to improve scalability.
| # Schedule temp dir cleanup — on return, register for later removal | ||
| import atexit | ||
| atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True)) |
There was a problem hiding this comment.
The use of atexit.register for cleaning up the temporary directory may lead to resource leaks, especially in a long-running or multi-user environment, as cleanup only occurs on process exit. If multiple requests are handled, temporary directories may accumulate. Consider using a context manager or explicit cleanup after the video is served to ensure timely resource release:
try:
# ...
result_path = engine.run(output_path=output_path)
return result_path, "Video generated successfully!"
finally:
shutil.rmtree(temp_dir, ignore_errors=True)Alternatively, schedule cleanup after the file is sent to the client.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In
detect_scenes, the v2 scenedetect path creates avideo_managerviasd_open_videobut never releases it; consider explicitly callingvideo_manager.release()(or using a context manager if available) to avoid leaking video resources. - The
kill_ids = set()variable inupdate_creaturesis declared but never used; it can be removed or wired into the deferred removal logic if it was intended for tracking. - You already determine
SCENEDETECT_V2at import time, so the repeatedfrom scenedetect import SceneManager/detectimports insidedetect_scenescould be simplified by relying on the module-level imports to reduce per-call overhead and keep the control flow clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `detect_scenes`, the v2 scenedetect path creates a `video_manager` via `sd_open_video` but never releases it; consider explicitly calling `video_manager.release()` (or using a context manager if available) to avoid leaking video resources.
- The `kill_ids = set()` variable in `update_creatures` is declared but never used; it can be removed or wired into the deferred removal logic if it was intended for tracking.
- You already determine `SCENEDETECT_V2` at import time, so the repeated `from scenedetect import SceneManager` / `detect` imports inside `detect_scenes` could be simplified by relying on the module-level imports to reduce per-call overhead and keep the control flow clearer.
## Individual Comments
### Comment 1
<location path="orithet/core.py" line_range="129-132" />
<code_context>
- scene_list = detect(video_path, ContentDetector())
+ if SCENEDETECT_V2:
+ from scenedetect import SceneManager
+ video_manager = sd_open_video(str(video_path))
+ sm = SceneManager()
+ sm.add_detector(ContentDetector())
+ sm.detect_scenes(video_manager)
+ scene_list = sm.get_scene_list()
+ else:
</code_context>
<issue_to_address>
**issue (bug_risk):** Video manager from scenedetect v2 is never released, which can leak resources.
In the v2 branch, `video_manager` is created but never closed, which can leak file handles/decoder resources over many runs. Please ensure it’s explicitly cleaned up (e.g., `video_manager.release()` or equivalent, ideally in a `finally` block) after `detect_scenes` completes.
</issue_to_address>
### Comment 2
<location path="orithet/gradio_ui.py" line_range="59" />
<code_context>
result_path = engine.run(output_path=output_path)
+ # Schedule temp dir cleanup — on return, register for later removal
+ import atexit
+ atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True))
return result_path, "Video generated successfully!"
</code_context>
<issue_to_address>
**issue (bug_risk):** Lambda captures `temp_dir` by late binding, so all handlers may target the same directory.
Because the closure captures `temp_dir` by reference, all registered lambdas will point to whatever `temp_dir` was last set to when the process exits. That means earlier temp dirs may never be cleaned up, and the last one may be deleted multiple times. You can fix this by binding the current value into the lambda default, e.g. `atexit.register(lambda d=temp_dir: shutil.rmtree(d, ignore_errors=True))`.
</issue_to_address>
### Comment 3
<location path="orithet/core.py" line_range="25" />
<code_context>
warnings.filterwarnings('ignore')
try:
- from scenedetect import detect, ContentDetector
+ import scenedetect
+ from scenedetect import ContentDetector
</code_context>
<issue_to_address>
**issue (complexity):** Consider centralizing the scenedetect version handling into a single backend helper and removing the unused `kill_ids` variable to streamline the code and reduce duplication.
You can simplify the scenedetect integration and remove a small bit of noise in the creature logic without changing behavior.
### 1. Centralize scenedetect version handling
Right now the v1/v2 branching and imports are split between the module top and `detect_scenes`, and you’re re-importing inside the method. You can keep all functionality and fallbacks intact by resolving the backend once at import time and exposing a single callable used by `detect_scenes`.
For example:
```python
# scenedetect setup
try:
import scenedetect
from scenedetect import ContentDetector
from scenedetect import open_video as sd_open_video
try:
# v2 backend
from scenedetect import SceneManager
SCENEDETECT_AVAILABLE = True
SCENEDETECT_BACKEND = "v2"
def _run_scenedetect(video_path):
video_manager = sd_open_video(str(video_path))
sm = SceneManager()
sm.add_detector(ContentDetector())
sm.detect_scenes(video_manager)
return sm.get_scene_list()
except ImportError:
# v1 backend
from scenedetect import detect as sd_detect
SCENEDETECT_AVAILABLE = True
SCENEDETECT_BACKEND = "v1"
def _run_scenedetect(video_path):
return sd_detect(str(video_path), ContentDetector())
except ImportError:
SCENEDETECT_AVAILABLE = False
SCENEDETECT_BACKEND = None
def _run_scenedetect(video_path):
return None
```
Then `detect_scenes` becomes simpler and doesn’t need to know about v1 vs v2 or re-import anything:
```python
def detect_scenes(self, video_path):
if SCENEDETECT_AVAILABLE:
try:
scene_list = _run_scenedetect(video_path)
if scene_list:
return [(s.get_frames(), e.get_frames()) for s, e in scene_list]
except Exception:
# keep broad catch if you want robustness, but it's now localized
pass
# fallback: fixed 5-second segments
cap = cv2.VideoCapture(str(video_path))
fps = cap.get(cv2.CAP_PROP_FPS)
frame_count = int(cap.get(cv2.CAP_PROP_FRAME_COUNT))
cap.release()
if fps == 0:
return [(0, frame_count)]
segment_length = int(fps * 5)
return [(i, min(i + segment_length, frame_count))
for i in range(0, frame_count, segment_length)]
```
This removes duplicated imports, centralizes version detection, and limits the places that “know” about the backend, while preserving your v1/v2 behavior and fallback.
### 2. Remove unused variable in `update_creatures`
`kill_ids` is currently unused and just adds noise:
```python
def update_creatures(self, step):
for c in self.creatures:
c['position'][0] = max(0, min(30, c['position'][0] + c['velocity'][0] + random.uniform(-0.1, 0.1)))
c['position'][1] = max(0, min(30, c['position'][1] + c['velocity'][1] + random.uniform(-0.1, 0.1)))
c['age'] += 1
for i in range(len(self.creatures)):
for j in range(i + 1, len(self.creatures)):
c1, c2 = self.creatures[i], self.creatures[j]
if c1.get('_dead') or c2.get('_dead'):
continue
dx = c1['position'][0] - c2['position'][0]
dy = c1['position'][1] - c2['position'][1]
if (dx * dx + dy * dy) < 4.0:
self.handle_creature_interaction(c1, c2, step)
# apply deferred removals
self.creatures = [c for c in self.creatures if not c.get('_dead')]
```
The `_dead` flag + deferred removal pattern is fine and clear as-is; the main win is just removing the unused variable and consolidating scenedetect logic.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| video_manager = sd_open_video(str(video_path)) | ||
| sm = SceneManager() | ||
| sm.add_detector(ContentDetector()) | ||
| sm.detect_scenes(video_manager) |
There was a problem hiding this comment.
issue (bug_risk): Video manager from scenedetect v2 is never released, which can leak resources.
In the v2 branch, video_manager is created but never closed, which can leak file handles/decoder resources over many runs. Please ensure it’s explicitly cleaned up (e.g., video_manager.release() or equivalent, ideally in a finally block) after detect_scenes completes.
| result_path = engine.run(output_path=output_path) | ||
| # Schedule temp dir cleanup — on return, register for later removal | ||
| import atexit | ||
| atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True)) |
There was a problem hiding this comment.
issue (bug_risk): Lambda captures temp_dir by late binding, so all handlers may target the same directory.
Because the closure captures temp_dir by reference, all registered lambdas will point to whatever temp_dir was last set to when the process exits. That means earlier temp dirs may never be cleaned up, and the last one may be deleted multiple times. You can fix this by binding the current value into the lambda default, e.g. atexit.register(lambda d=temp_dir: shutil.rmtree(d, ignore_errors=True)).
| warnings.filterwarnings('ignore') | ||
|
|
||
| try: | ||
| from scenedetect import detect, ContentDetector |
There was a problem hiding this comment.
issue (complexity): Consider centralizing the scenedetect version handling into a single backend helper and removing the unused kill_ids variable to streamline the code and reduce duplication.
You can simplify the scenedetect integration and remove a small bit of noise in the creature logic without changing behavior.
1. Centralize scenedetect version handling
Right now the v1/v2 branching and imports are split between the module top and detect_scenes, and you’re re-importing inside the method. You can keep all functionality and fallbacks intact by resolving the backend once at import time and exposing a single callable used by detect_scenes.
For example:
# scenedetect setup
try:
import scenedetect
from scenedetect import ContentDetector
from scenedetect import open_video as sd_open_video
try:
# v2 backend
from scenedetect import SceneManager
SCENEDETECT_AVAILABLE = True
SCENEDETECT_BACKEND = "v2"
def _run_scenedetect(video_path):
video_manager = sd_open_video(str(video_path))
sm = SceneManager()
sm.add_detector(ContentDetector())
sm.detect_scenes(video_manager)
return sm.get_scene_list()
except ImportError:
# v1 backend
from scenedetect import detect as sd_detect
SCENEDETECT_AVAILABLE = True
SCENEDETECT_BACKEND = "v1"
def _run_scenedetect(video_path):
return sd_detect(str(video_path), ContentDetector())
except ImportError:
SCENEDETECT_AVAILABLE = False
SCENEDETECT_BACKEND = None
def _run_scenedetect(video_path):
return NoneThen detect_scenes becomes simpler and doesn’t need to know about v1 vs v2 or re-import anything:
def detect_scenes(self, video_path):
if SCENEDETECT_AVAILABLE:
try:
scene_list = _run_scenedetect(video_path)
if scene_list:
return [(s.get_frames(), e.get_frames()) for s, e in scene_list]
except Exception:
# keep broad catch if you want robustness, but it's now localized
pass
# fallback: fixed 5-second segments
cap = cv2.VideoCapture(str(video_path))
fps = cap.get(cv2.CAP_PROP_FPS)
frame_count = int(cap.get(cv2.CAP_PROP_FRAME_COUNT))
cap.release()
if fps == 0:
return [(0, frame_count)]
segment_length = int(fps * 5)
return [(i, min(i + segment_length, frame_count))
for i in range(0, frame_count, segment_length)]This removes duplicated imports, centralizes version detection, and limits the places that “know” about the backend, while preserving your v1/v2 behavior and fallback.
2. Remove unused variable in update_creatures
kill_ids is currently unused and just adds noise:
def update_creatures(self, step):
for c in self.creatures:
c['position'][0] = max(0, min(30, c['position'][0] + c['velocity'][0] + random.uniform(-0.1, 0.1)))
c['position'][1] = max(0, min(30, c['position'][1] + c['velocity'][1] + random.uniform(-0.1, 0.1)))
c['age'] += 1
for i in range(len(self.creatures)):
for j in range(i + 1, len(self.creatures)):
c1, c2 = self.creatures[i], self.creatures[j]
if c1.get('_dead') or c2.get('_dead'):
continue
dx = c1['position'][0] - c2['position'][0]
dy = c1['position'][1] - c2['position'][1]
if (dx * dx + dy * dy) < 4.0:
self.handle_creature_interaction(c1, c2, step)
# apply deferred removals
self.creatures = [c for c in self.creatures if not c.get('_dead')]The _dead flag + deferred removal pattern is fine and clear as-is; the main win is just removing the unused variable and consolidating scenedetect logic.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for the scenedetect v2 API with a fallback to v1, implements deferred removal of dead creatures to prevent modification of lists during iteration, and schedules temporary directory cleanup in the Gradio UI. The review feedback highlights several critical issues: importing open_video at the top level breaks the v1 fallback on older versions of scenedetect, registering atexit handlers inside the request handler causes a memory leak in persistent Gradio servers, and using .index() on dictionaries is inefficient and risky. Additionally, the feedback points out an unused variable kill_ids and redundant casting and clipping operations in the image processing functions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| from scenedetect import open_video as sd_open_video | ||
| # try new v2 API | ||
| try: | ||
| from scenedetect import SceneManager | ||
| from scenedetect.backends import OpencvVideoStream | ||
| SCENEDETECT_V2 = True | ||
| except ImportError: | ||
| from scenedetect import detect as sd_detect | ||
| SCENEDETECT_V2 = False |
There was a problem hiding this comment.
Importing open_video at the top level of the try block will raise an ImportError on older versions of scenedetect (such as v0.5) where open_video does not exist. This will cause the outer except ImportError block to trigger, setting SCENEDETECT_AVAILABLE = False and completely disabling the v1 API fallback. Moving the open_video import inside the inner try block ensures that the v1 fallback remains functional when v2 is unavailable.
| from scenedetect import open_video as sd_open_video | |
| # try new v2 API | |
| try: | |
| from scenedetect import SceneManager | |
| from scenedetect.backends import OpencvVideoStream | |
| SCENEDETECT_V2 = True | |
| except ImportError: | |
| from scenedetect import detect as sd_detect | |
| SCENEDETECT_V2 = False | |
| # try new v2 API | |
| try: | |
| from scenedetect import SceneManager | |
| from scenedetect.backends import OpencvVideoStream | |
| from scenedetect import open_video as sd_open_video | |
| SCENEDETECT_V2 = True | |
| except ImportError: | |
| from scenedetect import detect as sd_detect | |
| SCENEDETECT_V2 = False |
| import os | ||
| import shutil | ||
| import tempfile | ||
| from .core import OrithetCore |
There was a problem hiding this comment.
To prevent memory leaks from registering a new atexit handler on every request, we can define a global list of temporary directories to clean up and register a single atexit handler once at startup.
import os
import shutil
import tempfile
import atexit
from .core import OrithetCore
_TEMP_DIRS_TO_CLEAN = []
def _cleanup_temp_dirs():
for d in _TEMP_DIRS_TO_CLEAN:
shutil.rmtree(d, ignore_errors=True)
atexit.register(_cleanup_temp_dirs)| # Schedule temp dir cleanup — on return, register for later removal | ||
| import atexit | ||
| atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True)) |
There was a problem hiding this comment.
Registering a new atexit handler on every request creates a memory leak because the handlers accumulate in a global list for the lifetime of the persistent Gradio server. Instead, append the directory to the global _TEMP_DIRS_TO_CLEAN list, which is processed by a single registered atexit handler.
| # Schedule temp dir cleanup — on return, register for later removal | |
| import atexit | |
| atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True)) | |
| # Schedule temp dir cleanup safely without leaking atexit handlers | |
| _TEMP_DIRS_TO_CLEAN.append(temp_dir) |
| def update_creatures(self, step): | ||
| kill_ids = set() | ||
| for c in self.creatures: |
There was a problem hiding this comment.
The kill_ids variable is initialized but never used in update_creatures. It can be safely removed to clean up the code.
| def update_creatures(self, step): | |
| kill_ids = set() | |
| for c in self.creatures: | |
| def update_creatures(self, step): | |
| for c in self.creatures: |
| # replace c1 with fused, mark c2 as dead (deferred removal) | ||
| idx1 = self.creatures.index(c1) | ||
| self.creatures[idx1] = fused | ||
| if c2 in self.creatures: | ||
| self.creatures.remove(c2) | ||
| c2['_dead'] = True |
There was a problem hiding this comment.
Using self.creatures.index(c1) is inefficient (==) on dictionaries. If two creatures happen to have identical attributes, .index() could return the wrong index and overwrite the wrong creature. Mutating c1 in-place using .clear() and .update() is safer, faster, and avoids the need for index lookups entirely.
| # replace c1 with fused, mark c2 as dead (deferred removal) | |
| idx1 = self.creatures.index(c1) | |
| self.creatures[idx1] = fused | |
| if c2 in self.creatures: | |
| self.creatures.remove(c2) | |
| c2['_dead'] = True | |
| # replace c1 with fused, mark c2 as dead (deferred removal) | |
| c1.clear() | |
| c1.update(fused) | |
| c2['_dead'] = True |
| result = frame.copy().astype(np.int16) | ||
| result[:, shift:, 0] = frame[:, :-shift, 0] | ||
| result[:, :-shift, 2] = frame[:, shift:, 2] | ||
| return result | ||
| return np.clip(result, 0, 255).astype(np.uint8) |
There was a problem hiding this comment.
In _rgb_shift, there are no arithmetic operations performed on the pixel values (only slicing and copying). Since the source values are already uint8 (0-255), they cannot overflow or underflow. Casting to np.int16 and calling np.clip is redundant and adds unnecessary performance overhead.
| result = frame.copy().astype(np.int16) | |
| result[:, shift:, 0] = frame[:, :-shift, 0] | |
| result[:, :-shift, 2] = frame[:, shift:, 2] | |
| return result | |
| return np.clip(result, 0, 255).astype(np.uint8) | |
| result = frame.copy() | |
| result[:, shift:, 0] = frame[:, :-shift, 0] | |
| result[:, :-shift, 2] = frame[:, shift:, 2] | |
| return result |
| blended = cv2.addWeighted(frame, 0.75, shifted.astype(np.uint8), 0.25, 0) | ||
| return np.clip(blended, 0, 255).astype(np.uint8) |
There was a problem hiding this comment.
In _datamosh, cv2.addWeighted automatically performs saturation clipping and returns a uint8 array when given uint8 inputs. Therefore, calling np.clip and .astype(np.uint8) on the result is redundant. Additionally, shifted is already uint8 because np.roll preserves the array type, so shifted.astype(np.uint8) is also unnecessary.
| blended = cv2.addWeighted(frame, 0.75, shifted.astype(np.uint8), 0.25, 0) | |
| return np.clip(blended, 0, 255).astype(np.uint8) | |
| return cv2.addWeighted(frame, 0.75, shifted, 0.25, 0) |
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Approve with suggestions
This PR addresses four key bugs: scenedetect API compatibility, color overflow, list mutation, and temp cleanup. The fixes are sound and well-implemented, with minor suggestions for maintainability.
📄 Documentation Diagram
This diagram documents the refactored scene detection flow with dual-API support.
sequenceDiagram
participant Core as OrithetCore
participant SM as SceneManager (v2 API)
participant Detect as detect() (v1 API)
participant Fallback as Fallback (5-sec segments)
Core->>Core: Check SCENEDETECT_AVAILABLE
alt SCENEDETECT_V2=True
Core->>SM: sd_open_video(str(video_path))
Core->>SM: SceneManager() and add_detector(ContentDetector())
SM->>SM: detect_scenes()
SM-->>Core: scene_list
Core->>Core: Return frames from get_scene_list()
else SCENEDETECT_V2=False
Core->>Detect: detect(str(video_path), ContentDetector())
Detect-->>Core: scene_list
Core->>Core: Return frames
else Exception or unavailable
note over Core: This PR added the v2 path and graceful fallback
Core->>Fallback: Use fixed 5-second segments
Fallback-->>Core: list of frame ranges
end
🌟 Strengths
- Solid bug fixes that address real runtime issues (uint8 overflow, list mutation) with safe patterns.
- Graceful fallback for scenedetect failures ensures robustness.
💡 Suggestions
- orithet/core.py: Unnecessary import of
OpencvVideoStreammay cause spurious fallback to fixed segments on valid scenedetect v2 installations; remove the unused import. - orithet/gradio_ui.py:
atexithandlers accumulate on each invocation; consider deduplication or immediate cleanup after serving the file.
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
| try: | ||
| from scenedetect import SceneManager | ||
| from scenedetect.backends import OpencvVideoStream | ||
| SCENEDETECT_V2 = True | ||
| except ImportError: | ||
| from scenedetect import detect as sd_detect | ||
| SCENEDETECT_V2 = False |
There was a problem hiding this comment.
P2 | Confidence: High
The import guard for scenedetect v2 API includes OpencvVideoStream (from scenedetect.backends import OpencvVideoStream), but this symbol is never used in the v2 code path (the code uses sd_open_video from scenedetect directly). If a legitimate scenedetect v2 installation lacks the OpencvVideoStream backend (e.g., an older v2 release or a stripped install), the condition fails, SCENEDETECT_V2 is set to False, and the code falls back to the v1 detect() function. In scenedetect v2 the detect() function may not exist, causing an ImportError at runtime inside detect_scenes’s else branch, which is caught and silently degrades to fixed 5‑second segments. The user loses scene detection without a clear warning. Removing the unnecessary import of OpencvVideoStream would allow the v2 API to be used in all v2 installations. The existing try block should only check for SceneManager.
| try: | |
| from scenedetect import SceneManager | |
| from scenedetect.backends import OpencvVideoStream | |
| SCENEDETECT_V2 = True | |
| except ImportError: | |
| from scenedetect import detect as sd_detect | |
| SCENEDETECT_V2 = False | |
| try: | |
| from scenedetect import SceneManager | |
| SCENEDETECT_V2 = True | |
| except ImportError: | |
| from scenedetect import detect as sd_detect | |
| SCENEDETECT_V2 = False |
Evidence: search:OpencvVideoStream
| # Schedule temp dir cleanup — on return, register for later removal | ||
| import atexit | ||
| atexit.register(lambda: shutil.rmtree(temp_dir, ignore_errors=True)) |
There was a problem hiding this comment.
P2 | Confidence: High
Each successful call to process_video registers a new atexit handler capturing the current temp_dir lambda. Over time, these handlers accumulate (one per invocation). While ignore_errors=True prevents failures, this pattern wastes resources and can cause confusion if multiple temp directories are removed during shutdown (order is LIFO, but the latest handler’s temp_dir may have been cleaned earlier). A more robust approach is to use a finally block or a Gradio close event to clean up immediately after the file is served. The PR description acknowledges this tradeoff. For a low-traffic tool this is acceptable, but it should be flagged for future improvement.
Code Suggestion:
# Alternative: deduplicate handlers
import atexit
_temp_dirs = set()
@atexit.register
def _cleanup_temp_dirs():
for d in list(_temp_dirs):
shutil.rmtree(d, ignore_errors=True)
# In process_video: _temp_dirs.add(temp_dir)1 participant
PR Summary by Qodo
fix: scenedetect API compat, color overflow, list mutation, temp cleanup
🐞 Bug fix🕐 10-20 MinutesWalkthroughs
User Description
changes
🐛 bug fixes
detect()) and v2 API (SceneManager). Falls back to 5-second segments on any failure._rgb_shiftand_datamoshnow usenp.clip(0, 255).astype(np.uint8)— shifted pixel values could previously overflow uint8, causing visual artifacts.update_creaturesno longer modifiesself.creatureswhile iterating. Uses a deferred_deadflag + post-loop filter.atexithandler to clean up temporary directories.🧹 cleanup
import shutiland ensuredcv2.VideoCapturealways receives a string path.AI Description
Diagram
graph TD A["gradio_ui.py"] -->|"calls"| B["OrithetCore"] B --> C["detect_scenes()"] C -->|"SCENEDETECT_V2=True"| D["SceneManager v2 API"] C -->|"SCENEDETECT_V2=False"| E["detect() v1 API"] C -->|"exception / unavailable"| F["5-sec fallback segments"] B --> G["_rgb_shift / _datamosh"] G -->|"int16 cast + np.clip"| H["uint8 frame output"] B --> I["update_creatures()"] I -->|"_dead flag + list filter"| J["safe creature removal"] A -->|"atexit.register"| K["shutil.rmtree temp_dir"] subgraph Legend direction LR _mod["Module"] ~~~ _fn(["Function"]) ~~~ _out{{"Output"}} endHigh-Level Assessment
The following are alternative approaches to this PR:
1. Immediate temp cleanup with try/finally
Recommendation: The PR's approach is sound for all four fixes. One minor concern: registering
atexitinside theprocess_videocallback means a new handler is registered on every invocation, accumulating closures over the process lifetime. A cleaner alternative would be to clean up the temp dir immediately after Gradio finishes serving the file (e.g., using afinallyblock or a Gradiocloseevent), but this is non-trivial with Gradio's async serving model. The atexit approach is a pragmatic tradeoff.File Changes
Bug fix (2)