Uh oh!
There was an error while loading. Please reload this page.
fix(win): ship the compositor addon beside its ffmpeg DLLs - #303
Merged
Conversation
The 1.9.0 Store build loads no compositor at all: the editor opens with a permanently blank preview while audio keeps playing. Audio comes from the renderer, every frame comes from the addon, so the symptom is exactly what an addon that failed to load produces — the service falls back to a silent no-op. The addon dlopens avcodec/avformat/avutil at require() time. It shipped from app.asar.unpacked, one directory away from electron/native/bin/win32-x64/*.dll, and the gap was bridged at runtime by ensureFfmpegSharedDllsOnPath prepending the DLL directory to PATH. That works for NSIS and does not work under MSIX, which resolves an addon's dependent DLLs through the package graph and ignores PATH. Measured inside a registered package, with the directory verifiably present and correctly prepended: dllDir existsSync : true require BEFORE PATH : FAILED: The specified module could not be found. require AFTER PATH : FAILED: The specified module could not be found. and with the addon beside those same DLLs, no PATH involved: require BEFORE PATH : LOADED OK Node loads .node files with LOAD_WITH_ALTERED_SEARCH_PATH, so the addon's own directory is searched for its dependencies. Colocating removes the PATH mechanism rather than repairing it, and works on every Windows packaging format. macOS has always done this; Windows was the outlier. No loader change: buildCandidatePaths already probes bin/<tag> first. before-pack now refuses to package unless the addon and each of avcodec, avformat and avutil are in that directory, on Windows as it already did on macOS and Linux. One requirement per library rather than a count over a combined regex — the trap LINUX_REQUIRED already documents, where several versioned copies of one library satisfy the count while another is missing.
📝 WalkthroughWalkthroughWindows packaging now places ChangesWindows native payload packaging
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BuildScript as build-windows-compositor-addon.mjs
participant NativeDir as electron/native/bin/win32-x64
participant ElectronBuilder as electron-builder
participant BeforePack as scripts/before-pack.cjs
BuildScript->>NativeDir: Copy compositor_view.node beside FFmpeg DLLs
ElectronBuilder->>NativeDir: Package colocated payload through extraResources
BeforePack->>NativeDir: Validate addon and required FFmpeg DLLs
BeforePack->>BeforePack: Check addon freshness and report result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@technical-documentation/engineering/build-and-packaging.md`:
- Around line 49-57: Add the text language identifier to both diagnostic
Markdown code fences in the documentation, including the fence containing the
dllDir/require output and the subsequent fence containing the LOADED OK output;
leave their contents unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18555bd7-279f-4f6e-aada-af3c7ac7a3db
📒 Files selected for processing (4)
electron-builder.json5scripts/before-pack.cjsscripts/build-windows-compositor-addon.mjstechnical-documentation/engineering/build-and-packaging.md
Comment on lines
+49
to
+57
| ``` | ||
| dllDir existsSync : true | ||
| require BEFORE PATH : FAILED: The specified module could not be found. | ||
| require AFTER PATH : FAILED: The specified module could not be found. | ||
| ``` | ||
| and with the addon sitting beside those same DLLs, no `PATH` involved: | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Specify a language for both diagnostic code fences.
The configured markdown linter reports MD040 for lines 49 and 57. Add text to both opening fences.
Proposed fix
-```+```text
dllDir existsSync : true
require BEFORE PATH : FAILED: The specified module could not be found.
require AFTER PATH : FAILED: The specified module could not be found.and with the addon sitting beside those same DLLs, no PATH involved:
- +text
require BEFORE PATH : LOADED OK
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 57-57: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@technical-documentation/engineering/build-and-packaging.md` around lines 49 -
57, Add the text language identifier to both diagnostic Markdown code fences in
the documentation, including the fence containing the dllDir/require output and
the subsequent fence containing the LOADED OK output; leave their contents
unchanged.
Source: Linters/SAST tools
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Fixes the Store build of 1.9.0, which is live and broken: the editor opens with a permanently blank preview while audio keeps playing. The NSIS build of the same commit is fine.
Why the symptom points at exactly one thing
Audio comes from the renderer; every preview frame comes from the native compositor addon. Sound with no picture is what an addon that failed to load produces —
CompositorViewServicefalls back to a silent no-op, which the code's own comment describes as "the editor silently ran without a compositor".Root cause
The addon dlopens
avcodec/avformat/avutilatrequire()time. It shipped fromapp.asar.unpacked/electron/native/compositor-view/build/, one directory away fromelectron/native/bin/win32-x64/*.dll, and the gap was bridged at runtime byensureFfmpegSharedDllsOnPathprepending the DLL directory toPATH.That works for NSIS. It does not work under MSIX, which resolves an addon's dependent DLLs through the package graph and ignores
PATH.Measured, not inferred
I registered the shipped 1.9.0 appx as a loose package (
Add-AppxPackage -Register, real package identity) and ran the loader's own logic inside it viaInvoke-CommandInDesktopPackage:That rules out the alternative explanation — it is not a wrong path, not a missing file, not an ACL problem.
PATHis correctly set and simply not consulted.Then, with the DLLs copied beside the addon in the same package, nothing else changed:
The fix
Ship the addon from
electron/native/bin/win32-x64/, beside the DLLs, viaextraResources— and stop routing it throughfilesinto the asar.Node loads
.nodefiles withLOAD_WITH_ALTERED_SEARCH_PATH, so the addon's own directory is searched for its dependencies. This removes thePATHmechanism rather than repairing it, and works on every Windows packaging format.macOS has always done this (
build-macos-compositor-addon.mjsinstalls intobin/darwin-<arch>/and vendors its dylibs there). Windows was the outlier.No loader change is needed:
buildCandidatePathsalready probesbin/<tag>/compositor_view.nodefirst.Regression guard
before-pack.cjsnow refuses to package unless the addon and each ofavcodec,avformat,avutilare in that directory — on Windows as it already did on macOS and Linux.One requirement per library rather than a count over a combined regex. That is the trap
LINUX_REQUIREDalready documents: three copies ofavcodec-60/61/62.dllleft by an earlier fetch would satisfyatLeast: 3whileavformatandavutilwere missing. Verified it catches exactly that case:All four paths exercised: missing directory, missing addon, missing DLLs, complete payload.
compositorViewServicetests still green (21).Testing before the Store update
The loose-registration route above reproduces the failure and will verify the fix on a real MSIX identity, without signing or a Store submission — so the 1.9.1 package can be validated locally before it goes anywhere near Partner Center.
Summary by CodeRabbit
Bug Fixes
Documentation