[draft] feat: Implement Video Export#325
Conversation
- Added core components for video export including VideoExportService, ExportLogger, FontManager, FilterValidator, MediaProcessor, TextProcessor, and FFmpegCommandBuilder. - Developed a processing pipeline for handling media elements and text overlays. - Implemented command building for FFmpeg with support for various filters and encoding settings. - Introduced error handling and logging mechanisms throughout the service. - Created a README documentation outlining the architecture, usage, and extension points for the video export service.
👷 Deploy request for appcut pending review.Visit the deploys page to approve it
|
|
@alamshafil is attempting to deploy a commit to the OpenCut OSS Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis update introduces a comprehensive video export system to the application. It adds modular export pipeline classes, a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EditorHeader
participant ExportDialog
participant VideoExportService
participant FFmpeg (WASM)
participant MediaStore
User->>EditorHeader: Clicks Export button
EditorHeader->>ExportDialog: Opens dialog
ExportDialog->>MediaStore: Fetches tracks, media items, settings
User->>ExportDialog: Configures export settings, starts export
ExportDialog->>VideoExportService: initialize(onProgress)
VideoExportService->>FFmpeg: Loads and prepares FFmpeg
VideoExportService->>MediaStore: Loads media files
VideoExportService->>FFmpeg: Writes media files to FS
VideoExportService->>FFmpeg: Builds and runs FFmpeg command
FFmpeg-->>VideoExportService: Progress updates
VideoExportService-->>ExportDialog: Progress callback
FFmpeg-->>VideoExportService: Output video file
VideoExportService-->>ExportDialog: Returns video blob
ExportDialog-->>User: Offers video download
Possibly related issues
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
You should change line 131 of apps/web/src/lib/export/ffmpeg-builder.ts to this:const finalVideoLayer = videoLayerCount > 0 ? `[overlay_${videoLayerCount - 1}]` : '0:v'; It makes it so that if you try to export a project that contains no video clips, images, or text, there wouldn't be an FS errror. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (10)
apps/web/src/lib/export/logger.ts (2)
4-22: Consider enabling console output for development and adding timestamps.The console output is commented out in all logging methods, which might hinder debugging during development. Consider:
- Making console output conditional based on environment
- Adding timestamps to log messages for better tracking
- log(message: string): void { - this.logs.push(message); - // console.log(`[VideoExport] ${message}`); - } + log(message: string): void { + const timestamp = new Date().toISOString(); + const logMessage = `[${timestamp}] ${message}`; + this.logs.push(logMessage); + if (process.env.NODE_ENV === 'development') { + console.log(`[VideoExport] ${logMessage}`); + } + }
24-26: Memory management consideration for long-running exports.The logs array could grow large during long export operations. Consider implementing a maximum log limit or automatic cleanup to prevent memory issues.
+ private readonly maxLogs = 1000; // Maximum number of logs to keep + getLogs(): string[] { + // Keep only the most recent logs + if (this.logs.length > this.maxLogs) { + this.logs = this.logs.slice(-this.maxLogs); + } return [...this.logs]; }apps/web/src/lib/export/font-manager.ts (2)
31-31: Address the TODO: Implement robust font filename generation.The current font filename generation using
replace(/\s+/g, '_').toLowerCase()is indeed fragile and could cause issues with special characters or create filename conflicts.- const fontFileName = `${fontFamily.replace(/\s+/g, '_').toLowerCase()}.ttf`; // TODO: Make this more robust + const fontFileName = `${fontFamily.replace(/[^a-zA-Z0-9]/g, '_').toLowerCase()}.ttf`;Would you like me to implement a more robust filename generation function?
37-44: Consider adding font file validation before loading.The current implementation attempts to load fonts without checking if they exist first. Consider adding validation to provide better error messages.
try { this.logger.log(`Loading font: ${fontFamily} from ${fontPath}`); + // Validate font file exists before attempting to load + const response = await fetch(fontPath, { method: 'HEAD' }); + if (!response.ok) { + throw new Error(`Font file not found: ${fontPath}`); + } const fontData = await fetchFile(fontPath); await this.ffmpeg.writeFile(fontFileName, fontData); this.loadedFonts.add(fontFileName); this.fontLoaded = true; this.logger.log(`Successfully loaded font: ${fontFamily}`); return fontFileName; } catch (error) {apps/web/src/lib/export/README.md (1)
3-4: Consider expanding the LLM authorship note.While transparency about LLM authorship is good, consider adding more context such as when it was generated and whether it has been reviewed/validated by the team.
-NOTE: Document written by LLM +NOTE: This document was initially generated by an LLM and has been reviewed for accuracy by the development team.apps/web/src/lib/export/text-processor.ts (1)
112-114: Address the TODO for robust text escaping.The current escaping implementation is comprehensive, but FFmpeg's drawtext filter has complex escaping requirements. Consider using a well-tested library or implementing more thorough testing for edge cases.
Would you like me to help implement a more robust escaping solution or create test cases for various edge cases?
apps/web/src/lib/export/filter-validator.ts (1)
11-26: Consider adding more filter validation checks.The current validation is good but could be enhanced with additional checks for common FFmpeg filter issues.
Consider adding validation for:
if (filter.includes('undefined') || filter.includes('null')) { this.logger.debug(`Invalid ${filterType} filter contains undefined/null: ${filter}`); return false; } + + // Check for unbalanced brackets + const openBrackets = (filter.match(/\[/g) || []).length; + const closeBrackets = (filter.match(/\]/g) || []).length; + if (openBrackets !== closeBrackets) { + this.logger.debug(`Invalid ${filterType} filter has unbalanced brackets: ${filter}`); + return false; + } + + // Check for empty label references + if (filter.includes('[]')) { + this.logger.debug(`Invalid ${filterType} filter has empty label: ${filter}`); + return false; + } return true;apps/web/src/components/editor/export-dialog.tsx (1)
341-342: Add null check for activeProject in debug data generationThe debug button's onClick handler should check if
activeProjectexists before proceeding, similar to the export handler.onClick={() => { - if (!activeProject) return; + if (!activeProject) { + toast.error("No active project"); + return; + }apps/web/src/lib/export/video-export-service.ts (1)
139-148: Consider verifying FFmpeg resource cleanupWhile setting references to null is good practice, FFmpeg WebAssembly may still hold memory. Consider adding a mechanism to verify that resources are properly released, especially for large video exports.
Would you like me to research best practices for FFmpeg.js memory cleanup in WebAssembly contexts to ensure complete resource deallocation?
apps/web/src/lib/export/media-processor.ts (1)
159-163: Ensure overlay timing handles edge cases correctlyThe overlay enable condition uses
between(t,start,end). Verify that this handles edge cases properly, especially when start time is 0 or when the video duration extends beyond the project duration.Consider adding a safety check:
-const overlayFilter = `${baseLayer}[${scaledLabel}]overlay=0:0:enable='between(t,${formattedStartTime},${formattedEndTime})'[${overlayLabel}]`; +const safeEndTime = Math.min(parseFloat(formattedEndTime), duration); +const overlayFilter = `${baseLayer}[${scaledLabel}]overlay=0:0:enable='between(t,${formattedStartTime},${safeEndTime})'[${overlayLabel}]`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
apps/web/public/fonts/comicneue.ttfis excluded by!**/*.ttfapps/web/public/fonts/inter.ttfis excluded by!**/*.ttfapps/web/public/fonts/opensans.ttfis excluded by!**/*.ttfapps/web/public/fonts/playfair.ttfis excluded by!**/*.ttfapps/web/public/fonts/roboto.ttfis excluded by!**/*.ttf
📒 Files selected for processing (18)
apps/web/src/components/editor-header.tsx(2 hunks)apps/web/src/components/editor/export-dialog.tsx(1 hunks)apps/web/src/components/editor/media-panel/views/text.tsx(2 hunks)apps/web/src/components/editor/timeline-track.tsx(2 hunks)apps/web/src/components/ui/font-picker.tsx(2 hunks)apps/web/src/constants/font-constants.ts(1 hunks)apps/web/src/lib/export/README.md(1 hunks)apps/web/src/lib/export/ffmpeg-builder.ts(1 hunks)apps/web/src/lib/export/filter-validator.ts(1 hunks)apps/web/src/lib/export/font-manager.ts(1 hunks)apps/web/src/lib/export/logger.ts(1 hunks)apps/web/src/lib/export/media-processor.ts(1 hunks)apps/web/src/lib/export/text-processor.ts(1 hunks)apps/web/src/lib/export/types.ts(1 hunks)apps/web/src/lib/export/video-export-service.ts(1 hunks)apps/web/src/lib/ffmpeg-utils.ts(2 hunks)apps/web/src/lib/media-processing.ts(4 hunks)apps/web/src/stores/media-store.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
apps/web/src/components/editor/timeline-track.tsx (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
apps/web/src/components/editor/media-panel/views/text.tsx (1)
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
apps/web/src/stores/media-store.ts (1)
Learnt from: khanguyen74
PR: OpenCut-app/OpenCut#340
File: apps/web/src/stores/media-store.ts:174-174
Timestamp: 2025-07-18T05:54:36.773Z
Learning: In the OpenCut media store, the user prefers using initialMediaCount only during loading state for skeleton rendering rather than maintaining a persistent mediaCount field, as it's simpler and serves the specific purpose without additional complexity.
🧬 Code Graph Analysis (9)
apps/web/src/components/ui/font-picker.tsx (1)
apps/web/src/constants/font-constants.ts (1)
getGoogleFonts(75-76)
apps/web/src/components/editor/timeline-track.tsx (1)
apps/web/src/constants/font-constants.ts (1)
DEFAULT_FONT(66-66)
apps/web/src/components/editor/media-panel/views/text.tsx (1)
apps/web/src/constants/font-constants.ts (1)
DEFAULT_FONT(66-66)
apps/web/src/components/editor-header.tsx (1)
apps/web/src/components/editor/export-dialog.tsx (1)
ExportDialog(24-388)
apps/web/src/lib/export/font-manager.ts (1)
apps/web/src/lib/export/logger.ts (2)
ExportLogger(1-31)error(14-17)
apps/web/src/lib/media-processing.ts (1)
apps/web/src/stores/media-store.ts (1)
getMediaDuration(133-152)
apps/web/src/lib/export/filter-validator.ts (2)
apps/web/src/lib/export/logger.ts (1)
ExportLogger(1-31)apps/web/src/lib/export/types.ts (1)
ValidationResult(34-38)
apps/web/src/lib/export/ffmpeg-builder.ts (4)
apps/web/src/lib/export/logger.ts (1)
ExportLogger(1-31)apps/web/src/lib/export/filter-validator.ts (1)
FilterValidator(4-99)apps/web/src/lib/export/types.ts (1)
ExportProject(13-18)apps/web/src/lib/export/text-processor.ts (1)
TextProcessor(7-180)
apps/web/src/lib/export/types.ts (3)
apps/web/src/types/editor.ts (1)
CanvasSize(3-6)apps/web/src/types/timeline.ts (1)
TimelineTrack(81-88)apps/web/src/stores/media-store.ts (1)
MediaItem(7-26)
🔇 Additional comments (23)
apps/web/src/constants/font-constants.ts (1)
66-66: LGTM! Good centralization of font management.The change from "Arial" to "Inter" as the default font, along with the descriptive comment, improves consistency across the application. Inter is properly defined in
FONT_OPTIONSas a Google font with appropriate weights.apps/web/src/components/editor/timeline-track.tsx (2)
26-26: Good centralization of font management.Importing
DEFAULT_FONTfrom the constants module improves maintainability and consistency across the application.
839-839: Consistent use of centralized font constant.Using
DEFAULT_FONTinstead of the hardcoded "Arial" string ensures consistency with the updated default font across the application.apps/web/src/components/editor/media-panel/views/text.tsx (2)
2-2: Good centralization of font management.Importing
DEFAULT_FONTfrom the constants module improves maintainability and consistency.
13-13: Consistent use of centralized font constant.Using
DEFAULT_FONTinstead of the hardcoded "Arial" string ensures consistency with the updated default font across the application.apps/web/src/components/ui/font-picker.tsx (1)
8-8: Import looks good for the new Google Fonts filtering.The addition of
getGoogleFontsimport supports the dynamic font filtering functionality.apps/web/src/lib/ffmpeg-utils.ts (3)
103-103: Good enhancement to video information extraction.Adding the
hasAudioproperty to the return type provides useful information about the media file's audio capabilities.
155-157: Correct audio stream detection implementation.The regex pattern
/Stream #\d+:\d+: Audio:/correctly matches FFmpeg output format for audio streams. The implementation is consistent with the existing video stream parsing logic.
163-164: Properly includes the new hasAudio property.The return object correctly includes the
hasAudioboolean value alongside the existing video properties.apps/web/src/components/editor-header.tsx (1)
44-53: Clean integration with the new export dialog system.The export button is properly wrapped in the
ExportDialogcomponent, maintaining the same UI structure while enabling the new dialog-based export workflow. The implementation follows React patterns correctly.apps/web/src/lib/media-processing.ts (1)
37-87: Logical implementation of audio detection for media items.The
hasAudioproperty is correctly implemented:
- For videos: Uses FFmpeg analysis when available, falls back to
falseon error- For audio files: Always set to
true(logical)- For images: Remains
undefined(appropriate)This enhancement will support the export pipeline in making informed decisions about audio handling.
apps/web/src/stores/media-store.ts (2)
18-18: Appropriate addition of hasAudio property to MediaItem interface.The optional
hasAudioproperty aligns with the media processing updates and provides essential metadata for the export pipeline.
272-275: Clean implementation of getMediaItem helper method.The helper method provides a straightforward way to retrieve media items by ID, which will be useful for the export pipeline.
apps/web/src/lib/export/font-manager.ts (1)
45-52: Potential infinite recursion risk in fallback logic.The fallback logic recursively calls
loadFont('Inter')if the initial font fails. If the Inter font also fails to load, this could cause issues since the error is thrown without additional fallback.} catch (error) { this.logger.warn(`Failed to load font ${fontFamily}: ${error}`); // Fallback to Inter if specific font fails if (fontFamily !== 'Inter') { - return await this.loadFont('Inter'); + try { + return await this.loadFont('Inter'); + } catch (fallbackError) { + this.logger.error(`Failed to load fallback font Inter: ${fallbackError}`); + throw new Error(`Failed to load both ${fontFamily} and fallback font Inter`); + } } throw error; }Likely an incorrect or invalid review comment.
apps/web/src/lib/export/ffmpeg-builder.ts (5)
15-47: Well-structured command building orchestration.The
buildCommandmethod follows a clear, logical sequence for assembling FFmpeg arguments. Good separation of concerns with dedicated methods for each aspect of the command.
49-65: Proper background video generation.Good implementation with appropriate defaults and color format handling. The duration formatting and logging are well implemented.
67-121: Robust filter complex handling with proper validation.Excellent defensive programming with multiple validation layers. The audio mixing logic correctly handles multiple audio streams.
129-138: Output mapping logic appears correct.The current implementation at line 131 already includes the fix suggested in the PR comments. It correctly falls back to
'0:v'(the background video) when no overlay layers exist, which should prevent the filesystem error mentioned.The logic handles both cases appropriately:
- When
videoLayerCount > 0: Uses the last overlay layer- When
videoLayerCount = 0: Uses the background video directly
163-174: Appropriate quality settings for different export levels.The CRF and preset values are well-chosen:
- Low: CRF 28 with fast preset (smaller file, faster encoding)
- Medium: CRF 23 with medium preset (balanced)
- High: CRF 18 with slower preset (higher quality, larger file)
apps/web/src/lib/export/text-processor.ts (1)
20-49: Well-structured text element processing pipeline.Good approach to sort text elements by start time to ensure consistent layering. The incremental video layer counting correctly maintains the overlay chain.
apps/web/src/lib/export/filter-validator.ts (1)
55-83: Comprehensive timing validation.Excellent coverage of timing edge cases including minimum duration, negative values, and non-finite numbers. The warning for start time exceeding video duration is particularly helpful.
apps/web/src/lib/export/types.ts (1)
1-38: Well-defined TypeScript interfaces for export functionality.The interfaces are clean, complete, and properly typed. Good use of union types for constrained values (quality, format, phase) and optional properties where appropriate.
apps/web/src/lib/export/media-processor.ts (1)
26-79: Empty-media export is safe: background video is always mapped
The builder’saddBackgroundVideoensures there’s always a video stream, and inaddOutputMappingvideoLayerCount = 0correctly falls back to-map 0:v. No filesystem errors will occur when a project has no media elements.
| </SelectTrigger> | ||
| <SelectContent> | ||
| {FONT_OPTIONS.map((font) => ( | ||
| {getGoogleFonts().map((font) => ( |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the impact of limiting font options to Google Fonts only.
The change from FONT_OPTIONS to getGoogleFonts() means the font picker will now only show Google Fonts, excluding system fonts like Arial, Helvetica, etc. This could be a breaking change for users who prefer system fonts.
Please verify that this change is intentional and consider the UX impact:
🏁 Script executed:
#!/bin/bash
# Check if system fonts are still available elsewhere in the codebase
echo "Checking for system font usage patterns..."
# Look for any hardcoded system font references
rg -i "arial|helvetica|times|georgia" --type ts --type tsx -A 2 -B 2
# Check if there are other font selection components that might use system fonts
fd -e ts -e tsx | xargs grep -l "font.*select\|font.*pick" | head -5Length of output: 445
🏁 Script executed:
#!/bin/bash
set -e
echo "Locating getGoogleFonts definition and usage..."
rg -n "getGoogleFonts"
echo
echo "Locating FONT_OPTIONS definition..."
rg -n "FONT_OPTIONS"
echo
echo "Showing snippet of font-picker.tsx (lines 1–50):"
sed -n '1,50p' apps/web/src/components/ui/font-picker.tsxLength of output: 2414
Action Required: Confirm or Restore System Font Options in FontPicker
The change from using the full FONT_OPTIONS array to getGoogleFonts() now excludes all system fonts (e.g. Arial, Helvetica, Times, Georgia) from the dropdown. Unless this was an intentional UX decision, you’ll want to either revert to the combined list or parameterize the picker to include system fonts.
• apps/web/src/components/ui/font-picker.tsx (line 27)
– Instead of
js {getGoogleFonts().map(font => (…))}
you could use
js {FONT_OPTIONS.map(font => (…))}
or add a prop (e.g. includeSystemFonts?: boolean) and choose between getGoogleFonts() and getSystemFonts().
• apps/web/src/components/ui/font-picker.tsx (line 8)
– Remove the now-unused FONT_OPTIONS import if you opt to keep only Google Fonts.
Please verify this change is intentional and adjust the component API or code accordingly.
🤖 Prompt for AI Agents
In apps/web/src/components/ui/font-picker.tsx at line 27, the code currently
maps over getGoogleFonts(), which excludes system fonts from the dropdown. To
fix this, either revert to using the combined FONT_OPTIONS array to include both
system and Google fonts, or add a prop like includeSystemFonts to conditionally
select between getGoogleFonts() and system fonts. Also, if you decide to keep
only Google fonts, remove the unused FONT_OPTIONS import at line 8. Confirm the
intended UX and update the code accordingly.
There was a problem hiding this comment.
This change is intentional, since the font files have to be imported via a file and not CSS. This could change later.
There was a problem hiding this comment.
@alamshafil, thank you for the clarification! That makes perfect sense given the video export functionality. The FontManager needs actual font files to load into FFmpeg's virtual filesystem, so Google Fonts (which can be downloaded as files) are the appropriate choice over system fonts that aren't available as files in the browser environment.
✏️ Learnings added
Learnt from: alamshafil
PR: OpenCut-app/OpenCut#325
File: apps/web/src/components/ui/font-picker.tsx:27-27
Timestamp: 2025-07-18T15:22:59.061Z
Learning: In the OpenCut video export system, the FontPicker component uses getGoogleFonts() instead of FONT_OPTIONS to limit font selection to Google Fonts only. This is because the video export functionality requires actual font files to be loaded into FFmpeg's virtual filesystem through the FontManager, rather than CSS-based fonts. System fonts like Arial and Helvetica are not available as downloadable files in the browser environment, making Google Fonts the appropriate choice for video export compatibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: simonorzel26
PR: OpenCut-app/OpenCut#324
File: apps/web/src/components/editor/snap-indicator.tsx:43-43
Timestamp: 2025-07-17T08:26:10.929Z
Learning: In the timeline refactor PR #324, the snap indicator component in apps/web/src/components/editor/snap-indicator.tsx requires the hard-coded `ml-48` class in addition to the calculated `leftPosition` for proper alignment. This is intentional and needed for the new timeline layout.
| private fontMapping = { | ||
| 'Inter': '/fonts/inter.ttf', | ||
| 'Roboto': '/fonts/roboto.ttf', | ||
| 'Open Sans': '/fonts/opensans.ttf', | ||
| 'Playfair Display': '/fonts/playfair.ttf', | ||
| 'Comic Neue': '/fonts/comicneue.ttf', | ||
| 'Arial': '/fonts/arial.ttf', | ||
| 'Helvetica': '/fonts/helvetica.ttf', | ||
| 'Times New Roman': '/fonts/times.ttf', | ||
| 'Georgia': '/fonts/georgia.ttf', | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify font file availability and consider dynamic font loading.
The hardcoded font mapping assumes specific font files exist at /fonts/ paths. This could fail in production environments where these files aren't available.
🏁 Script executed:
#!/bin/bash
# Check if font files exist in the public directory
echo "Checking for font files in public directory..."
fd -t f -e ttf . public/fonts/ 2>/dev/null || echo "No fonts directory found"
# Check for any font-related configuration
echo "Checking for font configuration..."
rg -A 5 -B 5 "fonts.*ttf|ttf.*fonts" --type typescript --type javascriptLength of output: 383
🏁 Script executed:
#!/bin/bash
echo "Searching for .ttf font files under apps/web..."
find apps/web -type f -name "*.ttf" 2>/dev/null || echo "No .ttf files found under apps/web"
echo
echo "Searching for directories named 'fonts' under apps/web..."
find apps/web -type d -name "fonts" 2>/dev/null || echo "No 'fonts' directories found under apps/web"
echo
echo "Searching for .ttf references in the apps/web code..."
rg "\.ttf" -n apps/web || echo "No .ttf references found in apps/web"Length of output: 1562
Ensure font mappings align with actual files & harden filename handling
The explicit mapping in apps/web/src/lib/export/font-manager.ts includes fonts whose .ttf files aren’t present in apps/web/public/fonts, and the fallback filename logic can still mis-generate names.
• Missing files under apps/web/public/fonts for these entries—update mapping or add files:
- Arial (
arial.ttf) - Helvetica (
helvetica.ttf) - Times New Roman (
times.ttf) - Georgia (
georgia.ttf)
• Line 31’s dynamic filename generation (fontFamily.replace(/\s+/g, '_').toLowerCase()) can produce names (e.g.playfair_display.ttf) that don’t match actual files—use a normalization function that strips punctuation and handles common variants, or derive available font names from the filesystem.
• Before passing a font path to FFmpeg, validate its existence (e.g.fs.existsSync), and log a clear error or fallback immediately if missing.
• Avoid infinite recursion in the fallback: if loading “Inter” fails, don’t callloadFont('Inter')again—either throw or return a no-op after the first retry.
🤖 Prompt for AI Agents
In apps/web/src/lib/export/font-manager.ts between lines 12 and 22, update the
fontMapping to only include fonts whose .ttf files actually exist in
apps/web/public/fonts by either adding the missing font files (arial.ttf,
helvetica.ttf, times.ttf, georgia.ttf) or removing those entries. Replace the
current dynamic filename generation logic with a robust normalization function
that removes punctuation and handles common font name variants to ensure
generated filenames match actual files. Before using a font path with FFmpeg,
check its existence using fs.existsSync and log a clear error or fallback if the
file is missing. Modify the fallback logic to prevent infinite recursion by not
retrying loadFont('Inter') if it already failed once; instead, throw an error or
return a no-op after the first retry.
| /** | ||
| * Escapes text for FFmpeg drawtext filter | ||
| */ | ||
| static escapeForFFmpeg(text: string): string { | ||
| return text | ||
| .replace(/\\/g, '') // Remove backslashes entirely (they cause issues) | ||
| .replace(/'/g, '') // Remove single quotes | ||
| .replace(/"/g, '') // Remove double quotes | ||
| .replace(/:/g, ' ') // Replace colons with spaces | ||
| .replace(/\[/g, '(') // Replace square brackets with parentheses | ||
| .replace(/\]/g, ')') // Replace square brackets with parentheses | ||
| .replace(/=/g, ' ') // Replace equals with spaces | ||
| .replace(/;/g, ' ') // Replace semicolons with spaces | ||
| .replace(/,/g, ' ') // Replace commas with spaces | ||
| .replace(/\n/g, ' ') // Replace newlines with spaces | ||
| .replace(/\r/g, ' ') // Replace carriage returns with spaces | ||
| .replace(/\t/g, ' ') // Replace tabs with spaces | ||
| .replace(/[^\w\s.-]/g, '') // Remove all other special characters except word chars, spaces, dots, hyphens | ||
| .replace(/\s+/g, ' ') // Collapse multiple spaces | ||
| .trim(); // Remove leading/trailing spaces | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider a more balanced text escaping approach.
The current implementation is very conservative, removing many characters that users might legitimately want in their text (quotes, commas, etc.). This could lead to unexpected text alterations.
Consider:
- Properly escaping characters instead of removing them
- Using FFmpeg's text file option to avoid command-line escaping issues
- Preserving more punctuation while still preventing injection
For a more robust solution, consider using the textfile parameter instead of inline text:
- `text='${escapedText}'`,
+ // Write text to a temporary file to avoid escaping issues
+ `textfile=${await this.fontManager.createTempTextFile(element.content)}`,This would require implementing a temporary file management system but would eliminate most escaping concerns.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/web/src/lib/export/text-processor.ts around lines 152 to 172, the
current escapeForFFmpeg method removes many characters like quotes and commas,
which may undesirably alter user text. Instead of stripping these characters,
update the method to properly escape them according to FFmpeg's requirements or
refactor the export logic to use FFmpeg's textfile option by writing the text to
a temporary file and referencing that file in the command. This approach
preserves punctuation and avoids complex escaping, so implement temporary file
creation and cleanup to support this safer and more user-friendly text handling.
| if (isExporting || progress?.phase === 'completed') { | ||
| setIsExporting(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider preserving completed exports when timeline changes
The current logic resets the export state when the timeline changes, even if the export has completed successfully. This means users lose the ability to download their exported video if they make any timeline changes after export completion.
Consider only resetting if actively exporting:
-if (isExporting || progress?.phase === 'completed') {
+if (isExporting) {📝 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.
| if (isExporting || progress?.phase === 'completed') { | |
| setIsExporting(false); | |
| // Only reset when actively exporting, not after completion | |
| - if (isExporting || progress?.phase === 'completed') { | |
| + if (isExporting) { | |
| setIsExporting(false); | |
| } |
🤖 Prompt for AI Agents
In apps/web/src/components/editor/export-dialog.tsx around lines 48 to 49, the
current code resets the export state even after a successful export when the
timeline changes, causing users to lose access to their completed export. Modify
the logic to reset the export state only if an export is actively in progress
(isExporting is true), and avoid resetting when the export phase is 'completed'
to preserve the completed export state.
|
Closed in favor of #460 |
2 participants
Description
Added core components for video export including
VideoExportService,ExportLogger,FontManager,FilterValidator,MediaProcessor,TextProcessor, andFFmpegCommandBuilder.Created a README documentation outlining the architecture, usage, and extension points for the video export service.
Changed font picker to use Google Fonts and Inter by default.
Updated ffmpeg.js to UMD (previous version did not work correctly)
Type of change
How Has This Been Tested?
Draft PR for now
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation