Conversation
* Fix reporting of sparklines for metrics * Allocate metrics slot for any metrics reporting * Instrument UDT with some basic metrics
There was a problem hiding this comment.
Pull request overview
This PR fixes and expands stream metrics reporting (including sparklines) by ensuring metrics slots are allocated when metrics are recorded, instrumenting Unified Detection Thread (UDT) with basic metrics, and adding smoothing to reported FPS/bitrate values.
Changes:
- Fix
/api/health?sparklines=...handling to correctly detect the query parameter. - Instrument UDT to record per-packet/frame metrics and recording lifecycle metrics.
- Update metrics slot allocation behavior and add basic EMA smoothing for FPS/bitrate sampling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/web/api_handlers_health.c | Fixes query-param handling for enabling sparklines in the health response. |
| src/video/unified_detection_thread.c | Adds stream metrics recording (frames/bytes) and recording lifecycle metrics for UDT. |
| src/video/stream_state.c | Removes explicit metrics slot pre-allocation; relies on allocation via metrics APIs. |
| src/telemetry/stream_metrics.c | Changes slot allocation strategy, adds smoothing to sampled FPS/bitrate, and adjusts configured FPS usage. |
| include/telemetry/stream_metrics.h | Removes the public metrics_get_slot API from the header. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| m->current_fps = 0.5 * m->current_fps + 0.5 * (double)df / dt; | ||
| m->current_bitrate_bps = 0.5 * m->current_bitrate_bps + 0.5 * ((double)db * 8.0) / dt; |
There was a problem hiding this comment.
The exponential smoothing update halves the first computed FPS/bitrate after startup because current_fps/current_bitrate_bps are initially 0.0; the first delta sample becomes 0.50 + 0.5instant. Consider initializing the EMA on the first computed sample (e.g., if prev_sample_time>0 but prior value is 0/uninitialized, assign the instantaneous value directly) to avoid systematically underreporting immediately after allocation/reset.
| m->current_fps = 0.5 * m->current_fps + 0.5 * (double)df / dt; | |
| m->current_bitrate_bps = 0.5 * m->current_bitrate_bps + 0.5 * ((double)db * 8.0) / dt; | |
| double instant_fps = (double)df / dt; | |
| double instant_bitrate_bps = ((double)db * 8.0) / dt; | |
| if (m->prev_frames_total == 0 && m->prev_bytes_received == 0) { | |
| m->current_fps = instant_fps; | |
| m->current_bitrate_bps = instant_bitrate_bps; | |
| } else { | |
| m->current_fps = 0.5 * m->current_fps + 0.5 * instant_fps; | |
| m->current_bitrate_bps = 0.5 * m->current_bitrate_bps + 0.5 * instant_bitrate_bps; | |
| } |
| if (!g_initialized || !stream_name) return; | ||
| int idx = find_active_slot(stream_name); | ||
| int idx = metrics_get_slot(stream_name); | ||
| if (idx < 0) return; |
There was a problem hiding this comment.
metrics_set_configured_fps() now stores the provided fps without any validation/clamping. Because the sampler compares current_fps against configured_fps * STREAM_HEALTH_FPS_RATIO, a caller passing 0 or a negative value will effectively disable the degraded-FPS check (or produce a negative threshold). Consider keeping the previous guard (e.g., clamp to a sane default/minimum) or explicitly rejecting invalid values.
| if (idx < 0) return; | |
| if (idx < 0) return; | |
| if (fps <= 0) { | |
| log_warn("Ignoring invalid configured FPS %.3f for stream '%s'", fps, stream_name); | |
| return; | |
| } |
| // Record stream metrics | ||
| metrics_record_frame(ctx->stream_name, pkt->size, is_video); |
There was a problem hiding this comment.
metrics_record_frame() updates last_frame_ts for every packet; in this call site it is invoked for both video and audio packets (is_video=false for audio), which can make stream health look "up" based on audio activity even if video is stalled. Consider only calling metrics_record_frame for video packets here, or change metrics_record_frame to update last_frame_ts only when is_video is true (and add a separate API/counter if you still want to track total bytes including audio).
| // Record stream metrics | |
| metrics_record_frame(ctx->stream_name, pkt->size, is_video); | |
| // Record stream metrics for video packets only so stream health reflects | |
| // actual video frame arrival rather than audio activity. | |
| if (is_video) { | |
| metrics_record_frame(ctx->stream_name, pkt->size, true); | |
| } |
Uh oh!
There was an error while loading. Please reload this page.