fix media recorder missing timestapm #226
Conversation
#131 thats what it fixes
This comment has been minimized.
This comment has been minimized.
🎉 MEGA PR Test & Analysis Complete!Hi @supervoidcoder! Your PR has been fully tested and analyzed! 🚀✨ 🌍 Test DeploymentLive Preview: https://OmniBlocks.github.io/scratch-gui/supervoidcoder/226/ 🧪 Test ResultsESLint: ❌ Found issues
Unit Tests: ✅
Integration Tests: ❌
📄 Full test outputs available in artifacts 🔦 Performance Audit (Lighthouse)✅ Audit completed successfully! 📊 Lighthouse Scores:
⚡ Core Web Vitals:
🟢 90-100 (Good) | 🟠 50-89 (Needs Improvement) | 🔴 0-49 (Poor) 📄 Full report in artifacts 📦 Bundle Size AnalysisTotal Build Size: 136M 📦 Top 5 Largest JavaScript Files:
🎨 Top 5 Largest CSS Files:
♿ Accessibility Testing✅ Accessibility scan completed!
📊 Build Details
💡 Pro Tip: Only one build was needed for all these checks - saving CI/CD minutes! 🎯 |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRecordings now default to WebM; when MP4 is requested the code attempts an ffmpeg conversion using added flags (-fflags +genpts, -video_track_timescale 90000, -muxpreload 0, -muxdelay 0) with verbose logging. On conversion failure the original WebM is returned as a fallback. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Recorder as MediaRecorder
participant FS as FileSystem
participant FFmpeg as ffmpeg (convertWebmToMp4)
Note over Recorder,FS: Recording always saved as WebM
User->>Recorder: Stop recording (selectedFormat = mp4?)
Recorder->>FS: save WebM blob
alt selectedFormat == mp4
Recorder->>FFmpeg: convert WebM -> MP4 (adds +genpts, timescale, mux flags)
FFmpeg-->>Recorder: success (MP4)
alt success
Recorder->>User: deliver MP4 (logged)
else error
Recorder->>User: deliver WebM (fallback, logged)
end
else selectedFormat != mp4
Recorder->>User: deliver WebM
end
Note right of FFmpeg: conversion logging added (start/complete/error)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thank you because i DIDN'T ASK that's what coderabbit is for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
bvxdfghjhgfdfghjhgfghjhgfghgghghhh dep update 67
🎉 MEGA PR Test & Analysis Complete!Hi @supervoidcoder! Your PR has been fully tested and analyzed! 🚀✨ 🌍 Test DeploymentLive Preview: https://OmniBlocks.github.io/scratch-gui/supervoidcoder/226/ 🧪 Test ResultsESLint: ❌ Found issues
Unit Tests: ✅
Integration Tests: ❌
📄 Full test outputs available in artifacts 🔦 Performance Audit (Lighthouse)✅ Audit completed successfully! 📊 Lighthouse Scores:
⚡ Core Web Vitals:
🟢 90-100 (Good) | 🟠 50-89 (Needs Improvement) | 🔴 0-49 (Poor) 📄 Full report in artifacts 📦 Bundle Size AnalysisTotal Build Size: 136M 📦 Top 5 Largest JavaScript Files:
🎨 Top 5 Largest CSS Files:
♿ Accessibility Testing✅ Accessibility scan completed!
📊 Build Details
💡 Pro Tip: Only one build was needed for all these checks - saving CI/CD minutes! 🎯 |
|
/test pr |
|
❌ MEGA PR Test Failed @supervoidcoder Something went wrong during the build or analysis process. Check the logs for details: View Logs |
|
@coderabbitai lol what the heck ive never seen a github workflow run out of internet before lol |
|
/test pr |
🎉 MEGA PR Test & Analysis Complete!Hi @supervoidcoder! Your PR has been fully tested and analyzed! 🚀✨ 🌍 Test DeploymentLive Preview: https://OmniBlocks.github.io/scratch-gui/supervoidcoder/226/ 🧪 Test ResultsESLint: ❌ Found issues
Unit Tests: ✅
Integration Tests: ❌
📄 Full test outputs available in artifacts 🔦 Performance Audit (Lighthouse)✅ Audit completed successfully! 📊 Lighthouse Scores:
⚡ Core Web Vitals:
🟢 90-100 (Good) | 🟠 50-89 (Needs Improvement) | 🔴 0-49 (Poor) 📄 Full report in artifacts 📦 Bundle Size AnalysisTotal Build Size: 136M 📦 Top 5 Largest JavaScript Files:
🎨 Top 5 Largest CSS Files:
♿ Accessibility Testing✅ Accessibility scan completed!
📊 Build Details
💡 Pro Tip: Only one build was needed for all these checks - saving CI/CD minutes! 🎯 |
|
Tip For best results, initiate chat on the files or code changes. @supervoidcoder, haha yes! That's a classic GitHub Actions transient network issue. The The good news is you already did the right thing by re-running the workflow with If the issue persists after a few retries, it could be:
But in most cases, the second run should succeed. You can monitor the new workflow run to see if it completes successfully. 🤞 |
|
@coderabbitai can you STOP SPAMMING |
|
Tip For best results, initiate chat on the files or code changes. @supervoidcoder, I sincerely apologize for the spam! That was absolutely not intentional. It looks like there was a bug that caused the Free Plan notification to be posted multiple times in quick succession. This shouldn't happen, and I understand how frustrating that must be. The issue appears to have resolved itself now, but if you continue to see any repetitive messages, please let me know and I can help escalate it. Again, my apologies for the noise. Is there anything else I can help you with regarding this PR? |
|
@coderabbitai bro pls delete all these spam comments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 5152 🏁 Script executed: Length of output: 4376 🏁 Script executed: Length of output: 2336 🏁 Script executed: Length of output: 1184 @supervoidcoder, I've successfully deleted all 9 duplicate spam comments! I kept only the legitimate responses (the PR summary, the network issue response, and my apology message). I'm truly sorry about that spam bug - it was definitely frustrating and shouldn't have happened. The issue has been cleaned up now, and your PR comment thread should be much cleaner. Thank you for your patience! 🙏 |
…th proper flags when needed
Change 1: Force WebM recording (lines 495-507)
// Determine recording format
const selectedFormat = opts.format || defaultFileExtension;
let recordMimeType;
// ALWAYS record as WebM (it works!), convert to MP4 later if needed
recordMimeType = supportedMimeTypes.find(m => m.startsWith("video/webm")) || defaultMimeType;
console.log(`Recording as ${recordMimeType}, will convert to ${selectedFormat} if needed`);
Change 2: Always convert when MP4 requested (lines 407-422)
🎉 MEGA PR Test & Analysis Complete!Hi @supervoidcoder! Your PR has been fully tested and analyzed! 🚀✨ 🌍 Test DeploymentLive Preview: https://OmniBlocks.github.io/scratch-gui/supervoidcoder/226/ 🧪 Test ResultsESLint: ❌ Found issues
Unit Tests: ✅
Integration Tests: ❌
📄 Full test outputs available in artifacts 🔦 Performance Audit (Lighthouse)✅ Audit completed successfully! 📊 Lighthouse Scores:
⚡ Core Web Vitals:
🟢 90-100 (Good) | 🟠 50-89 (Needs Improvement) | 🔴 0-49 (Poor) 📄 Full report in artifacts 📦 Bundle Size AnalysisTotal Build Size: 136M 📦 Top 5 Largest JavaScript Files:
🎨 Top 5 Largest CSS Files:
♿ Accessibility Testing✅ Accessibility scan completed!
📊 Build Details
💡 Pro Tip: Only one build was needed for all these checks - saving CI/CD minutes! 🎯 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/addons/addons/mediarecorder/userscript.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/addons/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Manage addons under src/addons/ and sync/patch sources from upstream via pull.js
Files:
src/addons/addons/mediarecorder/userscript.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: mega-test
| // Convert WebM to MP4 if user selected MP4 | ||
| if (selectedFormat === "mp4") { | ||
| try { | ||
| recordTextSpan.textContent = msg("converting"); | ||
| console.log('Converting WebM to MP4...'); | ||
| blob = await convertWebmToMp4(blob); | ||
| finalExtension = "mp4"; | ||
| console.log('Conversion complete!'); | ||
| } catch (e) { | ||
| console.error("WebM to MP4 conversion failed", e); | ||
| alert(msg("conversion-failed")); | ||
| // Fall back to original format | ||
| finalExtension = recorder.mimeType.split(";")[0].split("/")[1]; | ||
| // Fall back to WebM | ||
| finalExtension = "webm"; | ||
| } |
There was a problem hiding this comment.
Do not re-encode when MediaRecorder already produced MP4
Safari (and some Chromium builds) only expose video/mp4 from MediaRecorder. The new unconditional conversion still calls convertWebmToMp4, which feeds that MP4 into FFmpeg.wasm. This build has no H.264 decoder, so the conversion fails and we fall back with finalExtension = "webm" while the buffer still contains MP4 bytes—users end up downloading an MP4 mislabeled as WebM. Please gate the conversion on the recorder actually emitting WebM (and leave the extension alone when the source is already MP4).
- if (selectedFormat === "mp4") {
+ if (selectedFormat === "mp4" && recorder.mimeType.startsWith("video/webm")) {
try {
recordTextSpan.textContent = msg("converting");
console.log('Converting WebM to MP4...');
blob = await convertWebmToMp4(blob);
finalExtension = "mp4";
console.log('Conversion complete!');
} catch (e) {
console.error("WebM to MP4 conversion failed", e);
alert(msg("conversion-failed"));
// Fall back to WebM
finalExtension = "webm";
}
+ } else if (selectedFormat === "mp4") {
+ finalExtension = "mp4";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Convert WebM to MP4 if user selected MP4 | |
| if (selectedFormat === "mp4") { | |
| try { | |
| recordTextSpan.textContent = msg("converting"); | |
| console.log('Converting WebM to MP4...'); | |
| blob = await convertWebmToMp4(blob); | |
| finalExtension = "mp4"; | |
| console.log('Conversion complete!'); | |
| } catch (e) { | |
| console.error("WebM to MP4 conversion failed", e); | |
| alert(msg("conversion-failed")); | |
| // Fall back to original format | |
| finalExtension = recorder.mimeType.split(";")[0].split("/")[1]; | |
| // Fall back to WebM | |
| finalExtension = "webm"; | |
| } | |
| // Convert WebM to MP4 if user selected MP4 | |
| if (selectedFormat === "mp4" && recorder.mimeType.startsWith("video/webm")) { | |
| try { | |
| recordTextSpan.textContent = msg("converting"); | |
| console.log('Converting WebM to MP4...'); | |
| blob = await convertWebmToMp4(blob); | |
| finalExtension = "mp4"; | |
| console.log('Conversion complete!'); | |
| } catch (e) { | |
| console.error("WebM to MP4 conversion failed", e); | |
| alert(msg("conversion-failed")); | |
| // Fall back to WebM | |
| finalExtension = "webm"; | |
| } | |
| } else if (selectedFormat === "mp4") { | |
| finalExtension = "mp4"; | |
| } |
🤖 Prompt for AI Agents
In src/addons/addons/mediarecorder/userscript.js around lines 409 to 422, the
code unconditionally attempts to convert the recorded blob to MP4 which breaks
when the recorder already emitted MP4 (Safari/Chromium) and causes mislabeled
downloads; change the logic to first inspect the recorded blob MIME type (e.g.,
blob.type or recorder.mimeType) and only call convertWebmToMp4 when the source
is actually WebM (video/webm); if the source is already video/mp4, skip
conversion and keep blob and finalExtension as "mp4"; additionally, if
conversion fails, do not overwrite the blob/extension with "webm" when the
original was MP4—preserve the original blob and extension on any conversion
error.
There was a problem hiding this comment.
no we must always record with webm so that the timestamps actually work, the whole point of the pr is to fix them
There was a problem hiding this comment.
@supervoidcoder, I understand now—you're intentionally forcing WebM recording to fix the timestamp issue, which is the core purpose of this PR.
However, there's still an edge case to consider: if the browser doesn't support WebM recording (e.g., some Safari versions), the code falls back to defaultMimeType (likely MP4). In that scenario:
recorder.mimeTypewould be"video/mp4"- User selects MP4 format
- Code attempts to convert using
convertWebmToMp4(), which expects WebM input - Conversion fails (FFmpeg can't decode MP4 without H.264 decoder)
- Falls back to
finalExtension = "webm", but blob contains MP4 data
Suggested fix: Add a safety check before conversion:
// Convert WebM to MP4 if user selected MP4
if (selectedFormat === "mp4") {
// Only convert if source is actually WebM
if (recorder.mimeType.startsWith("video/webm")) {
try {
recordTextSpan.textContent = msg("converting");
console.log('Converting WebM to MP4...');
blob = await convertWebmToMp4(blob);
finalExtension = "mp4";
console.log('Conversion complete!');
} catch (e) {
console.error("WebM to MP4 conversion failed", e);
alert(msg("conversion-failed"));
finalExtension = "webm";
}
} else {
// Source is already MP4, no conversion needed
finalExtension = "mp4";
}
}Alternatively, you could prevent this edge case entirely by only offering MP4 as a format option when WebM recording is available (around lines 28-34):
const availableFormats = [];
if (supportedMimeTypes.some(m => m.startsWith("video/webm"))) {
availableFormats.push("webm");
// Only offer MP4 if we can record as WebM
availableFormats.push("mp4");
}
ampelectrecuted
removed
Medium-Priority
High-Priority
🕐 Stale PR ReminderHey @supervoidcoder! 🌟 Just swinging by to sprinkle a little reminder magic on your PR #226, which aims to fix that pesky media recorder timestamp issue. It’s been a week since the last update, and we’re all excited to see your awesome work in action! If there’s anything you need or if something's blocking your progress, just give us a shout! We’re here to help! 🎉 Keep up the great work! This is an automated friendly reminder. No pressure! 💙 |
🕐 Stale PR ReminderHey there, @supervoidcoder! 🌟 Just wanted to give you a gentle nudge about your PR #226, which is tackling the media recorder's timestamp issue. It’s a super important fix! 🎥✨ If there’s anything holding you back or if you need a hand with anything, feel free to shout! We're here to help! Keep up the great work! 🚀 This is an automated friendly reminder. No pressure! 💙 |
🕐 Stale PR ReminderHey @supervoidcoder! 👋 This PR has been quiet for a while. Need any help getting it across the finish line? Feel free to ask! 🚀 This is an automated friendly reminder. No pressure! 💙 |
2 similar comments
🕐 Stale PR ReminderHey @supervoidcoder! 👋 This PR has been quiet for a while. Need any help getting it across the finish line? Feel free to ask! 🚀 This is an automated friendly reminder. No pressure! 💙 |
🕐 Stale PR ReminderHey @supervoidcoder! 👋 This PR has been quiet for a while. Need any help getting it across the finish line? Feel free to ask! 🚀 This is an automated friendly reminder. No pressure! 💙 |
2 participants
#131 thats what it fixes
Resolves
What Github issue does this resolve (if any, if not then please include link)?
Proposed Changes
Describe what this Pull Request does
Reason for Changes
Explain why these changes should be made. Why is this helpful or necessary? Why should this be added?
Test Coverage
Please show how you have added tests to cover your changes
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet