Add Firebase Cloud Function for screen recording conversion - #1432
Add Firebase Cloud Function for screen recording conversion#1432shresthalucky wants to merge 4 commits into
Conversation
|
A preview of ccc07aa 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.
|
|
||
| ```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
3 participants
Summary
convertedmetadata otherwiseFirebaseStorageEngineat the converted assetSplit out of #1174 (replay videos should load faster).
Test plan
convertedin metadata