fix(video): add duration, width and height metadata to VideoMessage#265
fix(video): add duration, width and height metadata to VideoMessage#265dumilani wants to merge 1 commit intoasternic:mainfrom
Conversation
Summary of ChangesHello @dumilani, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves an issue where videos sent via the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds video metadata (duration, width, height) to VideoMessage, addressing a client-side playback issue. The implementation uses ffprobe for auto-detection and allows for API overrides, which is a solid approach. My review includes a couple of suggestions for the new getVideoMetadata helper function in helpers.go. One is a high-severity recommendation to improve security and performance by piping data to ffprobe via stdin instead of using temporary files. The other is a medium-severity suggestion to refactor the duration parsing logic to reduce code duplication and improve maintainability. Overall, this is a good fix that solves the described problem.
helpers.go
Outdated
| tmpFile, err := os.CreateTemp("", "video-probe-*.mp4") | ||
| if err != nil { | ||
| log.Warn().Err(err).Msg("getVideoMetadata: failed to create temp file") | ||
| return meta | ||
| } | ||
| defer os.Remove(tmpFile.Name()) | ||
|
|
||
| if _, err := tmpFile.Write(filedata); err != nil { | ||
| tmpFile.Close() | ||
| log.Warn().Err(err).Msg("getVideoMetadata: failed to write temp file") | ||
| return meta | ||
| } | ||
| tmpFile.Close() | ||
|
|
||
| cmd := exec.Command("ffprobe", | ||
| "-v", "quiet", | ||
| "-print_format", "json", | ||
| "-show_format", | ||
| "-show_streams", | ||
| "-select_streams", "v:0", | ||
| tmpFile.Name(), | ||
| ) |
There was a problem hiding this comment.
Using temporary files for passing data to an external process like ffprobe can introduce security vulnerabilities (TOCTOU - Time-of-check to time-of-use) and is less efficient due to disk I/O. A more secure and performant approach is to pipe the data directly to ffprobe's standard input. ffprobe supports this with the -i - argument. This avoids creating temporary files on disk altogether.
This pattern is also used in runFFmpegConversion, and a similar refactoring could be applied there for consistency and security.
cmd := exec.Command("ffprobe",
"-v", "quiet",
"-print_format", "json",
"-show_format",
"-show_streams",
"-select_streams", "v:0",
"-i", "-",
)
cmd.Stdin = bytes.NewReader(filedata)There was a problem hiding this comment.
Done! Switched to piping data via stdin using bytes.NewReader(filedata) with -i -. No more temp files.
| if len(probeResult.Streams) > 0 { | ||
| meta.Width = uint32(probeResult.Streams[0].Width) | ||
| meta.Height = uint32(probeResult.Streams[0].Height) | ||
| if probeResult.Streams[0].Duration != "" { | ||
| if dur, err := strconv.ParseFloat(probeResult.Streams[0].Duration, 64); err == nil && dur > 0 { | ||
| meta.DurationSeconds = uint32(dur + 0.5) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback to format-level duration if stream duration was not available | ||
| if meta.DurationSeconds == 0 && probeResult.Format.Duration != "" { | ||
| if dur, err := strconv.ParseFloat(probeResult.Format.Duration, 64); err == nil && dur > 0 { | ||
| meta.DurationSeconds = uint32(dur + 0.5) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's some code duplication in how the video duration is parsed from the stream and format information. This can be refactored to be more concise and maintainable by determining the correct duration string first, and then parsing it once.
var durationStr string
if len(probeResult.Streams) > 0 {
stream := probeResult.Streams[0]
meta.Width = uint32(stream.Width)
meta.Height = uint32(stream.Height)
durationStr = stream.Duration
}
// Fallback to format-level duration if stream duration was not available
if durationStr == "" && probeResult.Format.Duration != "" {
durationStr = probeResult.Format.Duration
}
if durationStr != "" {
if dur, err := strconv.ParseFloat(durationStr, 64); err == nil && dur > 0 {
meta.DurationSeconds = uint32(dur + 0.5)
}
}There was a problem hiding this comment.
Done! Refactored to extract durationStr first (stream → format fallback) and parse it once.
Videos sent via /chat/send/video were missing Seconds, Width, and Height fields in the VideoMessage proto, causing recipients to see 0-second duration and inability to play on WhatsApp Desktop and mobile clients. This change: - Adds getVideoMetadata() helper that uses ffprobe to extract video duration and dimensions from the file data - Sets Seconds, Width, and Height on the VideoMessage proto - Accepts optional Seconds/Width/Height in the API payload, which take priority over auto-detected values - Gracefully degrades to current behavior if ffprobe is unavailable
dumilani
force-pushed
the
fix/video-metadata
branch
from
abbd30b to
e3157f0
Compare
1 participant
Problem
Videos sent via
/chat/send/videoare missingSeconds,Width, andHeightfields in theVideoMessageproto. This causes recipients to see 0-second duration and the video fails to play on WhatsApp Desktop and mobile clients.The current
SendVideohandler constructs theVideoMessagewithout these fields:Solution
Auto-detect metadata via ffprobe — Added a
getVideoMetadata()helper function that extracts duration, width, and height from the video file data usingffprobe(already available in the Docker image via theffmpegpackage). It follows the same temp-file +exec.Commandpattern already used byrunFFmpegConversionfor sticker conversion.Accept optional metadata in the API payload — The
SendVideoendpoint now accepts optionalSeconds,Width, andHeightfields in the JSON payload. If provided, they take priority over auto-detected values. This gives API callers flexibility while maintaining backward compatibility.Graceful degradation — If
ffprobefails or is unavailable, the function returns zero values and the video is sent as before (same as current behavior).Changes
helpers.go: AddedVideoMetadatastruct andgetVideoMetadata()functionhandlers.go: ModifiedSendVideo()to extract metadata and setSeconds,Width,Heighton theVideoMessageprotoAPI Payload (backward compatible)
Existing payloads continue to work unchanged. New optional fields:
{ "Phone": "5519999999999", "Video": "data:video/mp4;base64,...", "Caption": "my video", "Seconds": 30, "Width": 1920, "Height": 1080 }