fix: rewrite Ogg parser for multi-page packets (#9256)#9322
Open
xeinherjer-dev wants to merge 34 commits into
Open
fix: rewrite Ogg parser for multi-page packets (#9256)#9322xeinherjer-dev wants to merge 34 commits into
xeinherjer-dev wants to merge 34 commits into
Conversation
dosubot
Bot
added
the
size:L
🎨 Storybook:
|
🎭 Playwright: ⏳ Running... |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors Ogg/Opus metadata extraction to read files into an ArrayBuffer, demux Ogg pages, reconstruct OpusTags packets across page boundaries, and parse Vorbis comments into JSON. Adds a comprehensive Vitest suite covering multi-page metadata, truncations, corrupted pages, and malformed comments. getOggMetadata now returns Promise. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Calling Code
participant Ogg as getOggMetadata()
participant Parser as OggPageParser
participant Extractor as extractOpusTags()
participant Vorbis as parseVorbisComments()
Client->>Ogg: call getOggMetadata(file)
Ogg->>Ogg: read file into ArrayBuffer
Ogg->>Parser: scan for OggS pages and headers
loop per OggS page
Parser->>Parser: parse header and segment table
Parser->>Extractor: emit segments (flag OpusTags segments)
end
Extractor->>Extractor: concatenate OpusTags segments (strip OggS/segment tables)
Extractor->>Vorbis: pass reconstructed OpusTags packet
Vorbis->>Vorbis: parse Vorbis comments → extract JSON fields
Vorbis->>Ogg: return { prompt?, workflow? }
Ogg->>Client: resolve Promise with metadata
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8d6459b06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/metadata/ogg.test.ts`:
- Around line 229-241: The test block around the `it('should safely handle:
${name}')` case currently treats any thrown error as a successful outcome;
change it so the expectation is deterministic by explicitly asserting one
behavior: either assert the promise resolves and then expect result.prompt and
result.workflow to be undefined (using `await
expect(getOggMetadata(file)).resolves.toMatchObject({ prompt: undefined,
workflow: undefined })` or equivalent), or if a specific error is expected,
assert `await expect(getOggMetadata(file)).rejects.toThrow(/* specific error or
RangeError */)`. Update the same pattern for the other similar test case that
uses `getOggMetadata` to ensure thrown errors are asserted with
`rejects.toThrow` rather than treated as success.
- Around line 247-251: Update the scenarios declaration to replace the unsafe
type "expectedPrompt?: any" with a concrete type such as "expectedPrompt?:
Record<string, unknown>" and then make the runtime check explicit in the test
logic that consumes it (where scenarios is iterated and expectedPrompt is
inspected) by using "expectedPrompt !== undefined" instead of a truthiness
check; reference the "scenarios" variable and the "expectedPrompt" field so you
update both the interface and the place in the test that branches on
expectedPrompt.
In `@src/scripts/metadata/ogg.ts`:
- Around line 51-55: The loop that builds OGG page segments (using
pageSegmentsCount, lengthsOffset, dataOffset, segmentLength, and data.subarray)
does not validate that segmentLength fits within remaining bytes and can
silently truncate corrupted lacing values; before calling
data.subarray(dataOffset, dataOffset + segmentLength) validate that
segmentLength is non-negative and that dataOffset + segmentLength <= data.length
(and that lengthsOffset + i is within bounds), and if not either throw a
descriptive error or handle the corruption path (e.g., mark packet invalid) so
corrupted segment lengths cannot produce truncated/incorrect segment buffers.
- Around line 89-117: The Vorbis comment parsing (uses packetView.getUint32,
readIndex, packetData.subarray, decoder.decode, and JSON.parse to populate
prompt and workflow) can throw on truncated/corrupt packets; add defensive
checks before every getUint32 and subarray/decoder.decode to ensure (readIndex +
4) <= packetData.byteLength before reading lengths and (readIndex +
commentLength) <= packetData.byteLength before slicing, and if a length is
invalid bail out of the loop or skip that comment; wrap decoder.decode and
JSON.parse calls for keys 'prompt' and 'workflow' in try/catch and on error skip
assigning or set prompt/workflow to null/default, and ensure readIndex is never
advanced past packetData.byteLength so parsing gracefully falls back instead of
throwing.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
b1.sh (1)
2-2: Usepnpm dlxinstead ofnpx.The project guidelines specify using
pnpm dlxorpnpxfor running packages, notnpx.Suggested fix
-pnpm install && npx nx build +pnpm install && pnpm dlx nx buildOr, if
nxis a project dependency, simply use:pnpm install && pnpm exec nx buildBased on learnings: "Use
pnpmas the package manager; for arbitrary packages not in scripts, usepnpxorpnpm dlx, nevernpx".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@b1.sh` at line 2, Replace the use of npx in the script command ("pnpm install && npx nx build") with a pnpm-compatible runner per project guidelines: either use "pnpm dlx" to run nx or, if nx is a project dependency, use "pnpm exec nx build" (keep the initial "pnpm install &&" intact); update the command in b1.sh accordingly so it no longer uses npx.src/scripts/metadata/ogg.test.ts (2)
368-384: Consider simplifying the assertion pattern.The
toSatisfycallback that runs nested expects and returnstrueis unconventional. A direct assertion would be clearer.Simplified assertion
for (const { name, createBuffer, expectedPrompt } of scenarios) { it(`should appropriately handle: ${name}`, async () => { const file = new File( [createBuffer()], `test_${name.replace(/\s+/g, '_')}.ogg` ) - await expect(getOggMetadata(file)).resolves.toSatisfy((result) => { - if (expectedPrompt) { - expect(result.prompt).toEqual(expectedPrompt) - } else { - expect(result.prompt).toBeUndefined() - } - expect(result.workflow).toBeUndefined() - return true - }) + const result = await getOggMetadata(file) + if (expectedPrompt !== undefined) { + expect(result.prompt).toEqual(expectedPrompt) + } else { + expect(result.prompt).toBeUndefined() + } + expect(result.workflow).toBeUndefined() }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/metadata/ogg.test.ts` around lines 368 - 384, Replace the unconventional expect(...).resolves.toSatisfy(...) pattern in the test loop with a direct awaited assertion: call getOggMetadata(file) with await, store the result, then use straightforward assertions against result (e.g., assert result.prompt equals expectedPrompt or is undefined, and assert result.workflow is undefined). Update the test block around the scenarios loop and the it callback to await getOggMetadata(file) and perform direct expect(...) checks against the returned object (referencing getOggMetadata and the local variables name/createBuffer/expectedPrompt).
306-319: Consider simplifying the assertion pattern.The current pattern chains
.then()withresolves.not.toThrow(). A cleaner approach would assert the resolved value directly.Simplified assertion
for (const { name, createBuffer } of scenarios) { it(`should safely handle: ${name}`, async () => { const file = new File( [createBuffer()], `test_${name.replace(/\s+/g, '_')}.ogg` ) - await expect( - getOggMetadata(file).then((result) => { - expect(result.prompt).toBeUndefined() - expect(result.workflow).toBeUndefined() - }) - ).resolves.not.toThrow() + const result = await getOggMetadata(file) + expect(result.prompt).toBeUndefined() + expect(result.workflow).toBeUndefined() }) }This makes the test intention clearer: the function should resolve (not throw) and return undefined values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/metadata/ogg.test.ts` around lines 306 - 319, The test currently uses getOggMetadata(file).then(...) combined with .resolves.not.toThrow(), which is awkward; change the assertion to assert the resolved value directly by awaiting or using expect(...).resolves and checking that the resolved object has prompt and workflow undefined (e.g. expect(getOggMetadata(file)).resolves.toEqual/expect.objectContaining({ prompt: undefined, workflow: undefined })). Update the test inside the loop that references getOggMetadata and the temporary File so it directly asserts the resolved value instead of chaining .then() with resolves.not.toThrow().src/scripts/metadata/ogg.ts (1)
114-114: Consider adding explicit types forpromptandworkflow.The variables are declared without types, which will cause them to be implicitly typed as
unknownafter assignment orundefinedbefore. For clarity, consider explicit typing.Optional improvement
- let prompt, workflow + let prompt: unknown + let workflow: unknown🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/metadata/ogg.ts` at line 114, Declare explicit types for the two variables so they aren't implicitly unknown/undefined: replace the loose declaration of prompt and workflow with typed declarations (for example, let prompt: string | undefined; let workflow: Workflow | undefined). If Workflow is a domain type, import or define the Workflow interface/type near where workflow is used; otherwise pick a concrete shape (e.g., Record<string, any> or a specific interface) or a safer union (unknown | ...) and narrow it where used. Ensure all subsequent code compiles against the chosen types and update any imports or type definitions accordingly.b3.sh (1)
2-2: Developer convenience script with hardcoded external path.This script assumes a specific directory structure exists at
../ComfyUI_windows_portable/. Consider adding a shebang (#!/bin/bash) and a check that the target directory exists before copying.Also, verify whether
python_embededis intentional or a typo ofpython_embedded.Optional improvement
+#!/bin/bash +set -e + +TARGET_DIR="../ComfyUI_windows_portable/python_embeded/Lib/site-packages/comfyui_frontend_package/static/" + +if [ ! -d "$TARGET_DIR" ]; then + echo "Error: Target directory does not exist: $TARGET_DIR" + exit 1 +fi -cp -Rf dist/* ../ComfyUI_windows_portable/python_embeded/Lib/site-packages/comfyui_frontend_package/static/ +cp -Rf dist/* "$TARGET_DIR"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@b3.sh` at line 2, Add a bash shebang and guard the hardcoded copy by checking the target directory exists before running the cp in b3.sh: ensure the file starts with "#!/bin/bash", verify the destination path "../ComfyUI_windows_portable/python_embeded/Lib/site-packages/comfyui_frontend_package/static/" exists (and exit with a clear error if not), and confirm whether "python_embeded" in that path is a typo that should be "python_embedded" and correct it if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@b1.sh`:
- Line 2: Replace the use of npx in the script command ("pnpm install && npx nx
build") with a pnpm-compatible runner per project guidelines: either use "pnpm
dlx" to run nx or, if nx is a project dependency, use "pnpm exec nx build" (keep
the initial "pnpm install &&" intact); update the command in b1.sh accordingly
so it no longer uses npx.
In `@b3.sh`:
- Line 2: Add a bash shebang and guard the hardcoded copy by checking the target
directory exists before running the cp in b3.sh: ensure the file starts with
"#!/bin/bash", verify the destination path
"../ComfyUI_windows_portable/python_embeded/Lib/site-packages/comfyui_frontend_package/static/"
exists (and exit with a clear error if not), and confirm whether
"python_embeded" in that path is a typo that should be "python_embedded" and
correct it if needed.
In `@src/scripts/metadata/ogg.test.ts`:
- Around line 368-384: Replace the unconventional
expect(...).resolves.toSatisfy(...) pattern in the test loop with a direct
awaited assertion: call getOggMetadata(file) with await, store the result, then
use straightforward assertions against result (e.g., assert result.prompt equals
expectedPrompt or is undefined, and assert result.workflow is undefined). Update
the test block around the scenarios loop and the it callback to await
getOggMetadata(file) and perform direct expect(...) checks against the returned
object (referencing getOggMetadata and the local variables
name/createBuffer/expectedPrompt).
- Around line 306-319: The test currently uses getOggMetadata(file).then(...)
combined with .resolves.not.toThrow(), which is awkward; change the assertion to
assert the resolved value directly by awaiting or using expect(...).resolves and
checking that the resolved object has prompt and workflow undefined (e.g.
expect(getOggMetadata(file)).resolves.toEqual/expect.objectContaining({ prompt:
undefined, workflow: undefined })). Update the test inside the loop that
references getOggMetadata and the temporary File so it directly asserts the
resolved value instead of chaining .then() with resolves.not.toThrow().
In `@src/scripts/metadata/ogg.ts`:
- Line 114: Declare explicit types for the two variables so they aren't
implicitly unknown/undefined: replace the loose declaration of prompt and
workflow with typed declarations (for example, let prompt: string | undefined;
let workflow: Workflow | undefined). If Workflow is a domain type, import or
define the Workflow interface/type near where workflow is used; otherwise pick a
concrete shape (e.g., Record<string, any> or a specific interface) or a safer
union (unknown | ...) and narrow it where used. Ensure all subsequent code
compiles against the chosen types and update any imports or type definitions
accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
b1.shb2.shb3.shsrc/scripts/metadata/ogg.test.tssrc/scripts/metadata/ogg.ts
Author
|
Note: The PR size increased to XL because I added test codes to cover the new JSON parsing fixes and maintain 100% coverage. |
xeinherjer-dev
force-pushed
the
fix-ogg-syntax-9256
branch
from
March 2, 2026 06:52
d5db7e7 to
9e868e1
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/metadata/ogg.ts`:
- Line 39: The loop boundary check in the Ogg parsing code (the while using
offset, OGG_HEADER_SIZE and data.length) incorrectly uses '<' and therefore
skips pages whose header is exactly OGG_HEADER_SIZE bytes (27 bytes, 0
segments); change the condition from using strict less-than to allow equality
(use <= semantics) so the parser accepts minimal valid Ogg pages while still
ensuring you don't read past the buffer.
- Line 1: The function getOggMetadata lacks explicit TypeScript types: define an
OggMetadata interface describing the returned metadata shape, annotate
getOggMetadata to return Promise<OggMetadata>, and add explicit types for the
local variables prompt and workflow where they are declared; also change all
catch clause variables to type unknown and narrow them before usage (e.g., using
typeof or instance checks) to avoid implicit any propagation.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scripts/metadata/ogg.test.tssrc/scripts/metadata/ogg.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/metadata/ogg.test.ts
xeinherjer-dev
force-pushed
the
fix-ogg-syntax-9256
branch
from
March 2, 2026 08:08
9e868e1 to
447295e
Compare
📦 Bundle: 4.47 MB gzip 🔴 +644 BDetailsSummary
Category Glance App Entry Points — 17.9 kB (baseline 17.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.03 MB (baseline 1.03 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 72.1 kB (baseline 72.1 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 436 kB (baseline 436 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 10 added / 10 removed User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 5 added / 5 removed Editors & Dialogs — 736 B (baseline 736 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 1 added / 1 removed UI Components — 46.9 kB (baseline 46.9 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed Data & Services — 2.58 MB (baseline 2.57 MB) • 🔴 +2.5 kBStores, services, APIs, and repositories
Status: 13 added / 13 removed Utilities & Hooks — 55.5 kB (baseline 55.5 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 11 added / 11 removed Vendor & Third-Party — 8.84 MB (baseline 8.84 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.89 MB (baseline 7.89 MB) • ⚪ 0 BBundles that do not match a named category
Status: 52 added / 52 removed |
xeinherjer-dev
force-pushed
the
fix-ogg-syntax-9256
branch
from
March 3, 2026 17:05
8e502c0 to
1748944
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scripts/metadata/ogg.ts`:
- Around line 139-148: Normalize the Vorbis comment key before comparing by
trimming and lowercasing the variable `key` (the one set from text.substring at
the start of the metadata parsing block) so comparisons against 'prompt' and
'workflow' are case-insensitive; update the matching logic in the same block
that assigns `result.prompt` / `result.workflow` (and any other key checks) to
use the normalized key, and optionally trim `value` as well before parsing to
avoid whitespace issues.
- Around line 41-82: The loop appends segments across pages after inOpusTags
becomes true without validating Ogg logical-stream continuity, which can stitch
unrelated/corrupt pages into the OpusTags packet; fix by capturing the page
bitstream serial (bytes at the Ogg header serial number field) and the page
sequence number and header-type flags when you first detect 'OpusTags' (use the
same offsets surrounding OGG_HEADER_SIZE/OGG_PAGE_SEGMENTS_OFFSET to read
header-type and serial/sequence), then only accept segments from subsequent
pages if the serial matches, the page sequence is the expected next sequence (or
a continuation is indicated via the header-type continuation flag), and the
header-type continuation flag semantics are honored; if any of those checks
fail, stop processing/abort collecting further segments to avoid stitching
unrelated pages into segments (apply checks where you currently set inOpusTags
and before pushing into segments).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/scripts/metadata/ogg.test.tssrc/scripts/metadata/ogg.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scripts/metadata/ogg.test.ts
Author
|
Summary of changes to address feedback:
|
christian-byrne
approved these changes
Mar 13, 2026
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
code looks good - clean rewrite with proper Ogg page traversal, thorough bounds checking, and comprehensive test coverage. all review feedback addressed.
christian-byrne
requested changes
Mar 13, 2026
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
Rescinding approval - the parser rewrite itself is solid and already wired via parser.ts, but the PR is missing E2E coverage that the other media format PRs include (e.g., PR #4420 for AVIF):
- Add a
workflow.oggtest asset tobrowser_tests/assets/workflowInMedia/(a real Ogg/Opus file with embedded ComfyUI prompt+workflow metadata using core nodes only) - Add
'workflow.ogg'to thefileNamesarray inbrowser_tests/tests/loadWorkflowInMedia.spec.ts
This ensures the full drag-and-drop → metadata extraction → workflow load path is covered end-to-end, not just the parser in isolation.
christian-byrne
left a comment
christian-byrne
left a comment
Contributor
There was a problem hiding this comment.
Some extra guidance on creating the test asset:
Generating workflow.ogg:
The easiest way is to use ComfyUI itself — run any workflow that uses a SaveAudio node (which outputs Opus/Ogg). The backend embeds prompt and workflow as Vorbis Comments in the OpusTags packet automatically via extra_pnginfo.
Important: use core nodes only. The workflow.avif test asset is currently commented out in the test file because it uses non-core nodes, which triggers a missing nodes dialog and breaks the screenshot comparison. To avoid this, build a simple workflow using only built-in nodes (e.g., EmptyLatentImage → KSampler → something that produces audio, or just any minimal core-only workflow).
Files to change:
- Add the generated
.oggfile tobrowser_tests/assets/workflowInMedia/workflow.ogg - Add
'workflow.ogg'to thefileNamesarray inbrowser_tests/tests/loadWorkflowInMedia.spec.ts(line ~27, after'workflow.svg')
Verifying locally:
pnpm test:browser:local -- --grep 'workflow.ogg'See PR #4420 for the full pattern (that PR added AVIF support with the same set of changes).
Author
|
I've confirmed that the E2E test for workflow.ogg passes locally without any issues, and I've pushed the updates. I also included a fix for the MIME type mapping in DragDropHelper.ts to ensure .ogg files are correctly identified as audio/ogg during the tests. 
|
The previous implementation used naive regex matching on binary data converted to strings. This approach failed entirely when a large prompt caused the `OpusTags` packet to span multiple Ogg pages. This commit replaces the fragile string search with a robust binary parser that strictly follows the Ogg logical bitstream and Vorbis Comments specifications. It correctly handles Ogg lacing and reassembles segmented packets, ensuring that large JSON metadata is successfully extracted. Also adds unit tests (ogg.test.ts) to verify the correct parsing of both standard and multi-page Ogg files.
Author
|
@christian-byrne Please let me know if this fix is still needed. If so, I will update the branch and resolve the conflicts. |
# Conflicts: # src/scripts/metadata/ogg.test.ts # src/scripts/metadata/ogg.ts
Applies optimizations and robustness improvements to the Ogg/Opus
metadata parser based on PR feedback:
- Use direct byte comparisons for magic strings ('OggS', 'OpusTags')
to avoid per-page TextDecoder allocations.
- Add explicit bounds checking during segment processing to prevent
infinite loops on corrupted files.
- Emit a console.warn when the OpusTags packet is truncated at the
2MB read cap instead of failing silently.
- Consolidate duplicated constants and clean up comments.
Author
|
@christian-byrne |
Resolved conflicts in ogg.ts by integrating the NaN/Infinity parsing logic from Comfy-Org#12217 into the new robust Ogg parser.
Tailwind class order differed from upstream likely because oxfmt pre-commit hook rewrote the class ordering during the first merge commit, which caused subsequent auto-merges to preserve the hook-modified version instead of upstream's.
🎭 Playwright: ⏳ Running...🎨 Storybook: 🚧 Building... |
|
✅ All contributors have signed the CLA. Thank you! This PR is ready to be merged. |
Author
|
I have read and agree to the Contributor License Agreement |
comfy-legal
added a commit
to Comfy-Org/comfy-cla
that referenced
this pull request
Jul 12, 2026
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
3 participants
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.
The previous implementation used naive regex matching on binary data converted to strings. This approach failed entirely when a large prompt caused the
OpusTagspacket to span multiple Ogg pages.This commit replaces the fragile string search with a robust binary parser that strictly follows the Ogg logical bitstream and Vorbis Comments specifications. It correctly handles Ogg lacing and reassembles segmented packets, ensuring that large JSON metadata is successfully extracted.
Also adds unit tests (ogg.test.ts) to verify the correct parsing of both standard and multi-page Ogg files.
Summary
Replaced the fragile string-based Ogg metadata extraction with a robust binary parser to successfully load workflows from large multi-page Ogg files.
Changes
OpusTagsmulti-page packets.file.arrayBuffer()API, replacingFileReaderand manual Promises to reduce overhead and improve processing efficiency.ogg.test.ts).Review Focus
Please review the binary parsing logic (specifically the handling of Ogg lacing and multi-page segments) and the newly added test cases in
ogg.test.ts.Fixes #9256
Screenshots (if applicable)
ogg.tswith 8 test cases covering both standard and multi-page Ogg files.Reference Diagram
┆Issue is synchronized with this Notion page by Unito