fix(openjpeg): correct the buffer-stream skip signature and decoded image dimensions

.github/workflows/release.yml

@@ -90,10 +90,25 @@ jobs:
     # second `-m` would do. That failure is invisible and lands in the worst
     # place: the release commit gets benched, seeding a duplicate CodSpeed
     # baseline. Prefix + identity fails safe where equality fails silent.
+    #
+    # The repository clause keeps this workflow out of forks. It triggers on
+    # push to `main`, and a contributor whose PR branch IS their fork's `main`
+    # gets the whole thing run inside their own repository on every push to that
+    # branch. That happened on #63: the release job bumped all nine packages and
+    # pushed a `chore(release): publish` commit onto the open pull request,
+    # authenticated with the fork's own GITHUB_TOKEN. Publishing to npm failed
+    # there for want of credentials, so nothing reached the registry, but the
+    # version commit still landed on the PR. A fork gets nothing useful from
+    # this workflow in any case.
+    #
+    # Guarding this job alone would gate the chain, since release needs build
+    # and publish needs release, but the release job repeats the condition: it
+    # is the one that writes to the repository.
     if: >-
-      github.event_name != 'push'
+      github.repository == 'cornerstonejs/codecs'
+      && (github.event_name != 'push'
       || !(startsWith(github.event.head_commit.message, 'chore(release): publish')
-      && github.event.head_commit.author.email == '41898282+github-actions[bot]@users.noreply.github.com')
+      && github.event.head_commit.author.email == '41898282+github-actions[bot]@users.noreply.github.com'))
     # Same wasm build as pr-checks.yml, minus the change detection: a release
     # publishes whatever moved, and dicom-codec ships ranges over every sibling,
     # so all dists must be current.
@@ -184,6 +199,10 @@ jobs:
     # Versions, tests and pushes. NO id-token: write — `pnpm install` runs in
     # this job, and nothing it pulls in has any business holding a credential
     # that can publish to npm. Publishing happens in the next job.
+    #
+    # Redundant with the guard on build, which this job depends on, but this is
+    # the job that commits and pushes — see the note there.
+    if: github.repository == 'cornerstonejs/codecs'
     needs: build
     runs-on: ubuntu-latest
     permissions:

packages/openjpeg/src/BufferStream.hpp

@@ -46,21 +46,39 @@ opj_write_to_buffer (void* p_buffer, OPJ_SIZE_T p_nb_bytes,
     return n;
 }
 
-static OPJ_SIZE_T
-opj_skip_from_buffer (OPJ_SIZE_T len, opj_buffer_info_t* psrc)
+/*
+ * Must match opj_stream_skip_fn exactly: OPJ_OFF_T (*)(OPJ_OFF_T, void*).
+ * OPJ_OFF_T is int64_t, so declaring this with OPJ_SIZE_T -- 32-bit under
+ * wasm32 -- made the cast below an (i64,i32)->i64 indirect call landing on an
+ * (i32,i32)->i32 table entry, which traps as "function signature mismatch".
+ *
+ * It only fires when a skip is actually delegated to this callback:
+ * opj_stream_read_skip serves anything within m_bytes_in_buffer straight from
+ * the 1MB chunk it has already read, so small JP2 box skips are invisible and
+ * only a skip past the buffered remainder -- a large tile-part, a large box --
+ * reaches us. Hence a multi-tile image failing where the same image re-tiled
+ * to a single tile decodes fine. Native builds tolerated the mismatched call,
+ * so this only ever showed up in wasm.
+ */
+static OPJ_OFF_T
+opj_skip_from_buffer (OPJ_OFF_T len, opj_buffer_info_t* psrc)
 {
     OPJ_SIZE_T n = psrc->buf + psrc->len - psrc->cur;
 
+    if (len < 0)
+        return (OPJ_OFF_T)-1;
+
     if (n) {
-        if (n > len)
-            n = len;
+        if (n > (OPJ_SIZE_T)len)
+            n = (OPJ_SIZE_T)len;
 
         psrc->cur += n;
+
+        return (OPJ_OFF_T)n;
     }
-    else
-        n = (OPJ_SIZE_T)-1;
 
-    return n;
+    /* buffer exhausted: cio.c compares the result against (OPJ_OFF_T)-1 */
+    return (OPJ_OFF_T)-1;
 }
 
 static OPJ_BOOL

packages/openjpeg/src/J2KDecoder.hpp

@@ -847,6 +847,11 @@ class J2KDecoder {
           return;
       }
 
+      /* Finalize decompression before the guards destroy the codec/stream */
+      if (!opj_end_decompress(l_codec, l_stream)) {
+          printf("[WARNING] opj_decompress: opj_end_decompress failed\n");
+      }
+
       if (image->color_space != OPJ_CLRSPC_SYCC
             && image->numcomps == 3 && image->comps[0].dx == image->comps[0].dy
             && image->comps[1].dx != 1) {
@@ -863,8 +868,12 @@ class J2KDecoder {
         color_esycc_to_rgb(image);
       }
 
-      frameInfo_.width = image->x1;
-      frameInfo_.height = image->y1;
+      // x1/y1 are absolute reference-grid coordinates, not pixel counts. For
+      // an image with a nonzero offset they overstate the decoded size, and
+      // the copy loop below indexes comps[].data with it, reading past the
+      // component buffer openjpeg allocated.
+      frameInfo_.width = image->x1 - image->x0;
+      frameInfo_.height = image->y1 - image->y0;
       frameInfo_.componentCount = image->numcomps;
       if (frameInfo_.componentCount != 1 && frameInfo_.componentCount != 3) {
         throw std::runtime_error("unsupported J2K component count");
@@ -905,7 +914,6 @@ class J2KDecoder {
       decoded_.resize(destinationSize);
 
       // Convert from int32 to native size
-      int comp_num;
       for (int y = 0; y < sizeAtDecompositionLevel.height; y++)
       {
         size_t lineStartPixel = y * sizeAtDecompositionLevel.width;

packages/openjpeg/test/cpp/CMakeLists.txt

@@ -11,5 +11,5 @@ target_compile_features(cpptest PUBLIC cxx_std_14)
 set(CMAKE_CXX_FLAGS_RELEASE "-O3")
 
 # add include path to openjpeg
-include_directories("../extern/openjpeg/src/lib/openjp2" "../build/extern/openjpeg/src/lib/openjp2"
-                    "../extern/openjpeg/src/bin/common" "../build/extern/openjpeg/src/bin/common")
+include_directories("../../extern/openjpeg/src/lib/openjp2" "../../build/extern/openjpeg/src/lib/openjp2"
+                    "../../extern/openjpeg/src/bin/common" "../../build/extern/openjpeg/src/bin/common")

packages/openjpeg/test/helpers/jp2.mjs

@@ -0,0 +1,130 @@
+// Builds a JP2 (box-wrapped) file around a bare J2K codestream.
+//
+// Why this is generated rather than committed as a fixture: the point of the
+// JP2 wrapper here is to make openjpeg delegate a skip to our own
+// opj_skip_from_buffer callback in BufferStream.hpp, and that only happens for
+// a skip LARGER than the stream's internal buffer. opj_stream_read_data always
+// refills a full OPJ_J2K_STREAM_CHUNK_SIZE (1MB) chunk, and
+// opj_stream_read_skip serves anything within m_bytes_in_buffer directly, so
+// the skipped box has to be over 1MB. That makes the file too big to want in
+// git, and it is trivially reproducible from a codestream we already ship.
+//
+// Run directly to write one out, e.g. to hand to a bug report:
+//   node test/helpers/jp2.mjs test/fixtures/j2k/CT1.j2k /tmp/CT1-boxed.jp2
+
+import { readFileSync, writeFileSync } from "node:fs"
+import { pathToFileURL } from "node:url"
+
+// opj_stream_default_create's buffer size (openjpeg.h OPJ_J2K_STREAM_CHUNK_SIZE).
+export const STREAM_CHUNK_SIZE = 0x100000
+
+// Comfortably over one chunk, so the skip cannot be served from the buffer no
+// matter how much of it the header reads happen to have consumed.
+export const DEFAULT_FILLER_BYTES = STREAM_CHUNK_SIZE + 4096
+
+function box(type, ...contents) {
+  const content = Buffer.concat(contents)
+  const header = Buffer.alloc(8)
+  header.writeUInt32BE(content.length + 8, 0)
+  header.write(type, 4, 4, "ascii")
+  return Buffer.concat([header, content])
+}
+
+function u32(value) {
+  const b = Buffer.alloc(4)
+  b.writeUInt32BE(value, 0)
+  return b
+}
+
+function u16(value) {
+  const b = Buffer.alloc(2)
+  b.writeUInt16BE(value, 0)
+  return b
+}
+
+/**
+ * Reads the SIZ marker segment of a raw J2K codestream, so the JP2 image
+ * header we synthesise describes the codestream it actually wraps.
+ */
+export function parseSiz(codestream) {
+  if (codestream.readUInt16BE(0) !== 0xff4f || codestream.readUInt16BE(2) !== 0xff51) {
+    throw new Error("not a raw J2K codestream (expected SOC then SIZ)")
+  }
+
+  const xsiz = codestream.readUInt32BE(8)
+  const ysiz = codestream.readUInt32BE(12)
+  const xosiz = codestream.readUInt32BE(16)
+  const yosiz = codestream.readUInt32BE(20)
+  const numComponents = codestream.readUInt16BE(40)
+
+  // Ssiz is (bitdepth - 1) with the top bit set when signed — the same
+  // encoding JP2's ihdr BPC field uses, so it can be copied across verbatim.
+  const ssiz = codestream.readUInt8(42)
+
+  return {
+    width: xsiz - xosiz,
+    height: ysiz - yosiz,
+    numComponents,
+    ssiz,
+    bitsPerSample: (ssiz & 0x7f) + 1,
+    isSigned: (ssiz & 0x80) !== 0,
+  }
+}
+
+/**
+ * Wraps a raw J2K codestream in the minimum set of JP2 boxes, with a filler
+ * box in front of the codestream that openjpeg has no handler for and will
+ * therefore skip (jp2.c, the unknown-box branch of opj_jp2_read_header).
+ *
+ * @param {Buffer} codestream a bare .j2k codestream
+ * @param {{fillerBytes?: number}} [options]
+ * @returns {Buffer} a JP2 file
+ */
+export function wrapInJp2(codestream, { fillerBytes = DEFAULT_FILLER_BYTES } = {}) {
+  const siz = parseSiz(codestream)
+
+  const signature = box("jP  ", Buffer.from([0x0d, 0x0a, 0x87, 0x0a]))
+  const fileType = box("ftyp", Buffer.from("jp2 ", "ascii"), u32(0), Buffer.from("jp2 ", "ascii"))
+
+  const ihdr = box(
+    "ihdr",
+    u32(siz.height),
+    u32(siz.width),
+    u16(siz.numComponents),
+    Buffer.from([
+      siz.ssiz,
+      7, // C: compression type, always 7
+      0, // UnkC: colourspace is known
+      0, // IPR: no intellectual property rights box
+    ])
+  )
+  // METH=1 (enumerated), PREC=0, APPROX=0, then EnumCS.
+  const enumCs = siz.numComponents >= 3 ? 16 /* sRGB */ : 17 /* greyscale */
+  const colr = box("colr", Buffer.from([1, 0, 0]), u32(enumCs))
+
+  // A 'free' box is exactly this: padding with no defined meaning. openjpeg
+  // has no handler for it, so it takes the skip path we are trying to reach.
+  const filler = box("free", Buffer.alloc(fillerBytes))
+
+  return Buffer.concat([
+    signature,
+    fileType,
+    box("jp2h", ihdr, colr),
+    filler,
+    box("jp2c", codestream),
+  ])
+}
+
+// CLI: node test/helpers/jp2.mjs <in.j2k> <out.jp2> [fillerBytes]
+if (process.argv[1] && import.meta.url === pathToFileURL(process.argv[1]).href) {
+  const [input, output, fillerBytes] = process.argv.slice(2)
+  if (!input || !output) {
+    console.error("usage: node test/helpers/jp2.mjs <in.j2k> <out.jp2> [fillerBytes]")
+    process.exit(1)
+  }
+  const jp2 = wrapInJp2(readFileSync(input), {
+    fillerBytes: fillerBytes ? Number(fillerBytes) : undefined,
+  })
+  writeFileSync(output, jp2)
+  console.log(`wrote ${output} (${jp2.length} bytes)`)
+}