Repository navigation
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 |
Return per-stream FPS samples from calculate_multi_stream_fps and surface them in the density report: add a p90 column and a seconds-below-pass-mark column so a stream's stability is visible, not just its average, plus a Lanes sustained line alongside Streams sustained. Replace count_pipeline_scaled_streams with count_valid_streams, dropping the num_pipelines multiplier that inflated the sustained-stream count; pipeline indices already span every lane. Update unit tests to match the new return arity and report format. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
Align the stream_density fallback defaults with the argparse defaults and the Makefile: INIT_DURATION falls back to 10 seconds instead of 120, and DEFAULT_MEASUREMENT_WINDOW_SECONDS is 100 instead of 30. Update the validate_and_setup_env unit tests to expect the 10 second fallback. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
Align the stream_density fallback defaults with the argparse defaults and the Makefile: INIT_DURATION falls back to 10 seconds instead of 120, and DEFAULT_MEASUREMENT_WINDOW_SECONDS is 100 instead of 30. Update the validate_and_setup_env unit tests to expect the 10 second fallback. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
|
jcork-intel
left a comment
There was a problem hiding this comment.
Thanks Sumana, and for answering each point inline. The 10 s warmup period, the 100 s measurement interval, and the p90 and seconds-below report all look good. So does the target order: TARGET_FPS only when someone passes it, then targetFps, then the camera's fps, which is what I asked for in my 16 Sep correction.
A few items before I approve:
-
Pass rule: on 24 Sep I asked you to hold this until it was confirmed. It's now committed (with the benchmarking team on 25 Sep, and again on the 29 Sep call), so let's move forward with it in this PR. Please switch the per-stream decision from p90 to total frames divided by the measurement interval, and keep p90 in the report.
-
Average column: please add the per-stream average (frames divided by time) as "measured (avg)" next to p90 and seconds below. What I asked for on 24 Sep, what the report shows today, and what it should show with the agreed terms (point 5) and p10 (point 3):
Asked for (24 Sep review) stream target pass mark measured (avg) p90 seconds below result stream0 15.00 14.25 14.6 16.00 40% pass stream3 15.00 14.25 14.3 16.90 50% pass Report today (your 29 Sep sample) stream target pass mark measured (avg) p90 seconds below pass mark result stream0 15.00 14.25 (missing) 15.94 62/100 (62%) pass Should be (agreed terms and columns, your sample) stream target throughput threshold measured (avg) p10 p90 seconds below threshold result stream0 15.00 14.25 (avg) (p10) 15.94 62/100 (62%) (from avg)In your sample, pipeline_stream0 was below 14.25 fps 62% of the time, and on p90 alone it reads as a comfortable pass.
-
p10: as discussed on the 29 Sep call, please report p10 alongside p90.
-
Minimum samples: the 100 s default gives about 100 samples. How is the 100-sample minimum enforced if someone sets a shorter measurement interval?
-
Terminology: this one is on us. My 24 Sep review used "pass mark" and "settle time", and we've since agreed one vocabulary with the benchmarking team. Please use these words in the report, logs and help text (variable and env names can stay as they are):
- "pass mark" becomes "throughput threshold" (column header, "seconds below throughput threshold", and the summary sentence)
- "settle time" becomes "warmup period"
- "measurement window" becomes "measurement interval"
- "consecutive pass windows" and "required agreement" become "run acceptance criterion (two consecutive passes)"
- "Lanes sustained" becomes "Use case density (lanes)", and "Streams sustained" becomes "Stream density (streams)"
-
Stream count: thanks for dropping the pipeline-count multiplier in count_valid_streams. Your comment is right that pipeline.sh already carries every lane's sources; our 5-lane run on 22 Sep produced 30 per-stream logs, so the old version would have reported 150. Could you add a unit test that pins 5 lanes of 6 cameras to 30 streams, so the multiplier doesn't come back?
-
Last-resort default: DEFAULT_TARGET_FPS in stream_density.py is still 14.95. Please make it 15. It only applies when a camera has neither targetFps nor fps, but then that camera is judged at 14.25 like every other 15 fps camera, not 14.20.
p90 FPS reflects a stream's best moments, not what it sustains, so it could overstate how many lanes a box holds. Pass/fail now uses each stream's time-weighted average FPS, with p10, p90 and time below threshold reported. An interval needs 100 samples per pipeline log or it is inconclusive. BREAKING CHANGE: Densities may be lower than with p90. Report and log labels use the documented terms, and DEFAULT_TARGET_FPS is now 15. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
Changes Implemented
|
FPS-only rows lack timing data, so treating them as one-second samples can skew weighted throughput and density pass/fail results. Make the duration requirement explicit and keep related regression coverage together, including the five-lane, six-camera stream count. BREAKING CHANGE: FPS-only pipeline log rows are ignored; consumers must provide both FPS and duration fields. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
jcork-intel
left a comment
There was a problem hiding this comment.
Thanks Sumana, and for the point-by-point reply. I checked each answer against f326aba:
- Pass rule (point 1): each stream is now judged on its time-weighted average (FPS times duration, summed, over the total duration), and p90 stays in the report. The test that fails a stream on its average while p90 passes is a good one to have.
- Average column, p10 and the agreed terms (points 2, 3 and 5): all in the report.
- Minimum samples (point 4): collection extends up to twice the measurement interval, and a short log makes the attempt inconclusive. Follow-up in item 3 below.
- 30-stream test (point 6) and the default of 15 (point 7): both in. All 21 tests pass for me.
Three items before I approve:
-
Per-stream targets go to the wrong cameras above one lane. We found this on Oct 2nd while running one camera at a lower frame rate.
build_per_stream_target_fps(line 127) andbuild_per_stream_camera_meta(line 231) both work out a stream's camera asstream index % number of cameras. That assumes thepipeline_stream<N>logs are in lane order (cam1 to cam6 for lane 1, then cam1 to cam6 for lane 2). They are in camera order. gvafpscounter reports per-stream FPS sorted by element name, so every lane'sstream1_*comes first, then every lane'sstream2_*, and so on.run-pipeline.shin intel-retail/loss-prevention#359 writes the logs in that order.With 6 cameras and cam6 set to
fps: 5, this is what f326aba does:lanes cam6 logs logs given the 5 fps target 1 stream5 stream5 2 stream10, stream11 stream5, stream11 5 stream25 to stream29 stream5, stream11, stream17, stream23, stream29The camera and workload labels in the new report are wrong in the same way. When every camera has the same target the order makes no difference, so the stock configs are fine today. With this PR, though, a camera's
fpsbecomes its fallback target, so any config with cameras at different frame rates gets the wrong thresholds, even withouttargetFps.Suggested fix for both functions: with
lanes = streams / cameras, the camera for logiis blocki // lanesin name-sorted order,sorted(range(cameras), key=lambda c: f"stream{c + 1}_"), so 10 or more cameras still sort the way gvafpscounter sorts them. This assumes one pipeline branch per camera. The generator merges cameras with identical steps into one branch. If that case needs supporting, the map should come from the gvafpscounter names in the generated pipeline instead. -
A multi-lane test for the target map.
test_build_per_stream_target_fps_mixed_valueshas 5 streams for 5 cameras. That is one lane, where both orders agree, so it can't catch item 1. Could you add a test with 6 cameras and cam6 at 5 fps that checks 2 lanes (target on stream10 and stream11) and 5 lanes (stream25 to stream29)? It can sit next to the 30-stream count test. -
An inconclusive interval ends the sweep as a fail. When the sample floor isn't met,
run_pipeline_iterationsreturns withmeet_target_fpsset to False. benchmark.py then prints "Result: Fail" and "Stream density did not meet the target", and the density already confirmed isn't reported. The loss-prevention README in #359 says an inconclusive interval "is not used for a pass/fail decision". Could the summary say "Inconclusive" and report the last confirmed density, or retry the interval once? Also, at about one sample per second, a measurement interval under about 50 s can't reach 100 samples even at twice its length, so it is always inconclusive. Rejecting values under 100 invalidate_and_setup_env, or saying so in the help text, would save someone a wasted run.
Two small ones, fine in this PR or later:
- benchmark.py lines 240 and 266 still print "streams sustained". "Stream density N streams" would match the report.
- The "measured (avg)" column and the summary sentence print six decimals (14.824225). Two decimals, like the other columns, is plenty given the counter rounds its values.
Per-stream targets and report labels used `stream index % cameras` to find a stream's camera. That only holds when pipeline_stream<N> logs are in lane order and every camera has one branch, so above one lane, cameras with different fps/targetFps got the wrong thresholds and labels. - Read src/pipelines/pipeline_streams.json, written by the pipeline generator, to map each pipeline_stream<N> log to its camera config index and branch workloads. This handles multi-branch cameras and skipped lp_vlm cameras. Fail if the manifest and the logs disagree. Fall back to `% cameras` with a warning when no manifest exists. - Label report rows with the branch's own workloads. An interval below the 100-sample floor used to end the sweep as a Fail and report a lane count that was never measured: - Retry an inconclusive interval once without restarting the pipelines. - If it is still inconclusive, end with status "inconclusive" and report the last passing density as not confirmed. - run_pipeline_iterations / run_stream_density now return a status (pass | fail | inconclusive) instead of a boolean. - benchmark.py prints Result: Pass / Fail / Inconclusive through one helper, using "use case density N lanes, stream density M streams". - Reject MEASUREMENT_WINDOW_SECONDS below 100. At about 1 sample/s with collection capped at 2x the interval, shorter intervals can never reach the floor. The --measurement_window_seconds help text says so. Other changes: - Print measured FPS with two decimals in the report table and summary. - consolidate_multiple_run_of_metrics: match ^pipeline.*\.log$ so the pipeline_streams_*.json manifest copies are not parsed as FPS logs. - clean_up_pipeline_logs also removes pipeline_streams_*.json copies. Tests: multi-lane target mapping with camera-ordered logs (2 and 5 lanes), multi-branch and skipped cameras, manifest/log mismatch, inconclusive retry and last-passing density, measurement interval validation, result printing for each status, manifest cleanup. Removed unused imports, so the tests no longer need the `mock` package. Signed-off-by: sumanaga <sumana.gargi.m@intel.com>
|
jcork-intel
left a comment
There was a problem hiding this comment.
Thanks Sumana. Fixing the camera order at the source with the manifest is better than what I suggested. Approved.



PR Checklist
What are you changing?
Removed faill threshould and Updated stream-density benchmarking to use an explicit measurement interval 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 warmup period and measurement-interval 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 15 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
Use case density (lanes) x
Stream density (streams) x
Warmup period 10 s (INIT_DURATION before measuring)
Measurement interval 100 s minimum; ramp-up is immediate, confirmation uses the run acceptance criterion (two consecutive passes)
Actual measurement time 101.0 s (minimum 100 s)
Samples per pipeline log 100-100 across x logs (minimum 100)
(each interval is measured on its own; the count is not accumulated)
passing intervals increase the count during ramp-up, and the final density is accepted only after the run acceptance criterion (two consecutive passes) is met
Throughput threshold target x 0.95 (pass tolerance ratio)
stream camera workload target throughput threshold measured (avg) p10 p90 seconds below throughput threshold result
Result: Pass - stream density x streams
The run was evaluated after a 10 second warmup period (INIT_DURATION), using the run acceptance criterion (two consecutive passes) over consecutive measurement intervals. All streams met the 14.25 FPS throughput threshold. The lowest-throughput stream was pipeline_stream9 (cam1 (asc_object_detection_yolo11n_classification_effnetb0)), targeting 15.00 FPS and measuring 14.688796 FPS against its 14.25 FPS throughput threshold, leaving 0.44 FPS (3.1% 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 - stream density x streams
The run was evaluated after a 10 second warmup period (INIT_DURATION), using the run acceptance criterion (two consecutive passes) over consecutive measurement intervals. All streams met the 14.25 FPS throughput threshold. The lowest-throughput stream was pipeline_stream9 (cam1 (asc_object_detection_yolo11n_classification_effnetb0)), targeting 15.00 FPS and measuring 14.688796 FPS against its 14.25 FPS throughput threshold, leaving 0.44 FPS (3.1% relative headroom).
the duration of that phase. Compare that value against the target.
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