fix: filenotfound logs to appear#160
Conversation
📝 WalkthroughWalkthroughEnhanced error handling in FFmpeg rendering pipeline by adding subprocess output buffering, return code validation, and granular error detection. Changes include explicit failure logging in MediaRenderer during concatenation and retryable error paths in RenderIntervalThread with support for corrupted interval recovery. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unsilence/lib/render_media/RenderIntervalThread.py (1)
103-127: Robust returncode-based error handling, but filter error check may be unreachable or redundant.The new error handling logic with returncode checking and recursive retry for corrupted intervals is well-structured. The defensive decoding with
errors='replace'is good practice.However, the check at lines 124-125 for
"Error initializing complex filter"now only executes whenreturncode == 0. Since FFmpeg would typically return a non-zero exit code for filter initialization failures, this check may never trigger—those cases would now hit theRuntimeErrorat line 122 instead.Consider whether this check is still needed, or if it should be integrated into the non-zero returncode branch:
🔎 Suggested consolidation
if console_output.returncode != 0: if "Conversion failed!" in output_str: if drop_corrupted_intervals: return False if apply_filter: return self.__render_interval( interval_output_file, interval, apply_filter=False, drop_corrupted_intervals=drop_corrupted_intervals, minimum_interval_duration=minimum_interval_duration ) else: raise IOError(f"Input file is corrupted between {interval.start} and {interval.end} (in seconds)") - + + if "Error initializing complex filter" in output_str: + raise ValueError("Invalid render options") + print(f"FFmpeg Error during interval rendering: {command}") print(output_str) raise RuntimeError(f"FFmpeg failed with return code {console_output.returncode}") - - if "Error initializing complex filter" in output_str: - raise ValueError("Invalid render options") return Trueunsilence/lib/render_media/MediaRenderer.py (1)
184-189: Error handling implementation looks good.The return code validation and error reporting improve debugging significantly. The subprocess output is now preserved and displayed when concatenation fails.
Optional: Consider using a logging framework instead of
print().For library code, using Python's
loggingmodule would be more appropriate thanprint(), as it allows users to control log output through their own logging configuration.🔎 Optional refactor using logging
At the top of the file, add the logging import:
+import logging import queueThen replace the print statements:
+ logger = logging.getLogger(__name__) console_output.wait() if console_output.returncode != 0: - print("FFMPEG Error during concatenation:") - print("".join(output_log)) + logger.error("FFMPEG Error during concatenation:\n%s", "".join(output_log)) raise RuntimeError("FFmpeg concatenation failed")Note on static analysis hint: The Ruff hint (TRY003) suggests defining custom exception classes for domain-specific errors. While the current message is concise enough, you could optionally create a
FFmpegConcatenationErrorexception class if you want more specific error handling elsewhere in the codebase.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
unsilence/lib/render_media/MediaRenderer.pyunsilence/lib/render_media/RenderIntervalThread.py
🧰 Additional context used
🧬 Code graph analysis (1)
unsilence/lib/render_media/RenderIntervalThread.py (2)
unsilence/command_line/EntryPoint.py (1)
run(29-115)unsilence/lib/intervals/Interval.py (4)
start(19-24)start(27-34)end(37-42)end(45-52)
🪛 Ruff (0.14.10)
unsilence/lib/render_media/MediaRenderer.py
189-189: Avoid specifying long messages outside the exception class
(TRY003)
unsilence/lib/render_media/RenderIntervalThread.py
58-58: subprocess call: check for execution of untrusted input
(S603)
59-63: Starting a process with a partial executable path
(S607)
68-68: Do not catch blind exception: Exception
(BLE001)
118-118: Avoid specifying long messages outside the exception class
(TRY003)
122-122: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
unsilence/lib/render_media/RenderIntervalThread.py (1)
49-70: Good error isolation for worker thread.The try/except pattern correctly prevents individual task failures from crashing the worker thread. The ffprobe validation is a sensible addition for interval checking.
One minor note: the broad
Exceptioncatch (flagged by static analysis) is acceptable here since the worker must remain resilient. However, you could consider logging the exception type for better diagnostics:except Exception as e: print(f"Thread {self.thread_id} failed on task {task.task_id}: {type(e).__name__}: {e}") completed = Falseunsilence/lib/render_media/MediaRenderer.py (1)
177-179: Good addition of output buffering for error reporting.Collecting FFmpeg output enables informative error messages when concatenation fails. This directly addresses the PR's goal of improving log visibility.
1 participant
Resolves: #148
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.