Add transition extraction and Crossfade#8
Conversation
Reviewer's GuideIntroduces a new transition extraction module that slices audio around transition boundaries, applies configurable crossfades, and outputs a final MP3 clip using ffmpeg utilities. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new module Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Runner as run_local_test.py
participant TE as transition_extraction.py
participant FFPROBE as ffprobe
participant FFMPEG as ffmpeg
participant FS as FileSystem
User->>Runner: main()
Runner->>TE: extract_segments(prev,mix,next,boundary)
TE->>FS: ensure out/transitions exists
TE->>FFPROBE: probe durations
FFPROBE-->>TE: durations
rect rgba(200,230,255,0.18)
note over TE: extract_segments cuts three clips
TE->>FFMPEG: cut prev_tail.wav, mix_mid.wav, next_head.wav
FFMPEG-->>FS: write WAV clips
end
rect rgba(220,255,220,0.12)
note over TE: first crossfade
TE->>TE: safe_crossfade_duration(...)
TE->>FFMPEG: acrossfade -> step1.wav
FFMPEG-->>FS: write step1.wav
end
rect rgba(255,240,200,0.12)
note over TE: final stitch to MP3
TE->>TE: safe_crossfade_duration(...)
TE->>FFMPEG: acrossfade + encode MP3 (libmp3lame 192k)
FFMPEG-->>FS: transition_demo.mp3
end
Runner-->>User: paths to generated files
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used🪛 Ruff (0.13.1)transitions/transition_extraction.py43-43: (S603) 44-44: Unused Remove unused (RUF100) 52-52: Unused Remove unused (RUF100) 59-59: (S603) 59-64: Starting a process with a partial executable path (S607) 70-70: Avoid specifying long messages outside the exception class (TRY003) 96-96: Avoid specifying long messages outside the exception class (TRY003) 100-100: Avoid specifying long messages outside the exception class (TRY003) 152-152: Comment contains ambiguous (RUF003) 168-168: Avoid specifying long messages outside the exception class (TRY003) 170-170: Avoid specifying long messages outside the exception class (TRY003) 🪛 Flake8 (7.2.0)transitions/transition_extraction.py[error] 3-3: block comment should start with '# ' (E265) [error] 6-6: block comment should start with '# ' (E265) [error] 10-10: block comment should start with '# ' (E265) [error] 11-11: block comment should start with '# ' (E265) [error] 12-12: block comment should start with '# ' (E265) [error] 18-18: block comment should start with '# ' (E265) [error] 26-26: block comment should start with '# ' (E265) [error] 83-83: block comment should start with '# ' (E265) [error] 84-84: block comment should start with '# ' (E265) [error] 85-85: block comment should start with '# ' (E265) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- Extract the
main()function and any hard-coded test paths into a separate script or entry point so this module only contains reusable logic. - Replace ad-hoc print statements for missing files and ffmpeg warnings with a consistent logging or exception strategy to let callers handle errors robustly.
- Parameterize hard-coded paths like
OUT_DIRand test file locations through function arguments or configuration to make the module more flexible across environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Extract the `main()` function and any hard-coded test paths into a separate script or entry point so this module only contains reusable logic.
- Replace ad-hoc print statements for missing files and ffmpeg warnings with a consistent logging or exception strategy to let callers handle errors robustly.
- Parameterize hard-coded paths like `OUT_DIR` and test file locations through function arguments or configuration to make the module more flexible across environments.
## Individual Comments
### Comment 1
<location> `transitions/transition_extraction.py:32-41` </location>
<code_context>
+ #print("→", " ".join(args))
+ subprocess.run(list(args), check=True)
+
+def duration(path: str) -> float:
+ """Return audio duration in seconds using ffprobe."""
+ out = subprocess.check_output([
+ "ffprobe", "-v", "error",
+ "-show_entries", "format=duration",
+ "-of", "default=noprint_wrappers=1:nokey=1",
+ path
+ ])
+
+ # Error Handling
+ try:
+ return float(out.strip())
+ except ValueError as e:
+ raise RuntimeError(f"Could not decode ffprobe output for {path}: {out!r}") from e
</code_context>
<issue_to_address>
**nitpick:** The final return statement after the exception block is unreachable.
The final 'return float(out.strip())' is redundant and should be removed.
</issue_to_address>
### Comment 2
<location> `transitions/transition_extraction.py:143-148` </location>
<code_context>
+ # Use helper to determine best crossfade
+
+ # Check for existence of input files
+ if not a_path.exists():
+ print(f"Input file does not exist: {a_path}")
+ return
+ if not b_path.exists():
+ print(f"Input file does not exist: {b_path}")
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning early on missing input files may mask errors downstream.
Raising an exception for missing input files will ensure errors are properly surfaced and handled throughout the workflow.
```suggestion
if not a_path.exists():
raise FileNotFoundError(f"Input file does not exist: {a_path}")
if not b_path.exists():
raise FileNotFoundError(f"Input file does not exist: {b_path}")
```
</issue_to_address>
### Comment 3
<location> `transitions/transition_extraction.py:30` </location>
<code_context>
subprocess.run(list(args), check=True)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
transitions/transition_extraction.py (2)
77-80: Avoid clobbering segment outputs between runsLine 77 hardcodes the filenames in
out/transitions, so each call overwrites the previous run. That makes it unsafe to process multiple boundaries in one session or run jobs in parallel. Please bake in a unique prefix (e.g., derived from track name + boundary) or allow the caller to provide output paths so each invocation produces distinct artifacts.- p_tail = OUT_DIR / "prev_tail.wav" - m_mid = OUT_DIR / "mix_mid.wav" - n_head = OUT_DIR / "next_head.wav" + prefix = f"{Path(mix_path).stem}_{boundary_s:.3f}" + p_tail = OUT_DIR / f"{prefix}_prev_tail.wav" + m_mid = OUT_DIR / f"{prefix}_mix_mid.wav" + n_head = OUT_DIR / f"{prefix}_next_head.wav"
46-46: Remove the unreachable returnLine 46 repeats the same
return float(out.strip())that already executed inside thetry, so it will never run. Please drop the redundant return.- return float(out.strip())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
transitions/transition_extraction.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
transitions/transition_extraction.py
30-30: subprocess call: check for execution of untrusted input
(S603)
34-34: subprocess call: check for execution of untrusted input
(S603)
34-39: Starting a process with a partial executable path
(S607)
45-45: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Avoid specifying long messages outside the exception class
(TRY003)
75-75: Avoid specifying long messages outside the exception class
(TRY003)
127-127: Comment contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Sourcery review
- GitHub Check: Sourcery review
| def safe_crossfade_duration(a_path: Path, b_path: Path, desired_cross: float) -> float: | ||
|
|
||
| # Returns a safe crossfade duration that won't exceed the length of either audio file. | ||
|
|
||
| len_a = duration(str(a_path)) | ||
| len_b = duration(str(b_path)) | ||
|
|
||
| # pick the shortest file’s length * 0.8 to stay safe | ||
| max_possible = min(len_a, len_b) * 0.8 | ||
| if desired_cross > max_possible: | ||
| print(f"Crossfade {desired_cross:.2f}s too long for clip pair " | ||
| f"({len_a:.2f}s, {len_b:.2f}s). Using {max_possible:.2f}s instead.") | ||
| return max_possible | ||
| return desired_cross |
There was a problem hiding this comment.
Enforce positive crossfade durations
Line 120 currently only clamps the upper bound. If the caller passes cross=0 or the shortest clip ends up zero-length (e.g., pre_tail=0 in extract_segments), safe_crossfade_duration returns 0 and ffmpeg’s acrossfade rejects d<=0, raising CalledProcessError. Guard against non-positive durations before spawning ffmpeg.
@@
- max_possible = min(len_a, len_b) * 0.8
- if desired_cross > max_possible:
+ max_possible = min(len_a, len_b) * 0.8
+ if desired_cross <= 0:
+ raise ValueError(f"Crossfade duration must be positive (got {desired_cross}).")
+ if max_possible <= 0:
+ raise ValueError(f"Cannot crossfade clips with no audio ({len_a:.2f}s, {len_b:.2f}s).")
+ if desired_cross > max_possible:
print(f"Crossfade {desired_cross:.2f}s too long for clip pair "
f"({len_a:.2f}s, {len_b:.2f}s). Using {max_possible:.2f}s instead.")
return max_possible📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def safe_crossfade_duration(a_path: Path, b_path: Path, desired_cross: float) -> float: | |
| # Returns a safe crossfade duration that won't exceed the length of either audio file. | |
| len_a = duration(str(a_path)) | |
| len_b = duration(str(b_path)) | |
| # pick the shortest file’s length * 0.8 to stay safe | |
| max_possible = min(len_a, len_b) * 0.8 | |
| if desired_cross > max_possible: | |
| print(f"Crossfade {desired_cross:.2f}s too long for clip pair " | |
| f"({len_a:.2f}s, {len_b:.2f}s). Using {max_possible:.2f}s instead.") | |
| return max_possible | |
| return desired_cross | |
| def safe_crossfade_duration(a_path: Path, b_path: Path, desired_cross: float) -> float: | |
| # Returns a safe crossfade duration that won't exceed the length of either audio file. | |
| len_a = duration(str(a_path)) | |
| len_b = duration(str(b_path)) | |
| # pick the shortest file’s length * 0.8 to stay safe | |
| max_possible = min(len_a, len_b) * 0.8 | |
| if desired_cross <= 0: | |
| raise ValueError(f"Crossfade duration must be positive (got {desired_cross}).") | |
| if max_possible <= 0: | |
| raise ValueError(f"Cannot crossfade clips with no audio ({len_a:.2f}s, {len_b:.2f}s).") | |
| if desired_cross > max_possible: | |
| print(f"Crossfade {desired_cross:.2f}s too long for clip pair " | |
| f"({len_a:.2f}s, {len_b:.2f}s). Using {max_possible:.2f}s instead.") | |
| return max_possible | |
| return desired_cross |
🧰 Tools
🪛 Ruff (0.13.1)
127-127: Comment contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
1 participant
Summary by Sourcery
Add a Python module to extract and crossfade transition clips from DJ mixes by slicing adjacent track segments and stitching them into a final MP3 file
New Features:
Summary by CodeRabbit