From efcc3f45cae560ab4a01208e9e925edf5e100c7f Mon Sep 17 00:00:00 2001 From: Jonathan Locala Date: Mon, 3 Aug 2026 15:45:30 -0400 Subject: [PATCH 1/6] fix(upload): enforce the upload contract before a video leaves the device Recordings reached the server at 3840x2160 / ~28 Mbps HEVC with the index at the end of the file, while the recorder's own config asked for 1080p / 5 Mbps. Phones could not play them back. Measured on real clips from two builds, same phone: resolution bitrate codec faststart container pre-07-21 3840x2160 28.80 Mbps hevc no qt 2026-07-29 1920x1080 8.24 Mbps hevc no qt The resolution breach is already fixed upstream. `targetResolution` is not a setting but a bias in a weighted vote: VisionCamera scores every camera format against all outputs, and ranks its preview output above ours. The preview wants a format at least as large as the screen, every modern iPhone screen is taller than 1920px, and a 1080p format therefore took an aspect-ratio penalty weighted 100x. 4K won by roughly 20x. VisionCamera 5.2.0 rescored that comparison and 1080p now wins on its own -- confirmed on a device, above. So this does three things, in increasing order of how much they can be trusted: - Name the video output's resolutionBias explicitly, first in the constraint list. This is no longer the fix, it is margin: 5.2.0 wins by ~13% on the largest screens, and an explicit bias makes it ~6x. - Log the negotiated capture format on session start and warn when it breaches, so a lost vote can never again go unnoticed for weeks. - Gate uploads on the contract itself: probe, and re-encode only on a breach. A compliant file costs one probe and is uploaded untouched. The gate is the part that matters, and the 07-29 numbers are why: resolution is fixed, but bitrate is still 65% over a pin that has been set since 07-20, the codec is still HEVC (undecodable in Android Chrome), and the file is still not faststart. Settings are requests that a subsystem may ignore -- targetResolution did for weeks, targetBitRate and fileType still do. A gate does not ask. The gate sits in the upload path, not the export path: Share and Save-to-Photos keep full capture quality. The conditioned path is persisted so an after-kill resume re-uploads the same bytes rather than a fresh encode. Also stop swallowing import normalization failures: the fallback to the original bytes is correct, being silent about it is not. --- src/app/recorder.tsx | 15 +- src/features/recorder/use-recorder.ts | 43 +++++- src/features/upload/upload-manager.ts | 29 +++- src/utils/ensure-upload-contract.ts | 71 +++++++++ src/utils/import-normalization.ts | 8 +- src/utils/upload-contract.test.ts | 201 ++++++++++++++++++++++++++ src/utils/upload-contract.ts | 178 +++++++++++++++++++++++ 7 files changed, 536 insertions(+), 9 deletions(-) create mode 100644 src/utils/ensure-upload-contract.ts create mode 100644 src/utils/upload-contract.test.ts create mode 100644 src/utils/upload-contract.ts diff --git a/src/app/recorder.tsx b/src/app/recorder.tsx index 8b800a4..c68b0d1 100644 --- a/src/app/recorder.tsx +++ b/src/app/recorder.tsx @@ -276,9 +276,20 @@ export default function RecorderScreen() { }, [device]); // Pinned 1080p output + 30fps so every recorded clip is format-uniform (fast-path merge). + // + // `resolutionBias` FIRST, deliberately. VisionCamera picks the capture format by scoring every + // format against a weighted list of constraints — weight is `count - index`, so earlier entries + // outrank later ones — and it auto-appends one `{ resolutionBias: output }` per output, in the + // order the outputs are given. `` puts its own preview output ahead of ours, and the + // preview asks for a format at least as large as the SCREEN. Every modern iPhone screen is taller + // than 1920px, so 1080p could never satisfy the preview and the vote elected 4K: clips shipped at + // 3840x2160 / ~23 Mbps while `useVideoOutput` below asked for 1080p / 5 Mbps. VisionCamera 5.2.0 + // rescored that case so 1080p wins on its own, but only by ~13% on the largest screens — naming + // the video output's bias explicitly, at the top of the list, turns a margin into a mandate. + // See `logNegotiatedResolution` in use-recorder.ts for the runtime check that this held. const constraints = useMemo( - () => [{ videoStabilizationMode: stabilization }, { fps: 30 }], - [stabilization], + () => [{ resolutionBias: videoOutput }, { videoStabilizationMode: stabilization }, { fps: 30 }], + [stabilization, videoOutput], ); const outputs = useMemo(() => [videoOutput], [videoOutput]); diff --git a/src/features/recorder/use-recorder.ts b/src/features/recorder/use-recorder.ts index bcb3293..7cbcc96 100644 --- a/src/features/recorder/use-recorder.ts +++ b/src/features/recorder/use-recorder.ts @@ -33,6 +33,35 @@ import { generateThumbnailFile, getDurationMs } from '@/utils/video'; import CallDetector from '../../../modules/expo-call-detector/src/CallDetectorModule'; import { useCallState } from './use-call-state'; +import { UPLOAD_MAX_LONG_EDGE } from '@/utils/upload-contract'; + +/** + * Report what the camera session ACTUALLY negotiated, and shout if it isn't what we asked for. + * + * `targetResolution` on the video output is a bias in a weighted vote across all outputs, not a + * setting — the preview output's preference outranked it and every clip came out 4K while the + * config said 1080p, silently, for weeks. The failure mode of a lost vote is indistinguishable + * from success unless something looks. This looks. + * + * Cheap (one property read on session start) and non-fatal by design: a device that can only + * offer something larger should still record, it just shouldn't do so unnoticed. + */ +function logNegotiatedResolution(output: { + currentResolution?: { width: number; height: number }; +}) { + const size = output.currentResolution; + if (!size) return; + const longEdge = Math.max(size.width, size.height); + if (longEdge > UPLOAD_MAX_LONG_EDGE) { + console.warn( + `[recorder] capture format negotiated to ${size.width}x${size.height}, above the ${UPLOAD_MAX_LONG_EDGE} ` + + `long-edge target — clips will be re-encoded before upload. Check the resolutionBias constraint order.`, + ); + return; + } + console.log(`[recorder] capture format: ${size.width}x${size.height}`); +} + // 'cinematic' is an iOS-only AVCaptureVideoStabilizationMode — CameraX has no equivalent, so // Android only cycles through the modes it can actually honor. The union type keeps 'cinematic' // on both platforms so persisted iOS prefs and shared UI maps still typecheck. @@ -422,7 +451,13 @@ export function useRecorder(initialDraftId?: string) { if (probe) { const decision = decideImport(probe); if (decision.action === 'normalize') { - const normalized = await compress(picked.uri, decision.options).catch(() => null); + const normalized = await compress(picked.uri, decision.options).catch((e: unknown) => { + // Falling back to the original bytes is the right call — a failed normalize should not + // block the import — but it must not be SILENT. This is how a 4K HDR master enters a + // draft looking exactly like a clip that was normalized successfully. + console.warn('[import] normalize failed; importing the original', decision.reasons, e); + return null; + }); if (normalized) { normalizedPath = normalized.outputPath; sourceUri = normalized.outputPath; @@ -529,7 +564,11 @@ export function useRecorder(initialDraftId?: string) { callActive, appActive, reportMicPriorityError, - onCameraReady: () => setCameraReady(true), + onCameraReady: () => { + // The session has started, so the output is attached and its negotiated format is readable. + logNegotiatedResolution(videoOutput); + setCameraReady(true); + }, toggleRecording, finalizeRecording, importClip: () => void importClip(), diff --git a/src/features/upload/upload-manager.ts b/src/features/upload/upload-manager.ts index 281d12a..606e13a 100644 --- a/src/features/upload/upload-manager.ts +++ b/src/features/upload/upload-manager.ts @@ -20,6 +20,7 @@ import { getDraftToken } from '@/db/secure-token'; import { getDraftTranscriptRow } from '@/db/transcripts'; import { linesToVtt } from '@/features/transcription/vtt'; import { parseTranscriptLines } from '@/features/transcription/whisper'; +import { ensureUploadContract } from '@/utils/ensure-upload-contract'; import { absolutize, toFileUri } from '@/utils/file-store'; import { effFile } from '@/utils/segment-window'; import { generateThumbnailFile } from '@/utils/video'; @@ -578,6 +579,26 @@ class BackgroundUploadManager { private async uploadMerged(session: UploadSession, signal: AbortSignal): Promise { const { draftId, destination, segments, merged } = session; if (!merged) throw new Error('Export is not ready yet'); + + // Bring the video into the upload contract (H.264 / <=1920 long edge / <=5 Mbps / AAC) before + // anything reads its bytes. A compliant file costs one probe and is returned untouched; only a + // breach pays for a re-encode. This is the gate, NOT the export step: Share and Save-to-Photos + // read `state.outputPath` directly and must keep full capture quality — only what leaves the + // device for a browser to play is constrained. + const contract = await ensureUploadContract(merged.path); + if (contract.changed) { + // Persist the conditioned path so an after-kill resume re-uploads the SAME bytes. Without + // this, resume would re-encode from the original and a TUS PATCH could continue a transfer + // with bytes from a different encode. (Re-running the gate on an already-conditioned file is + // a no-op passthrough, so this is a cost saving as well as a correctness one.) + merged.path = contract.path; + await setUploadMerged(draftId, merged); + } + if (contract.failure) { + // Fail open, loudly: the upload proceeds with the original bytes, but it is not silent. + console.warn(`[contract] uploading unconditioned video for ${draftId}: ${contract.failure}`); + } + // merged.path is a bare filesystem path on Android (RNVT) — normalize to a file:// URI or the // File API rejects it outright ("URI is not absolute"). const file = new File(toFileUri(merged.path)); @@ -652,7 +673,13 @@ class BackgroundUploadManager { const result = await this.uploadOne( draftId, destination, - { artifactId: destination.artifactId, filename: `${draftId}.mp4`, kind: 'video', name: draftName, file }, + { + artifactId: destination.artifactId, + filename: `${draftId}.mp4`, + kind: 'video', + name: draftName, + file, + }, destination.resourceUrl, checksum, signal, diff --git a/src/utils/ensure-upload-contract.ts b/src/utils/ensure-upload-contract.ts new file mode 100644 index 0000000..125c041 --- /dev/null +++ b/src/utils/ensure-upload-contract.ts @@ -0,0 +1,71 @@ +import { compress, probeVideo } from 'react-native-video-trim'; + +import { decideUploadContract } from './upload-contract'; + +/** + * What conditioning did to a file on its way to being uploaded. + * + * `path` is always usable — on any failure it falls back to the input, so a broken probe + * or a failed encode degrades to "upload the original", never to "upload nothing". + */ +export type ContractResult = { + /** The file to upload: the conditioned copy, or the input when nothing was needed. */ + path: string; + /** True when `path` differs from the input. */ + changed: boolean; + /** Human-readable contract breaches that triggered the re-encode, for logging/UI. */ + reasons: string[]; + /** + * Set when the file could NOT be brought into the contract and the original is being + * uploaded instead. Never silently empty — the whole point of this gate is that a + * failure is visible. (`importClip` swallows exactly this case today, which is how a + * 4K master can still enter a draft.) + */ + failure?: string; +}; + +/** + * Bring a file into the upload contract before it is uploaded: probe it, and re-encode + * only if it breaches (see {@link decideUploadContract}). + * + * A compliant file is returned untouched — no copy, no re-encode, no quality generation + * spent. That is the intended steady state once the recorder emits 1080p/5 Mbps: this + * gate costs one probe and nothing else. It exists for the case where the recorder's + * format negotiation loses on some device we have not tested, or an import slips a 4K + * master through — outcomes we cannot prevent, only catch. + * + * Failure policy is deliberately "fail open, loudly": an upload that happens at reduced + * quality is better than an upload that does not happen, but it must be reported rather + * than absorbed. + */ +export async function ensureUploadContract(path: string): Promise { + const probe = await probeVideo(path).catch((e: unknown) => { + console.warn('[contract] probe failed; uploading the original', e); + return null; + }); + if (!probe) { + return { path, changed: false, reasons: [], failure: 'could not probe the file' }; + } + + const decision = decideUploadContract(probe); + if (decision.action === 'passthrough') { + return { path, changed: false, reasons: [] }; + } + + const result = await compress(path, { ...decision.options, outputExt: 'mp4' }).catch( + (e: unknown) => { + console.warn('[contract] re-encode failed; uploading the original', decision.reasons, e); + return null; + }, + ); + if (!result) { + return { + path, + changed: false, + reasons: decision.reasons, + failure: `could not re-encode (${decision.reasons.join(', ')})`, + }; + } + + return { path: result.outputPath, changed: true, reasons: decision.reasons }; +} diff --git a/src/utils/import-normalization.ts b/src/utils/import-normalization.ts index ac75a42..909f7bd 100644 --- a/src/utils/import-normalization.ts +++ b/src/utils/import-normalization.ts @@ -45,7 +45,7 @@ export const NORMALIZE_MAX_BITRATE = 8_000_000; * are re-encoded once at import time rather than leaking into merged artifacts. */ const NATIVE_VIDEO_CODECS = new Set(['h264']); /** HDR transfer functions: HLG (iPhone camera default) and PQ (HDR10 / Dolby Vision 8.x). */ -const HDR_TRANSFERS = new Set(['arib-std-b67', 'smpte2084']); +export const HDR_TRANSFERS = new Set(['arib-std-b67', 'smpte2084']); export type ImportDecision = | { action: 'passthrough' } @@ -56,17 +56,17 @@ export type ImportDecision = * suffix (yuv420p10le, p010le, ...) — matching the suffix rather than a bare `includes('10')` * keeps 8-bit chroma-subsampling names like `yuv410p` from being misclassified. */ -function is10Bit(pixelFormat: string): boolean { +export function is10Bit(pixelFormat: string): boolean { return /10(le|be)?$/.test(pixelFormat); } /** Effective fps for the decision: average when known (catches VFR), else nominal. */ -function effectiveFps(probe: VideoProbeResult): number { +export function effectiveFps(probe: VideoProbeResult): number { return probe.averageFps > 0 ? probe.averageFps : probe.nominalFps; } /** Display (post-rotation) dimensions: a 90/270 rotation swaps coded width/height. */ -function displaySize(probe: VideoProbeResult): { width: number; height: number } { +export function displaySize(probe: VideoProbeResult): { width: number; height: number } { const swapped = probe.rotation % 180 !== 0; return { width: swapped ? probe.height : probe.width, diff --git a/src/utils/upload-contract.test.ts b/src/utils/upload-contract.test.ts new file mode 100644 index 0000000..3e8ee0f --- /dev/null +++ b/src/utils/upload-contract.test.ts @@ -0,0 +1,201 @@ +import { describe, expect, it } from '@jest/globals'; +import type { VideoProbeResult } from 'react-native-video-trim'; + +import { + decideUploadContract, + effectiveBitrate, + UPLOAD_MAX_LONG_EDGE, + UPLOAD_TARGET_BITRATE, + UPLOAD_TARGET_FPS, +} from './upload-contract'; + +/** A clip that already satisfies the contract; override one field to test one rule. */ +function probe(overrides: Partial = {}): VideoProbeResult { + return { + hasVideo: true, + videoCodec: 'h264', + width: 1920, + height: 1080, + rotation: 0, + nominalFps: 30, + averageFps: 30, + bitrate: 5_000_000, + pixelFormat: 'yuv420p', + colorTransfer: 'bt709', + hasAudio: true, + audioCodec: 'aac', + audioSampleRate: 48000, + audioChannels: 2, + duration: 8000, + fileSize: 5_000_000, + ...overrides, + }; +} + +/** + * The two PulseCam uploads that were actually sitting on the dev box, measured with + * ffprobe. These are the files that would not play on a phone — the regression test + * for this whole gate is that both of them get normalized. + */ +const REAL_UPLOADS: Record = { + '669b7a78 (343MB, 110s)': probe({ + videoCodec: 'hevc', + width: 3840, + height: 2160, + bitrate: 24_761_167, + duration: 110_588, + fileSize: 343_727_447, + }), + 'a9dd0919 (579MB, 166s)': probe({ + videoCodec: 'hevc', + width: 3840, + height: 2160, + bitrate: 27_831_557, + duration: 165_821, + fileSize: 579_118_404, + }), +}; + +describe('decideUploadContract', () => { + describe('the real 4K HEVC uploads that broke phone playback', () => { + for (const [name, p] of Object.entries(REAL_UPLOADS)) { + it(`normalizes ${name} on every count`, () => { + const decision = decideUploadContract(p); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + + expect(decision.reasons).toEqual( + expect.arrayContaining([ + expect.stringContaining('video codec hevc'), + expect.stringContaining('3840x2160'), + expect.stringContaining('Mbps'), + ]), + ); + expect(decision.options).toMatchObject({ + codec: 'h264', + bitrate: UPLOAD_TARGET_BITRATE, + frameRate: UPLOAD_TARGET_FPS, + width: UPLOAD_MAX_LONG_EDGE, + }); + // Landscape source: the long edge is pinned via width, height follows the aspect ratio. + expect(decision.options.height).toBeUndefined(); + }); + } + }); + + it('passes a compliant clip through untouched — the whole point of the recorder pin', () => { + expect(decideUploadContract(probe())).toEqual({ action: 'passthrough' }); + }); + + it('converts HEVC that is otherwise perfect (this is where it diverges from imports)', () => { + const decision = decideUploadContract(probe({ videoCodec: 'hevc' })); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + expect(decision.reasons).toEqual(['video codec hevc']); + // Nothing is wrong with the geometry, so no scaling is requested. + expect(decision.options.width).toBeUndefined(); + expect(decision.options.height).toBeUndefined(); + expect(decision.options.codec).toBe('h264'); + }); + + it('pins the long edge by HEIGHT for a rotated (portrait) 4K clip, not width', () => { + // 3840x2160 coded + 90deg rotation displays as 2160x3840 — portrait. + const decision = decideUploadContract( + probe({ videoCodec: 'hevc', width: 3840, height: 2160, rotation: 90, bitrate: 24_000_000 }), + ); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + expect(decision.options.height).toBe(UPLOAD_MAX_LONG_EDGE); + expect(decision.options.width).toBeUndefined(); + expect(decision.reasons).toEqual( + expect.arrayContaining([expect.stringContaining('2160x3840')]), + ); + }); + + it('accepts a clip sitting exactly on the long-edge cap, and rejects one pixel over', () => { + expect(decideUploadContract(probe({ width: 1920, height: 1080 })).action).toBe('passthrough'); + expect(decideUploadContract(probe({ width: 1921, height: 1080 })).action).toBe('normalize'); + }); + + it('leaves a slightly-over-target bitrate alone rather than burning a generation on 7%', () => { + expect(decideUploadContract(probe({ bitrate: 5_400_000 })).action).toBe('passthrough'); + expect(decideUploadContract(probe({ bitrate: 8_000_000 })).action).toBe('normalize'); + }); + + it('caps high frame rates', () => { + expect(decideUploadContract(probe({ nominalFps: 60, averageFps: 60 })).action).toBe( + 'normalize', + ); + // 29.97 NTSC must pass untouched. + expect(decideUploadContract(probe({ nominalFps: 29.97, averageFps: 29.97 })).action).toBe( + 'passthrough', + ); + }); + + it('normalizes 10-bit and HDR sources', () => { + expect(decideUploadContract(probe({ pixelFormat: 'yuv420p10le' })).action).toBe('normalize'); + expect(decideUploadContract(probe({ colorTransfer: 'arib-std-b67' })).action).toBe('normalize'); + expect(decideUploadContract(probe({ colorTransfer: 'smpte2084' })).action).toBe('normalize'); + // 8-bit format whose name merely contains "10". + expect(decideUploadContract(probe({ pixelFormat: 'yuv410p' })).action).toBe('passthrough'); + }); + + it('conforms audio only, copying the video, when just the audio codec is wrong', () => { + const decision = decideUploadContract(probe({ audioCodec: 'opus' })); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + expect(decision.options).toEqual({ copyVideo: true }); + expect(decision.reasons).toEqual(['audio codec opus']); + }); + + it('does a full re-encode (not a video copy) when audio AND video are both wrong', () => { + const decision = decideUploadContract(probe({ videoCodec: 'hevc', audioCodec: 'opus' })); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + expect(decision.options.copyVideo).toBeUndefined(); + expect(decision.options.codec).toBe('h264'); + expect(decision.reasons).toEqual(['video codec hevc', 'audio codec opus']); + }); + + it('ignores audio entirely on a silent clip', () => { + expect(decideUploadContract(probe({ hasAudio: false, audioCodec: '' })).action).toBe( + 'passthrough', + ); + }); + + it('passes through anything with no video stream', () => { + expect(decideUploadContract(probe({ hasVideo: false })).action).toBe('passthrough'); + }); +}); + +describe('effectiveBitrate', () => { + it('prefers the declared stream bitrate', () => { + expect(effectiveBitrate(probe({ bitrate: 4_000_000 }))).toBe(4_000_000); + }); + + it('derives from size and duration when the container declares nothing', () => { + // 10 MB over 10 s = 8 Mbps. + expect( + effectiveBitrate(probe({ bitrate: -1, fileSize: 10_000_000, duration: 10_000 })), + ).toBeCloseTo(8_000_000); + }); + + it('reports unknown rather than guessing when neither source is usable', () => { + expect(effectiveBitrate(probe({ bitrate: -1, fileSize: 0, duration: 0 }))).toBe(-1); + }); + + it('does not treat an undeclared bitrate as a reason to re-encode', () => { + expect(decideUploadContract(probe({ bitrate: -1, fileSize: 0, duration: 0 }))).toEqual({ + action: 'passthrough', + }); + }); + + it('catches a 4K master whose container declares no bitrate, via size/duration', () => { + const decision = decideUploadContract( + probe({ bitrate: -1, fileSize: 343_727_447, duration: 110_588, videoCodec: 'h264' }), + ); + expect(decision.action).toBe('normalize'); + if (decision.action !== 'normalize') return; + expect(decision.reasons).toEqual(expect.arrayContaining([expect.stringContaining('Mbps')])); + }); +}); diff --git a/src/utils/upload-contract.ts b/src/utils/upload-contract.ts new file mode 100644 index 0000000..3fd36c8 --- /dev/null +++ b/src/utils/upload-contract.ts @@ -0,0 +1,178 @@ +import type { CompressOptions, VideoProbeResult } from 'react-native-video-trim'; + +import { displaySize, effectiveFps, HDR_TRANSFERS, is10Bit } from './import-normalization'; + +/** + * The upload contract (§ playback). + * + * Everything PulseCam uploads must satisfy: + * + * H.264 · long edge <= 1920 · <= 5 Mbps · AAC · faststart + * + * This is not a new target — it is what the recorder already asks for + * (`useVideoOutput` in use-recorder.ts) and what PulseClip's own exports already + * produce. The difference is that this module *enforces* it. + * + * Why enforcement is needed at all, measured on real recordings from two builds: + * + * resolution bitrate codec faststart container + * pre-07-21 3840x2160 28.80 Mbps hevc no qt + * 2026-07-29 1920x1080 8.24 Mbps hevc no qt + * + * The resolution breach is fixed — VisionCamera 5.2.0 rescored the format vote that the + * recorder's `targetResolution` participates in, so 1080p now wins it. Everything else + * still breaches: the bitrate lands 65% over a pin that has been set since 07-20, the + * codec is HEVC (fine on iOS, undecodable in Android Chrome), and the index is written + * at the end of the file. + * + * That is the case for a gate rather than a set of settings. `targetResolution` was + * silently ignored for weeks; `targetBitRate` and `fileType` still are. Each is a + * request to a subsystem that may or may not honour it, on hardware we have not tested. + * A probe-and-transcode gate does not ask. + * + * So: the recorder pin makes the common case FREE (a compliant file passes through + * untouched), and this gate makes every case CORRECT. Keep both. Neither replaces + * the other. + * + * How this differs from {@link decideImport}, which enforces a similar-looking policy + * at the Photos-import boundary: + * + * - **HEVC is not acceptable here.** Imports may keep it (iOS decodes it natively and + * the merge engine's fast path likes format-uniform clips), but an upload is watched + * in a browser, and Android Chrome will not decode HEVC at any size. Uploads convert. + * - **The bitrate ceiling is tighter** — an upload is streamed over a phone network, + * not read off local flash. + * + * Faststart is deliberately absent from the decision below: `moov` placement is not + * visible in a `probeVideo()` result. It is guaranteed on the writing side instead — + * the export/merge/compress paths emit it — because a file that is otherwise compliant + * should not be re-encoded just to move its index. + */ + +/** Long-edge cap. A 1080p long edge is 4x fewer pixels than 4K — the single biggest win. */ +export const UPLOAD_MAX_LONG_EDGE = 1920; +/** Re-encode target when a clip breaches the contract. Matches the recorder's own pin. */ +export const UPLOAD_TARGET_BITRATE = 5_000_000; +/** + * Re-encode trigger, deliberately above {@link UPLOAD_TARGET_BITRATE}. A clip that is + * already close to target is left alone: re-encoding 5.4 Mbps down to 5.0 costs a full + * transcode and a generation of quality to save ~7% of the bytes. Only a real breach + * (a 4K master at 23 Mbps) is worth the pass. + */ +export const UPLOAD_MAX_BITRATE = 6_500_000; +/** Frame-rate ceiling: passes 29.97/30 with margin, catches 60/120 (slo-mo, screen caps). */ +export const UPLOAD_MAX_FPS = 33; +/** Re-encode target frame rate. */ +export const UPLOAD_TARGET_FPS = 30; +/** The one codec that plays everywhere the browser lane cares about, Android Chrome included. */ +export const UPLOAD_VIDEO_CODEC = 'h264'; +/** The one audio codec that is MP4-muxable by stream copy across our paths. */ +export const UPLOAD_AUDIO_CODEC = 'aac'; + +export type UploadContractDecision = + | { action: 'passthrough' } + | { action: 'normalize'; options: Partial; reasons: string[] }; + +/** + * Effective video bitrate in bits per second. + * + * `probe.bitrate` is the *stream* bitrate and is `-1` when the container does not + * declare one — which is exactly the case for some camera-written MP4s, i.e. the files + * this gate exists to catch. Falling back to size/duration slightly overstates the + * video rate (it includes audio and container overhead), but it overstates in the safe + * direction: it can only push a borderline file towards being normalized, never away. + * + * Returns `-1` when neither source is usable, which the caller treats as "unknown" — + * an unknown bitrate is not by itself grounds to re-encode. + */ +export function effectiveBitrate(probe: VideoProbeResult): number { + if (probe.bitrate > 0) return probe.bitrate; + const seconds = probe.duration / 1000; + if (seconds > 0 && probe.fileSize > 0) return (probe.fileSize * 8) / seconds; + return -1; +} + +/** + * Decide how a file must be conditioned before it is uploaded: send the original bytes, + * conform only its audio, or re-encode it into the contract. + * + * Pure — feed it a `probeVideo()` result. The caller (`ensureUploadContract`) owns the + * file I/O; keeping the policy pure is what makes it testable without a device. + */ +export function decideUploadContract(probe: VideoProbeResult): UploadContractDecision { + // No video stream to constrain (audio-only artifacts ride other paths). Nothing to do. + if (!probe.hasVideo) return { action: 'passthrough' }; + + const reasons: string[] = []; + + if (probe.videoCodec !== UPLOAD_VIDEO_CODEC) { + // Includes HEVC, which is the common case: it is what the iPhone records by default, + // it is fine on iOS, and it is undecodable in Android Chrome. + reasons.push(`video codec ${probe.videoCodec || 'unknown'}`); + } + + const display = displaySize(probe); + const longEdge = Math.max(display.width, display.height); + const needsDownscale = longEdge > UPLOAD_MAX_LONG_EDGE; + if (needsDownscale) { + reasons.push(`${display.width}x${display.height} exceeds ${UPLOAD_MAX_LONG_EDGE}`); + } + + const bitrate = effectiveBitrate(probe); + if (bitrate > UPLOAD_MAX_BITRATE) { + reasons.push( + `${(bitrate / 1_000_000).toFixed(1)} Mbps exceeds ${UPLOAD_MAX_BITRATE / 1_000_000}`, + ); + } + + const fps = effectiveFps(probe); + if (fps > UPLOAD_MAX_FPS) { + reasons.push(`${Math.round(fps)} fps exceeds ${UPLOAD_MAX_FPS}`); + } + + // 10-bit / HDR: hardware H.264 encoders reject 10-bit input, and an HDR clip tone-maps + // unpredictably in a browser. Both force the SDR 8-bit re-encode. + if (is10Bit(probe.pixelFormat)) { + reasons.push(`10-bit pixel format ${probe.pixelFormat}`); + } + if (HDR_TRANSFERS.has(probe.colorTransfer)) { + reasons.push(`HDR transfer ${probe.colorTransfer}`); + } + + const audioHostile = probe.hasAudio && probe.audioCodec !== UPLOAD_AUDIO_CODEC; + + if (reasons.length === 0) { + if (audioHostile) { + // Video already satisfies the contract — stream-copy it and pay only for the audio. + return { + action: 'normalize', + options: { copyVideo: true }, + reasons: [`audio codec ${probe.audioCodec}`], + }; + } + return { action: 'passthrough' }; + } + + if (audioHostile) { + reasons.push(`audio codec ${probe.audioCodec}`); + } + + const options: Partial = { + codec: UPLOAD_VIDEO_CODEC, + bitrate: UPLOAD_TARGET_BITRATE, + frameRate: UPLOAD_TARGET_FPS, + }; + if (needsDownscale) { + // FFmpeg auto-rotates before filters run, so the cap is applied against DISPLAY + // orientation: pin the long edge, let the other follow the aspect ratio (-2). Pinning + // width unconditionally would upscale a portrait clip to 1920 wide — 4x the pixels + // the contract is trying to remove. + if (display.width >= display.height) { + options.width = UPLOAD_MAX_LONG_EDGE; + } else { + options.height = UPLOAD_MAX_LONG_EDGE; + } + } + + return { action: 'normalize', options, reasons }; +} From c66af0095870c15577f2d743972452f7e1604807 Mon Sep 17 00:00:00 2001 From: Jonathan Locala Date: Tue, 4 Aug 2026 12:28:01 -0400 Subject: [PATCH 2/6] fix(upload): apply the contract to segment uploads too The gate only ran in uploadMerged. uploadSegments sent every clip exactly as recorded. That mattered because `uploadUnit` is chosen by the DESTINATION SERVER, not by the app -- the pairing decides whether a draft uploads as one merged video or as separate clips. So a server asking for segments silently turned the contract off: no warning, no log, the protection simply did not run. Same shape as the bug this whole change exists for -- a safeguard that looks present on every path and isn't. Conditioning for segments goes to a stable path (drafts/{id}/upload/{seg}.mp4) rather than a cache temp name, because segment uploads resume byte-wise via TUS HEAD + PATCH: a resumed run has to send the bytes it began with, and re-encoding on resume would splice a second, subtly different encode into a half-finished transfer. A fixed path means a resumed run finds what it already produced. It lives inside the draft dir so deleteDraftDir reclaims it, and beside segments/ rather than in it so it can never be taken for a clip. The merged path solves the same problem by persisting its conditioned path in the draft row, which segments have no column for. --- src/features/upload/upload-manager.ts | 17 +++++++++-- src/utils/ensure-upload-contract.ts | 42 +++++++++++++++++++++++++++ src/utils/file-store.ts | 25 ++++++++++++++-- 3 files changed, 80 insertions(+), 4 deletions(-) diff --git a/src/features/upload/upload-manager.ts b/src/features/upload/upload-manager.ts index 606e13a..bdbd465 100644 --- a/src/features/upload/upload-manager.ts +++ b/src/features/upload/upload-manager.ts @@ -20,7 +20,7 @@ import { getDraftToken } from '@/db/secure-token'; import { getDraftTranscriptRow } from '@/db/transcripts'; import { linesToVtt } from '@/features/transcription/vtt'; import { parseTranscriptLines } from '@/features/transcription/whisper'; -import { ensureUploadContract } from '@/utils/ensure-upload-contract'; +import { ensureUploadContract, ensureUploadContractCached } from '@/utils/ensure-upload-contract'; import { absolutize, toFileUri } from '@/utils/file-store'; import { effFile } from '@/utils/segment-window'; import { generateThumbnailFile } from '@/utils/video'; @@ -766,7 +766,20 @@ class BackgroundUploadManager { for (const [index, segment] of segments.entries()) { reportClip(index + 1, index); - const file = new File(absolutize(effFile(segment))); + // Same contract as the merged unit. `uploadUnit` is chosen by the DESTINATION SERVER, not + // by us, so leaving this path ungated would silently disable the contract for any server + // that asks for segments — the protection would look present and not run. + const contract = await ensureUploadContractCached( + absolutize(effFile(segment)), + draftId, + segment.id, + ); + if (contract.failure) { + console.warn( + `[contract] uploading unconditioned clip ${segment.id} for ${draftId}: ${contract.failure}`, + ); + } + const file = new File(toFileUri(contract.path)); const checksum = await md5Checksum(file); const videoKey = `${segment.id}:video` as const; diff --git a/src/utils/ensure-upload-contract.ts b/src/utils/ensure-upload-contract.ts index 125c041..d38fae4 100644 --- a/src/utils/ensure-upload-contract.ts +++ b/src/utils/ensure-upload-contract.ts @@ -1,5 +1,7 @@ +import { File } from 'expo-file-system'; import { compress, probeVideo } from 'react-native-video-trim'; +import { toFileUri, uploadDest } from './file-store'; import { decideUploadContract } from './upload-contract'; /** @@ -69,3 +71,43 @@ export async function ensureUploadContract(path: string): Promise { + const dest = uploadDest(draftId, segmentId); + if (dest.exists && (dest.size ?? 0) > 0) { + // Already conditioned on an earlier attempt — reuse verbatim. + return { path: dest.uri, changed: true, reasons: [] }; + } + + const result = await ensureUploadContract(sourcePath); + if (!result.changed) return result; + + try { + // compress() writes into the OS-purgeable cache dir; move it somewhere a resume can find it. + await new File(toFileUri(result.path)).move(dest); + return { ...result, path: dest.uri }; + } catch (e) { + // The conditioned bytes exist but could not be parked. Upload them from where they are + // rather than falling back to the oversized original; a resume may re-encode, which is + // worse than this but still better than uploading 4K. + console.warn('[contract] could not park the conditioned clip; using the cache copy', e); + return result; + } +} diff --git a/src/utils/file-store.ts b/src/utils/file-store.ts index c06297d..2b942ff 100644 --- a/src/utils/file-store.ts +++ b/src/utils/file-store.ts @@ -5,14 +5,14 @@ import { Directory, File, Paths } from 'expo-file-system'; // between launches without invalidating references (§2.2). // drafts/{draftId}/segments/{segmentId}.mp4 — pristine original // drafts/{draftId}/segments/{segmentId}.edited.{rev}.mp4 — re-encoded editor output +// drafts/{draftId}/upload/{segmentId}.mp4 — upload-contract copy (see uploadDir) /** * Normalize a bare filesystem path to a `file://` URI. `merge()` / `getFrameAt` / the camera hand * back bare paths, but expo's `File`, expo-video, whisper, sharing, etc. all want a URI. A value * that already has a scheme is returned unchanged. */ -export const toFileUri = (path: string): string => - path.startsWith('/') ? `file://${path}` : path; +export const toFileUri = (path: string): string => (path.startsWith('/') ? `file://${path}` : path); export function segmentRelPath(draftId: string, segmentId: string): string { return `drafts/${draftId}/segments/${segmentId}.mp4`; @@ -55,6 +55,27 @@ function segmentsDir(draftId: string): Directory { return dir; } +/** + * The draft's conditioned-upload dir, creating it (and any missing parents) if needed. + * + * A STABLE location inside the draft's own directory, not a cache temp name. Segment uploads + * resume byte-wise (TUS HEAD + PATCH), so a resumed run must re-send the exact bytes it started + * with — re-encoding on resume would splice a second encode into a half-finished transfer. A + * fixed path means the conditioned file is found and reused instead. Living under + * `drafts/{draftId}/` means `deleteDraftDir` reclaims it with the draft, and it sits beside + * `segments/` rather than inside it so it can never be mistaken for a clip. + */ +function uploadDir(draftId: string): Directory { + const dir = new Directory(Paths.document, 'drafts', draftId, 'upload'); + dir.create({ intermediates: true, idempotent: true }); + return dir; +} + +/** The on-disk conditioned upload copy for a clip, creating the upload dir if needed. */ +export function uploadDest(draftId: string, segmentId: string): File { + return new File(uploadDir(draftId), `${segmentId}.mp4`); +} + /** The on-disk pristine segment file for a draft, creating the segments dir if needed. */ function segmentDest(draftId: string, segmentId: string): File { return new File(segmentsDir(draftId), `${segmentId}.mp4`); From c2cc150038237068a5d43841fa58f8e88a6939bb Mon Sep 17 00:00:00 2001 From: Jonathan Locala Date: Fri, 7 Aug 2026 08:49:04 -0400 Subject: [PATCH 3/6] fix(upload): normalise the contract input to a file:// URI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `uploadMerged` hands `merged.path` straight to `ensureUploadContract`, and on Android that is a bare filesystem path from react-native-video-trim. `probeVideo`/`compress` want a file:// URI, so the probe threw, and the catch turns a failed probe into "upload the original" — the gate failed open on every Android merged upload while still reading as present in the code. Normalise once at the entry instead of at each call: `toFileUri` is a no-op on input that is already a URI, so the iOS and segment paths (`absolutize` already yields a URI) are unchanged. `ContractResult.path` is now always a URI, which is what both call sites already assumed when they wrapped it in `toFileUri`. Reported by Copilot on #142. --- src/utils/ensure-upload-contract.test.ts | 102 +++++++++++++++++++++++ src/utils/ensure-upload-contract.ts | 27 ++++-- 2 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 src/utils/ensure-upload-contract.test.ts diff --git a/src/utils/ensure-upload-contract.test.ts b/src/utils/ensure-upload-contract.test.ts new file mode 100644 index 0000000..d287f4b --- /dev/null +++ b/src/utils/ensure-upload-contract.test.ts @@ -0,0 +1,102 @@ +import { beforeEach, describe, expect, it, jest } from '@jest/globals'; +import { compress, probeVideo, type VideoProbeResult } from 'react-native-video-trim'; + +import { ensureUploadContract } from './ensure-upload-contract'; + +// `jest.mock` is hoisted above these imports by babel-plugin-jest-hoist, so the factories run +// first and the imports above resolve to the doubles below. The mock functions are created +// INSIDE the factories rather than captured from module scope: a `const` declared out here is +// still in its temporal dead zone when the hoisted factory runs. +// +// expo-file-system is a native module that `file-store` imports at load. Stubbing it keeps the +// REAL `toFileUri` in play, which is the behaviour under test. +jest.mock('expo-file-system', () => ({ + File: class {}, + Directory: class {}, + Paths: { document: '/doc', cache: '/cache' }, +})); +jest.mock('react-native-video-trim', () => ({ + probeVideo: jest.fn(), + compress: jest.fn(), +})); + +const mockProbeVideo = probeVideo as jest.MockedFunction; +const mockCompress = compress as unknown as jest.MockedFunction< + (p: string, o: unknown) => Promise<{ outputPath: string }> +>; + +/** A clip that already satisfies the contract. */ +function compliant(overrides: Partial = {}): VideoProbeResult { + return { + hasVideo: true, + videoCodec: 'h264', + width: 1920, + height: 1080, + rotation: 0, + nominalFps: 30, + averageFps: 30, + bitrate: 5_000_000, + pixelFormat: 'yuv420p', + colorTransfer: 'bt709', + hasAudio: true, + audioCodec: 'aac', + audioSampleRate: 48000, + audioChannels: 2, + duration: 8000, + fileSize: 5_000_000, + ...overrides, + } as VideoProbeResult; +} + +/** + * The merged upload unit arrives as a bare filesystem path on Android (react-native-video-trim + * returns one). `probeVideo`/`compress` want a file:// URI, and a failed probe is swallowed into + * "upload the original" — so passing the bare path through made the gate fail open on every + * Android merged upload while still looking present in the code. + */ +describe('ensureUploadContract — path normalisation', () => { + beforeEach(() => { + mockProbeVideo.mockReset(); + mockCompress.mockReset(); + }); + + it('probes a bare Android path as a file:// URI', async () => { + mockProbeVideo.mockResolvedValue(compliant()); + await ensureUploadContract('/data/user/0/app/cache/merged.mp4'); + expect(mockProbeVideo).toHaveBeenCalledWith('file:///data/user/0/app/cache/merged.mp4'); + }); + + it('re-encodes from the normalised URI, not the bare path', async () => { + mockProbeVideo.mockResolvedValue(compliant({ videoCodec: 'hevc' })); + mockCompress.mockResolvedValue({ outputPath: '/cache/out.mp4' }); + await ensureUploadContract('/data/merged.mp4'); + expect(mockCompress).toHaveBeenCalledWith('file:///data/merged.mp4', expect.anything()); + }); + + it('leaves an input that is already a URI untouched', async () => { + mockProbeVideo.mockResolvedValue(compliant()); + await ensureUploadContract('file:///doc/drafts/a/segments/s.mp4'); + expect(mockProbeVideo).toHaveBeenCalledWith('file:///doc/drafts/a/segments/s.mp4'); + }); + + it('returns a file:// URI on every path — passthrough, re-encode, and failure', async () => { + mockProbeVideo.mockResolvedValue(compliant()); + expect((await ensureUploadContract('/data/a.mp4')).path).toBe('file:///data/a.mp4'); + + mockProbeVideo.mockResolvedValue(compliant({ videoCodec: 'hevc' })); + mockCompress.mockResolvedValue({ outputPath: '/cache/out.mp4' }); + expect((await ensureUploadContract('/data/b.mp4')).path).toBe('file:///cache/out.mp4'); + + mockProbeVideo.mockRejectedValue(new Error('no such file')); + const failed = await ensureUploadContract('/data/c.mp4'); + expect(failed.path).toBe('file:///data/c.mp4'); + expect(failed.failure).toBeTruthy(); + }); + + it('a probe failure still fails open rather than dropping the upload', async () => { + mockProbeVideo.mockRejectedValue(new Error('boom')); + const r = await ensureUploadContract('/data/d.mp4'); + expect(r.changed).toBe(false); + expect(r.path).toBeTruthy(); + }); +}); diff --git a/src/utils/ensure-upload-contract.ts b/src/utils/ensure-upload-contract.ts index d38fae4..0ef1493 100644 --- a/src/utils/ensure-upload-contract.ts +++ b/src/utils/ensure-upload-contract.ts @@ -11,7 +11,11 @@ import { decideUploadContract } from './upload-contract'; * or a failed encode degrades to "upload the original", never to "upload nothing". */ export type ContractResult = { - /** The file to upload: the conditioned copy, or the input when nothing was needed. */ + /** + * The file to upload: the conditioned copy, or the input when nothing was needed. Always a + * `file://` URI — callers hand it straight to Expo `File`, and the merged unit's input is a + * bare path on Android. + */ path: string; /** True when `path` differs from the input. */ changed: boolean; @@ -41,20 +45,29 @@ export type ContractResult = { * than absorbed. */ export async function ensureUploadContract(path: string): Promise { - const probe = await probeVideo(path).catch((e: unknown) => { + // The merged unit arrives as a BARE filesystem path on Android (react-native-video-trim + // returns one, and `uploadMerged` documents it at the `new File(toFileUri(merged.path))` + // call one step later). `probeVideo`/`compress` want a file:// URI, so a bare path throws — + // and the catch below turns that into "upload the original". The gate would therefore fail + // open on EVERY Android merged upload: present in the code, never actually enforcing. + // `toFileUri` is a no-op on input that is already a URI, so the iOS/segment paths are + // unchanged (`absolutize` already yields a URI). + const uri = toFileUri(path); + + const probe = await probeVideo(uri).catch((e: unknown) => { console.warn('[contract] probe failed; uploading the original', e); return null; }); if (!probe) { - return { path, changed: false, reasons: [], failure: 'could not probe the file' }; + return { path: uri, changed: false, reasons: [], failure: 'could not probe the file' }; } const decision = decideUploadContract(probe); if (decision.action === 'passthrough') { - return { path, changed: false, reasons: [] }; + return { path: uri, changed: false, reasons: [] }; } - const result = await compress(path, { ...decision.options, outputExt: 'mp4' }).catch( + const result = await compress(uri, { ...decision.options, outputExt: 'mp4' }).catch( (e: unknown) => { console.warn('[contract] re-encode failed; uploading the original', decision.reasons, e); return null; @@ -62,14 +75,14 @@ export async function ensureUploadContract(path: string): Promise Date: Fri, 7 Aug 2026 09:44:45 -0400 Subject: [PATCH 4/6] fix(upload): actually enforce faststart, not just claim it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The contract has always listed faststart, but the gate never checked it: `probeVideo()` reports codecs and geometry, not box order, so `decideUploadContract` said nothing about `moov` placement. In practice that went unnoticed, because the files missing faststart were also breaching the codec and bitrate rules, and the re-encode those triggered moved the index to the front as a side effect. mieweb/pulse#143 removes the breach. Once the recorder pins H.264 and the 5 Mbps bitrate lands, a raw clip is compliant on everything a probe can see and passes straight through with its index still at the tail. The two paths that skip the merge engine are exactly the ones affected: a single-clip draft (use-export returns the recorder's file verbatim) and every segment upload. Both are raw AVCaptureMovieFileOutput files, and that API has no faststart option at all. Without this, they would have been left to the server-side backstop alone. Check it by reading the file instead of the probe. An MP4 is a flat list of `[size][type]` boxes, so walking them until `moov` or `mdat` appears answers the question in two 16-byte reads and never touches the payload. When the index is at the tail, remux with `copyVideo` — the video track is stream-copied and the fork applies `+faststart` to the output, so it costs about a file copy rather than a quality generation. The scan returns three values, not two: an unreadable header or an unfamiliar container reads as `null`, and only an explicit `false` does any work. Guessing in the other direction would mean re-encoding every upload forever. --- src/utils/ensure-upload-contract.test.ts | 99 ++++++++++++++++++ src/utils/ensure-upload-contract.ts | 41 ++++++++ src/utils/faststart.test.ts | 123 +++++++++++++++++++++++ src/utils/faststart.ts | 105 +++++++++++++++++++ src/utils/upload-contract.ts | 16 ++- 5 files changed, 380 insertions(+), 4 deletions(-) create mode 100644 src/utils/faststart.test.ts create mode 100644 src/utils/faststart.ts diff --git a/src/utils/ensure-upload-contract.test.ts b/src/utils/ensure-upload-contract.test.ts index d287f4b..38e8115 100644 --- a/src/utils/ensure-upload-contract.test.ts +++ b/src/utils/ensure-upload-contract.test.ts @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, jest } from '@jest/globals'; import { compress, probeVideo, type VideoProbeResult } from 'react-native-video-trim'; import { ensureUploadContract } from './ensure-upload-contract'; +import { hasFaststart } from './faststart'; // `jest.mock` is hoisted above these imports by babel-plugin-jest-hoist, so the factories run // first and the imports above resolve to the doubles below. The mock functions are created @@ -19,7 +20,11 @@ jest.mock('react-native-video-trim', () => ({ probeVideo: jest.fn(), compress: jest.fn(), })); +// The scanner has its own tests against synthetic box layouts (faststart.test.ts); here we +// only care what the gate DOES with each of its three answers. +jest.mock('./faststart', () => ({ hasFaststart: jest.fn() })); +const mockHasFaststart = hasFaststart as jest.MockedFunction; const mockProbeVideo = probeVideo as jest.MockedFunction; const mockCompress = compress as unknown as jest.MockedFunction< (p: string, o: unknown) => Promise<{ outputPath: string }> @@ -58,6 +63,9 @@ describe('ensureUploadContract — path normalisation', () => { beforeEach(() => { mockProbeVideo.mockReset(); mockCompress.mockReset(); + // These cases are about path handling, so keep faststart out of the picture. + mockHasFaststart.mockReset(); + mockHasFaststart.mockReturnValue(true); }); it('probes a bare Android path as a file:// URI', async () => { @@ -100,3 +108,94 @@ describe('ensureUploadContract — path normalisation', () => { expect(r.path).toBeTruthy(); }); }); + +/** + * `moov` placement is the one contract term a probe cannot see, so it is enforced here rather + * than in `decideUploadContract`. It only bites on files that skip the merge engine — a + * single-clip draft and every segment upload — which are raw AVCaptureMovieFileOutput files + * and therefore always index-at-the-tail. Before the recorder pinned H.264 they were re-encoded + * anyway for breaching codec/bitrate, and got faststart as a side effect; now they are otherwise + * compliant, so without this they would upload with the index still at the end. + */ +describe('ensureUploadContract — faststart', () => { + beforeEach(() => { + mockProbeVideo.mockReset(); + mockCompress.mockReset(); + mockHasFaststart.mockReset(); + mockProbeVideo.mockResolvedValue(compliant()); + }); + + it('remuxes a compliant clip whose moov is at the end', async () => { + mockHasFaststart.mockReturnValue(false); + mockCompress.mockResolvedValue({ outputPath: '/cache/remuxed.mp4' }); + + const r = await ensureUploadContract('file:///doc/segments/s.mp4'); + + expect(r.changed).toBe(true); + expect(r.path).toBe('file:///cache/remuxed.mp4'); + expect(r.reasons).toEqual(['moov atom at the end of the file']); + }); + + it('stream-copies the video rather than transcoding it', async () => { + mockHasFaststart.mockReturnValue(false); + mockCompress.mockResolvedValue({ outputPath: '/cache/remuxed.mp4' }); + + await ensureUploadContract('file:///doc/segments/s.mp4'); + + // copyVideo maps to `-c:v copy` in the fork, which also applies `+faststart` to the + // output. Re-encoding here would spend a quality generation to move four bytes. + expect(mockCompress).toHaveBeenCalledWith( + 'file:///doc/segments/s.mp4', + expect.objectContaining({ copyVideo: true, outputExt: 'mp4' }), + ); + const options = mockCompress.mock.calls[0][1] as Record; + expect(options.bitrate).toBeUndefined(); + expect(options.width).toBeUndefined(); + expect(options.height).toBeUndefined(); + }); + + it('leaves a merged clip that already has faststart completely alone', async () => { + mockHasFaststart.mockReturnValue(true); + + const r = await ensureUploadContract('file:///cache/merged.mp4'); + + expect(r.changed).toBe(false); + expect(r.reasons).toEqual([]); + expect(mockCompress).not.toHaveBeenCalled(); + }); + + it('does nothing when the scan cannot tell', async () => { + // A short read or an unrecognised container. Guessing would mean a needless re-encode on + // every upload, which is worse than the stall it would be avoiding. + mockHasFaststart.mockReturnValue(null); + + const r = await ensureUploadContract('file:///cache/odd.mp4'); + + expect(r.changed).toBe(false); + expect(mockCompress).not.toHaveBeenCalled(); + }); + + it('fails open loudly when the remux itself fails', async () => { + mockHasFaststart.mockReturnValue(false); + mockCompress.mockRejectedValue(new Error('ffmpeg exploded')); + + const r = await ensureUploadContract('file:///doc/segments/s.mp4'); + + expect(r.changed).toBe(false); + expect(r.path).toBe('file:///doc/segments/s.mp4'); + expect(r.failure).toBeTruthy(); + }); + + it('does not double-handle a clip that is already being re-encoded', async () => { + // A breaching file goes down the normalize path, and compress() writes faststart there + // too — so the scan must not add a second pass on top. + mockProbeVideo.mockResolvedValue(compliant({ videoCodec: 'hevc' })); + mockHasFaststart.mockReturnValue(false); + mockCompress.mockResolvedValue({ outputPath: '/cache/out.mp4' }); + + const r = await ensureUploadContract('file:///doc/segments/s.mp4'); + + expect(mockCompress).toHaveBeenCalledTimes(1); + expect(r.reasons).toEqual(['video codec hevc']); + }); +}); diff --git a/src/utils/ensure-upload-contract.ts b/src/utils/ensure-upload-contract.ts index 0ef1493..90f9e11 100644 --- a/src/utils/ensure-upload-contract.ts +++ b/src/utils/ensure-upload-contract.ts @@ -1,9 +1,13 @@ import { File } from 'expo-file-system'; import { compress, probeVideo } from 'react-native-video-trim'; +import { hasFaststart } from './faststart'; import { toFileUri, uploadDest } from './file-store'; import { decideUploadContract } from './upload-contract'; +/** Reported when a file is compliant in every respect except its `moov` placement. */ +const FASTSTART_REASON = 'moov atom at the end of the file'; + /** * What conditioning did to a file on its way to being uploaded. * @@ -64,6 +68,43 @@ export async function ensureUploadContract(path: string): Promise { + console.warn('[contract] faststart remux failed; uploading the original', e); + return null; + }, + ); + if (!remuxed) { + return { + path: uri, + changed: false, + reasons: [FASTSTART_REASON], + failure: `could not remux for faststart (${FASTSTART_REASON})`, + }; + } + return { + path: toFileUri(remuxed.outputPath), + changed: true, + reasons: [FASTSTART_REASON], + }; + } return { path: uri, changed: false, reasons: [] }; } diff --git a/src/utils/faststart.test.ts b/src/utils/faststart.test.ts new file mode 100644 index 0000000..c76cae5 --- /dev/null +++ b/src/utils/faststart.test.ts @@ -0,0 +1,123 @@ +import { describe, expect, it, jest } from '@jest/globals'; + +import { type ByteReader, scanForFaststart } from './faststart'; + +// The module imports expo-file-system for `hasFaststart`, which jest cannot parse as shipped. +// These cases only exercise the pure scanner, so a stub is enough to let the import resolve. +jest.mock('expo-file-system', () => ({ File: class {} })); + +/** + * Build a fake MP4 as a list of top-level boxes and hand back a reader over it. Only the + * headers matter to the scanner, so box bodies are zero-filled: a real `mdat` is hundreds + * of megabytes and the whole point is that we never read it. + */ +function mp4(boxes: { type: string; size: number }[]): ByteReader { + const total = boxes.reduce((n, b) => n + b.size, 0); + const bytes = new Uint8Array(total); + let at = 0; + for (const box of boxes) { + bytes[at] = (box.size >>> 24) & 0xff; + bytes[at + 1] = (box.size >>> 16) & 0xff; + bytes[at + 2] = (box.size >>> 8) & 0xff; + bytes[at + 3] = box.size & 0xff; + for (let i = 0; i < 4; i++) bytes[at + 4 + i] = box.type.charCodeAt(i); + at += box.size; + } + return (offset, length) => { + if (offset >= bytes.length) return null; + return bytes.subarray(offset, Math.min(offset + length, bytes.length)); + }; +} + +describe('scanForFaststart', () => { + it('reports faststart when moov precedes mdat', () => { + expect( + scanForFaststart( + mp4([ + { type: 'ftyp', size: 32 }, + { type: 'moov', size: 4096 }, + { type: 'mdat', size: 1024 }, + ]), + ), + ).toBe(true); + }); + + it('reports moov-at-end for a raw recorder clip', () => { + // What AVCaptureMovieFileOutput writes: ftyp, then samples, then the index. + expect( + scanForFaststart( + mp4([ + { type: 'ftyp', size: 32 }, + { type: 'mdat', size: 8192 }, + { type: 'moov', size: 4096 }, + ]), + ), + ).toBe(false); + }); + + it('skips the filler boxes real writers emit before the payload', () => { + expect( + scanForFaststart( + mp4([ + { type: 'ftyp', size: 32 }, + { type: 'wide', size: 8 }, + { type: 'free', size: 64 }, + { type: 'moov', size: 4096 }, + ]), + ), + ).toBe(true); + }); + + it('follows a 64-bit largesize box', () => { + // size == 1 means the real size lives in the 8 bytes after the header. + const bytes = new Uint8Array(64); + const write = (at: number, type: string, size: number, large?: number) => { + const s = large ? 1 : size; + bytes[at] = (s >>> 24) & 0xff; + bytes[at + 1] = (s >>> 16) & 0xff; + bytes[at + 2] = (s >>> 8) & 0xff; + bytes[at + 3] = s & 0xff; + for (let i = 0; i < 4; i++) bytes[at + 4 + i] = type.charCodeAt(i); + if (large) { + // High word stays zero; low word carries the size. + bytes[at + 12] = (large >>> 24) & 0xff; + bytes[at + 13] = (large >>> 16) & 0xff; + bytes[at + 14] = (large >>> 8) & 0xff; + bytes[at + 15] = large & 0xff; + } + }; + write(0, 'ftyp', 0, 24); + write(24, 'moov', 16); + + const read: ByteReader = (offset, length) => + offset >= bytes.length + ? null + : bytes.subarray(offset, Math.min(offset + length, bytes.length)); + expect(scanForFaststart(read)).toBe(true); + }); + + it('gives up rather than guessing on a short read', () => { + expect(scanForFaststart(() => new Uint8Array(4))).toBeNull(); + expect(scanForFaststart(() => null)).toBeNull(); + }); + + it('gives up on a malformed size instead of looping forever', () => { + // A box claiming to be smaller than its own header would never advance the cursor. + expect(scanForFaststart(mp4([{ type: 'ftyp', size: 4 }]))).toBeNull(); + }); + + it('gives up when a box runs to the end of the file before moov', () => { + // size == 0 means "to EOF", so nothing follows and we never saw an index. + const bytes = new Uint8Array(16); + for (let i = 0; i < 4; i++) bytes[4 + i] = 'mdaX'.charCodeAt(i); + expect( + scanForFaststart((offset, length) => + offset >= bytes.length ? null : bytes.subarray(offset, offset + length), + ), + ).toBeNull(); + }); + + it('gives up on a file that is not an MP4 at all', () => { + expect(scanForFaststart(mp4([{ type: 'RIFF', size: 32 }]))).toBeNull(); + }); +}); diff --git a/src/utils/faststart.ts b/src/utils/faststart.ts new file mode 100644 index 0000000..d9b6c55 --- /dev/null +++ b/src/utils/faststart.ts @@ -0,0 +1,105 @@ +import { File } from 'expo-file-system'; + +/** + * Faststart detection (§ playback). + * + * An MP4 is a flat sequence of boxes, each `[4-byte big-endian size][4-byte ASCII type]`. + * "Faststart" just means the `moov` box (the index a player needs before it can render a + * single frame) sits ahead of `mdat` (the samples) rather than after it. With `moov` last, + * a browser has to fetch or seek to the tail of the file before playback can begin, which + * on a 100 MB upload over a phone network is the difference between "plays" and "spins". + * + * `probeVideo()` cannot see this — it reports codecs and geometry, not box order — which is + * why {@link decideUploadContract} deliberately says nothing about faststart. So we read the + * box headers ourselves. It costs two ranged reads of 16 bytes: the walk stops at whichever + * of `moov`/`mdat` comes first, and in a real file that is the second or third box. + * + * This matters because the two upload paths that skip the merge engine — a single-clip draft + * (`use-export.ts` returns the recorder's file verbatim) and every segment upload — hand us a + * raw `AVCaptureMovieFileOutput` file, and that API has no faststart option at all. Those + * files are always `moov`-at-end. Everything the merge/compress layer writes already has + * `+faststart` applied by the video-trim fork. + */ + +/** Reads `length` bytes at `offset`. Returns null (or a short read) at EOF or on error. */ +export type ByteReader = (offset: number, length: number) => Uint8Array | null; + +/** Boxes to walk before giving up. Real files reach `moov`/`mdat` within two or three. */ +const MAX_BOXES = 8; + +/** A 64-bit `largesize` needs 8 more bytes after the 8-byte header. */ +const HEADER_BYTES = 16; + +function readU32(b: Uint8Array, at: number): number { + return ((b[at] << 24) >>> 0) + (b[at + 1] << 16) + (b[at + 2] << 8) + b[at + 3]; +} + +function boxType(b: Uint8Array): string { + return String.fromCharCode(b[4], b[5], b[6], b[7]); +} + +/** + * Walk the top-level boxes and report whether `moov` precedes `mdat`. + * + * Returns `true` for faststart, `false` for `moov`-at-end, and **`null` for "cannot tell"** — + * a short read, a malformed size, a non-MP4 container, or a file that runs out of boxes. + * Callers must treat `null` as "leave it alone": guessing wrong here costs a needless + * re-encode on every upload, which is worse than the stall it would be trying to avoid. + * + * Pure, so the parsing is testable without a device or a real file. + */ +export function scanForFaststart(read: ByteReader): boolean | null { + let offset = 0; + + for (let i = 0; i < MAX_BOXES; i++) { + const head = read(offset, HEADER_BYTES); + if (!head || head.length < 8) return null; + + const type = boxType(head); + if (type === 'moov') return true; + if (type === 'mdat') return false; + + let size = readU32(head, 0); + if (size === 1) { + // 64-bit largesize. Split across two u32 reads because a single u64 does not fit a + // JS number; anything past 2^53 is not a file we could have written anyway. + if (head.length < 16) return null; + size = readU32(head, 8) * 2 ** 32 + readU32(head, 12); + } else if (size === 0) { + // "Extends to end of file", so there is no box after this one and we never saw `moov`. + return null; + } + + // A box cannot be smaller than its own header; a zero/negative step would spin forever. + if (!Number.isSafeInteger(size) || size < 8) return null; + offset += size; + } + + return null; +} + +/** + * {@link scanForFaststart} against a real file. Never throws: any failure reads as `null`, + * which the gate treats as "do nothing". + */ +export function hasFaststart(uri: string): boolean | null { + let handle: ReturnType | null = null; + try { + handle = new File(uri).open(); + const h = handle; + return scanForFaststart((offset, length) => { + h.offset = offset; + const bytes = h.readBytes(length); + return bytes && bytes.length > 0 ? bytes : null; + }); + } catch (e) { + console.warn('[contract] could not read box headers; assuming nothing about faststart', e); + return null; + } finally { + try { + handle?.close(); + } catch { + // Closing a handle we may never have opened is not worth reporting. + } + } +} diff --git a/src/utils/upload-contract.ts b/src/utils/upload-contract.ts index 3fd36c8..17a8b9c 100644 --- a/src/utils/upload-contract.ts +++ b/src/utils/upload-contract.ts @@ -43,10 +43,18 @@ import { displaySize, effectiveFps, HDR_TRANSFERS, is10Bit } from './import-norm * - **The bitrate ceiling is tighter** — an upload is streamed over a phone network, * not read off local flash. * - * Faststart is deliberately absent from the decision below: `moov` placement is not - * visible in a `probeVideo()` result. It is guaranteed on the writing side instead — - * the export/merge/compress paths emit it — because a file that is otherwise compliant - * should not be re-encoded just to move its index. + * Faststart is absent from the decision below because `moov` placement is not visible in + * a `probeVideo()` result — but it is still part of the contract, and it is still + * enforced. `ensureUploadContract` checks it separately by reading the file's box headers + * and remuxing (stream-copy, not transcode) when the index is at the tail. Keeping it out + * of this function is what lets the function stay pure and testable on a probe alone. + * + * That split matters more than it looks. The merge/compress paths emit `+faststart`, so + * for a long time the only files that missed it were also breaching the codec or bitrate + * rules, and the re-encode below moved their index as a side effect. Fixing the recorder + * (mieweb/pulse#143) removed the breach and would have removed the accident with it, + * leaving single-clip drafts and segment uploads — the two paths that skip the merge + * engine entirely — shipping `moov`-at-end with nothing client-side to catch it. */ /** Long-edge cap. A 1080p long edge is 4x fewer pixels than 4K — the single biggest win. */ From 874b24951c4021920f4a209b5db19bb207a849e0 Mon Sep 17 00:00:00 2001 From: morepriyam Date: Sat, 8 Aug 2026 18:58:34 +0530 Subject: [PATCH 5/6] refactor: move the upload gate into features/upload; share probe readers via utils/probe The three gate modules (upload-contract, faststart, ensure-upload-contract) are upload-domain logic consumed only by upload-manager, so they live with it in features/upload/ alongside tus-client and beat-manifest rather than diluting utils/. Their tests move with them; relative imports keep the jest doubles working unchanged. The probe readers (displaySize, effectiveFps, is10Bit, HDR_TRANSFERS) that upload-contract borrowed from import-normalization now live in utils/probe.ts: the upload contract no longer reaches backwards into the import pipeline, and the two policies read a probe through one shared lens instead of drifting apart. import-normalization's exports revert to module-private. No behavior change: tsc clean, expo lint clean, 175/175 tests pass including the e2e suite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/features/recorder/use-recorder.ts | 2 +- .../upload}/ensure-upload-contract.test.ts | 0 .../upload}/ensure-upload-contract.ts | 3 +- .../upload}/faststart.test.ts | 0 src/{utils => features/upload}/faststart.ts | 0 .../upload}/upload-contract.test.ts | 0 .../upload}/upload-contract.ts | 2 +- src/features/upload/upload-manager.ts | 2 +- src/utils/import-normalization.ts | 27 +------------- src/utils/probe.ts | 37 +++++++++++++++++++ 10 files changed, 44 insertions(+), 29 deletions(-) rename src/{utils => features/upload}/ensure-upload-contract.test.ts (100%) rename src/{utils => features/upload}/ensure-upload-contract.ts (99%) rename src/{utils => features/upload}/faststart.test.ts (100%) rename src/{utils => features/upload}/faststart.ts (100%) rename src/{utils => features/upload}/upload-contract.test.ts (100%) rename src/{utils => features/upload}/upload-contract.ts (99%) create mode 100644 src/utils/probe.ts diff --git a/src/features/recorder/use-recorder.ts b/src/features/recorder/use-recorder.ts index 7cbcc96..f90e7f6 100644 --- a/src/features/recorder/use-recorder.ts +++ b/src/features/recorder/use-recorder.ts @@ -33,7 +33,7 @@ import { generateThumbnailFile, getDurationMs } from '@/utils/video'; import CallDetector from '../../../modules/expo-call-detector/src/CallDetectorModule'; import { useCallState } from './use-call-state'; -import { UPLOAD_MAX_LONG_EDGE } from '@/utils/upload-contract'; +import { UPLOAD_MAX_LONG_EDGE } from '@/features/upload/upload-contract'; /** * Report what the camera session ACTUALLY negotiated, and shout if it isn't what we asked for. diff --git a/src/utils/ensure-upload-contract.test.ts b/src/features/upload/ensure-upload-contract.test.ts similarity index 100% rename from src/utils/ensure-upload-contract.test.ts rename to src/features/upload/ensure-upload-contract.test.ts diff --git a/src/utils/ensure-upload-contract.ts b/src/features/upload/ensure-upload-contract.ts similarity index 99% rename from src/utils/ensure-upload-contract.ts rename to src/features/upload/ensure-upload-contract.ts index 90f9e11..1745a5b 100644 --- a/src/utils/ensure-upload-contract.ts +++ b/src/features/upload/ensure-upload-contract.ts @@ -1,8 +1,9 @@ import { File } from 'expo-file-system'; import { compress, probeVideo } from 'react-native-video-trim'; +import { toFileUri, uploadDest } from '@/utils/file-store'; + import { hasFaststart } from './faststart'; -import { toFileUri, uploadDest } from './file-store'; import { decideUploadContract } from './upload-contract'; /** Reported when a file is compliant in every respect except its `moov` placement. */ diff --git a/src/utils/faststart.test.ts b/src/features/upload/faststart.test.ts similarity index 100% rename from src/utils/faststart.test.ts rename to src/features/upload/faststart.test.ts diff --git a/src/utils/faststart.ts b/src/features/upload/faststart.ts similarity index 100% rename from src/utils/faststart.ts rename to src/features/upload/faststart.ts diff --git a/src/utils/upload-contract.test.ts b/src/features/upload/upload-contract.test.ts similarity index 100% rename from src/utils/upload-contract.test.ts rename to src/features/upload/upload-contract.test.ts diff --git a/src/utils/upload-contract.ts b/src/features/upload/upload-contract.ts similarity index 99% rename from src/utils/upload-contract.ts rename to src/features/upload/upload-contract.ts index 17a8b9c..1885187 100644 --- a/src/utils/upload-contract.ts +++ b/src/features/upload/upload-contract.ts @@ -1,6 +1,6 @@ import type { CompressOptions, VideoProbeResult } from 'react-native-video-trim'; -import { displaySize, effectiveFps, HDR_TRANSFERS, is10Bit } from './import-normalization'; +import { displaySize, effectiveFps, HDR_TRANSFERS, is10Bit } from '@/utils/probe'; /** * The upload contract (§ playback). diff --git a/src/features/upload/upload-manager.ts b/src/features/upload/upload-manager.ts index bdbd465..44f911e 100644 --- a/src/features/upload/upload-manager.ts +++ b/src/features/upload/upload-manager.ts @@ -20,7 +20,7 @@ import { getDraftToken } from '@/db/secure-token'; import { getDraftTranscriptRow } from '@/db/transcripts'; import { linesToVtt } from '@/features/transcription/vtt'; import { parseTranscriptLines } from '@/features/transcription/whisper'; -import { ensureUploadContract, ensureUploadContractCached } from '@/utils/ensure-upload-contract'; +import { ensureUploadContract, ensureUploadContractCached } from './ensure-upload-contract'; import { absolutize, toFileUri } from '@/utils/file-store'; import { effFile } from '@/utils/segment-window'; import { generateThumbnailFile } from '@/utils/video'; diff --git a/src/utils/import-normalization.ts b/src/utils/import-normalization.ts index 909f7bd..8ccb866 100644 --- a/src/utils/import-normalization.ts +++ b/src/utils/import-normalization.ts @@ -1,5 +1,7 @@ import type { CompressOptions, VideoProbeResult } from 'react-native-video-trim'; +import { displaySize, effectiveFps, HDR_TRANSFERS, is10Bit } from './probe'; + /** * Import normalization policy (§ imports). * @@ -44,36 +46,11 @@ export const NORMALIZE_MAX_BITRATE = 8_000_000; * output, uploads) is standardized on H.264 for universal browser playback — HEVC imports * are re-encoded once at import time rather than leaking into merged artifacts. */ const NATIVE_VIDEO_CODECS = new Set(['h264']); -/** HDR transfer functions: HLG (iPhone camera default) and PQ (HDR10 / Dolby Vision 8.x). */ -export const HDR_TRANSFERS = new Set(['arib-std-b67', 'smpte2084']); export type ImportDecision = | { action: 'passthrough' } | { action: 'normalize'; options: Partial; reasons: string[] }; -/** - * True for 10-bit pixel formats. FFmpeg names these with a `10`/`10le`/`10be` bit-depth - * suffix (yuv420p10le, p010le, ...) — matching the suffix rather than a bare `includes('10')` - * keeps 8-bit chroma-subsampling names like `yuv410p` from being misclassified. - */ -export function is10Bit(pixelFormat: string): boolean { - return /10(le|be)?$/.test(pixelFormat); -} - -/** Effective fps for the decision: average when known (catches VFR), else nominal. */ -export function effectiveFps(probe: VideoProbeResult): number { - return probe.averageFps > 0 ? probe.averageFps : probe.nominalFps; -} - -/** Display (post-rotation) dimensions: a 90/270 rotation swaps coded width/height. */ -export function displaySize(probe: VideoProbeResult): { width: number; height: number } { - const swapped = probe.rotation % 180 !== 0; - return { - width: swapped ? probe.height : probe.width, - height: swapped ? probe.width : probe.height, - }; -} - /** * Decide how an imported clip enters the draft: byte-for-byte passthrough, an audio-only * conform (video stream-copied), or a full re-encode bounded to the recorder's signature. diff --git a/src/utils/probe.ts b/src/utils/probe.ts new file mode 100644 index 0000000..e140fb3 --- /dev/null +++ b/src/utils/probe.ts @@ -0,0 +1,37 @@ +import type { VideoProbeResult } from 'react-native-video-trim'; + +/** + * Shared readers for `probeVideo()` results. + * + * Both policy modules that interpret a probe — `import-normalization.ts` (what may enter a + * draft) and `features/upload/upload-contract.ts` (what may leave the device) — need the same + * low-level answers: real display geometry, effective frame rate, bit depth, HDR-ness. They + * live here so the upload contract does not have to reach into the import pipeline for them, + * and so the two policies cannot silently drift apart on how they read the same probe. + */ + +/** HDR transfer functions: HLG (iPhone camera default) and PQ (HDR10 / Dolby Vision 8.x). */ +export const HDR_TRANSFERS = new Set(['arib-std-b67', 'smpte2084']); + +/** + * True for 10-bit pixel formats. FFmpeg names these with a `10`/`10le`/`10be` bit-depth + * suffix (yuv420p10le, p010le, ...) — matching the suffix rather than a bare `includes('10')` + * keeps 8-bit chroma-subsampling names like `yuv410p` from being misclassified. + */ +export function is10Bit(pixelFormat: string): boolean { + return /10(le|be)?$/.test(pixelFormat); +} + +/** Effective fps for the decision: average when known (catches VFR), else nominal. */ +export function effectiveFps(probe: VideoProbeResult): number { + return probe.averageFps > 0 ? probe.averageFps : probe.nominalFps; +} + +/** Display (post-rotation) dimensions: a 90/270 rotation swaps coded width/height. */ +export function displaySize(probe: VideoProbeResult): { width: number; height: number } { + const swapped = probe.rotation % 180 !== 0; + return { + width: swapped ? probe.height : probe.width, + height: swapped ? probe.width : probe.height, + }; +} From 4c1f971d956e7b8bdd2fd004423fe7ed67ad6276 Mon Sep 17 00:00:00 2001 From: morepriyam Date: Sat, 8 Aug 2026 19:10:56 +0530 Subject: [PATCH 6/6] fix(upload): key the conditioned-segment cache by source basename, not segment id MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A destructive edit swaps a clip's effective file to {segmentId}.edited.{rev}.mp4 while the segment id stays put, so the id-keyed cache kept returning the conditioned copy of the PRE-edit bytes — an edited clip re-uploaded stale video. The source basename carries the revision, so keying the cache path by it makes an edit miss the cache and re-condition the new bytes, while resume-of-identical- bytes behavior is unchanged for the unedited case. Superseded copies are bounded (one per destructive edit) and reclaimed with the draft by deleteDraftDir. Found by Copilot review on #142. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/features/upload/ensure-upload-contract.ts | 7 ++++++- src/utils/file-store.ts | 14 +++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/features/upload/ensure-upload-contract.ts b/src/features/upload/ensure-upload-contract.ts index 1745a5b..8114c32 100644 --- a/src/features/upload/ensure-upload-contract.ts +++ b/src/features/upload/ensure-upload-contract.ts @@ -145,7 +145,12 @@ export async function ensureUploadContractCached( draftId: string, segmentId: string, ): Promise { - const dest = uploadDest(draftId, segmentId); + // The cache key is the source's BASENAME, not the segment id. `effFile` swaps to + // `{segmentId}.edited.{rev}.mp4` on a destructive edit while the id stays the same, so an + // id-keyed cache would return the conditioned copy of the pre-edit clip and upload the wrong + // bytes. The basename encodes the revision, so edited bytes miss the cache and re-condition. + const sourceName = sourcePath.split('/').pop() || `${segmentId}.mp4`; + const dest = uploadDest(draftId, sourceName); if (dest.exists && (dest.size ?? 0) > 0) { // Already conditioned on an earlier attempt — reuse verbatim. return { path: dest.uri, changed: true, reasons: [] }; diff --git a/src/utils/file-store.ts b/src/utils/file-store.ts index 2b942ff..e51e775 100644 --- a/src/utils/file-store.ts +++ b/src/utils/file-store.ts @@ -71,9 +71,17 @@ function uploadDir(draftId: string): Directory { return dir; } -/** The on-disk conditioned upload copy for a clip, creating the upload dir if needed. */ -export function uploadDest(draftId: string, segmentId: string): File { - return new File(uploadDir(draftId), `${segmentId}.mp4`); +/** + * The on-disk conditioned upload copy for a clip's SOURCE file, creating the upload dir if + * needed. Keyed by the source's basename, not the segment id: a destructive edit swaps the + * clip's effective file to `{segmentId}.edited.{rev}.mp4` while the id stays put, so an + * id-keyed cache would keep serving the conditioned copy of the pre-edit bytes. The basename + * carries the revision, so an edit naturally misses the cache and conditions the new bytes. + * (The superseded copy lingers until `deleteDraftDir` reclaims the draft — bounded, one file + * per destructive edit, and never re-uploaded because nothing references its name anymore.) + */ +export function uploadDest(draftId: string, sourceName: string): File { + return new File(uploadDir(draftId), sourceName); } /** The on-disk pristine segment file for a draft, creating the segments dir if needed. */