feat: add adaptive frame extraction to best-frame procedure - #1350
Conversation
Closes #1349 Implements dynamic frame extraction that adapts to video duration: **Round 1 Improvements:** - Calculate adaptive FPS based on video duration (targets 20-50 frames) - Bound FPS between 0.1-2.0 fps to avoid extremes - Add minimum 10-frame guarantee with automatic re-extraction **Round 2 Improvements:** - Adaptive window sizing: ±0.5s for videos <10s, ±1.0s for longer videos - Adaptive FPS: 20fps for short videos, 10fps for longer videos - Fix winner timestamp calculation to use actual frame interval **Benefits:** - Short videos (5-10s): Adequate coverage without missing key moments - Long videos (5+ min): Efficient extraction without hundreds of frames - Consistent ~30 frame target across medium-length videos **Testing:** Added test_adaptive_fps.sh to validate FPS calculations across various video durations (3s to 1 hour).
|
⏳ Code review in progress. Analyzing for code quality issues and best practices. Detailed findings will be posted upon completion. Using Amazon Q Developer for GitHubAmazon Q Developer1 is an AI-powered assistant that integrates directly into your GitHub workflow, enhancing your development process with intelligent features for code development, review, and transformation. Slash Commands
FeaturesAgentic Chat Code Review CustomizationYou can create project-specific rules for Amazon Q Developer to follow:
Example rule: FeedbackTo provide feedback on Amazon Q Developer, create an issue in the Amazon Q Developer public repository. For more detailed information, visit the Amazon Q for GitHub documentation. Footnotes
|
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall Assessment
This PR implements a well-thought-out adaptive frame extraction system that addresses real problems with video processing across different durations. The core concept is solid and the implementation shows good understanding of the domain requirements.
Critical Issues to Address
Logic Inconsistencies: The most critical issue is the mismatch between the test script's validation logic and the actual procedure implementation. The test script checks EXPECTED_FRAMES while the procedure correctly checks FRAME_COUNT, leading to potentially misleading test results.
Error Handling: Missing validation for ffprobe output and potential division by zero scenarios could cause script failures with corrupted or edge-case video files.
Positive Aspects
- Adaptive Design: The dual-round approach with different parameters for short vs long videos is well-designed
- Practical Bounds: FPS capping between 0.1-2.0 prevents extreme extraction scenarios
- Comprehensive Testing: Good coverage of different video duration scenarios in the test script
Recommendations
- Fix the test script to accurately reflect the actual procedure logic
- Add proper error handling for
ffprobeand duration validation - Consider making
TARGET_FRAMESconfigurable or document the fixed choice - Correct the documentation inconsistencies in the test script comments
The implementation shows strong engineering thinking, but these issues should be resolved before merging to ensure reliability and maintainability.
|
|
||
| # Test various video durations | ||
| echo "VERY SHORT VIDEOS:" | ||
| test_duration 3 # 3 second clip |
There was a problem hiding this comment.
🛑 Logic Error: The minimum frame guarantee logic has a critical flaw. The condition checks if EXPECTED_FRAMES < 10, but EXPECTED_FRAMES is calculated from the already-capped FPS value, not the actual frame count from ffmpeg extraction. This means the re-extraction logic may never trigger even when fewer than 10 frames are actually extracted, especially for edge cases where ffmpeg produces fewer frames than mathematically expected.
| Cap FPS between reasonable bounds (0.1 to 2.0 fps): | ||
| !if (( $(echo "$FPS > 2.0" | bc -l) )); then FPS=2.0; fi |
There was a problem hiding this comment.
Error Handling: Missing error handling for the ffprobe command. If the video file is corrupted or ffprobe fails, DURATION will be empty, causing the subsequent bc calculations to fail with cryptic errors. This could lead to unexpected behavior or script crashes.
| Cap FPS between reasonable bounds (0.1 to 2.0 fps): | |
| !if (( $(echo "$FPS > 2.0" | bc -l) )); then FPS=2.0; fi | |
| DURATION=$(ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 "$VIDEO_PATH") | |
| if [ -z "$DURATION" ] || [ "$DURATION" = "N/A" ]; then | |
| echo "Error: Could not determine video duration. File may be corrupted or invalid." | |
| exit 1 | |
| fi | |
| echo "Video duration: ${DURATION}s" |
| !if (( $(echo "$FPS > 2.0" | bc -l) )); then FPS=2.0; fi | ||
| !if (( $(echo "$FPS < 0.1" | bc -l) )); then FPS=0.1; fi | ||
| !FRAME_INTERVAL=$(echo "scale=2; 1 / $FPS" | bc) | ||
| !echo "Using adaptive FPS: $FPS (1 frame every ${FRAME_INTERVAL}s)" |
There was a problem hiding this comment.
Performance Issue: The TARGET_FRAMES=30 is hardcoded but the PR description mentions "targets 20-50 frames". Consider making this configurable or at least document why 30 was chosen as the fixed target when the range suggests it should be adaptive.
| !fi | ||
|
|
||
| ## Step 3: Tournament Selection Using Claude |
There was a problem hiding this comment.
Logic Error: The minimum frame guarantee check uses FRAME_COUNT (actual extracted frames) but the test script uses EXPECTED_FRAMES (calculated frames). This inconsistency means the test script doesn't accurately validate the real procedure logic. The procedure logic is correct, but the test is misleading.
| !fi | |
| ## Step 3: Tournament Selection Using Claude | |
| if [ "$FRAME_COUNT" -lt "$MIN_FRAMES" ]; then | |
| echo "Warning: Only $FRAME_COUNT frames extracted. Re-extracting with higher FPS for better coverage..." | |
| ffmpeg -i "$VIDEO_PATH" -vf "fps=2.0" -q:v 2 "$FRAMES_DIR/frame_%04d.jpg" -loglevel error -y | |
| FRAME_COUNT=$(ls -1 "$FRAMES_DIR"/frame_*.jpg 2>/dev/null | wc -l) | |
| echo "Re-extracted $FRAME_COUNT frames for analysis" | |
| fi |
|
|
||
| Extract frames at adaptive intervals: | ||
| !ffmpeg -i "$VIDEO_PATH" -vf "fps=$FPS" -q:v 2 "$FRAMES_DIR/frame_%04d.jpg" -loglevel error | ||
|
|
There was a problem hiding this comment.
Potential Division by Zero: If DURATION is 0 or very close to 0, the FPS calculation $TARGET_FRAMES / $DURATION could result in division by zero or extremely high values. While the FPS capping helps, it's better to validate duration before calculation.
| Extract frames at adaptive intervals: | |
| !ffmpeg -i "$VIDEO_PATH" -vf "fps=$FPS" -q:v 2 "$FRAMES_DIR/frame_%04d.jpg" -loglevel error | |
| if (( $(echo "$DURATION <= 0" | bc -l) )); then | |
| echo "Error: Invalid video duration: ${DURATION}s" | |
| exit 1 | |
| fi | |
| FPS=$(echo "scale=3; $TARGET_FRAMES / $DURATION" | bc) |
| Extract refined frames around the winner: | ||
| !ffmpeg -ss $START_TIME -i "$VIDEO_PATH" -t $DURATION_R2 -vf "fps=$ROUND2_FPS" -q:v 2 "$FRAMES_DIR/round2/refined_%03d.jpg" -loglevel error | ||
| !echo "Extracted $(ls -1 "$FRAMES_DIR"/round2/*.jpg 2>/dev/null | wc -l) refined frames for Round 2" | ||
|
|
There was a problem hiding this comment.
Edge Case: The START_TIME calculation uses a conditional to prevent negative values, but this could result in a window that's not centered around the winner timestamp for videos shorter than the window size. For very short videos, this might extract frames from the beginning rather than around the actual winner.
| echo "================================================================" | ||
| echo "Test complete! All calculations follow expected behavior." | ||
| echo "" | ||
| echo "Key observations:" |
There was a problem hiding this comment.
Test Coverage Gap: The test script doesn't validate the actual minimum frame guarantee logic that checks FRAME_COUNT vs MIN_FRAMES. It only shows when the guarantee "would trigger" based on calculated frames, not actual extraction results. Consider adding a test that simulates actual frame extraction scenarios.
| echo "================================================================" | ||
| echo "Test complete! All calculations follow expected behavior." | ||
| echo "" | ||
| echo "Key observations:" |
There was a problem hiding this comment.
Documentation Issue: The comment states "Very short videos (<10s) hit minimum FPS of 0.1" but this is incorrect. Very short videos (3-5s) actually hit the maximum FPS of 2.0 due to the TARGET_FRAMES/DURATION calculation. The 0.1 FPS minimum only applies to very long videos (>300s).
1 participant
Summary
Implements adaptive frame extraction for the
extract-best-frameprocedure that adjusts based on video duration, fixing issues with both very short and very long videos.Closes #1349
Changes
Round 1 - Adaptive Initial Extraction
Round 2 - Adaptive Refinement
Testing
test_adaptive_fps.shto validate FPS calculationsExamples
Benefits
Test Plan