Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions TODO.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
## Convertify 2.0 - Mejoras pendientes

- [x] 1) main.py: loggear excepciones con logger.exception en vez de `sys.exit(1)` silencioso
- [x] 2) Semántica SKIPPED vs FAILED:
- [x] Ajustar `VideoConversionService` para que “locked/skip” no cuente como FAILED (definir success=True o que main.py discrimine SKIPPED)
- [x] 3) (Siguiente) Timeout configurable para FFmpeg en `MoviePyVideoConverter`
- [x] 4) (Siguiente) Paralelismo real respetando `max_workers`
- [ ] 5) (Siguiente) Optimizar locked-check (psutil fallback/caching)
- [ ] 6) (Siguiente) Centralizar configuración de rutas UNC
- [ ] 7) (Siguiente) Tests adicionales para semántica SKIPPED y timeout
19 changes: 15 additions & 4 deletions main.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

def main():
"""Convert AVI videos to MP4 format."""
container: Container | None = None
try:
dirs = [
Path(r"\\192.168.1.200\Team-design\4. PREPARAR RESUMEN"),
Expand All @@ -33,8 +34,8 @@ def main():
results = use_case.execute(dirs, config)

if results:
successful = sum(1 for r in results if r.success)
failed = sum(1 for r in results if not r.success)
successful = sum(1 for r in results if r.video_file.status.value == "completed")
failed = sum(1 for r in results if r.video_file.status.value == "failed")
total_time = sum(r.duration_seconds for r in results)

logger.info(
Expand All @@ -43,11 +44,21 @@ def main():
)
for r in results:
status = "Success" if r.success else "Failed"
logger.info(f"[{status}] {r.video_file.filename} in {r.duration_seconds:.1f}s")
logger.info(
f"[{status}] {r.video_file.filename} in {r.duration_seconds:.1f}s"
)
else:
logger.info("No AVI files found to convert.")

except Exception:
except Exception as e:
# Ensure background/service failures are logged with traceback
try:
logger = container.logger if container else None
if logger:
logger.exception(f"Convertify crashed: {str(e)}")
except Exception:
# Fallback: no logger available
pass
sys.exit(1)


Expand Down
37 changes: 23 additions & 14 deletions src/application/services/video_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,9 @@ def convert_video(self, video_file: VideoFile, config: ConversionConfig) -> Conv
return ConversionResult(
video_file=video_file,
success=False,
output_path=None,
error_message="File is locked or in use",
duration_seconds=0.0,
)

# Retry logic
Expand All @@ -64,6 +66,12 @@ def convert_video(self, video_file: VideoFile, config: ConversionConfig) -> Conv
video_file.status = ConversionStatus.IN_PROGRESS
result = self.converter.convert(video_file, config)

# Seguridad: loguear el estado real devuelto por el conversor
self.logger.info(
f"Converter returned: filename={video_file.filename}, success={result.success}, "
f"status={video_file.status}, output_path={result.output_path}"
)

if result.success:
video_file.status = ConversionStatus.COMPLETED
self.logger.info(
Expand All @@ -82,21 +90,22 @@ def convert_video(self, video_file: VideoFile, config: ConversionConfig) -> Conv
)

return result

# Si no fue exitoso: NO marcar como success y continuar reintentos
last_error = result.error_message
if attempt < config.max_retries - 1:
self.logger.warning(
f"Conversion failed (attempt {attempt + 1}/{config.max_retries}): "
f"{video_file.filename} - {result.error_message}"
)
time.sleep(config.retry_delay_seconds)
else:
last_error = result.error_message
if attempt < config.max_retries - 1:
self.logger.warning(
f"Conversion failed (attempt {attempt + 1}/{config.max_retries}): "
f"{video_file.filename} - {result.error_message}"
)
time.sleep(config.retry_delay_seconds)
else:
video_file.status = ConversionStatus.FAILED
self.logger.error(
f"Conversion failed after {config.max_retries} attempts: "
f"{video_file.filename}"
)
return result
video_file.status = ConversionStatus.FAILED
self.logger.error(
f"Conversion failed after {config.max_retries} attempts: "
f"{video_file.filename} - {result.error_message}"
)
return result

except Exception as e:
last_error = str(e)
Expand Down
49 changes: 37 additions & 12 deletions src/application/use_cases/convert_videos.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,18 +68,43 @@ def execute(
results: list[ConversionResult] = []
total = len(video_files)

# Use ProcessPoolExecutor for CPU-bound video conversion
# Note: In practice, moviepy is CPU-intensive, so process pool is better
# However, for simplicity and to avoid pickling issues, we'll use sequential
# processing here. For true parallel processing, you'd need to refactor
# the converter to be pickle-able or use a different approach.

for idx, video_file in enumerate(video_files, start=1):
result = self.video_service.convert_video(video_file, config)
results.append(result)

if progress_callback:
progress_callback(idx, total)
# Parallel conversion (safe because each task runs an external ffmpeg subprocess
# and we don't share mutable state in the worker beyond read-only services).
#
# Note: We intentionally use ThreadPoolExecutor (not ProcessPoolExecutor) because
# movie/ffmpeg invocation is largely I/O + subprocess-bound and ProcessPool would
# require picklable callables/objects (often problematic on Windows).
from concurrent.futures import ThreadPoolExecutor, as_completed

max_workers = max(1, self.max_workers)
with ThreadPoolExecutor(max_workers=max_workers) as executor:
futures = {
executor.submit(self.video_service.convert_video, video_file, config): idx
for idx, video_file in enumerate(video_files, start=1)
}

# Preserve deterministic ordering in `results` by collecting with index
ordered: dict[int, ConversionResult] = {}

for future in as_completed(futures):
idx = futures[future]
try:
ordered[idx] = future.result()
except Exception as e:
# Should not happen often (VideoConversionService already catches),
# but ensure one task failure doesn't stop the batch.
video_file = video_files[idx - 1]
ordered[idx] = ConversionResult(
video_file=video_file,
success=False,
error_message=f"Unexpected conversion error: {e}",
duration_seconds=0.0,
)

if progress_callback:
progress_callback(idx, total)

results = [ordered[i] for i in range(1, total + 1)]

# Summary
successful = sum(1 for r in results if r.success)
Expand Down
3 changes: 2 additions & 1 deletion src/container.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ def video_converter(self):
def convert_videos_use_case(self):
"""Get convert videos use case instance (singleton)."""
if self._convert_videos_use_case is None:
# Modo A (producción): sin paralelismo para evitar issues con UNC/locks/transitorios.
self._convert_videos_use_case = ConvertVideosUseCase(
converter=self.video_converter,
file_repository=self.file_repository,
logger=self.logger,
max_workers=4,
max_workers=1,
)
return self._convert_videos_use_case

Expand Down
3 changes: 3 additions & 0 deletions src/domain/entities.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ class ConversionConfig:
skip_if_exists: bool = True
max_retries: int = 3
retry_delay_seconds: float = 1.0
ffmpeg_timeout_seconds: float = 0.0 # 0/<=0 means no timeout

def __post_init__(self) -> None:
"""Validate configuration values."""
Expand All @@ -108,3 +109,5 @@ def __post_init__(self) -> None:
raise ValueError("max_retries must be non-negative")
if self.retry_delay_seconds < 0:
raise ValueError("retry_delay_seconds must be non-negative")
if self.ffmpeg_timeout_seconds < 0:
raise ValueError("ffmpeg_timeout_seconds must be non-negative")
24 changes: 20 additions & 4 deletions src/infrastructure/file_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,26 @@ def find_avi_files(self, directory: Path, recursive: bool = True) -> list[Path]:

# Perform the scan
try:
for file_path in directory.rglob("*.avi"):
# Skip hidden directories (starting with .)
if not any(part.startswith(".") for part in file_path.parts):
avi_files.append(file_path)
if use_smart_scan:
# If cache tells us which subdirs to scan, only scan those.
# This avoids doing directory-wide rglob over very large trees.
if not dirs_to_scan:
return []

# If cache indicates we need a full scan (e.g., first time / stale cache),
# fall back to scanning the whole directory.
if directory in dirs_to_scan or len(dirs_to_scan) == 1:
scan_roots = [directory]
Comment on lines +139 to +140

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Scan the singleton cached subdirectory instead of the root

When the cache reports exactly one new or modified subdirectory, len(dirs_to_scan) == 1 makes this fall back to scan_roots = [directory], so a common incremental case still performs a full rglob over the entire UNC tree. DirectoryCache.get_directories_to_scan() already uses {root_dir} to request a full scan and can return a singleton {subdir} for one changed folder, so the extra length check defeats the intended cache optimization.

Useful? React with 👍 / 👎.

else:
scan_roots = list(dirs_to_scan)
else:
scan_roots = [directory]

for scan_root in scan_roots:
for file_path in scan_root.rglob("*.avi"):
# Skip hidden directories (starting with .)
if not any(part.startswith(".") for part in file_path.parts):
avi_files.append(file_path)

# Update cache after successful scan
if use_smart_scan:
Expand Down
22 changes: 20 additions & 2 deletions src/infrastructure/video_converter.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,26 @@ def convert(self, video_file: VideoFile, config: ConversionConfig) -> Conversion
cmd.insert(1, "-threads")
cmd.insert(2, str(config.threads))

# Run ffmpeg conversion
result = subprocess.run(cmd, capture_output=True, text=True, check=False)
# Run ffmpeg conversion (with optional timeout)
run_kwargs: dict = {
"capture_output": True,
"text": True,
"check": False,
}
if config.ffmpeg_timeout_seconds and config.ffmpeg_timeout_seconds > 0:
run_kwargs["timeout"] = config.ffmpeg_timeout_seconds

try:
result = subprocess.run(cmd, **run_kwargs)
except subprocess.TimeoutExpired:
return ConversionResult(
Comment on lines +104 to +105

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove partial outputs after FFmpeg timeouts

When ffmpeg_timeout_seconds is configured and FFmpeg has already created output_path, this timeout path returns without deleting the partial MP4. The retry loop then calls convert() again, and the existing-file check at the top treats any existing output as a successful skip; with the default remove_source=True, that can mark a timed-out conversion successful and delete the original AVI while leaving a truncated MP4.

Useful? React with 👍 / 👎.

video_file=video_file,
success=False,
error_message=(
f"FFmpeg timeout after {config.ffmpeg_timeout_seconds}s"
),
duration_seconds=time.time() - start_time,
)

if result.returncode != 0:
error_msg = f"FFmpeg error: {result.stderr}"
Expand Down