Enforce the upload contract before a video leaves the device

src/app/recorder.tsx

@@ -277,9 +277,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. `<Camera>` 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<Constraint[]>(
-    () => [{ videoStabilizationMode: stabilization }, { fps: 30 }],
-    [stabilization],
+    () => [{ resolutionBias: videoOutput }, { videoStabilizationMode: stabilization }, { fps: 30 }],
+    [stabilization, videoOutput],
   );
   const outputs = useMemo(() => [videoOutput], [videoOutput]);
 

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 '@/features/upload/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.
@@ -433,7 +462,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;
@@ -549,7 +584,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);
+    },
     // Wire to <Camera onConfigured>: fires whenever the session's connections are (re)formed —
     // cold open, enableAudio output rebuild, camera flip. Bumping the epoch re-arms the H.264
     // pin for the NEW video connection (the codec is applied per-connection natively, so it

src/features/upload/ensure-upload-contract.test.ts

@@ -0,0 +1,201 @@
+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
+// 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(),
+}));
+// 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<typeof hasFaststart>;
+const mockProbeVideo = probeVideo as jest.MockedFunction<typeof probeVideo>;
+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> = {}): 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();
+    // 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 () => {
+    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();
+  });
+});
+
+/**
+ * `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<string, unknown>;
+    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']);
+  });
+});