Repository navigation
docs(specs): viewport fidelity indicator specification #6254
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
09eb881
0f0fc42
9fdfe6b
bf48306
c54066c
7d14d3d
f1e4550
8df00aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| --- | ||
| sidebar_position: 0 | ||
| sidebar_label: Overview | ||
| title: Specifications | ||
| summary: How OHIF writes behavioural specifications in EARS format, the requirement-ID register, and the index of component specifications. | ||
| --- | ||
|
|
||
| # Specifications | ||
|
|
||
| This section holds behavioural specifications for OHIF, written in **EARS** (Easy Approach to | ||
| Requirements Syntax). A specification says *what the viewer must do* and is stable across | ||
| implementations; a companion design document says *how we intend to build it* and is expected | ||
| to change. | ||
|
|
||
| Each specification covers one **component** — a coherent area of behaviour — and owns a | ||
| reserved requirement-ID prefix so that specifications can cite one another without collisions. | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Index of component specifications | ||
|
|
||
| | Prefix | Component | Specification | Design | Status | | ||
| | --- | --- | --- | --- | --- | | ||
| | `VF` | **Viewport fidelity** — the indicator and the detail view that tell the user when the displayed image is not a faithful rendering of the stored pixel data | [Viewport fidelity indicator](./viewport-fidelity/requirements.md) | *pending* | Draft | | ||
|
|
||
| ### 1.1 Reserving a prefix | ||
|
|
||
| Add a row to the table above before writing the specification. A prefix is two uppercase | ||
| letters, unique across this section, and never reused after retirement. | ||
|
|
||
| --- | ||
|
|
||
| ## 2. Requirement identifiers | ||
|
|
||
| ``` | ||
| <PREFIX>-<GROUP>-<n> | ||
| │ │ └── sequence within the group, never renumbered once published | ||
| │ └───────── short uppercase group name, scoped to the prefix | ||
| └────────────────── the component prefix reserved in §1 | ||
| ``` | ||
|
|
||
| Examples: `SB-COMP-2`, `RS-SAVE-11`, `SG-EDIT-4`. | ||
|
|
||
| **Identifiers are stable.** Once a specification is merged, an identifier is never reassigned | ||
| to different behaviour. To retire a requirement, mark it *Withdrawn* in place and leave the | ||
| number occupied; design documents, tests, and pull requests cite these identifiers and must not | ||
| silently start meaning something else. | ||
|
|
||
| **Cross-component citation** uses the full identifier. A segmentation specification that | ||
| inherits the sidebar contract writes "the segmentation sidebar shall satisfy `SB-*`" rather | ||
| than restating those requirements. | ||
|
|
||
| --- | ||
|
|
||
| ## 3. EARS patterns | ||
|
|
||
| Every requirement uses exactly one of these forms, or a documented combination. *The system* | ||
| means the OHIF Viewer application unless the specification narrows it. | ||
|
|
||
| | Pattern | Form | Use for | | ||
| | --- | --- | --- | | ||
| | **Ubiquitous** | The system shall `<response>`. | Behaviour that always holds. | | ||
| | **Event-driven** | WHEN `<trigger>`, the system shall `<response>`. | A response to something happening. | | ||
| | **State-driven** | WHILE `<state>`, the system shall `<response>`. | Behaviour that holds for the duration of a state. | | ||
| | **Optional feature** | WHERE `<feature is included>`, the system shall `<response>`. | Behaviour that exists only in some configurations or deployments. | | ||
| | **Unwanted behaviour** | IF `<trigger>`, THEN the system shall `<response>`. | Error handling, refusal, degradation. | | ||
| | **Complex** | Any of the above combined, e.g. WHILE `<state>`, WHEN `<trigger>`, the system shall `<response>`. | Use sparingly; prefer splitting. | | ||
|
|
||
| ### 3.1 House rules | ||
|
|
||
| 1. **One requirement, one sentence, one `shall`.** If a requirement needs "and also", split it. | ||
| 2. **No design in a requirement.** Name the observable behaviour, not the class, hook, or | ||
| store that produces it. Rationale, algorithms, and file layout belong in the design document. | ||
| 3. **Testable or it is not a requirement.** Each one must map to something a unit test or a | ||
| Playwright test can assert. Include a verification table (see §4). | ||
| 4. **Use `WHERE` for deferred scope.** Behaviour that is out of scope now but must not be | ||
| designed out is written as an optional-feature requirement with a note, rather than omitted. | ||
| This reserves the seam without committing to the work. | ||
| 5. **Notes are not requirements.** Anything in a `>` blockquote is rationale and carries no | ||
| `shall`. | ||
| 6. **Answer the open questions.** A specification that inherits open questions from an issue | ||
| states its resolution for each, or records it as deliberately still open. | ||
|
|
||
| --- | ||
|
|
||
| ## 4. Expected structure of a specification | ||
|
|
||
| ``` | ||
| 1. Purpose why this exists, and the issue or discussion it comes from | ||
| 2. Scope what is in, what is deferred, and what the deferral constrains | ||
| 3. Definitions every term the requirements use in a narrow sense | ||
| 4..n Requirements grouped, each group with its own <PREFIX>-<GROUP> identifier | ||
| n+1 Open questions resolved and still-open, with the resolution stated | ||
| n+2 Traceability requirement → source issue section, and → superseded issues | ||
| n+3 Verification requirement group → how it is tested | ||
| ``` | ||
|
|
||
| A companion `design.md` in the same folder cites the identifiers rather than restating them. | ||
|
|
||
| ### 4.1 User requirements and implementation requirements | ||
|
|
||
| A specification has two requirement sections, and the two sections stay clearly separate: | ||
|
|
||
| 1. **User requirements**: what the user must be able to do, or must be able to determine, to | ||
| work safely. | ||
| 2. **Implementation requirements**: how the work is done, or the recommended approach. | ||
|
Comment on lines
+102
to
+106
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Align the stability rule with the two-section model. Lines 10–13 define specifications as stable and implementation-independent. Lines 102–106 define implementation requirements as how the work is done. The linked
|
||
|
|
||
| Each group has its own `<PREFIX>-<GROUP>` identifiers, so a reader sees immediately which section | ||
| holds a requirement. The Scope section of the specification states the change rule: | ||
|
|
||
| - An implementation requirement can change when a better approach appears. | ||
| - A user requirement changes only when it describes the behaviour wrongly, or when the intended | ||
| user-facing behaviour changes. An implementation that is inconvenient is not a reason to change | ||
| a user requirement. | ||
|
|
||
| This is the preferred convention for new specifications. A specification that is older than the | ||
| convention is not changed to follow it, unless a separate change asks for that. The first example | ||
| is the [viewport fidelity indicator](./viewport-fidelity/requirements.md). | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| # [Feature Request] Viewport fidelity indicator | ||
|
|
||
| **Assignee:** @dan-rukas (design) | ||
| **Labels:** `Community: Request :hand:` | ||
| **Funded scope:** OHIF / FU Berlin Set-Aside, task **T6** (Aim 2.3, required) | ||
| **Specification:** `platform/docs/docs/development/specs/viewport-fidelity/requirements.md` | ||
| **Related:** OHIF implementation issue and Cornerstone3D fidelity-state issue (T3), both | ||
| @mbellehumeur | ||
|
|
||
| --- | ||
|
|
||
| ## What feature or change would you like to see made? | ||
|
|
||
| When OHIF cannot render stored pixel data as requested, it renders something else instead — a | ||
| decimated volume, a partially decoded image, a reduced output resolution, a coarser sampling | ||
| density. None of that is visible to the user today. The image simply looks the way it looks. | ||
|
|
||
| Add **one consistent per-viewport fidelity indicator** that appears when the displayed image is | ||
| lossy, plus a detail view saying what was lost, why, and whether it will improve. | ||
|
|
||
| **The premise, and it is a hard requirement rather than a preference** (§4.1 `VF-AUTO`): the | ||
| system decides what to render, not the user. The previous decimated-views attempt failed | ||
| because it made users dictate the settings needed to view things quickly, burdening them with | ||
| detail they should never have had to hold. Users do not want to configure a renderer; they want | ||
| to read a study. An indicator is what makes automatic decision-making acceptable rather than | ||
| opaque — the system decides silently and continuously, and the indicator discloses when a | ||
| decision cost the user something. Anything that answers "the image might be degraded" by | ||
| handing the user controls has reproduced the original failure with more steps. This is the | ||
| substantive reason the indicator and its detail view are **read-only**. | ||
|
|
||
| The model is a state machine over lossiness *through time*, not a severity scale: | ||
|
|
||
| | State | Indicator | Meaning to the user | | ||
| |---|---|---| | ||
| | Never lossy | *nothing shown* | Nothing has been lost here. | | ||
| | No data yet | *nothing shown* | Not a fidelity question. | | ||
| | Transiently lossy | Yellow | Lossy now, expected to improve. | | ||
| | Terminally lossy | Red | Lossy now, and this is as good as it gets. | | ||
| | Formerly lossy | Green, ~1s | Was lossy; is not any more. | | ||
|
|
||
| The indicator never appears for a viewport that has never been lossy — its presence is itself | ||
| information. A progressive load shows yellow while arriving, then green for about a second on | ||
| reaching full resolution, or **red** if it completes without ever getting there. | ||
|
|
||
| Clicking the indicator opens a read-only detail view with three sections: how much was lost | ||
| (`Decimated volume x/2, y/2, z/4`), why (`This GPU only supports textures up to <n>` versus | ||
| `The requested performance target is too slow at this size`), and the interactive performance | ||
| operating point (`Output resolution 50% to maintain 30 fps`). | ||
|
Comment on lines
+45
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Define non-pointer activation for the detail and recovery states. This request makes the detail view click-based and makes the recovered state hover-based. The requirements use activation and require the state to remain re-revealable, but these instructions do not define keyboard focus, keyboard activation, or touch behavior. Add an equivalent non-pointer path and test it; otherwise some users cannot complete Also applies to: 62-66 🤖 Prompt for AI Agents |
||
|
|
||
| **Design scope (@dan-rukas):** icon, colours and their accessibility treatment, detail-view | ||
| container and layout, behaviour at small viewport sizes, and the discoverability problem below. | ||
| Plus review of and edits to the user-facing wording as each contributing piece lands, rather | ||
| than authoring it up front. Full requirements, with identifiers, are in the specification linked | ||
| above — §4 is the user requirements and is the acceptance surface; §5.4 is a starting suggestion | ||
| to argue with. | ||
|
|
||
| Two problems worth attention before pixels: | ||
|
|
||
| 1. **Red and yellow differ only in whether waiting helps**, and the image gives the user no | ||
| independent clue which they are looking at. Colour alone carries that entire distinction, | ||
| which argues for separating them by glyph or shape as well. | ||
| 2. **The recovered state is deliberately hard to see.** Green shows for ~1s and then withdraws | ||
| completely, leaving no mark over the image, because anything resident over the image can | ||
| hide a finding — on a mammogram the corners carry diagnostic content. So the hover target | ||
| that re-reveals it must exist while nothing is drawn there, and a user not looking at that | ||
| viewport during the announcement will not learn it recovered. | ||
|
|
||
| ## Why should we prioritize this feature? | ||
|
|
||
| It is a required, funded deliverable (T6) and a prerequisite for the adaptive rendering work | ||
| being safe to enable by default. The point of Aim 2.3 is to substitute lower-fidelity renderings | ||
| automatically when a device limit or a performance target demands it — and substituting pixels | ||
| silently, with no disclosure, is not something to ship. A user who cannot distinguish "this | ||
| lesion is small" from "this rendering discarded three quarters of the z-axis" is being asked to | ||
| trust a picture whose provenance is hidden from them. | ||
|
|
||
| It interacts with existing features rather than sitting beside them: the existing loading and | ||
| progress affordances keep answering "how far along is this load", while the indicator answers | ||
| "can I trust what I am seeing yet". The detail view is the single place the two are reconciled. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| # Viewport fidelity state and events | ||
|
|
||
| **Repository:** `cornerstonejs/cornerstone3D` | ||
| **Assignee:** @mbellehumeur | ||
| **Funded scope:** OHIF / FU Berlin Set-Aside, task **T3** (Aim 2.3, required) | ||
| **Consumed by:** OHIF viewport fidelity indicator (T6) — see the OHIF feature request for the | ||
| `VF-` requirement identifiers cited below. | ||
|
|
||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| Cornerstone3D needs to expose, per viewport, what fidelity was requested versus what is | ||
| actually being displayed — as a defined interface with viewport events, plus example | ||
| implementations wired to the existing progressive loaders. | ||
|
|
||
| This is the prerequisite for the OHIF fidelity indicator (T6). It is also read directly by the | ||
| T11 measurement harness and the T18 final comparison, both of which record "current viewport | ||
| fidelity state", so it must be assertable without going through a UI. | ||
|
|
||
| ## Scope | ||
|
|
||
| 1. **A defined class and interface surface** representing viewport fidelity information | ||
| (`VF-CS3D-1`). | ||
|
|
||
| 2. **Viewport events** emitted when fidelity information changes (`VF-CS3D-2`). A getter alone | ||
| is not sufficient — the OHIF indicator subscribes rather than polls (`VF-STATE-3`), and must | ||
| update without a study reload (`VF-STATE-2`). | ||
|
|
||
| 3. **The information carries, separately** (`VF-CS3D-3`): | ||
| - the affected **dimension** — data resolution, display resolution, sampling density, | ||
| decoded extent, pixel value quantisation, transform approximation, or rendering path; | ||
| - the **cause** — a device capability limit, a performance target, an incomplete load, or an | ||
| irreversibly lossy decode; | ||
| - the **quantified magnitude** — per-axis decimation factors, decoded bytes, output | ||
| resolution percentage, sampling ratio. Consumers must be able to render | ||
| `Decimated volume x/2, y/2, z/4` or `Progressive decode, 32 kB initial`, not just | ||
| "reduced quality"; | ||
| - whether the difference is **transient or standing**. | ||
|
Comment on lines
+30
to
+39
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Define the T3/T6 state and event contract before implementation.
Also applies to: 72-75 🤖 Prompt for AI Agents |
||
|
|
||
| Separating dimension from cause is what lets a consumer render "showing less detail" | ||
| alongside "because this GPU cannot hold the full volume" without a combinatorial string | ||
| table. | ||
|
|
||
| 4. **The transition must be observable, not just the current value.** A progressive load that | ||
| *completes without reaching full resolution* moves from transient to standing, and that | ||
| transition is the single most important thing the indicator reports (`VF-INFO-5a`) — a | ||
| stalled lossy load is visually indistinguishable from one still in flight. | ||
|
Comment on lines
+45
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Define The requirements define terminally lossy as not expected to improve for the current data, viewport, and device. This file only defines 🤖 Prompt for AI Agents |
||
|
|
||
| 5. **Example implementation for stack progressive loading** (`VF-CS3D-4`). | ||
|
|
||
| 6. **Example implementation for volume progressive loading** (`VF-CS3D-5`). | ||
|
|
||
| 7. **Both examples source their settings and values from the existing progressive loaders** | ||
| (`VF-CS3D-6`) rather than introducing a parallel configuration path. | ||
|
|
||
| 8. **Readable programmatically without a UI** (`VF-CS3D-7`). | ||
|
|
||
| ## Relationship to `ImageQualityStatus` | ||
|
|
||
| `ImageQualityStatus` (`FAR_REPLICATE = 1`, `ADJACENT_REPLICATE = 3`, `SUBRESOLUTION = 6`, | ||
| `LOSSY = 7`, `FULL_RESOLUTION = 8`) stays exactly as it is, with its consumers unchanged | ||
| (`VF-CS3D-8`). It is internal to progressive image loading: per-**image**, numeric, ordered, | ||
| readable via `cache.getImageQuality(imageId)` and carried on `IImage.imageQualityStatus`. | ||
|
|
||
| The new state is per-**viewport** and distinguishes requested from displayed — a different | ||
| shape, not a replacement. The viewport state may draw on `ImageQualityStatus` as one input | ||
| among several. | ||
|
|
||
| ## Notes for implementation | ||
|
|
||
| - `Events.IMAGE_RETRIEVAL_STAGE` fires from `ProgressiveRetrieveImages` with detail | ||
| `{stageId, numberOfFailures, numberOfImages, stageDurationInMS, startDurationInMS}` — no | ||
| fidelity field — and has **no OHIF subscribers today**. Either its detail gains the | ||
| information or a new per-viewport event carries it. | ||
| - Contributing sources of state, as those tasks land: **T9** records the pre-allocation | ||
| downsample and its reason; **T14** the active interaction reduction; **T15** the ordered | ||
| fallback reason; **T4** the device-class limits that make a cause explicable. **T13**, if | ||
| delivered, adds rendering-path substitution. | ||
| - T14 degrades fidelity every frame during rotate, zoom and pan by design, so the state changes | ||
| at interaction rates. It must be truthful frame-by-frame; the consumer is responsible for | ||
| presenting that unobtrusively (`VF-STATE-5`), not the state for smoothing it. | ||
| - Per-image granularity matters to consumers: the OHIF indicator reports the state of the image | ||
| *currently displayed*, not the worst across the display set (`VF-VIS-4a`). | ||
|
|
||
| ## Out of scope | ||
|
|
||
| - Detection, prevention, or recovery from insufficient browser memory (excluded by T9). | ||
| - The GPU taxonomy itself (T4), the level binding (T9), and the selection policy (T15). | ||
| - Any settable fidelity API. The state is queryable and observable; selection stays a | ||
| deterministic side-effect-free decision of the T15 policy. | ||
| - Fidelity of overlay layers such as segmentation labelmaps — acknowledged gap, unfunded. | ||
|
|
||
| ## Verification | ||
|
|
||
| Unit and example-level tests in the Cornerstone3D suite: dimension, cause, magnitude and | ||
| persistence populated correctly per fixture; events fire on change; the transient→standing | ||
| transition on a load that completes lossy; state assertable without a UI. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use a full identifier in the cross-component citation example.
The rule requires a full identifier, but the example uses
SB-*, which is a prefix wildcard. Replace it with a concrete identifier such asSB-COMP-2, or define the wildcard form and its traceability semantics.