You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(protocol,electron): export the pixel video as MP4 instead of WebM - #174
DEFAULT_FILENAMES.video is video.mp4 (was video.webm); per-display members are video.<n>.mp4. The manifest entry's format is derived from the extension, so it now says mp4 — the worker and viewer read it as the default H.264 MP4 and skip the WebM transcode. Android already writes video.mp4.
Comments and design docs (electron.md, per-window-replay.md) updated; tests across protocol/core/node/electron now expect .mp4, and the electron e2e fake video starts with an MP4 ftyp box.
Contract change for hosts
The SDK ships no muxer: the host's encode / MediaRecorder seam must now produce MP4 (H.264). Nothing validates the bytes, so a host that keeps producing WebM would ship a WebM labelled mp4.
…he mobile-canonical name, instead of WebM
The default file name of the video FileType is now video.mp4 (video.<n>.mp4 per display), and the manifest entry states format mp4, so the worker and the viewer take it as the default H.264 MP4 with no transcode. The host-supplied muxer / MediaRecorder encoder must now produce MP4.
Refs #54
🤖 Generated with [Claude Code](https://claude.com/claude-code)
PR review: export pixel video as MP4 instead of WebM
I read the diff and the surrounding code (manifestEntryForType, encodePixelVideo, the MediaRecorder and capturePage sources). I did not run tests or tooling in this review.
The rename is consistent. DEFAULT_FILENAMES.video, the per-display video.<n>.mp4 names and the format attr (derived from the extension in manifestEntryForType) all line up. Tests across protocol, core, node and electron match.
Findings
P2 Medium: the MP4 contract is not validated (inline comment on packages/electron/src/pixel-video-controller.ts).
encodePixelVideo returns whatever bytes the host's encode / toBytes seam produced, and the manifest labels them mp4 from the filename alone.
A host that keeps producing WebM now ships a WebM labelled mp4. That is the mislabel docs/design/backend-beta-readiness.md:208-209 describes, so the PR can reintroduce it silently. The failure shows up only in the worker or viewer.
Fix: sniff the magic bytes (ftyp at offset 4 versus 1A 45 DF A3) and report through onError, or label the manifest with the sniffed container. Also guard the MediaRecorder path with isTypeSupported('video/mp4;codecs=avc1'), which the PR notes is unverified.
P3 Low: stale docs.
docs/design/backend-beta-readiness.md:208-209 still says "Electron video is WebM, but the manifest carries no codec/format attributes". This is wrong now, and was already outdated once format was added (inline comment on docs/design/electron.md). Remove or update it, and add a line in electron.md stating the breaking contract for hosts.
The comment in packages/core/src/bundle-assembler.ts:293-296 (manifestEntryForType) still frames format as the WebM transcode workaround. That file isn't in the diff, so I couldn't attach it inline. Reword it to say the container is stated explicitly from the filename.
Summary
Overall risk: Low-Medium. The change is a mechanical rename with consistent tests, and the risk is in the unvalidated host contract.
Merge recommendation: Merge after addressing the P2, or after explicitly accepting it. Two points are unresolved in the PR itself: whether Electron's MediaRecorder can emit video/mp4;codecs=avc1 on your minimum Electron version, and whether WebM stays available as an opt-in. Verify the first before shipping.
Most important to fix: the missing validation or sniff, then the stale backend-beta-readiness.md item.
Positives:
The format label is derived from the extension in one place, not hardcoded.
The e2e fake video now starts with a real ftyp box.
The new MP4_FTYP constant is shared across tests.
The PR description is candid about the open questions.
Pixel-capture videos are now saved as MP4 files (video.mp4), including per-display recordings. Bundle metadata identifies the video format as MP4.
Existing video content and display information remain unchanged.
The default video filename changes from video.webm to video.mp4. Design records and code comments identify MP4 output. Bundle, Electron, and Node tests now expect MP4 filenames or format metadata; Electron end-to-end tests use MP4 fixture bytes. The summaries report no changes to the encoder implementation, node snapshot wiring, permission gate, or launch lifecycle.
Possibly related PRs
bugsee/bugsee-javascript#173: Adds streaming bundle assembly and per-display pixel-video members, which this PR updates to use the MP4 filename and format metadata.
Suggested labels:javascript
Change: Feature
Merge Risk: Moderate · previous review 7c209
Hosts still producing WebM can generate mislabeled, unplayable video after this change. Validate the container and clarify the JPEG-to-H.264 encoding requirement before merging.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly and concisely describes the main change: exporting pixel video as MP4 instead of WebM.
Description check
The description directly explains the MP4 filename, manifest, host encoder contract, documentation updates, tests, and known open points.
Docstring Coverage
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
Explanation
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 1 functions across 10 files. (2 skipped: 2 unsupported.)
Finishing Touches
Generate docstrings
Commit to this branch
Create a new PR
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Review: export pixel video as MP4
This PR does one production thing: DEFAULT_FILENAMES.video becomes video.mp4, so manifestEntryForType now stamps attrs.format: "mp4" and the worker/viewer skip the WebM transcode. Tests, comments, and the Electron design docs were updated to match. That alignment with Android’s video.mp4 is the right target — but the SDK still does not produce those bytes, and nothing checks that the host did.
I traced the live path: host VideoCaptureSource.snapshot → encodePixelVideo (verbatim copy) → assembler names the member from DEFAULT_FILENAMES and derives format from the extension. createMediaRecorderVideoSource still concatenates whatever recorderFactory emits; createCapturePageVideoSource still dumps whatever encode() returns. Chromium’s new MediaRecorder(stream) (the seam’s own example, with no required mime type) defaults to WebM.
P1 — High: renaming to .mp4 re-opens the WebM-as-MP4 mislabel this format attr exists to prevent
Problem:format is taken only from the canonical filename, never from the payload. After this change it is always mp4, so the worker skips transcode even when the bytes are still WebM (EBML 1a 45 df a3). The previous video.webm + format: "webm" pair was the thing that made the worker transcode host WebM/MJPEG into playable H.264.
Impact: Opt-in pixel video lands in the bundle, looks like mobile video.mp4, and does not play. No SDK error, no onError. Same failure the assembler comments describe for a video entry with no attrs.
Scenario: A host wires the documented seam as recorderFactory: () => new MediaRecorder(stream) (or { mimeType: 'video/webm' }, or the previous webm/mjpeg encode muxer) and upgrades to this PR. Snapshot bytes are WebM; the member is video.mp4 with format: "mp4". Worker skips transcode; viewer labels video/mp4.
Fix: Sniff magic at encode/assembly time (ftyp vs EBML) and set filename + format from the bytes — keep format: "webm" so the worker still transcodes when the host has not actually emitted H.264. Fail closed (drop video, onError) if the name is .mp4 and the payload is not ftyp. Update recorderFactory JSDoc to require video/mp4;codecs=avc1andMediaRecorder.isTypeSupported before claiming the new contract. Do not merge a hard MP4 label while the intended encoder is still unverified (the PR’s own open question).
Residual (not filed)
Chromium H.264 encode in MediaRecorder is platform-dependent; this repo has not run the pixel-video path on real Electron 45 (README “Not run”). Linux/Electron builds that cannot emit video/mp4;codecs=avc1 have no remaining transcode fallback after this change.
encodePixelVideo still byte-concatenates multiple unlaned payloads. That is invalid ISO-BMFF (two ftyp/moov boxes). Display lanes are split and tested; a host that merges capturePage + MediaRecorder snapshots without lanes would still hit it.
VideoLaunchOptions / README still do not say the host must emit H.264 MP4. docs/design/backend-beta-readiness.md still says Electron video is WebM with no format attrs (stale; not in this diff).
Full GitHub CI (lint/coverage/e2e) was still in progress at review time. The suites above assert the new name and a fake ftyp prefix; none assert that host/MediaRecorder bytes actually are MP4, which is why the P1 survives them.
Overall
Overall risk: Medium — opt-in feature, unpublished SDK, but silent unplayable video for the feature this PR ships.
Merge recommendation: Request changes.
Most important to fix: Do not stamp format: "mp4" unless the payload starts with ftyp (or keep video.webm / format: "webm" until a real H.264 producer exists).
Positive: The rename is consistent (protocol → both assemblers → node/electron tests → design docs). Per-display video.<n>.mp4 + format: "mp4" is correct (nthFileName / manifestEntryForType). The e2e fake was updated off the EBML header onto an ftyp prefix instead of leaving a WebM fixture under an MP4 name.
…tract
Review findings: the manifest labels the entry format 'mp4' from the file TYPE alone and the worker skips its transcode for an MP4, but
nothing checked the bytes, so a host encode/MediaRecorder that still yields WebM (Chromium's MediaRecorder default) would ship WebM
labelled MP4 and not play, with no error.
- the pixel-video controller checks the ftyp box of every video entry in snapshot(): WebM (named in the message) or anything else is
dropped and reported through onError; the bundle still builds, without the video; other entry types are untouched
- encode / MediaRecorder seam docs say what they must produce (an H.264 encoder plus an MP4 muxer; an MP4 mimeType)
- electron.md states the contract (breaking for a host that supplied WebM); backend-beta-readiness.md's 'Electron video is WebM' item
is marked resolved
- tests: MP4-shaped fakes; WebM / short / wrong-box / non-bytes dropped with distinct messages; mutants on the check killed
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Review of PR #174: export pixel video as MP4 instead of WebM
I re-read the diff against origin/main, including the second commit 2e2f82fb, and the surrounding code: pixel-video-controller.ts, video-capture.ts, manifestEntryForType and the launch-main.ts wiring. I did not run tests or tooling in this pass.
Earlier findings: fixed by 2e2f82fb
P1/P2, WebM mislabelled as format: "mp4" (Cursor, Claude, CodeRabbit). This is fixed. createPixelVideoController.snapshot (pixel-video-controller.ts:149-158) now checks every video entry before it leaves the controller.
It requires ftyp at bytes 4–8.
It drops anything else and calls onError with the reason. A WebM is named explicitly.
It returns the remaining entries, so the bundle still builds without the bad video.
The check is not in encodePixelVideo, which is correct. The assembler does not guard encoder throws.
The same snapshot function is the only producer wired in by launchMain (launch-main.ts:309-311), and fileEncoders.video is overridden by encodePixelVideo. I found no path where a host video entry reaches the bundle unchecked.
The new tests cover all of these cases:
MP4 passes through untouched.
WebM is dropped and named.
Too-short, wrong-box and non-bytes payloads are dropped while a good MP4 beside them is kept.
Entries of other types are left alone.
A throwing onError sink is tolerated.
P3, stale backend-beta-readiness.md item. It is now struck through with a pointer to electron.md.
P3, electron.md contract note. It now says the change is breaking for hosts, that the bytes must be MP4 (H.264), that non-MP4 entries are dropped and reported, and that Chromium's MediaRecorder default is WebM.
recorderFactory and encode JSDoc (video-capture.ts:44-48,126-130) now state the MP4 requirement, with video/mp4;codecs=avc1 as the example mime type.
New findings
None at P2 or above. Three minor points, none blocking:
P3, encodePixelVideo can still join several MP4s into one invalid file (pixel-video-controller.ts:174-186).
The new ftyp check validates each entry on its own, so two unlaned entries would both pass. Concatenating them gives two ftyp/moov sets, which is not a valid MP4.
The built-in sources do not hit this today. createCapturePageVideoSource sets one lane per display, and the assembler writes lanes as separate video.<n>.mp4 members. The multi-entry branch is "belt-and-braces", but its video.mp4 comment now gives a stronger guarantee than it can keep.
If you want to harden it, make the multi-entry case a reported drop instead of a byte join. A custom source that returned two unlaned entries would otherwise ship a corrupt MP4 with no error.
P3, manifestEntryForType comment (packages/core/src/bundle-assembler.ts:~291). It still describes format in terms of the WebM transcode workaround. Say instead that the container is stated explicitly from the filename. Cosmetic.
Still unverified: whether Electron's MediaRecorder can emit video/mp4;codecs=avc1 on your minimum Electron version, as the PR description says. The drop-and-report behaviour means a host that gets this wrong loses the video visibly (an onError call) instead of shipping a mislabelled file. The result is not silent corruption, but a host on an unsupported platform ends up with no pixel video at all. An isTypeSupported hint in the README or JSDoc would help integrators. The previous WebM transcode fallback no longer exists for such a host.
Summary
Overall risk: Low. It is an opt-in feature in an unpublished SDK. The mislabelling risk raised in review is now guarded and tested.
Merge recommendation: Approve to merge once CI is green. Confirm, or explicitly accept as unverified, the Electron MediaRecorder MP4 support question. Decide whether WebM should remain an opt-in (issue Switch exported video from WebM to MP4 for Electron #54 has an empty body).
Most important to fix, all optional: the multi-entry concatenation in encodePixelVideo, and the manifestEntryForType comment.
Positives:
The fix puts validation at the seam where untrusted host bytes enter, and it fails closed with a clear, actionable error.
Test coverage includes the negative cases and the sink-throws case.
format is derived in one place from the extension, and per-display video.<n>.mp4 stays consistent.
The docs are updated, including closing the stale open item.
The PR description is candid about open questions.
· branch electron-mp4-video
Prior P1 (WebM labelled video.mp4 / format: "mp4", worker skips transcode, no onError) is fully_addressed.
createPixelVideoController.snapshot now sniffs every video entry: ftyp at offset 4 passes; EBML (1a 45 df a3) is dropped as WebM; anything else (short, zeros, non-bytes) is dropped as not MP4. Each drop goes through the existing onError sink (a throwing sink still does not reject the snapshot). The report still builds without that member. That is the fail-closed path asked for, and it sits in the controller rather than encodePixelVideo so a throwing encoder cannot take down bundle assembly.
I re-read the gate against the intended host path (video: { source } → controller snapshot → reportSnapshots → assembler). WebM from new MediaRecorder(stream) or a leftover webm/mjpeg encode no longer ships. Tests cover MP4 pass-through, WebM named in the message, mixed good+bad entries, non-bytes, and a throwing sink. Docs (electron.md contract paragraph, backend-beta-readiness.md struck item) and the encode / recorderFactory JSDocs match.
Chromium MediaRecorder.isTypeSupported('video/mp4;codecs=avc1') is still unverified; a host that cannot produce MP4 now gets a dropped video + onError instead of a silent unplayable file.
Overall
Overall risk: Low
Merge recommendation: Approve
Most important issues to fix: none remaining
Positive: The check is in the right place (controller snapshot, fail-closed, bundle still ships). Distinct error strings for WebM vs not-bytes vs no-ftyp. Design docs no longer claim the worker/viewer will see a mislabelled WebM.
The reason will be displayed to describe this comment to others. Learn more.
Review: export pixel video as MP4 (re-review at 2e2f82f)
The follow-up puts the container check on the live path. createPixelVideoController.snapshot sniffs ftyp at offset 4, drops WebM / short / wrong-box / non-bytes video entries, and reports through onError so the rest of the bundle still uploads. That is the right place for the gate: encodePixelVideo is an assembler encoder and a throw there would fail the whole report. The rename (video.mp4 → attrs.format: "mp4") plus this fail-closed filter is the Android-canonical contract without reopening the WebM-as-MP4 mislabel.
I re-traced host VideoCaptureSource.snapshot → controller filter → reportSnapshots extras → assembler fileEncoders.video. launchMain always wraps the opt-in source in the controller, so production pixel video cannot skip the sniff. Tests now use MP4-shaped fixtures (MP4_HEAD / MP4_FTYP) instead of shipping EBML under an .mp4 name.
Prior P1 — fully addressed
The previous finding (hard format: "mp4" with no byte check, so a default MediaRecorder WebM would skip transcode and not play) is fixed. WebM is named in the onError message; mixed snapshots keep the MP4 beside the dropped entries; a throwing sink still resolves [].
No new P0–P3
No remaining correctness, lifecycle, or mislabel bug on the production path.
Residual (not blocking)
encodePixelVideo still concatenates unlaned payloads. Two complete MP4s in one member is invalid ISO-BMFF. Display lanes are split and tested; a host that merges two sources without lanes would still hit it.
Chromium MediaRecordervideo/mp4;codecs=avc1 is still unverified (the PR’s own open question). That is now fail-closed: unsupported/default WebM is dropped with onError, not labelled mp4.
The sniff is container-only (ftyp), not codec (avc1). A JPEG-only muxer that emits MJPEG-in-MP4 would pass; the seam JSDoc now says an H.264 encoder plus muxer is required.
GitHub CI on this HEAD: Lint · Typecheck · Cycles · Coverage pass; E2E (node · bun · deno · real frameworks) pass; CodeQL pass.
Overall
Overall risk: Low — opt-in feature, unpublished SDK, fail-closed on the mislabel that blocked the last review.
Merge recommendation: Approve.
Most important to fix: Nothing remaining on this PR. Confirm MediaRecorder MP4 on the minimum Electron you ship before calling the pixel path done in product, not as a gate here.
Positive: Check is at snapshot (with onError), not in the unguarded encoder. Distinct drop reasons (WebM vs no ftyp vs not bytes). Per-display video.<n>.mp4 + format: "mp4" still lines up (nthFileName / manifestEntryForType). Contract is written on the seams and in electron.md; the stale “Electron video is WebM” backend item is marked resolved.
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
javascriptPull requests that update javascript code
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.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Refs #54.
What
DEFAULT_FILENAMES.videoisvideo.mp4(wasvideo.webm); per-display members arevideo.<n>.mp4. The manifest entry'sformatis derived from the extension, so it now saysmp4— the worker and viewer read it as the default H.264 MP4 and skip the WebM transcode. Android already writesvideo.mp4.electron.md,per-window-replay.md) updated; tests across protocol/core/node/electron now expect.mp4, and the electron e2e fake video starts with an MP4ftypbox.Contract change for hosts
The SDK ships no muxer: the host's
encode/MediaRecorderseam must now produce MP4 (H.264). Nothing validates the bytes, so a host that keeps producing WebM would ship a WebM labelled mp4.Open
MediaRecordercan emitvideo/mp4;codecs=avc1depends on the Electron/Chromium version — not verified here.Test plan
🤖 Generated with Claude Code