feat: add export resolution selector (Closes #17)#31
Conversation
Let users choose the export resolution before starting an export, with hardware tier gating. - Add resolution dropdown in ExportBar with options: 720p, 1080p, 4K. - Resize export offscreen canvas to match the selected resolution. - Hardware tier gating: Low tier (720p only), Medium tier (720p + 1080p), High tier (all resolutions). Unavailable options are disabled. - Display an estimated file size based on resolution and duration. - Default resolution: 1080p on Medium/High tier, 720p on Low tier. Co-authored-by: socialawy <24765060+socialawy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
|
|
🚨 Escalation: This PR needs human review. @socialawy please check. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces configurable export resolutions (720p, 1080p, and 4K) with hardware tier-based gating and bitrate scaling. Key changes include state management for resolution in the settings store, UI updates in the ExportBar for selection and size estimation, and engine updates to support target dimensions. Review feedback highlights the need to handle aspect ratio preservation during resizing to avoid stretching, suggests moving resolution gating logic from a component-level useEffect into the store for cleaner state management, and recommends using shared constants instead of magic numbers for bitrate multipliers.
| if (resolution === '720p') { | ||
| targetWidth = 1280; | ||
| targetHeight = 720; | ||
| } else if (resolution === '1080p') { | ||
| targetWidth = 1920; | ||
| targetHeight = 1080; | ||
| } else if (resolution === '4K') { | ||
| targetWidth = 3840; | ||
| targetHeight = 2160; | ||
| } |
There was a problem hiding this comment.
Forcing fixed dimensions (e.g., 1280x720) for export presets will cause stretching or letterboxing issues if the project's native aspect ratio (defined in spec.meta) is not 16:9. Consider calculating the target dimensions based on the requested height while preserving the original aspect ratio. Additionally, these values should be centralized in a constant shared with the UI.
| if (resolution === '720p') { | |
| targetWidth = 1280; | |
| targetHeight = 720; | |
| } else if (resolution === '1080p') { | |
| targetWidth = 1920; | |
| targetHeight = 1080; | |
| } else if (resolution === '4K') { | |
| targetWidth = 3840; | |
| targetHeight = 2160; | |
| } | |
| const RESOLUTION_HEIGHTS = { | |
| '720p': 720, | |
| '1080p': 1080, | |
| '4K': 2160, | |
| }; | |
| if (resolution && RESOLUTION_HEIGHTS[resolution]) { | |
| const targetH = RESOLUTION_HEIGHTS[resolution]; | |
| const scale = targetH / spec.meta.height; | |
| targetWidth = Math.round((spec.meta.width * scale) / 2) * 2; // Ensure even dimensions for encoders | |
| targetHeight = targetH; | |
| } |
| useEffect(() => { | ||
| if (!exportResolution) { | ||
| setExportResolution(tier === 'low' ? '720p' : '1080p'); | ||
| } else if (tier === 'low' && exportResolution !== '720p') { | ||
| setExportResolution('720p'); | ||
| } else if (tier === 'medium' && exportResolution === '4K') { | ||
| setExportResolution('1080p'); | ||
| } | ||
| }, [tier, exportResolution, setExportResolution]); |
There was a problem hiding this comment.
Managing resolution gating and defaults via useEffect in the component can lead to unnecessary re-renders and potential 'flashes' of invalid state in the UI. It is generally better to handle this logic within the store itself (e.g., using a middleware or by enforcing constraints in the setExportResolution action) or by deriving the 'effective' resolution in a selector.
| let resolutionMultiplier = 1; | ||
| if (exportResolution === '1080p') resolutionMultiplier = 2.25; // roughly 1920x1080 / 1280x720 | ||
| else if (exportResolution === '4K') resolutionMultiplier = 9; // roughly 3840x2160 / 1280x720 |
There was a problem hiding this comment.
These hardcoded multipliers are magic numbers that are directly tied to the resolution presets. It would be cleaner to define a shared constant or map that associates resolution names with their dimensions and bitrate multipliers to ensure consistency across the UI and the engine.
| let resolutionMultiplier = 1; | |
| if (exportResolution === '1080p') resolutionMultiplier = 2.25; // roughly 1920x1080 / 1280x720 | |
| else if (exportResolution === '4K') resolutionMultiplier = 9; // roughly 3840x2160 / 1280x720 | |
| const BITRATE_MULTIPLIERS: Record<string, number> = { | |
| '720p': 1, | |
| '1080p': 2.25, | |
| '4K': 9, | |
| }; | |
| const resolutionMultiplier = exportResolution ? BITRATE_MULTIPLIERS[exportResolution] : 1; |
…ctor-14768703165762109044 feat: add export resolution selector (Closes #17)
2 participants
Let users choose the export resolution before starting an export, with hardware tier gating.
PR created automatically by Jules for task 14768703165762109044 started by @socialawy