Improve video file check by excluding attached pictures#321
Conversation
Enhanced is_video_file function to exclude attached pictures from video stream checks.
📝 WalkthroughWalkthroughThe ChangesVideo File Detection Enhancement
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/modules/ffmpeg_helper.py`:
- Around line 88-101: The disposition parsing fails because ffprobe was run with
nokey=1 so lines contain only values (e.g. "0"/"1") and the code checks for the
literal "attached_pic". Change the ffprobe invocation to include keys (remove
nokey=1) or request key=value output, then parse key=value pairs: extract
codec_type from the line like "codec_type=video" and find the disposition key
"attached_pic=1" (or "attached_pic=0") and return True only when codec_type ==
"video" and attached_pic != "1"; update the variables used in this block
(result, codec_type, disposition/attached_pic) accordingly so the check is
correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c3fe9784-0192-4eeb-a43a-8b30e426e9ed
📒 Files selected for processing (1)
src/modules/ffmpeg_helper.py
|
|
||
| if result.returncode != 0 or not result.stdout.strip(): | ||
| return False | ||
|
|
||
| # Parse output to check for actual video (not attached picture) | ||
| lines = result.stdout.strip().split('\n') | ||
| if len(lines) < 2: | ||
| return False | ||
|
|
||
| codec_type = lines[0].strip() | ||
| disposition = lines[1].strip() if len(lines) > 1 else "" | ||
|
|
||
| # Return True only if it's video AND not an attached picture | ||
| return codec_type == "video" and "attached_pic" not in disposition |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify ffprobe output format with nokey=1 vs without nokey
# Create a test file (if ffprobe and ffmpeg are available in the sandbox)
# For demonstration, show the command that would reveal the difference:
echo "=== Current command (with nokey=1) ==="
echo "ffprobe -v error -select_streams v:0 -show_entries stream=codec_type,disposition -of default=noprint_wrappers=1:nokey=1 <file>"
echo ""
echo "Expected output format (numeric values only, no keys):"
echo "video"
echo "0"
echo "0"
echo "..."
echo ""
echo "=== Recommended command (without nokey=1) ==="
echo "ffprobe -v error -select_streams v:0 -show_entries stream=codec_type,disposition:stream_disposition=attached_pic -of default=noprint_wrappers=1 <file>"
echo ""
echo "Expected output with keys:"
echo "codec_type=video"
echo "default=0"
echo "..."
echo "attached_pic=1"Repository: rakuri255/UltraSinger
Length of output: 573
🏁 Script executed:
# First, examine the actual source file to understand full context
find . -type f -name "ffmpeg_helper.py" -o -name "*ffmpeg*" | head -20Repository: rakuri255/UltraSinger
Length of output: 96
🏁 Script executed:
# Check if the file exists and read the relevant section
cat -n src/modules/ffmpeg_helper.py | sed -n '75,110p'Repository: rakuri255/UltraSinger
Length of output: 1489
🏁 Script executed:
# Verify ffprobe availability and test actual output formats
which ffprobe ffmpegRepository: rakuri255/UltraSinger
Length of output: 111
🏁 Script executed:
# Test ffprobe output with nokey=1 vs without (if tools available)
if command -v ffprobe &> /dev/null; then
echo "=== Testing ffprobe output formats ==="
# Create a simple test video file if ffmpeg is available
if command -v ffmpeg &> /dev/null; then
# Generate a minimal test video
ffmpeg -f lavfi -i testsrc=s=320x240:d=1 -f lavfi -i sine=f=1000:d=1 -c:v libx264 -c:a aac -shortest /tmp/test_video.mp4 2>/dev/null
if [ -f /tmp/test_video.mp4 ]; then
echo "Test video created successfully"
echo ""
echo "=== Output WITH nokey=1 ==="
ffprobe -v error -select_streams v:0 -show_entries stream=codec_type,disposition -of default=noprint_wrappers=1:nokey=1 /tmp/test_video.mp4 2>/dev/null || echo "Command failed"
echo ""
echo "=== Output WITHOUT nokey=1 ==="
ffprobe -v error -select_streams v:0 -show_entries stream=codec_type,disposition -of default=noprint_wrappers=1 /tmp/test_video.mp4 2>/dev/null || echo "Command failed"
fi
else
echo "ffmpeg not available, cannot create test video"
fi
else
echo "ffprobe not available in sandbox"
fiRepository: rakuri255/UltraSinger
Length of output: 98
🌐 Web query:
ffprobe nokey=1 output format default option documentation
💡 Result:
In ffprobe, the nokey option is a writer-specific parameter used to suppress the printing of field keys in the output [1][2]. To use it with the default output format, you must pass it as an option to the print_format (or -of) flag using the syntax: ffprobe -print_format default=nokey=1 [other_options] input_file [3] Key details regarding this option: 1. Syntax: The option must be attached to the writer name using an equals sign, and multiple options can be chained using colons [3]. For example, to hide both keys and section wrappers, use: ffprobe -print_format default=nokey=1:noprint_wrappers=1 input_file [3] 2. Default Behavior: By default, the nokey option is set to 0 [1][2]. When set to 1, ffprobe will output only the values of the fields rather than the key=value pairs [1][4]. 3. Writer Specifics: While nokey is available for the default writer, it is also commonly used with other writers like csv, where it may have different default behaviors (e.g., it defaults to 1 for the csv writer) [1][2]. Do not confuse this with the -skip_frame nokey option, which is a separate, unrelated flag used to instruct the decoder to skip non-key frames [3][5].
Citations:
- 1: https://roundup.ffmpeg.org/ffprobe.html
- 2: https://www.gsp.com/cgi-bin/man.cgi?section=1&topic=FFPROBE-ALL
- 3: https://stackoverflow.com/questions/14137641/passing-options-to-ffprobe
- 4: https://ffmpeg.org/ffprobe.html
- 5: https://video.stackexchange.com/questions/23631/how-does-ffprobe-identifies-i-frame-and-idr-when-it-sets-key-frame-1
🌐 Web query:
ffprobe disposition output format codec_type with noprint_wrappers nokey option
💡 Result:
To output only the codec_type of a stream using ffprobe while suppressing keys and section wrappers, use the following command structure [1][2][3]: ffprobe -v error -show_entries stream=codec_type -of default=noprint_wrappers=1:nokey=1 input_file Explanation of options: - -v error: Suppresses non-essential log output [2][3]. - -show_entries stream=codec_type: Specifies that only the codec_type field from the stream section should be displayed [4][5]. - -of default=...: Sets the output format to the default writer [4][6]. - noprint_wrappers=1 (or nw=1): Disables the printing of section headers and footers [4][7]. - nokey=1 (or nk=1): Disables the printing of the field key (e.g., "codec_type="), leaving only the value [4][1][3]. You can combine these options using a colon separator within the -of (or -print_format) argument [1][3].
Citations:
- 1: https://stackoverflow.com/questions/14137641/passing-options-to-ffprobe
- 2: https://stackoverflow.com/questions/32278277/using-ffprobe-to-check-if-file-is-audio-or-video-only/32289198
- 3: https://video.stackexchange.com/questions/16356/how-to-use-ffprobe-to-obtain-certain-information-about-mp4-h-264-files/16359
- 4: https://roundup.ffmpeg.org/ffprobe.html
- 5: https://github.com/FFmpeg/FFmpeg/blob/master/doc/ffprobe.texi
- 6: https://gensoft.pasteur.fr/docs/ffmpeg/4.3.1/ffprobe-all.html
- 7: https://ffmpeg.org/ffprobe.html
Critical: The disposition check will not work as intended.
The ffprobe format option nokey=1 suppresses field names, causing ffprobe to output only values without keys. When requesting stream=codec_type,disposition, the output will be:
- Line 0:
video(the codec_type value) - Lines 1+: Numeric values
0or1for each disposition flag (default, dub, original_language, comment, lyrics, karaoke, forced, hearing_impaired, visual_impaired, clean_effects, attached_pic, timed_thumbnails)
The code at line 101 attempts to check "attached_pic" not in disposition against lines[1], which will be a numeric string like "0" or "1". The substring "attached_pic" will never be found, making it impossible to detect attached pictures.
Fix: Remove nokey=1 from line 83 and update the parsing logic to handle key=value pairs, or alternatively, query only the specific attached_pic disposition flag:
- "-of", "default=noprint_wrappers=1:nokey=1",
+ "-of", "default=noprint_wrappers=1",Then parse the key=value output to identify the codec_type and attached_pic disposition status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/modules/ffmpeg_helper.py` around lines 88 - 101, The disposition parsing
fails because ffprobe was run with nokey=1 so lines contain only values (e.g.
"0"/"1") and the code checks for the literal "attached_pic". Change the ffprobe
invocation to include keys (remove nokey=1) or request key=value output, then
parse key=value pairs: extract codec_type from the line like "codec_type=video"
and find the disposition key "attached_pic=1" (or "attached_pic=0") and return
True only when codec_type == "video" and attached_pic != "1"; update the
variables used in this block (result, codec_type, disposition/attached_pic)
accordingly so the check is correct.
1 participant
Enhanced is_video_file function to exclude attached pictures from video stream checks.
Summary by CodeRabbit