feat: standardize pipeline on H.264 for web playback - #143
Conversation
- Recorder: force codec h264 via setOutputSettings once per video-output instance (re-applied after the enableAudio rebuild reverts it), gated on cameraReady && !isRecording so a live session is never mutated. iOS-only; Android CameraX is already AVC. HEVC recordings never decoded in Firefox and often not in Chrome. - Import normalization: drop hevc from NATIVE_VIDEO_CODECS and pin codec:'h264' on the re-encode, so HEVC imports (all iPhone Photos videos) are conformed once at import time instead of leaking into merged output. Recordings and normalized imports now share one signature, widening the merge engine's zero-re-encode fast path. - Bump react-native-video-trim to the fork commit adding +faststart to all merge/trim/compress outputs, and the pulsevault submodule to the web-ready backstop. Known limits, documented in-code (verified against VisionCamera source, measurements from #142): AVCaptureMovieFileOutput has no faststart API, so raw per-clip files stay moov-at-end — faststart is owned by the merge layer, the upload contract gate (#142), and the server backstop. The 5 Mbps targetBitRate is applied inside the session-configuration batch and can silently fail to land (~8 Mbps measured), so nothing downstream assumes it. Fixes #140
9c3d357 to
8051573
Compare
There was a problem hiding this comment.
Pull request overview
Standardizes the client-side video pipeline on H.264 to ensure universal web playback by preventing HEVC from being recorded or passing through import normalization, and updates tests/dependencies to reflect the new contract.
Changes:
- Force iOS recorder output codec to H.264 via
setOutputSettings(fail-open on error). - Update import normalization to treat HEVC as non-native (re-encode to H.264) and pin
codec: 'h264'for re-encodes. - Update unit/e2e fixtures and bump
react-native-video-trimto a newer fork commit.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/features/recorder/use-recorder.ts | Forces H.264 codec on iOS video output instances post-ready (and documents limitations like moov-at-end). |
| src/utils/import-normalization.ts | Removes HEVC from passthrough codecs and explicitly sets H.264 for normalization re-encodes. |
| src/utils/import-normalization.test.ts | Updates fixture expectations so HEVC inputs now trigger re-encode with “video codec hevc” reasons. |
| src/utils/import-pipeline.e2e.test.ts | Updates e2e expectations: HEVC fixtures re-encode; audio-less sources remain audio-less after normalization. |
| package.json | Bumps react-native-video-trim git commit reference. |
| package-lock.json | Updates lock entry for the bumped react-native-video-trim commit (but currently resolves via git+ssh). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review finding: the effect recorded the output instance in the ref before the native call resolved and never cleared it on rejection. Native-side, setOutputSettings runs on the output's own queue and throws while the output is not yet connected; the session reconfigure that attaches a rebuilt output (every enableAudio flip) runs on a different queue, so the first attempt can race it and reject — permanently pinning that instance to HEVC with only a console.warn. Now the ref is committed on resolve, rejections retry on a short bounded backoff to ride out the reconfigure window, and cleanup cancels retries if a recording starts. Also bumps the pulsevault submodule to the review-fix commit (legacy sidecar status semantics, tmp-name collision, checksum caveat).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/features/recorder/use-recorder.ts:126
useRef<unknown>(null)loses useful type checking here; this ref is only ever used to hold the currentvideoOutputinstance identity. Typing it to the actual output instance type makes the intent clearer and prevents accidental misuse.
const h264OutputRef = useRef<unknown>(null);
src/features/recorder/use-recorder.ts:135
setOutputSettings({ codec: 'h264' })is started asynchronously, but there’s nothing preventingstartRecording()from being called while that Promise is still in flight. That undermines the stated safety gate (!isRecording) because the native codec mutation can still land after recording begins, potentially reintroducing the historical crash you’re trying to avoid, and it also means the first clip aftercameraReadycan still record as HEVC if recording starts before the pin completes.
Consider introducing an explicit “codec pinned” readiness (e.g., a Promise/ref) and blocking record start / enabling the record button until the pin has either succeeded or definitively failed, so the mutation cannot overlap an active capture.
let cancelled = false;
let timer: ReturnType<typeof setTimeout> | null = null;
const attempt = (retriesLeft: number) => {
output.setOutputSettings({ codec: 'h264' }).then(
() => {
…serialization - h264OutputRef typed to the video output instance instead of unknown. - Document why an in-flight codec pin cannot overlap an active capture: setOutputSettings and createRecorder share the output's serial native queue, so the mutation and recorder creation are ordered — worst case is a fail-open HEVC clip, never a mid-recording mutation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/import-normalization.ts:15
- The module header still states recordings are pinned to "5 Mbps", but use-recorder.ts now documents that
targetBitRatecan silently fail to apply (measured ~8 Mbps). To avoid misleading future changes to import policy (e.g., bitrate thresholds), update the comment to describe 5 Mbps as an intent/target rather than a guarantee.
* - non-H.264 video codecs — HEVC included: iPhone Photos imports are HEVC, which Firefox
* never decodes and Chrome usually can't, and letting them pass through means an
* HEVC-dominated draft merges back to HEVC on iOS. The one-time re-encode here (inside
* the existing import progress UI) is what guarantees every uploaded artifact is H.264.
* It also makes imports signature-match the H.264 recorder clips, so mixed drafts hit
…cess-fatal
react-native-vision-camera 5.2.0 round-trips the fully-resolved output settings
dict (including AVVideoWidthKey/AVVideoHeightKey, which AVFoundation refuses to
set back), so the H.264 pin's setOutputSettings({ codec: 'h264' }) aborted the
whole app with an uncatchable NSInvalidArgumentException the moment the recorder
opened (upstream mrousavy/react-native-vision-camera#4037, fixed by #4081 in
v5.2.1). 5.2.1 is therefore a hard floor for this branch — the 'fail-open'
behavior of the pin effect only holds from there.
Bumps worklets in lockstep and react-native-nitro-modules to 0.36.5 (the
nitrogen version VC 5.2.2 is generated with). Verified on-device: recorder
opens without crashing, clips probe h264 @ ~5.5 Mbps faststart — the 5 Mbps
targetBitRate pin now actually lands, since #4081 writes codec+bitrate in one
minimal dict.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…on the ultra-wide lens The CameraX guard from the Android parity work gated the zoom/torchMode props on cameraReady, which only flips at onStarted. On iOS that left the session with no zoom bound at startup, so fused multi-cam iPhones opened on the native default lens (ultra-wide — dark preview) and visibly snapped to 1x once the gate opened (TestFlight builds 27/28; build 26 bound zoom from the first frame and was fine). The guard exists purely for CameraX (OperationCanceledException on control calls before the session starts — iOS quietly tolerates them), so scope it to Android and restore the immediate binding on iOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Two findings from testing this branch on a real device (iPhone 17 Pro Max), both fixed in the branch now:
End-to-end check on the branch as it stands: recorder opens clean, and a 2-segment trim plus merge probes h264, 1080p, ~5.5 Mbps, AAC, ftyp/moov/mdat (faststart). In other words it passes #142's contract as a no-op and needs no server-side backstop intervention. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/utils/import-normalization.test.ts:136
- decideImport() now explicitly sets
options.codec: 'h264'for full re-encodes, but the re-encode unit test doesn’t assert it. Adding an assertion will prevent regressions where the codec default changes or the option is accidentally removed.
['hdr-hlg-portrait-1080p-30-hevc10', ['video codec hevc', '10-bit', 'HDR transfer arib-std-b67']],
['hdr-pq-landscape-4k-30-hevc10', ['video codec hevc', '10-bit', 'HDR transfer smpte2084']],
['rot270-portrait-1080p-30-hevc', ['video codec hevc']],
['timelapse-landscape-1080p-30-hevc-noaudio', ['video codec hevc']],
src/utils/import-normalization.ts:16
- The doc comment says the import re-encode “guarantees every uploaded artifact is H.264”, but the import runtime is fail-open: in use-recorder.ts the probe/encode path falls back to importing the original bytes when probeVideo()/compress() fails. This wording is misleading; please describe it as best-effort/fail-open (and/or defer the guarantee to the upload gate/backstop).
* never decodes and Chrome usually can't, and letting them pass through means an
* HEVC-dominated draft merges back to HEVC on iOS. The one-time re-encode here (inside
* the existing import progress UI) is what guarantees every uploaded artifact is H.264.
* It also makes imports signature-match the H.264 recorder clips, so mixed drafts hit
* the merge engine's zero-re-encode fast path instead of a selective conform.
morepriyam/react-native-video-trim#4 (faststart on all MP4/MOV outputs) and mieweb/pulsevault#58 (server-side web-ready backstop) have landed; move the git dep and the submodule from the PR-head SHAs to the merge commits on their default branches, as this PR's description planned. Content is unchanged — both merges were fast-forwards of the tested heads. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Companion PRs have landed and this branch is now re-pinned to their merge commits (29edbef):
Both were clean merges of the exact SHAs this branch was tested against, so no content change. Re-ran tsc and jest after the re-pin, all green. The re-pin chore from the PR description is done and this PR is ready to land. |
|
@jlocala1 could you take a look at this one? A lot of it builds on what you found in #142 (the dead HybridFrameRecorder patch, the bitrate pin not landing). Since then the branch has been tested end to end on a real device. One nice update: with vision-camera 5.2.2 the 5 Mbps pin actually works now, clips probe at around 5.5 Mbps. That means your gate in #142 becomes a cheap no-op probe on the happy path, which is exactly what you designed it for. The two companion PRs (the video-trim fork faststart and the pulsevault backstop) are merged and re-pinned here as well. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/utils/import-normalization.test.ts:136
- The unit test suite doesn’t currently assert the new guarantee that full re-encodes explicitly set
options.codec: 'h264'. Because the e2ecompressArgs()defaultscodecto'h264'when undefined, a future regression (removing this field) would still pass tests even though the intent is “never rely on native defaults”. Add an assertion ford.options.codecin the normalize/re-encode test case.
['hdr-hlg-portrait-1080p-30-hevc10', ['video codec hevc', '10-bit', 'HDR transfer arib-std-b67']],
['hdr-pq-landscape-4k-30-hevc10', ['video codec hevc', '10-bit', 'HDR transfer smpte2084']],
['rot270-portrait-1080p-30-hevc', ['video codec hevc']],
['timelapse-landscape-1080p-30-hevc-noaudio', ['video codec hevc']],
Problem
Videos uploaded from the app play poorly (or not at all) in browsers:
NATIVE_VIDEO_CODECSincludedhevc, so iPhone Photos imports skipped normalization and could dominate an iOS merge back into HEVC output.Changes
use-recorder.ts): forcecodec: 'h264'viasetOutputSettingsonce per video-output instance — re-applied when theenableAudioflip rebuilds the output (which silently reverts the codec), gated oncameraReady && !isRecordingso a live session is never mutated. Running post-ready also means the connection exists, unlike the configure-time bitrate path. Fail-open: on error the clip records HEVC, exactly today's behavior. iOS-only; Android CameraX is already AVC.import-normalization.ts): drophevcfromNATIVE_VIDEO_CODECSand pincodec: 'h264'on the re-encode. HEVC imports pay a one-time conform inside the existing import progress UI. Recordings and normalized imports now share one merge signature, so mixed drafts hit the zero-re-encode fast path more often than before.What this deliberately does NOT claim (verified findings from #142)
An earlier revision of this branch patched vision-camera's
HybridFrameRecorder.shouldOptimizeForNetworkUse. @jlocala1's review in #142 is correct and I verified it against the source:createRecorderactually returnsHybridVideoRecorderwrappingAVCaptureMovieFileOutput, which has no faststart API at all — the patch was dead code and is removed. Two consequences, now documented in-code:+faststart), the upload contract gate (Enforce the upload contract before a video leaves the device #142), and the pulsevault backstop (feat: web-ready backstop — faststart remux + H.264 transcode on upload complete pulsevault#58).targetBitRate: 5_000_000can silently fail to land (~8 Mbps measured on-device; VisionCamera applies it inside the session-configuration batch where the connection can be absent and failures only log). The pin stays as intent, but nothing downstream assumes it — Enforce the upload contract before a video leaves the device #142's gate is the enforcement point.Tests
import-normalization.test.ts/import-pipeline.e2e.test.ts: hevc fixtures moved passthrough → re-encode; the audio-less timelapse fixture exposed a latent blanketaacassertion that never accounted for no-audio sources, fixed to expect audio-less output.PULSE_E2E=1(16, real ffmpeg);tsc --noEmitclean.Deliberately out of scope: bitrate reduction (tracked separately) and upload-time enforcement (#142 owns that).
Fixes #140