Add video download / play and extraction from mcap #3659
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideIntroduces a new Recorder Extractor service and UI to list, stream, download, delete, and generate thumbnails for MP4 recordings extracted from MCAP files, wiring it through nginx, frontend routing, and helper utilities (including improved file-in-use checks). Sequence diagram for listing, viewing, and deleting recordingssequenceDiagram
actor User
participant Browser
participant RecordsView
participant RecordsStore
participant Nginx
participant RecorderAPI
participant RecorderDir as Recorder_directory
User->>Browser: Open Records page
Browser->>RecordsView: Mount RecordsView
RecordsView->>RecordsStore: refresh()
RecordsStore->>Nginx: GET /recorder-extractor/v1.0/recorder/files
Nginx->>RecorderAPI: Proxy request
RecorderAPI->>RecorderDir: Scan for *.mp4
RecorderDir-->>RecorderAPI: List of MP4 files
RecorderAPI-->>Nginx: 200 List[RecordingFile]
Nginx-->>RecordsStore: 200 List[RecordingFile]
RecordsStore-->>RecordsView: Update recordings state
RecordsView-->>User: Show recording cards with thumbnails
User->>RecordsView: Click thumbnail or play button
RecordsView->>Browser: Open dialog with video player
Browser->>Nginx: GET activeRecord.stream_url (MP4)
Nginx->>RecorderAPI: Proxy to get_recording
RecorderAPI->>RecorderDir: Open MP4 file
RecorderDir-->>RecorderAPI: File stream
RecorderAPI-->>Nginx: 200 video/mp4 (stream)
Nginx-->>Browser: Stream video
Browser-->>User: Play video
User->>RecordsView: Click delete icon
RecordsView->>RecordsStore: deleteRecording(file)
RecordsStore->>Nginx: DELETE /recorder-extractor/v1.0/recorder/files/{file.path}
Nginx->>RecorderAPI: Proxy to delete_recording
RecorderAPI->>RecorderDir: Delete file
RecorderDir-->>RecorderAPI: File removed
RecorderAPI-->>Nginx: 204 No Content
Nginx-->>RecordsStore: 204 No Content
RecordsStore->>RecordsStore: fetchRecordings()
RecordsStore->>Nginx: GET /recorder-extractor/v1.0/recorder/files
Nginx->>RecorderAPI: Proxy request
RecorderAPI->>RecorderDir: Rescan MP4 files
RecorderDir-->>RecorderAPI: Updated list
RecorderAPI-->>RecordsStore: 200 Updated list
RecordsStore-->>RecordsView: Update recordings
RecordsView-->>User: Updated list without deleted file
Sequence diagram for automatic MCAP extraction loopsequenceDiagram
participant ExtractLoop as extract_mcap_recordings
participant RecorderDir as Recorder_directory
participant FileCheck as file_is_open_async
participant McapExtractor as mcap-foxglove-video-extract
loop Every_10_seconds
ExtractLoop->>RecorderDir: ensure_recorder_dir()
RecorderDir-->>ExtractLoop: Base path
ExtractLoop->>RecorderDir: rglob("*.mcap")
RecorderDir-->>ExtractLoop: List of MCAP files
loop For_each_mcap_path
ExtractLoop->>RecorderDir: Compute output_dir (no .mcap suffix)
alt output_dir exists
ExtractLoop->>ExtractLoop: Skip (already extracted or deleted)
else output_dir missing
ExtractLoop->>FileCheck: file_is_open_async(mcap_path)
alt File is open
FileCheck-->>ExtractLoop: True
ExtractLoop->>ExtractLoop: Log "Skipping MCAP extract, file in use"
else File is not open
FileCheck-->>ExtractLoop: False
ExtractLoop->>McapExtractor: Spawn process with arguments
McapExtractor-->>ExtractLoop: stdout, stderr, returncode
alt returncode == 0
ExtractLoop->>ExtractLoop: Log success
else extraction failed
ExtractLoop->>ExtractLoop: Log error with stderr
end
end
end
end
end
Class diagram for Recorder Extractor backend and frontend typesclassDiagram
class RecordingFileModel {
+str name
+str path
+int size_bytes
+float modified
+str download_url
+str stream_url
+str thumbnail_url
}
class RecorderRouter {
+list_recordings() List~RecordingFileModel~
+get_recording_thumbnail(filename str) StreamingResponse
+delete_recording(filename str) None
+get_recording(filename str) FileResponse
}
class RecorderExtractorApp {
+FastAPI fast_api_app
+VersionedFastAPI app
+main() None
}
class ExtractLoop {
+extract_mcap_recordings() None
}
class FileOpenUtils {
+_file_is_open_command(path Path) list~str~
+_file_is_open_logic_lsof(returncode int, stdout str, stderr str) bool
+file_is_open(path Path) bool
+file_is_open_async(path Path) bool
}
class RecordingFileTs {
<<interface>>
+string name
+string path
+number size_bytes
+number modified
+string download_url
+string stream_url
+string thumbnail_url
}
class RecordsStoreTs {
+string API_URL
+RecordingFileTs[] recordings
+boolean loading
+string error
+setLoading(value boolean) void
+setRecordings(files RecordingFileTs[]) void
+setError(message string) void
+fetchRecordings() Promise~void~
+deleteRecording(file RecordingFileTs) Promise~void~
}
RecorderRouter --> RecordingFileModel : uses
RecorderExtractorApp --> RecorderRouter : includes
RecorderExtractorApp --> ExtractLoop : starts
ExtractLoop --> FileOpenUtils : uses
RecordsStoreTs --> RecordingFileTs : manages
RecordingFileTs .. RecordingFileModel : mirrors_structure
Flow diagram for thumbnail generation from MP4 recordingsflowchart TD
A_start["Request /files/{filename}/thumbnail"] --> B_resolve["resolve_recording(filename)"]
B_resolve --> C_ok_path{Is path valid existing .mp4 file?}
C_ok_path -- No --> X_http_error["Return HTTP 4xx (invalid or missing recording)"]
C_ok_path -- Yes --> D_thread["asyncio.to_thread(build_thumbnail_bytes, path)"]
D_thread --> E_discover["Run gst-discoverer-1.0 file://path"]
E_discover --> F_rc_ok{returncode == 0?}
F_rc_ok -- No --> Y_http_error500["Log error and return HTTP 500 (inspect failed)"]
F_rc_ok -- Yes --> G_parse["parse_duration_ns(discover_output)"]
G_parse --> H_target["Compute target_ns = duration_ns/2 (or 0) and target_sec"]
H_target --> I_build_pipeline["Build gst-play-1.0 command with JPEG fdsink pipeline"]
I_build_pipeline --> J_run_play["Run gst-play-1.0 at target_sec"]
J_run_play --> K_success{returncode == 0 and stdout not empty?}
K_success -- No --> Z_http_error500["Log error and return HTTP 500 (thumbnail failed)"]
K_success -- Yes --> L_bytes["Return stdout bytes as JPEG data"]
L_bytes --> M_stream["StreamingResponse(BytesIO(thumbnail_bytes), image/jpeg)"]
M_stream --> N_end["Send JPEG thumbnail to client"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
RecordsView.vue,onThumbnailErroris called withfile.namebutbrokenThumbnailsis keyed bypath, so thumbnails will never be marked as broken; passfile.path(or change the keying) to ensure the fallback thumbnail is actually used. - In
records.ts, the delete URL interpolatesfile.pathdirectly into the path segment, but the backend expects a URL-encoded{filename:path}like the generateddownload_url; consider either URL-encodingfile.pathor issuing the DELETE againstfile.download_urlto avoid issues with nested paths or special characters.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RecordsView.vue`, `onThumbnailError` is called with `file.name` but `brokenThumbnails` is keyed by `path`, so thumbnails will never be marked as broken; pass `file.path` (or change the keying) to ensure the fallback thumbnail is actually used.
- In `records.ts`, the delete URL interpolates `file.path` directly into the path segment, but the backend expects a URL-encoded `{filename:path}` like the generated `download_url`; consider either URL-encoding `file.path` or issuing the DELETE against `file.download_url` to avoid issues with nested paths or special characters.
## Individual Comments
### Comment 1
<location> `core/frontend/src/views/RecordsView.vue:38-47` </location>
<code_context>
+ @error="onThumbnailError(file.name)"
</code_context>
<issue_to_address>
**issue (bug_risk):** Thumbnail error handler keys by name but lookup uses path, so the fallback thumbnail is never applied.
`thumbnailSrc` checks `this.brokenThumbnails[file.path]`, but the `@error` handler calls `onThumbnailError(file.name)`, so the flag is keyed by name while the lookup uses path. As a result, the fallback image is never used. Pass `file.path` to `onThumbnailError` (and/or rename the parameter) so the key matches the lookup.
</issue_to_address>
### Comment 2
<location> `core/services/recorder_extractor/main.py:128` </location>
<code_context>
+ f"Thumbnail target: duration_ns={duration_ns} target_ns={target_ns} target_sec={target_sec:.3f} file={path}"
+ )
+ logger.info(f"Thumbnail command: {' '.join(play_cmd)}")
+ result = subprocess.run(play_cmd, capture_output=True, check=False)
+ if result.returncode != 0 or not result.stdout:
+ stderr = result.stderr.decode("utf-8", "ignore")
</code_context>
<issue_to_address>
**issue (bug_risk):** Running gst-play without a timeout risks hanging request handlers if the subprocess stalls.
`subprocess.run` is called without a timeout, so if `gst-play-1.0` hangs (e.g. on malformed input), the FastAPI worker can block indefinitely. Please add a `timeout=` and handle `subprocess.TimeoutExpired` so the handler can fail gracefully instead of tying up the worker.
</issue_to_address>
### Comment 3
<location> `core/services/recorder_extractor/main.py:83-92` </location>
<code_context>
+ mcap_path,
+ stdout.decode("utf-8", "ignore").strip(),
+ )
+ except Exception as exception:
+ logger.exception("MCAP extraction loop failed: %s", exception)
+
+
+def to_http_exception(endpoint: Callable[..., Any]) -> Callable[..., Any]:
+ is_async = asyncio.iscoroutinefunction(endpoint)
+
+ @wraps(endpoint)
+ async def wrapper(*args: Any, **kwargs: Any) -> Any:
+ try:
+ if is_async:
+ return await endpoint(*args, **kwargs)
+ return endpoint(*args, **kwargs)
+ except HTTPException as exception:
+ raise exception
+ except Exception as exception:
+ logger.exception("Recorder endpoint failed")
+ raise HTTPException(status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, detail=str(exception)) from exception
+
+ return wrapper
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Wrapping unexpected exceptions with HTTPException using the raw exception string may leak internal details.
The wrapper currently returns a 500 with `detail=str(exception)`, which can reveal internal implementation details or sensitive information to clients. Instead, return a generic error message in `detail` and rely on `logger.exception` for full diagnostics.
</issue_to_address>
### Comment 4
<location> `core/libs/commonwealth/src/commonwealth/utils/general.py:187-194` </location>
<code_context>
result = subprocess.run(
cmd,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
text=True,
timeout=5,
check=False,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location> `core/services/recorder_extractor/main.py:98` </location>
<code_context>
discover = subprocess.run(discover_cmd, capture_output=True, text=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location> `core/services/recorder_extractor/main.py:128` </location>
<code_context>
result = subprocess.run(play_cmd, capture_output=True, check=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
patrickelectric
force-pushed
the
video-extract-service
branch
7 times, most recently
from
7178de2 to
4a53194
Compare
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
Signed-off-by: Patrick José Pereira <[email protected]>
… async version Signed-off-by: Patrick José Pereira <[email protected]>
patrickelectric
force-pushed
the
video-extract-service
branch
from
4a53194 to
75c5d65
Compare
joaomariolago
approved these changes
Dec 8, 2025
joaomariolago
dismissed
sourcery-ai[bot]’s stale review
This sub process run is intended to run with dynamic command, in this context specifically will not introduce a direct security fail. Other points commented in review were adjusted.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce a new Recorder Extractor service and UI for browsing, streaming, and downloading recorded MP4 sessions extracted from MCAP files.
New Features:
Bug Fixes:
Chores: