Fix/improve thumbnail extraction#8
Conversation
PR Review SummaryThis PR improves the thumbnail extraction process by avoiding potential black frames at the start of videos and introduces environment variable support for Celery configuration. Severity Distribution:
Please see the line-by-line feedback for detailed recommendations. |
![top-dog-reviews[bot]](https://avatars.githubusercontent.com/in/3175285?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have reviewed the changes. The PR correctly addresses the thumbnail extraction issue and improves configuration via environment variables. However, there are potential crash scenarios for short videos and some remaining hardcoded paths that should be addressed for consistency.
| @@ -61,8 +61,8 @@ async def upload_handler(request: Request, file: UploadFile = File(...)): | |||
| media_length = mediainfo.get("format").get("duration") | |||
| thumbnail_path = os.path.join(STATIC_DIR, f"{os.path.splitext(file.filename)[0]}_thumbnail.jpg") | |||
|
|
|||
There was a problem hiding this comment.
✨ good_to_have: You've added STATIC_DIR and UPLOAD_DIR to .env.example, but they are still hardcoded at the top of api.py. For consistency with your changes in celery_worker.py, these should be loaded from environment variables.
# At the top of api.py:
UPLOAD_DIR = os.getenv("UPLOAD_DIR", "uploads")
STATIC_DIR = os.getenv("STATIC_DIR", "static")| ffmpeg.input(file_path, ss=random.randint(0, int(float(media_length)))).filter('scale', 1280, -1).output( | ||
| # Extract thumbnail from a random timestamp (at least 1 second in to avoid black frames) | ||
| ffmpeg.input(file_path, ss=random.randint(1, int(float(media_length)))).filter('scale', 1280, -1).output( | ||
| thumbnail_path, vframes=1).overwrite_output().run() |
There was a problem hiding this comment.
🚧 medium: The use of random.randint(1, int(float(media_length))) will raise a ValueError if the video duration is less than 1 second (e.g., a short GIF or clip), as randint(a, b) requires a <= b.
# Extract thumbnail from a random timestamp (at least 1 second in to avoid black frames, fallback to 0 if short)
ss_timestamp = random.randint(1, int(float(media_length))) if float(media_length) >= 1 else 0
ffmpeg.input(file_path, ss=ss_timestamp).filter('scale', 1280, -1).output(
thumbnail_path, vframes=1).overwrite_output().run()
There was a problem hiding this comment.
I have reviewed the new changes. A major bug has been introduced where the source video file is deleted before the Celery workers can process it. This will cause all transcoding tasks to fail. I have also kept the previous findings in mind regarding thumbnail extraction and configuration.
1 participant
No description provided.