Repository navigation
feat: @dcmjs-org/video — MP4 wrapping, image datasets, and the video event source - #585
Draft
awatson1978 wants to merge 155 commits into
Draft
awatson1978 wants to merge 155 commits into
awatson1978 wants to merge 155 commits into
Conversation
Supersedes the 2026-08-07 plan. Describes the package split, the 1.0-beta release branch, per-package staging branches, the test-first sequence for core, routing of the 33 review findings, and lockstep publishing with semantic-release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two trivial assertions on the committed sample-sr.dcm fixture. The purpose is to exercise the CI checks on the 1.0-beta branch and prove they run and report on pull requests. See RELEASE_PLAN.md section 6, step 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 24 fs.readFileSync can return a small file inside a larger shared memory pool. Taking .buffer on that result hands the parser the whole pool, with the file sitting at a nonzero offset, and the DICM marker check then fails. Node 24 pools more aggressively, which is why anonymizer.test.js and data.test.js fail there today (7 tests) while Node 22 passes by luck of alignment. This adds readFileAsArrayBuffer() to testUtils and routes all 22 file-read sites through it. Typed-array .buffer uses are untouched. Same class of problem as issue #311. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ware helper The first pass converted single-line fs.readFileSync(...).buffer sites. CI on Node 24 then failed in a second population: reads assigned to a variable first, with .buffer taken on a later line. This converts those sites in data.test.js, data-options.test.js, lossless-read-write.test.js and normalizers.test.js, and drops the imports that became unused. Sites that already handle byteOffset correctly are untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ensureTestDataDir checked for the directory and then created it. Two parallel jest workers can both pass the check, and the slower mkdir then fails with EEXIST, which failed test_multiframe_1 on one CI leg. mkdirSync with recursive:true succeeds whether or not the directory exists, so the check goes away. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three process documents the release plan calls for. The style guide defines the audience (healthcare-literate, no assumed programming or imaging jargon) and bans machine-flavored prose. The PR template requires plain-language descriptions, runnable examples, test evidence, and fixture citations. DATACITATION.md records the origin and license of every committed DICOM test file, with the legacy files honestly marked as provenance-pending. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
semantic-release computes one version from conventional commits; stamp-workspace-versions.mjs writes it into every publishable workspace package.json, and pnpm -r publish releases them together under the npm beta tag. The publish workflow now also fires on 1.0-beta and release/0.5x, and passes NODE_AUTH_TOKEN for pnpm publish. Nothing publishes until the repository owner adds 1.0-beta to the publish environment allowlist. See RELEASE_PLAN.md sections 9 and 12. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adapts the rules from Cursor's unslop skill: the fuller AI-vocabulary list, filler phrases, fancy synonyms for is, vague attributions, synonym cycling, false ranges, punctuation and formatting rules (em dashes, colons, bold, sentence-case headings, straight quotes), abstract metaphor nouns, and a plain-speech section that includes the rule against over-compressed arrow-speak. Also states the boundary this guide draws that unslop does not: metaphors that teach a concept to a non-technical reader stay, metaphors that decorate go. The guide's own prose now follows its own em-dash rule. Source: https://github.com/cursor/plugins/blob/main/pstack/skills/unslop/SKILL.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adopts wayfarer3130's review comments: the engines split from the data types (core = building blocks only; legacy = today's readers and writers; parser = the new streaming reader; generator = the new writers). The vendored dicom-parser tokenizer is dropped from the 1.0 stream entirely, and about 300 tests retire with it. @dcmjs/media is added as a bundle over dicomdir, pdfs, and video, which all remain individually installable. The wrapper publishes as @dcmjs/dcmjs during the beta so the real dcmjs name stays untouched until the team is ready. FHIR stays in the plan with its landing order marked as an open question. Section 7 is rewritten to the simpler tests-travel-with-code approach discussed since the first draft. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s sequencing Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e git mv) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…hrough fixturePath() Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: move committed test DICOM fixtures into packages/fixtures
…regression bank Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: add the issue triage catalog and part10 walker for the regression bank
…avior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gth behavior Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rdering Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d strict-write Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… value-handling behaviors Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
test: pin round-trip integrity, UUID-derived UIDs, and UN padding behavior
test: pin encapsulated write, latin1 fallback, and CS multi-value length behavior
test: pin private tag handling, undefined value writes, and element ordering
Hand-written rewiring for the pure-move commit before it: - the writers import the contract from @dcmjs/parser (EventStreamListener, CollectorListener) and the write machinery from @dcmjs/core (WriteBufferStream, the syntax constants, writeDataSet, writeTagObject); StreamingPart10Writer and DicomWebJsonWriter carry no other edges - Part10Writer keeps its by-design delegation to the canonical DicomDict.write encoder, now as an explicit — and TEMPORARY — @dcmjs/legacy import, flagged in the package README and the PR - a shim at each old path under src/eventStream/ re-exports from the new packages/generator location, so every existing import in src/, test/, and the barrel keeps working unchanged - packages/generator/src/index.js exports the three sinks; the eventStream barrel and the DicomEventStream facade stay in root src/ as wrapper-level glue, unchanged Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: split the event-stream writer sinks into packages/generator
The @dcmjs npm organization is not ours; per review discussion the packages publish under @dcmjs-org (matching the GitHub organization), and the team can reconsider the shorter scope if its owner responds. Every package name in the plan now reads @dcmjs-org/. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
refactor: publish the workspace packages under the @dcmjs-org scope
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@dcmjs-org/core, @dcmjs-org/legacy, @dcmjs-org/parser, and @dcmjs-org/generator drop their scaffold-era private flags so the lockstep publish can ship them. @dcmjs-org/fixtures stays private per RELEASE_PLAN section 11, and the root package stays private until the wrapper PR. Publishing remains gated behind the publish GitHub environment — merging this changes nothing on npm. BREAKING CHANGE: dcmjs 1.0 replaces the single monolithic package with the @dcmjs-org scoped package set (core, legacy, parser, generator, schemas). The legacy package preserves today's reader and writer surfaces; the event-stream engines are new. See RELEASE_PLAN.md and docs/BREAKING_CHANGES.md for the behavior changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat: mark the engine packages publishable
The package shell for the video wave: name, entry point, MIT license, repository directory, and the @dcmjs-org/core workspace dependency. The image-module index exports only the header parsers for now; the dataset builder joins it in the next slice. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verbatim from v2.0-development: src/image/jpegInfo.js (SOF geometry and the JPEG transfer syntax mapping), src/image/mp4Info.js (the ISO BMFF box walk over a random-access reader), the makeTinyMp4 synthesized-fixture helper, and the ported mp4Info suite. Only the test import paths moved with the files; no behavior changed in this commit — review findings 9 and 15 are fixed on top, each with its regression observed red first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 9 (HIGH): the marker-walk guard only required four readable bytes, but the SOF fields span bytes[offset+4 .. offset+9]. Out-of-range reads coerce to 0 in the bitwise math, so the review map's vector — a 480x640 JPEG cut one byte into the width field — returned columns 512 instead of throwing, and a file cut right after the length field returned rows 0, columns 0. The reader now validates that the declared SOF segment fits inside the input (and is at least 8 bytes) before reading any field. The new jpegInfo suite is hand-written (v2 carried no suite for this file): the exact review vector, the cut-after-length shape, and valid SOF0/SOF1/SOF2/SOF3 paths. Both finding tests were observed red against the ported-unfixed reader before this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 15 (MED): the stts fallback loop trusted the raw uint32 entry_count. With stsz sample_count 0 and stts entry_count 0x00FFFFFF the parser spun 16.7 million iterations over zero-coerced reads past the box and fabricated numberOfFrames/frameRate from unrelated moov bytes; at 0xFFFFFFFF that is roughly 40 seconds of blocked thread. The loop now requires 16 + entryCount * 8 to fit inside the stts box and throws on the mismatch. The regression corrupts a synthesized MP4 exactly that way and asserts the parse rejects in under two seconds — observed red (the unfixed parser resolved with fabricated values) before this fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat(video): scaffold @dcmjs-org/video with the JPEG and MP4 header parsers
Verbatim from v2.0-development src/image/buildImageDataset.js and its 21-test suite, with two mechanical adjustments for the package boundary: DicomMetaDictionary and validationLog now come from @dcmjs-org/core, and the test imports the builder from the package's image index (which now exports it alongside the header parsers). No behavior changed in this commit — review finding 10 is fixed on top with its regressions observed red first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review finding 10 (HIGH): numberOfFrames came only from decodedImage.numberOfFrames || 1 and was never derived from or checked against options.encapsulated.frames.length, and the attribute was only written when above 1. Three encapsulated fragments therefore produced an instance with no NumberOfFrames — a conformant reader showed frame 1 and the rest were unreachable — while an explicit numberOfFrames 7 over 2 fragments emitted the bogus claim unchecked. In encapsulated mode the fragment list is now the ground truth: the count defaults to frames.length and an explicit mismatch throws. Both regressions were observed red against the ported-unfixed builder before this fix; two companion tests pin that a matching explicit count is accepted and that a single fragment still omits the attribute. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat(video): port buildImageDataset with the encapsulated frame-count fix
Verbatim from v2.0-development src/encapsulated/encapsulatedVideo.js and its 8-test suite, with one mechanical adjustment for the package boundary: DicomMetaDictionary and isVideoTransferSyntax now come from @dcmjs-org/core instead of relative paths. buildVideoDataset shapes the Supplement 225 Video Photographic Image shell (XC modality, cine timing, the (7FE0,0003) uint64 total length as a BigInt), encapsulateVideo splits the MP4 into fragment-sized exact ArrayBuffer slices, and extractEncapsulatedVideo concatenates fragments and truncates to the declared length so the odd-fragment pad byte drops on extraction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The video half of v2's encapsulated index only — the PDF half (encapsulatedPdf) belongs to the media wave. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat(video): port the buffered video encapsulation layer
@dcmjs-org/parser joins the dependencies — the video event source imports fromDataSet and EVENT_STREAM_VOCABULARY — and @dcmjs-org/generator the devDependencies (StreamingPart10Writer, tests only). Deliberately NOT @dcmjs-org/legacy: this package stays off the legacy surface. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
createVideoEventSource — MP4 → contract events with the Supplement 225
dataset shell replayed first and the stream emitted as encapsulated
PixelData fragments, one in memory at a time, backpressure between
fragments via awaitDrain().
The run loop, exactFragment, withoutEndDataSet and sortDict are verbatim
from v2.0-development src/eventStream/fromVideo.js. One seam is reworked
and hand-written: v2 imported datasetToDict from what is now
@dcmjs-org/legacy, which this package must not depend on, so a local
datasetToMetaDict helper mints the same file meta group (element list and
implementation UID copied from legacy's datasetToDict) and denaturalizes
both halves via core's DicomMetaDictionary — returning the plain
{ meta, dict } shape, which is all parser's fromDataSet reads.
The suite is the v2 fromVideo suite reworked off the DicomEventStream
facade (media wave): the source drives StreamingPart10Writer directly
and reads back through fromPart10/fromPart10Stream + NaturalizedListener
+ extractEncapsulatedVideo. All load-bearing assertions carried over:
byte-identical round trips (buffered and streaming reader), the
(7FE0,0003) UV uint64 pinned on the wire, the Sup 225 fragment layout,
single-endDataSet ordering, per-fragment backpressure, and the ffmpeg
corrective error. The facade-specific toVideo/fromPdf case stays with
the media wave, as does fromImage.test.js.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
feat(video): port the video event source, legacy-free
…ented GHSA-vfj7-8cjw-p6xm was published 2026-10-02 with a claimed fix range of >=3.0.4, but no such braces release exists on the registry, so there is nothing to override to. Every path is dev tooling; braces never reaches the published packages. Same entry the fhir staging branch already carries (#589) — remove it the moment a patched braces ships. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 7261970)
Per review on #542: the guide existed to hold the staged-landing PR descriptions to one standard, and every description is now written. Removed rather than kept stale; git history preserves it if the team wants it back later. The PR template keeps the one-line spirit of it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 2a5935e)
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the assembly PR for the video package: it brings
release/1.0-beta-videointo1.0-beta, carrying PRs #576, #578, #581 and #583 on top of the core engine branch, each individually reviewed with its own plain-English description. Like the other assemblies (#542, #543, #575, #577, #584), it's open as a draft on purpose — please don't merge it yet; the landing order and the trunk overlap (details below) mean the assemblies should go in deliberately, not as soon as they're green.What this is in plain terms.
@dcmjs-org/videogets video and camera imagery into DICOM without re-encoding. Its header parsers (parseJpegInfo,parseMp4Info) read the geometry and timing DICOM needs — rows, columns, frame count, frame rate, codec profile — straight from the JPEG or MP4 bytes, without decoding a single pixel, so the compressed stream can travel verbatim into encapsulated PixelData.encapsulateVideowraps an MP4's H.264 stream into a Video Photographic Image instance andextractEncapsulatedVideorecovers it byte-identically.buildImageDatasetgoes the other way for already-decoded pixels, producing a conformant image instance. AndcreateVideoEventSourceis the streaming path: MP4 to contract events, one fragment in memory at a time, feeding any event-stream sink. The package imports only@dcmjs-org/coreand@dcmjs-org/parser— never legacy.A worked example, both directions — run on this branch, output verbatim.
Creating: an MP4 becomes a complete Part 10 file, streamed one fragment at a time (the header parsers recover the geometry; nothing is re-encoded):
Reading: the Part 10 file comes back through the engine, and the original MP4 comes out byte-identical:
The buffered pair works the same way without the event stream:
encapsulateVideo(mp4, attrs)gives the naturalized dataset directly, andextractEncapsulatedVideorecovers the stream from any dataset however it was read.What's in it. Four PRs, in the order they landed:
@dcmjs-org/videopackage (publishable, MIT, 0.0.0 like its siblings),jpegInfo.jsandmp4Info.jswith the synthetic-MP4 test factory (makeTinyMp4), the ported mp4Info suite, and a brand-new jpegInfo suite — v2 never had one.encapsulateVideo/extractEncapsulatedVideoand the fragment helpers, with their 8-test suite.createVideoEventSource, reworked legacy-free: v2'sdatasetToDict(a legacy import) is replaced by a ~29-line local helper producing the plain{meta, dict}shape the parser'sfromDataSetaccepts.The three review findings fixed here, from the PR #512 review map — each regression was written against the ported-but-unfixed code and observed failing first:
parseJpegInfo's scan guard checked only 4 bytes of headroom while the SOF parse reads 10, so a truncated JPEG silently returned wrong geometry (the review's exact vector —FF D8 FF C0 00 11 08 01 E0 02— came back as columns 512 instead of failing). Truncation now throws.buildImageDatasetnever derivedNumberOfFramesfrom the encapsulated fragment list — a three-frame encapsulation produced a dataset with noNumberOfFramesat all. The frame list is now the ground truth: it fills the default and an explicit mismatch throws.parseMp4Info'ssttsfallback trusted the box's raw entry count, so a malformed file claiming 0xFFFFFF entries blocked the thread for tens of seconds and fabricated a frame count. The entry count is now bounded by the box's own size; the regression asserts the malformed file is rejected fast.One open question for maintainers, raised in #576 and deliberately not fixed: 12-bit extended-sequential JPEG currently yields
BitsAllocated 12, but PS3.5 allows only 8 or 16 (correct would beBitsAllocated 16withBitsStored 12). It's a ~5-line fix inbuildImageDatasetif you agree.Test evidence. 913 tests across 75 suites on this branch — 901 passing, 12 skipped (the pre-existing skips), zero failures, on Node 22 and 24. The package itself is 57 tests in 5 suites, including the new jpegInfo suite and the finding regressions. CI is green at the branch tip
58dba59: tests, lint/format, audit. One honest gap, declared in #576: every MP4 in the suite is synthetic (makeTinyMp4) — no real-world camera file exercises the box parser yet. Adding one later means a DATACITATION row and a license check, like every committed fixture.Test usage. From the repo root:
Expected: 5 suites, 57 tests, all passing in a few seconds.
GitHub issues. None map directly — the historical backlog has no video-area issues (video support itself arrived with the 1.0 rewrite). The three fixes above trace to the #512 review map rather than filed issues.
The numbers. The package is +2,100 lines under
packages/video/(about two-thirds verbatim v2 port, declared per-PR; the hand-written remainder is the three fixes, their regressions, the new jpegInfo suite, and the legacy-free rework of the event source) across 4 PRs and their conventional commits, merged (not squashed) so the release tooling sees each one. The raw diff against1.0-betais much larger because this branch is cut from the core engine tip (e5f46e4) — it carries the whole core trunk that draft #542 also carries. Whichever assembly merges first absorbs that trunk, and this diff shrinks to just the video package.Breaking changes. None — this is a new package; nothing under the existing
src/surface changes here. Within the port itself: truncated JPEGs and malformed MP4s that previously produced silently-wrong values now throw, and encapsulated multi-frame datasets gain theNumberOfFramesattribute they were always supposed to carry.Still to come: the
DicomEventStreamfacade factories (fromVideo,fromVideoStream,toVideo,fromImage) land with the media wave, which also carries the review's finding 14 (the lazy source-promise fix) — that wave touches the shared facade file, so it sequences after the three bulk-data packages rather than inside any one of them.🤖 Generated with Claude Code