Skip to content

feat(stream-density)!: judge pass/fail using time-weighted FPS - #254

Open
sumanaga wants to merge 12 commits into
intel-retail:mainfrom
sumanaga:new-changes
Open

sumanaga wants to merge 12 commits into
intel-retail:mainfrom
sumanaga:new-changes

Conversation

@sumanaga

@sumanaga sumanaga commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

PR Checklist

  • Added label to the Pull Request for easier discoverability and search
  • Commit Message meets guidelines as indicated in the URL https://github.com/intel-retail/performance-tools/blob/main/CONTRIBUTING.md
  • Every commit is a single defect fix and does not mix feature addition or changes
  • Unit Tests have been added for new changes
  • Updated Documentation as relevant to the changes
  • All commented code has been removed
  • If you've added a dependency, you've ensured license is compatible with repository license and clearly outlined the added dependency.
  • PR change contains code related to security
  • PR introduces changes that breaks compatibility with other modules (If YES, please provide details below)

What are you changing?

  1. Stream density: define the measurement window in seconds, and delete the unused fail threshold #250
    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.
  2. Stream density: report the rate that was measured and the rule that was applied [Stream density: report the rate that was measured and the rule that was applied #247]
    1. The stream-density benchmark resolves target FPS in this order:
      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).

  1. Replace the per-stream percentile with frames processed during the measurement phase, divided by
    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

  1. To override default measurement window
    make benchmark-stream-density
    MEASUREMENT_WINDOW_SECONDS=60
  2. To override TARGET_FPS in the configuration file
    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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 High severity · 3 Medium severity · 1 Low severity

Open (6)
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.

Comment thread benchmark-scripts/stream_density.py Outdated
Comment thread benchmark-scripts/stream_density_test.py Outdated
Comment thread benchmark-scripts/benchmark.py
Comment thread benchmark-scripts/stream_density.py Outdated
Comment thread benchmark-scripts/stream_density.py
Comment thread benchmark-scripts/stream_density.py Outdated
@sumanaga
sumanaga marked this pull request as ready for review September 24, 2026 08:03

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. 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.63 and 15.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.

  2. 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.

  3. 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 ?= 60 and MEASUREMENT_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_success also 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.

@jcork-intel

Copy link
Copy Markdown

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 benchmark.py and the LP stream density settings are in intel-retail/documentation: docs/user-guide/documentation/benchmark.md and docs/user-guide/loss-prevention/lp-benchmarking.md. Both will be out of date once this merges: there's no --measurement_window_seconds, --init_duration shows the old default, and the target FPS fallback order is the old one. Could you open a PR there alongside this one? I've put the details in my review on loss-prevention#359.

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: <link>").

@jcork-intel

Copy link
Copy Markdown

On commit messages: this repo's CONTRIBUTING.md asks for Conventional Commits. Before merge, could the six Update <file>.py commits be reworded, or squashed into one with a conventional message, for example feat(stream-density): measure inside a fixed window and drop the unused fail threshold? PRs here merge with merge commits, so these titles go into main's history as they are, and "Update stream_density.py" only repeats the file list.

@jcork-intel

Copy link
Copy Markdown

Correction to my follow-up about the docs: the live page is docs_src/performance-tools/benchmark.md, not the docs/user-guide/ copy. Details on loss-prevention#359.

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>
@sumanaga

sumanaga commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor Author

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.

  1. The "measured" column shows the 90th percentile, not the measured rate. "Report the rate that was measured and the rule that was applied" (Stream density: 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 Stream density: report the rate that was measured and the rule that was applied #247, so this is in scope. In the sample report, 15 fps cameras show measured 16.63 and 15.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 Stream density: report the rate that was measured and the rule that was applied #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" (Stream density: decide pass or fail on frames processed divided by measurement time #245). I'm confirming that with the EDO team this week, so this PR doesn't need to change the rule.
    [Made this change: Added p90, seconds below pass mark]
    Didn't change the pass/fail rule as yet to get the confirmation

stream            camera  workload                                            target  pass mark       p90   seconds below pass mark  result
pipeline_stream0  cam1    asc_obj_detection_age_prediction_face_detection      15.00      14.25     15.94              62/100 (62%)    pass
  1. 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.
    [Made the default value changes]
  2. 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 ?= 60 and MEASUREMENT_WINDOW_SECONDS ?= 30. I'll ask for the same values there, a 10 second settle and a 100 second window.
    [Made the default value changes]

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_success also asserts the stream value is the top sample (the p90 rule), so it will need updating when "measured" becomes the average.
    [Implemented in this PR]
  • 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?
    [Will create a task, targeted Sprint 3]
  • All six commits are titled "Update .py". A one-line description per commit would make the history readable.
    > [Will be done when merging using squash and merge option]
    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.

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.

  2. 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.

  3. p10: as discussed on the 29 Sep call, please report p10 alongside p90.

  4. Minimum samples: the 100 s default gives about 100 samples. How is the 100-sample minimum enforced if someone sets a shorter measurement interval?

  5. 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)"
  6. 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?

  7. 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>
@sumanaga sumanaga changed the title Enhancements/Fixes(2026.3.0) Sprint 1 feat(stream-density)!: judge pass/fail using time-weighted FPS Oct 7, 2026
@sumanaga

sumanaga commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor Author

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:

  1. 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.
    Changes Implemented
    During the measurement interval, each FPS sample is paired with the time it covers.
    The benchmark estimates total frames by summing sample FPS × sample duration, then divides by the total sampled duration.
    This gives a time-weighted average FPS for the interval; it’s an estimate because the counter rounds its values.

  2. 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.

Changes Implemented

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.1 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
pipeline_stream0   cam1    asc_obj_detection_age_prediction_face_detection     15.00                 14.25       14.824225    13.85    15.96                       23.07 s (23%)    pass
  1. p10: as discussed on the 29 Sep call, please report p10 alongside p90.
    Implemented
  1. Minimum samples: the 100 s default gives about 100 samples. How is the 100-sample minimum enforced if someone sets a shorter measurement interval?
  • The collector waits for the configured interval, then counts samples in each expected log.
  • If any log has fewer than 100 samples, collection extends, polling once per second.
  • It stops when every log has 100 samples or when elapsed time reaches twice the configured interval.
  • If any log is still short, the attempt is reported as inconclusive and exits before the FPS pass/fail calculation.
  1. 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)"
      Implemented
  2. 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?
    Added unit test case

  3. 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.
    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>
@sumanaga
sumanaga requested a review from jcork-intel October 7, 2026 11:29

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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) and build_per_stream_camera_meta (line 231) both work out a stream's camera as stream index % number of cameras. That assumes the pipeline_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's stream1_* comes first, then every lane's stream2_*, and so on. run-pipeline.sh in 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, stream29
    

    The 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 fps becomes its fallback target, so any config with cameras at different frame rates gets the wrong thresholds, even without targetFps.

    Suggested fix for both functions: with lanes = streams / cameras, the camera for log i is block i // lanes in 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.

  2. A multi-lane test for the target map. test_build_per_stream_target_fps_mixed_values has 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.

  3. An inconclusive interval ends the sweep as a fail. When the sample floor isn't met, run_pipeline_iterations returns with meet_target_fps set 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 in validate_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>
@sumanaga

sumanaga commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

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:

  1. 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) and build_per_stream_camera_meta (line 231) both work out a stream's camera as stream index % number of cameras. That assumes the pipeline_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's stream1_* comes first, then every lane's stream2_*, and so on. run-pipeline.sh in 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, stream29
    

    The 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 fps becomes its fallback target, so any config with cameras at different frame rates gets the wrong thresholds, even without targetFps.
    Suggested fix for both functions: with lanes = streams / cameras, the camera for log i is block i // lanes in 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.
    Fixed this at the source instead of working out the camera order after the fact, because i // lanes only works with one branch per camera. The generator can create several branches for one camera (one per distinct model/device signature among its workloads) and skips lp_vlm cameras, so the number of logs per lane doesn't always equal the number of cameras.
    -loss-prevention#359: gst-pipeline-generator.py now names each counter stream_l<lane:04d>_c<camera:03d>_b<branch:02d>. Sorting by name therefore matches the order the pipeline is built in, so run-pipeline.sh writes the logs in lane order.
    Example 1: 2 lanes, 2 cameras, one branch each
    stream_l0000_c000_b00: lane 1, cam1 → pipeline_stream0
    stream_l0000_c001_b00: lane 1, cam2 → pipeline_stream1
    stream_l0001_c000_b00: lane 2, cam1 → pipeline_stream2
    stream_l0001_c001_b00: lane 2, cam2 → pipeline_stream3
    -Manifest: the generator also writes pipelines/pipeline_streams.json. For each log it records log_index, lane, the camera's index in the original config, camera_id, branch and the branch's workloads. run-pipeline.sh checks the manifest against the pipeline's sources and copies it to results.
    -performance-tools#254: build_per_stream_target_fps and build_per_stream_camera_meta now look up each log's camera in the manifest instead of using % cameras. Report labels use the branch's own workload. If the manifest and the logs disagree, the run stops with an error. Without a manifest it falls back to % cameras and prints a warning; that fallback is correct for one-branch-per-camera configs now that the logs are in lane order.
    Checked with a two-lane run with cam1 at 15 fps and cam2 at 5 fps: logs 1 and 3 (cam2) got the 5 fps target, and logs 0 and 2 (cam1) got 15.

  1. A multi-lane test for the target map. test_build_per_stream_target_fps_mixed_values has 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.
    Added test_build_per_stream_target_fps_camera_ordered_logs_above_one_lane next to the 30-stream count test. It uses 6 cameras with cam6 at fps: 5 and checks that only stream10/stream11 (2 lanes) and stream25–stream29 (5 lanes) get the 5 fps target. Its manifest is in camera order, where % cameras gives the wrong answer, so the test fails if the code goes back to that guess.
    With the new lane-first names, real runs write cam6 to stream5 and stream11. That order is covered by test_target_fps_uses_manifest_for_multiple_lanes, and by a generator test in #359 that checks the names sort in lane order with 12 cameras and 11 lanes.
    Also added tests for cameras with several branches, skipped cameras, and a manifest that doesn't match the logs.
  1. An inconclusive interval ends the sweep as a fail. When the sample floor isn't met, run_pipeline_iterations returns with meet_target_fps set 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 in validate_and_setup_env, or saying so in the help text, would save someone a wasted run.
    Done:
    -An inconclusive interval is retried once at the same lane count, without restarting the pipelines.
    If the retry is also inconclusive, the run returns a new inconclusive status. run_pipeline_iterations and run_stream_density now return a status (pass / fail / inconclusive) instead of a boolean.
    -benchmark.py prints Result: Inconclusive and the last passing density, labelled as not confirmed by the run acceptance criterion, or "No measurement interval had passed" if no step passed. It reports the last passing density rather than the last confirmed one because meeting the acceptance criterion already ends the run as a pass. The returned count is now always a measured lane count, never the next step that was never measured.
    -validate_and_setup_env rejects MEASUREMENT_WINDOW_SECONDS below 100, and the --measurement_window_seconds help text explains why.
    -The README in loss prevention line in #359 now describes the retry and the inconclusive result.

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.
    Fixed. Both result branches now go through one helper that prints "use case density N lanes, stream density M streams", matching 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.
    Fixed. The "measured (avg)" column and the summary sentence print two decimals. The RATE-CALC line in stream_density.log keeps full precision for debugging.

@jcork-intel jcork-intel left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Sumana. Fixing the camera order at the source with the manifest is better than what I suggested. Approved.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stream density: define the measurement window in seconds, and delete the unused fail threshold

3 participants