Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions apps/cli/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ impl Export {
compression: cap_export::mp4::ExportCompression::Maximum,
custom_bpp: None,
force_ffmpeg_decoder: false,
optimize_filesize: false,
}
.export(exporter_base, move |_f| {
// print!("\rrendered frame {f}");
Expand Down
46 changes: 46 additions & 0 deletions apps/web/__tests__/unit/playback-source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
canPlayRawContentType,
detectCrossOriginSupport,
resolvePlaybackSource,
shouldFallbackToRawPlaybackSource,
} from "@/app/s/[videoId]/_components/playback-source";

function createResponse(
Expand Down Expand Up @@ -149,6 +150,40 @@ describe("resolvePlaybackSource", () => {
});
});

it("can prefer the raw preview after the MP4 source fails in the player", async () => {
const fetchImpl = vi.fn<typeof fetch>().mockResolvedValueOnce(
createResponse("https://cap.so/raw-upload.webm", {
status: 206,
headers: { "content-type": "video/webm;codecs=vp9,opus" },
redirected: true,
}),
);

const result = await resolvePlaybackSource({
videoSrc: "/api/playlist?videoType=mp4",
rawFallbackSrc: "/api/playlist?videoType=raw-preview",
preferredSource: "raw",
fetchImpl,
now: () => 250,
createVideoElement: () => ({
canPlayType: vi.fn().mockReturnValue("probably"),
}),
});

expect(fetchImpl).toHaveBeenCalledTimes(1);
expect(fetchImpl).toHaveBeenCalledWith(
"/api/playlist?videoType=raw-preview&_t=250",
{
headers: { range: "bytes=0-0" },
},
);
expect(result).toEqual({
url: "https://cap.so/raw-upload.webm",
type: "raw",
supportsCrossOrigin: false,
});
});

it("rejects raw webm previews when the browser cannot play them", async () => {
const fetchImpl = vi
.fn<typeof fetch>()
Expand Down Expand Up @@ -196,3 +231,14 @@ describe("resolvePlaybackSource", () => {
expect(result).toBeNull();
});
});

describe("shouldFallbackToRawPlaybackSource", () => {
it("allows a single mp4-to-raw fallback", () => {
expect(shouldFallbackToRawPlaybackSource("mp4", "/raw", false)).toBe(true);
expect(shouldFallbackToRawPlaybackSource("mp4", "/raw", true)).toBe(false);
expect(shouldFallbackToRawPlaybackSource("raw", "/raw", true)).toBe(false);
expect(shouldFallbackToRawPlaybackSource("mp4", undefined, false)).toBe(
false,
);
});
});
69 changes: 52 additions & 17 deletions apps/web/app/api/playlist/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,47 @@ const ApiLive = HttpApiBuilder.api(Api).pipe(
),
);

const resolveRawPreviewKey = (video: Video.Video) =>
Effect.gen(function* () {
const db = yield* Database;
const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getBucketAccess happens before we know whether we even need S3 (when rawFileKey is already in DB). Small perf/readability win to defer it until after the DB check.

Suggested change
const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId);
const db = yield* Database;
const [uploadRecord] = yield* db.use((db) =>
db
.select({ rawFileKey: Db.videoUploads.rawFileKey })
.from(Db.videoUploads)
.where(eq(Db.videoUploads.videoId, video.id)),
);
if (uploadRecord?.rawFileKey) {
return uploadRecord.rawFileKey;
}
if (video.source.type !== "webMP4") {
return yield* Effect.fail(new HttpApiError.NotFound());
}
const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId);

Also worth double-checking: headObject(...).pipe(Effect.option) will treat any S3 failure (timeouts/credentials/etc) as “missing” and return a 404 here. If the intent is only to ignore “not found”, it may be better to only swallow that specific error and let other S3 errors bubble to the existing 500 mapping.

const [uploadRecord] = yield* db.use((db) =>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor perf: resolveRawPreviewKey always calls getBucketAccess even when rawFileKey is present (common path). Moving it into the webMP4 probe branch avoids the extra bucket/client lookup.

Suggested change
const [uploadRecord] = yield* db.use((db) =>
const resolveRawPreviewKey = (video: Video.Video) =>
Effect.gen(function* () {
const db = yield* Database;
const [uploadRecord] = yield* db.use((db) =>
db
.select({ rawFileKey: Db.videoUploads.rawFileKey })
.from(Db.videoUploads)
.where(eq(Db.videoUploads.videoId, video.id)),
);
if (uploadRecord?.rawFileKey) {
return uploadRecord.rawFileKey;
}
if (video.source.type !== "webMP4") {
return yield* Effect.fail(new HttpApiError.NotFound());
}
const [s3] = yield* S3Buckets.getBucketAccess(video.bucketId);
const candidateKeys = [
`${video.ownerId}/${video.id}/raw-upload.mp4`,
`${video.ownerId}/${video.id}/raw-upload.webm`,
];
const headResults = yield* Effect.all(
candidateKeys.map((key) => s3.headObject(key).pipe(Effect.option)),
{ concurrency: "unbounded" },
);
for (let i = 0; i < candidateKeys.length; i++) {
const rawHead = headResults[i];
if (Option.isSome(rawHead) && (rawHead.value.ContentLength ?? 0) > 0) {
return candidateKeys[i];
}
}
return yield* Effect.fail(new HttpApiError.NotFound());
});

db
.select({ rawFileKey: Db.videoUploads.rawFileKey })
.from(Db.videoUploads)
.where(eq(Db.videoUploads.videoId, video.id)),
);

if (uploadRecord?.rawFileKey) {
return uploadRecord.rawFileKey;
}

if (video.source.type !== "webMP4") {
return yield* Effect.fail(new HttpApiError.NotFound());
}

const candidateKeys = [
`${video.ownerId}/${video.id}/raw-upload.mp4`,
`${video.ownerId}/${video.id}/raw-upload.webm`,
];
const headResults = yield* Effect.all(
candidateKeys.map((key) => s3.headObject(key).pipe(Effect.option)),
{ concurrency: "unbounded" },
);
for (const [index, candidateKey] of candidateKeys.entries()) {
const rawHead = headResults[index];
if (
rawHead &&
Option.isSome(rawHead) &&
(rawHead.value.ContentLength ?? 0) > 0
) {
return candidateKey;
}
}

return yield* Effect.fail(new HttpApiError.NotFound());
});
Comment on lines +86 to +125
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Redundant getBucketAccess call on the happy path

resolveRawPreviewKey unconditionally acquires an S3 client via S3Buckets.getBucketAccess(video.bucketId) at the very start of its generator, but when uploadRecord.rawFileKey is already set (the common case) that client is never used — the function returns the DB key immediately. Meanwhile getPlaylistResponse (the only caller) already holds its own s3 from its own getBucketAccess call at line 128.

The S3 client inside resolveRawPreviewKey is only needed for the headObject probe in the candidate-key fallback branch (the webMP4 path). Moving yield* S3Buckets.getBucketAccess(video.bucketId) to just before the candidateKeys block — after the early returns for the DB hit and the non-webMP4 guard — would skip the bucket lookup entirely for the common path.

Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/app/api/playlist/route.ts
Line: 86-121

Comment:
**Redundant `getBucketAccess` call on the happy path**

`resolveRawPreviewKey` unconditionally acquires an S3 client via `S3Buckets.getBucketAccess(video.bucketId)` at the very start of its generator, but when `uploadRecord.rawFileKey` is already set (the common case) that client is never used — the function returns the DB key immediately. Meanwhile `getPlaylistResponse` (the only caller) already holds its own `s3` from its own `getBucketAccess` call at line 128.

The S3 client inside `resolveRawPreviewKey` is only needed for the `headObject` probe in the candidate-key fallback branch (the `webMP4` path). Moving `yield* S3Buckets.getBucketAccess(video.bucketId)` to just before the `candidateKeys` block — after the early returns for the DB hit and the non-`webMP4` guard — would skip the bucket lookup entirely for the common path.

How can I resolve this? If you propose a fix, please make it concise.


const getPlaylistResponse = (
video: Video.Video,
urlParams: (typeof GetPlaylistParams)["Type"],
Expand All @@ -93,20 +134,9 @@ const getPlaylistResponse = (
video.source.type === "desktopMP4" || video.source.type === "webMP4";

if (urlParams.videoType === "raw-preview") {
const db = yield* Database;
const [uploadRecord] = yield* db.use((db) =>
db
.select({ rawFileKey: Db.videoUploads.rawFileKey })
.from(Db.videoUploads)
.where(eq(Db.videoUploads.videoId, urlParams.videoId)),
);

if (!uploadRecord?.rawFileKey) {
return yield* Effect.fail(new HttpApiError.NotFound());
}

const rawFileKey = yield* resolveRawPreviewKey(video);
return yield* s3
.getSignedObjectUrl(uploadRecord.rawFileKey)
.getSignedObjectUrl(rawFileKey)
.pipe(Effect.map(HttpServerResponse.redirect));
}

Expand Down Expand Up @@ -201,12 +231,17 @@ const getPlaylistResponse = (
s3.listObjects({ prefix: audioPrefix, maxKeys: 1 }),
]);

const videoMetadata = yield* s3.headObject(
videoSegment.Contents?.[0]?.Key ?? "",
);
const videoSegmentKey = videoSegment.Contents?.[0]?.Key;
if (!videoSegmentKey) {
return yield* Effect.fail(new HttpApiError.NotFound());
}

const videoMetadata = yield* s3.headObject(videoSegmentKey);
const audioMetadata =
audioSegment?.KeyCount && audioSegment.KeyCount > 0
? yield* s3.headObject(audioSegment.Contents?.[0]?.Key ?? "")
? audioSegment.Contents?.[0]?.Key
? yield* s3.headObject(audioSegment.Contents[0].Key)
: undefined
: undefined;

const generatedPlaylist = generateMasterPlaylist(
Expand Down
47 changes: 44 additions & 3 deletions apps/web/app/s/[videoId]/_components/CapVideoPlayer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
import {
type ResolvedPlaybackSource,
resolvePlaybackSource,
shouldFallbackToRawPlaybackSource,
} from "./playback-source";
import {
MediaPlayer,
Expand Down Expand Up @@ -139,6 +140,8 @@ export function CapVideoPlayer({
const [hasError, setHasError] = useState(false);
const [isRetryingProcessing, setIsRetryingProcessing] = useState(false);
const [playerDuration, setPlayerDuration] = useState(fallbackDuration ?? 0);
const [preferredSource, setPreferredSource] = useState<"mp4" | "raw">("mp4");
const [hasTriedRawFallback, setHasTriedRawFallback] = useState(false);
const queryClient = useQueryClient();

useEffect(() => {
Expand Down Expand Up @@ -166,14 +169,21 @@ export function CapVideoPlayer({
const shouldDeferResolvedSource = shouldDeferPlaybackSource(uploadProgress);

const resolvedSrc = useQuery<ResolvedPlaybackSource | null>({
queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin],
queryKey: [
"resolvedSrc",
videoSrc,
rawFallbackSrc,
enableCrossOrigin,
preferredSource,
],
queryFn: shouldDeferResolvedSource
? skipToken
: () =>
resolvePlaybackSource({
videoSrc,
rawFallbackSrc,
enableCrossOrigin,
preferredSource,
}),
refetchOnWindowFocus: false,
staleTime: Number.POSITIVE_INFINITY,
Expand All @@ -186,6 +196,8 @@ export function CapVideoPlayer({
setVideoLoaded(false);
setHasError(false);
setShowPlayButton(false);
setPreferredSource("mp4");
setHasTriedRawFallback(false);
}, [videoSrc, rawFallbackSrc]);

// Track video duration for comment markers
Expand Down Expand Up @@ -280,6 +292,21 @@ export function CapVideoPlayer({
};

const handleError = () => {
if (
shouldFallbackToRawPlaybackSource(
resolvedSrc.data?.type,
rawFallbackSrc,
hasTriedRawFallback,
)
) {
setHasTriedRawFallback(true);
setPreferredSource("raw");
setVideoLoaded(false);
setHasError(false);
setShowPlayButton(false);
return;
}

setHasError(true);
};

Expand Down Expand Up @@ -365,7 +392,14 @@ export function CapVideoPlayer({
captionTrack.removeEventListener("cuechange", handleCueChange);
}
};
}, [hasPlayedOnce, resolvedSrc.isPending, videoRef.current]);
}, [
hasPlayedOnce,
hasTriedRawFallback,
rawFallbackSrc,
resolvedSrc.data?.type,
resolvedSrc.isPending,
videoRef.current,
]);

const generateVideoFrameThumbnail = useCallback(
(time: number): string => {
Expand Down Expand Up @@ -443,12 +477,19 @@ export function CapVideoPlayer({
) {
setHasError(false);
void queryClient.invalidateQueries({
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When processing finishes (or upload completes), this can stay stuck preferring raw if we previously fell back. You probably want to reset to MP4 + invalidate both cache entries so we re-probe the processed URL.

Suggested change
void queryClient.invalidateQueries({
setHasError(false);
setPreferredSource("mp4");
setHasTriedRawFallback(false);
void queryClient.invalidateQueries({
queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin],
});

queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin],
queryKey: [
"resolvedSrc",
videoSrc,
rawFallbackSrc,
enableCrossOrigin,
preferredSource,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When upload processing completes, consider resetting preferredSource back to "mp4" (and hasTriedRawFallback to false) before invalidating, so a raw fallback switches back to the optimized MP4 once it becomes available.

],
});
Comment on lines 479 to 487
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once preferredSource flips to "raw", this invalidation only targets that cache entry. If the intent is to retry MP4 after processing completes, consider resetting the preference here and invalidating by the base key prefix so both variants get cleared.

Suggested change
void queryClient.invalidateQueries({
queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin],
queryKey: [
"resolvedSrc",
videoSrc,
rawFallbackSrc,
enableCrossOrigin,
preferredSource,
],
});
setPreferredSource("mp4");
setHasTriedRawFallback(false);
void queryClient.invalidateQueries({
queryKey: ["resolvedSrc", videoSrc, rawFallbackSrc, enableCrossOrigin],
});

}
prevUploadProgress.current = uploadProgress;
}, [
enableCrossOrigin,
preferredSource,
queryClient,
rawFallbackSrc,
uploadProgress,
Expand Down
66 changes: 44 additions & 22 deletions apps/web/app/s/[videoId]/_components/playback-source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type ResolvePlaybackSourceInput = {
fetchImpl?: typeof fetch;
now?: () => number;
createVideoElement?: () => Pick<HTMLVideoElement, "canPlayType">;
preferredSource?: "mp4" | "raw";
};

function appendCacheBust(url: string, timestamp: number): string {
Expand Down Expand Up @@ -92,14 +93,56 @@ export function canPlayRawContentType(
);
}

export function shouldFallbackToRawPlaybackSource(
resolvedSourceType: ResolvedPlaybackSource["type"] | null | undefined,
rawFallbackSrc: string | undefined,
hasTriedRawFallback: boolean,
): boolean {
return Boolean(
rawFallbackSrc && resolvedSourceType === "mp4" && !hasTriedRawFallback,
);
}

export async function resolvePlaybackSource({
videoSrc,
rawFallbackSrc,
enableCrossOrigin = false,
fetchImpl = fetch,
now = () => Date.now(),
createVideoElement,
preferredSource = "mp4",
}: ResolvePlaybackSourceInput): Promise<ResolvedPlaybackSource | null> {
const resolveRaw = async (): Promise<ResolvedPlaybackSource | null> => {
if (!rawFallbackSrc) {
return null;
}

const rawResult = await probePlaybackSource(rawFallbackSrc, fetchImpl, now);

if (!rawResult) {
return null;
}

const contentType = rawResult.response.headers.get("content-type") ?? "";

if (
!canPlayRawContentType(contentType, rawResult.url, createVideoElement)
) {
return null;
}

return {
url: rawResult.url,
type: "raw",
supportsCrossOrigin:
enableCrossOrigin && detectCrossOriginSupport(rawResult.url),
};
};

if (preferredSource === "raw") {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

preferredSource === "raw" currently short-circuits and returns null if the raw probe fails, even if MP4 would resolve. If preferredSource is meant as a preference (not exclusive), consider trying raw first then falling back to the MP4 probe.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If preferredSource is "raw" but the raw preview probe returns null (404/unplayable), we currently bail out without ever trying the MP4 probe. That can turn a transient raw-preview issue into a hard playback failure even when MP4 is available.

Suggested change
if (preferredSource === "raw") {
if (preferredSource === "raw") {
const raw = await resolveRaw();
if (raw) {
return raw;
}
}

return await resolveRaw();
}

const mp4Result = await probePlaybackSource(videoSrc, fetchImpl, now);

if (mp4Result) {
Expand All @@ -111,26 +154,5 @@ export async function resolvePlaybackSource({
};
}

if (!rawFallbackSrc) {
return null;
}

const rawResult = await probePlaybackSource(rawFallbackSrc, fetchImpl, now);

if (!rawResult) {
return null;
}

const contentType = rawResult.response.headers.get("content-type") ?? "";

if (!canPlayRawContentType(contentType, rawResult.url, createVideoElement)) {
return null;
}

return {
url: rawResult.url,
type: "raw",
supportsCrossOrigin:
enableCrossOrigin && detectCrossOriginSupport(rawResult.url),
};
return await resolveRaw();
}
1 change: 1 addition & 0 deletions crates/cap-test/src/suites/performance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ async fn benchmark_export(recording_path: &Path) -> Result<ExportMetrics> {
compression: ExportCompression::Social,
custom_bpp: None,
force_ffmpeg_decoder: false,
optimize_filesize: false,
};

let total_frames = exporter_base.total_frames(settings.fps);
Expand Down
1 change: 1 addition & 0 deletions crates/export/examples/export-benchmark-runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ async fn run_mp4_export(
compression,
custom_bpp: None,
force_ffmpeg_decoder: false,
optimize_filesize: false,
};

let total_frames = exporter_base.total_frames(fps);
Expand Down
1 change: 1 addition & 0 deletions crates/export/examples/export_startup_time.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ async fn main() -> Result<(), String> {
compression: ExportCompression::Maximum,
custom_bpp: None,
force_ffmpeg_decoder: false,
optimize_filesize: false,
};

let temp_out = tempfile::Builder::new()
Expand Down
Loading
Loading