Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved target override, initialization propagation, stream counting, failure reporting, and test-execution issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (6)
Honor CLI target_fps in stream target calculations · New Fix undefined total_fps_per_stream reference · New Propagate init_duration to multi-container runs · New Scale report stream counts by pipeline count · New Emit detailed report for failed benchmark outcomes · New Correct explanation of ramp-up pass-window behavior · New
What changed in this PR
This PR enhances stream-density benchmarking with configurable measurement windows, per-camera FPS targets, and richer reporting.
Changes:
- Adds timed measurement windows and removes unused hysteresis handling.
- Reports targets, pass thresholds, measured FPS, and headroom.
- Updates CLI integration, result handling, and unit tests.
| File | Summary |
|---|---|
benchmark-scripts/stream_density.py |
Implements target resolution, windowed measurements, and detailed reporting. |
benchmark-scripts/stream_density_test.py |
Updates tests for new measurement behavior and return values. |
benchmark-scripts/benchmark.py |
Adds measurement-window CLI support and revised result handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jcork-intel
left a comment
There was a problem hiding this comment.
Thanks Sumana, and thank you for turning this around so quickly. Thanks also for filling in the PR template fully; the description and sample output made this much faster to review. Removing the fail threshold, measuring only inside a fixed window after the settle time, and taking each camera's target from its own fps are all things we asked for, and the new end-of-run summary, with the per-stream table and the pass rule written out, is much easier to read. I ran stream_density_test.py locally at the latest commit and all 11 tests pass. They cover the per-camera targets and the stream count, but not the two main changes, the measurement window and the new summary. The Copilot findings look fixed in the second round of commits.
I'd like three changes before this merges, because each one changes a number or a word a customer sees.
-
The "measured" column shows the 90th percentile, not the measured rate. "Report the rate that was measured and the rule that was applied" (#247) asks for the measured per-stream rate next to the target, and this PR closes #247, so this is in scope. In the sample report, 15 fps cameras show
measured 16.63and15.92. A camera can't deliver more frames than its source sends, so a customer reading that table will think the tool is broken.Here is an example of the same result written both ways, with illustrative values:
As this PR writes it:
stream target pass mark measured result pipeline_stream0 15.00 14.25 16.0 pass pipeline_stream1 15.00 14.25 17.0 pass pipeline_stream2 15.00 14.25 16.9 pass pipeline_stream3 15.00 14.25 16.9 pass The lowest-throughput stream was pipeline_stream0, ... measuring 16.0 FPS against its 14.25 FPS minimum, leaving 1.75 FPS (12% relative headroom).What I'd like:
stream target pass mark measured (avg) p90 seconds below pass mark result pipeline_stream0 15.00 14.25 14.6 16.0 40% pass pipeline_stream1 15.00 14.25 14.8 17.0 40% pass pipeline_stream2 15.00 14.25 14.7 16.9 40% pass pipeline_stream3 15.00 14.25 14.3 16.9 50% pass The lowest-throughput stream was pipeline_stream3, ... measuring 14.3 FPS (frames delivered / measurement time) against its 14.25 FPS minimum, leaving 0.05 FPS (under 1% relative headroom).The first version can name the wrong stream as the slowest, and it reports plenty of headroom at a lane count where the next lane up fails. The second version shows the box is at its limit. Please make "measured" the average over the window (frames divided by time), and show p90 and the share of seconds below the pass mark next to it. That also meets #247's requirement that every number in the result can be worked out from the raw log. Changing the pass/fail rule itself from p90 to the average is "Frames over measurement time" (#245). I'm confirming that with the EDO team this week, so this PR doesn't need to change the rule.
-
A 30 second window is too few samples. It gives about 30 one-second samples per stream. We agreed with Xintian on at least 100 samples per stream after the warmup. In our internal testing, at a lane count right at the limit, a stream's average varied noticeably between short blocks of samples, and it stopped getting steadier at about 100 samples. Please make the default window 100 seconds, or add a minimum of 100 samples. I haven't sent the team the outcome of my discussion with Xintian yet, so this is the first you're hearing of the 100-sample minimum. I'll send a written summary of what we agreed later today.
-
The settle time can be much shorter. Switching from the 30% ratio to a fixed settle time is the right change. In our internal testing almost every stream was steady within a few seconds, so a 60 to 120 second settle mostly adds time to every step of a density run. We're proposing 10 seconds. The defaults Loss Prevention users actually get are set in the loss-prevention Makefile in PR #359 (intel-retail/loss-prevention#359):
INIT_DURATION ?= 60andMEASUREMENT_WINDOW_SECONDS ?= 30. I'll ask for the same values there, a 10 second settle and a 100 second window.
Three smaller things:
- Please add tests for the measurement window (samples from before the window start or after its end are left out) and for the summary (target, pass mark and measured rate appear as separate numbers).
test_calculate_multi_stream_fps_successalso asserts the stream value is the top sample (the p90 rule), so it will need updating when "measured" becomes the average. - No CI workflow runs
stream_density_test.py, so these tests only help if someone runs them by hand. Can we add them to a workflow, in this PR or a follow-up? - All six commits are titled "Update .py". A one-line description per commit would make the history readable.
This changes how the published density number is produced, so please do a before-and-after run: the stock LP lane on a Panther Lake box, with main and with this branch, showing the lane count and each stream's measured FPS. Because this repo is public and benchmark results need legal approval before they're published, please don't post the numbers here. Email them to me, or put them in SharePoint and link to that from the PR. I'll spot-check one point on our side.
|
One more for this PR, and it came from the template. The "Updated Documentation" box was unchecked, which made me ask what this change does to the docs we already have, so I went looking. The public docs for I'm glad the team is using the template now; it's what caught this. It helps most when each item gets a real look, with a short note on any box left unchecked saying why it doesn't apply (for example, "no docs affected" or "docs PR to follow: |
|
On commit messages: this repo's CONTRIBUTING.md asks for Conventional Commits. Before merge, could the six |
|
Correction to my follow-up about the docs: the live page is |



PR Checklist
What are you changing?
Removed faill threshould and Updated stream-density benchmarking to use an explicit measurement window in seconds for each density step, with consecutive windows used to confirm pass/fail results. Removed the unused fail-threshold configuration and related handling. Documentation now explains MEASUREMENT_WINDOW_SECONDS, INIT_DURATION, target FPS resolution, and how these settings affect benchmark duration and results. The final report includes the settle time and measurement-window details for clearer result interpretation.
TARGET_FPS environment variable: overrides the target for every stream.
Per-camera targetFps in the selected camera_to_workload_*.json.
Per-camera fps in the camera configuration.
Default target of 14.95 FPS if neither JSON field contains a positive value.
For example, if TARGET_FPS=12 is set, every stream uses 12 FPS, even if its camera has targetFps: 15. Without the environment override, each camera can use its own targetFps; otherwise its source fps is used as the fallback.
2. Updated stream-density reports to show the measured FPS for each stream alongside its target FPS and pass threshold. The final summary identifies the stream with the lowest throughput or headroom and reports the exact rule used to determine success. Results now clearly distinguish uniform-target runs from runs with individual per-stream targets. This makes the benchmark outcome easier to audit and interpret.
3. For different targets, report logic is as below:
For each stream, the benchmark calculates:
pass mark = target FPS × pass tolerance ratio
relative headroom = (measured FPS − pass mark) ÷ pass mark
When streams have different targets, the stream with the smallest relative headroom is reported as the lowest- headroom stream:
cam1: target 15, pass mark 14.25, measured 15.00
headroom = (15.00 - 14.25) / 14.25 = 5.3%
cam2: target 10, pass mark 9.50, measured 9.60
headroom = (9.60 - 9.50) / 9.50 = 1.1%
Although cam1 has the larger absolute margin (0.75 FPS versus 0.10 FPS), cam2 has less relative headroom and is therefore reported as the limiting stream. All streams must still meet their own individual pass marks for the overall result to pass.
Sample report output for same targets:
1. Beginning of the log
INFO: Stream density per-camera target FPS configuration: cam1=15.00 FPS from fps, cam2=15.00 FPS from fps with container_name gst0 and INIT_DURATION set for 60 seconds; measurement window 30 seconds; required agreement 2 pass / 2 fail windows
2. End of the log
Stream density result
Streams sustained 36
Settle time 60 s (INIT_DURATION before measuring)
Measurement window 30 s per window; 2 consecutive windows must agree
(each window is measured on its own; the count moves
only after 2 windows in a row agree - windows are not added together)
Pass mark target x 0.95 (pass tolerance ratio)
stream camera workload target pass mark measured result
pipeline_stream0 cam1 asc_obj_detection_age_prediction_face_detection 15.00 14.25 15.65 pass
pipeline_stream1 cam2 asc_obj_detection_age_prediction 15.00 14.25 15.92 pass
pipeline_stream2 cam1 asc_obj_detection_age_prediction_face_detection 15.00 14.25 16.63 pass
Result: Pass - 36 concurrent streams sustained
The run was evaluated after a 60 second settle time (INIT_DURATION), over 2 consecutive measurement windows of 30 seconds each. All streams met the 14.25 FPS minimum requirement. The lowest-throughput stream was pipeline_stream35 (cam2 (asc_obj_detection_age_prediction)), targeting 15.00 FPS and measuring 14.53 FPS against its 14.25 FPS minimum, leaving 0.28 FPS (2.0% relative headroom).
Sample report output for Different targets
1. Beginning of the log
INFO: Stream density per-camera target FPS configuration: cam1=14.00 FPS from targetFps, cam2=13.00 FPS from targetFps with container_name gst0 and INIT_DURATION set for 60 seconds; measurement window 30 seconds; required agreement 2 pass / 2 fail windows
2. End of the log
Result: Pass - 38 concurrent streams sustained
The run was evaluated after a 60 second settle time (INIT_DURATION),over 2 consecutive measurement windows of 30 seconds each. All streams met their individual minimum FPS requirements. The lowest-headroom stream was pipeline_stream34 (cam1 (asc_obj_detection_age_prediction_face_detection)), targeting 14.00 FPS and measuring 14.74 FPS against its 13.30 FPS minimum, leaving 1.44 FPS (10.8% relative headroom).
Issue this PR will close
close: #250 , #247
Anything the reviewer should know when reviewing this PR?
Test Instructions if applicable
make benchmark-stream-density
MEASUREMENT_WINDOW_SECONDS=60
make benchmark-stream-density
TARGET_FPS=13
If the there are associated PRs in other repositories, please link them here (i.e. intel-retail/performance-tools )
intel-retail/loss-prevention#359