diff --git a/TODO.md b/TODO.md new file mode 100644 index 0000000..79a37ef --- /dev/null +++ b/TODO.md @@ -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 diff --git a/main.py b/main.py index ff6c9dd..c82ceca 100644 --- a/main.py +++ b/main.py @@ -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"), @@ -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( @@ -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) diff --git a/src/application/services/video_service.py b/src/application/services/video_service.py index 02cacf2..c9c3dac 100644 --- a/src/application/services/video_service.py +++ b/src/application/services/video_service.py @@ -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 @@ -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( @@ -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) diff --git a/src/application/use_cases/convert_videos.py b/src/application/use_cases/convert_videos.py index 837e323..af2f478 100644 --- a/src/application/use_cases/convert_videos.py +++ b/src/application/use_cases/convert_videos.py @@ -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) diff --git a/src/container.py b/src/container.py index d8147b4..9868bea 100644 --- a/src/container.py +++ b/src/container.py @@ -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 diff --git a/src/domain/entities.py b/src/domain/entities.py index f3f155f..b98ba78 100644 --- a/src/domain/entities.py +++ b/src/domain/entities.py @@ -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.""" @@ -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") diff --git a/src/infrastructure/file_repository.py b/src/infrastructure/file_repository.py index 0fc9a00..9f214a6 100644 --- a/src/infrastructure/file_repository.py +++ b/src/infrastructure/file_repository.py @@ -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] + 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: diff --git a/src/infrastructure/video_converter.py b/src/infrastructure/video_converter.py index 85e04c5..3a34eef 100644 --- a/src/infrastructure/video_converter.py +++ b/src/infrastructure/video_converter.py @@ -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( + 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}"