Add Firebase Cloud Function for screen recording conversion - #1432
shresthalucky wants to merge 6 commits into
Conversation
|
A preview of 3f0f2b1 is uploaded and can be seen here: ✨ https://revisit.dev/study/PR1432 ✨ Changes may take a few minutes to propagate. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2e86cb035
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const fileName = path.basename(filePath); | ||
| const tmpInput = path.join(os.tmpdir(), fileName); | ||
| const tmpOutput = path.join(os.tmpdir(), `${fileName}.tmp`); |
There was a problem hiding this comment.
When concurrent events have the same basename—for example, two studies both uploading p1_task1—they use identical input and output paths in the shared instance temp directory. A second download or cleanup can overwrite/delete files while the first invocation is processing them, and because each invocation uploads that shared output to its own event-specific destination, recordings can be corrupted or copied into the wrong study. Create a unique temporary directory or include a unique event/object identifier in both paths.
Useful? React with 👍 / 👎.
| await bucket.upload(tmpOutput, { | ||
| destination: filePath, | ||
| metadata: { contentType: 'video/webm', metadata: { converted: 'true' } }, | ||
| }); |
There was a problem hiding this comment.
When conversion succeeds, this upload creates a replacement object generation while supplying only the new converted custom metadata. It therefore drops the original firebaseStorageDownloadTokens metadata created by the Firebase client upload; subsequent calls to getDownloadURL in FirebaseStorageEngine cannot construct a download URL for the converted object. Preserve the existing token metadata or explicitly issue a new token when overwriting the recording.
Useful? React with 👍 / 👎.
| if (!await isWebmCopyCompatible(tmpInput)) { | ||
| logger.info(`Skipping: codecs not compatible with WebM stream copy: ${filePath}`); | ||
| return; |
There was a problem hiding this comment.
For recordings containing H.264 and/or AAC streams, this predicate returns false and the handler exits without producing WebM output. The client constructs MediaRecorder without selecting a MIME type, so browsers that default to an H.264/AAC MP4 recording—most notably Safari—naturally take this path and remain in the original format, defeating the stated cross-browser conversion. These streams need transcoding rather than being rejected from the conversion pipeline.
Useful? React with 👍 / 👎.
JackWilb
left a comment
JackWilb
left a comment
There was a problem hiding this comment.
Request changes on the exact current head e2e86cb.\n\nI verified the three existing Codex inline findings and agree they are blockers:\n- temporary-file collisions across overlapping invocations;\n- loss of Firebase download-token metadata when replacing the object;\n- H.264/AAC recordings being skipped instead of transcoded.\n\nAdditional verified blockers are included inline for missing ffprobe, stale-generation overwrites, the asynchronous source/final-readiness race, incorrect IAM setup guidance, and the stated converted-metadata contract.\n\nPlease address these cases and add function-specific coverage for probe failure, codec decisions, metadata/token preservation, invocation isolation, and stale-event handling before re-requesting review.
|
|
||
| admin.initializeApp(); | ||
| setGlobalOptions({ maxInstances: 5 }); | ||
| ffmpeg.setFfmpegPath(ffmpegInstaller.path); |
There was a problem hiding this comment.
[P1] Provision ffprobe separately\n\nThis configures the bundled ffmpeg executable, but fluent-ffmpeg.ffprobe() still resolves ffprobe through PATH. In a deployment without a system ffprobe, the callback converts the probe error to false at lines 24-27, and every recording then takes the silent skip path at lines 59-61. Bundle a platform-appropriate ffprobe binary and call setFfprobePath, then cover this in a deployment/emulator test.
| }); | ||
|
|
||
| logger.info(`Uploading ${filePath}`); | ||
| await bucket.upload(tmpOutput, { |
There was a problem hiding this comment.
[P1] Guard the replacement against stale generations\n\nThe handler ignores event.data.generation and overwrites the same object path without an ifGenerationMatch precondition. If an older finalization event is delayed or retried after a newer upload replaces that path, this invocation can upload its older conversion over the newer recording. Download the event generation explicitly and use a generation-match precondition; treat a failed precondition as a stale event.
| await bucket.file(filePath).download({ destination: tmpInput }); | ||
|
|
||
| if (!await isWebmCopyCompatible(tmpInput)) { | ||
| logger.info(`Skipping: codecs not compatible with WebM stream copy: ${filePath}`); |
There was a problem hiding this comment.
[P2] Keep the converted-metadata contract consistent\n\nThe PR test plan says non-convertible files are skipped and still marked converted, but this return leaves the original object and metadata untouched. That also makes a probe failure indistinguishable from an intentional terminal skip. Either persist an explicit terminal status while preserving the download metadata, or update the acceptance contract and add coverage for the chosen behavior.
| ): Promise<string | null> { | ||
| const storage = getStorage(); | ||
|
|
||
| // Fetches webm converted by firebase function |
There was a problem hiding this comment.
[P2] Do not expose the source while conversion is pending\n\nThe browser upload, asynchronous function output, and analysis read all use the same object path. A reader can therefore fetch the original recording after upload but before the function replaces it, so this path does not reliably return the converted asset. Use distinct source/final object identities or make the read path wait for terminal conversion metadata.
There was a problem hiding this comment.
One important consequence is caching: the upload path applies a one-year public cache immediately after the source upload. If analysis fetches that URL before conversion finishes, a browser or CDN can keep serving the original bytes even after the function replaces the object. Please keep the source non-cacheable or make the reader wait for the converted state.
|
|
||
| ```bash | ||
| gcloud projects add-iam-policy-binding YOUR_PROJECT_ID \ | ||
| --member="serviceAccount:service-PROJECT_NUMBER@gcp-sa-eventarc.iam.gserviceaccount.com" \ |
There was a problem hiding this comment.
[P2] Document the correct least-privilege IAM principals\n\nThis grants project-wide roles/storage.admin to the Eventarc service agent, although the function runtime is the identity that performs the Storage download/upload. The direct Cloud Storage Eventarc setup also requires the Cloud Storage service agent publisher role and the trigger/runtime identities appropriate roles. Please document the correct principals with least privilege, and make deployment project selection explicit instead of relying on an ignored .firebaserc and unqualified firebase deploy.
97731ee to
ccc07aa
Compare
| }, | ||
| "main": "lib/index.js", | ||
| "dependencies": { | ||
| "@ffmpeg-installer/ffmpeg": "^1.1.0", |
There was a problem hiding this comment.
[P1] Use a maintained FFmpeg build
This package resolves the Linux deployment to FFmpeg 4.1.0 from 2018. The function parses participant-uploaded media, so malformed media can reach a very old parser. Please replace it with a maintained, security-patched build and verify that the deployed binary still supports VP9 and Opus.
|
|
||
| export const convertScreenRecording = onObjectFinalized( | ||
| { | ||
| bucket: BUCKET, memory: '1GiB', timeoutSeconds: 60, maxInstances: 10, |
There was a problem hiding this comment.
[P1] Limit video conversion concurrency
At 1 GiB, second-generation functions default to 80 concurrent requests per instance, while this handler starts one CPU-heavy FFmpeg job per request. With maxInstances set to 10, that can overload instances and make the 60-second timeout routine for longer recordings. Set an explicit low concurrency and choose timeout, CPU, and memory from representative recordings.
| logger.info(`Skipping stale recording event: ${filePath}`); | ||
| return; | ||
| } | ||
| logger.error(`Conversion failed for ${filePath}`, err); |
There was a problem hiding this comment.
[P1] Retry recoverable conversion failures
This handler rethrows download, FFmpeg, and upload failures, but the trigger does not enable retries. A transient failure therefore leaves the original object without a converted or failed state. Please enable retries only with idempotent generation checks and record a terminal status for permanent input failures.
| @@ -0,0 +1,10 @@ | |||
| VITE_FIREBASE_CONFIG=' | |||
There was a problem hiding this comment.
[P2] Keep the bucket and deployment project aligned
The committed function environment still hardcodes revisit-utah while the README now tells operators to deploy any project. Deploying with another --project leaves this trigger reading and writing against revisit-utah. Please derive the bucket from the deployed Firebase configuration or require one explicit project setting, and remove the duplicate unqualified deploy path.
| "eslint": "^8.9.0", | ||
| "eslint-config-airbnb-base": "^15.0.0", | ||
| "eslint-plugin-import": "^2.25.4", | ||
| "firebase-functions-test": "^3.4.1", |
There was a problem hiding this comment.
[P2] Add function-specific regression coverage
This package includes firebase-functions-test but has no test script or function tests, and the existing root CI and lockfile workflow do not cover this directory. The generation guard and metadata/conversion behavior need focused tests plus a CI job under Node 22 before we can rely on this function.
| const BUCKET: string = firebaseConfig.storageBucket; | ||
|
|
||
| admin.initializeApp(); | ||
| setGlobalOptions({ maxInstances: 5 }); |
There was a problem hiding this comment.
[P3] Remove the unused global instance limit
This global maxInstances value is immediately overridden by the function's maxInstances setting, so it has no effect today. Please remove the unused global setting and the unused tsconfig.dev.json if it is not needed, keeping the new function surface minimal.
JackWilb
left a comment
JackWilb
left a comment
There was a problem hiding this comment.
The final independent review found the remaining issues called out inline on commit 3f0f2b1. The earlier findings about temporary-file isolation, Firebase tokens, Safari transcoding, ffprobe, and stale generations are addressed. I also added the cache consequence to the existing readiness thread. Please address these remaining correctness, deployment, and coverage issues before re-requesting review.
3 participants
Summary
Split out of #1174 (replay videos should load faster).
Test plan