Skip to content

feat(export): Server-side FFmpeg rendering with proxy workflow#10

Merged
terisuke merged 3 commits intomainfrom
dev
Oct 18, 2025
Merged

feat(export): Server-side FFmpeg rendering with proxy workflow#10
terisuke merged 3 commits intomainfrom
dev

Conversation

@terisuke
Copy link
Copy Markdown
Contributor

概要

サーバーサイドでのFFmpegレンダリングとプロキシワークフローを実装しました。

主な変更点

1. データベースマイグレーション (006_proxy_workflow.sql)

  • テーブルにプロキシメタデータ列を追加
  • 新しいストレージバケットとを作成
  • テーブルに列を追加
  • 適切なRLSポリシーを設定

2. プロキシ生成機能 (lib/media/proxy.ts)

  • サーバーサイドで自動的に720pプロキシを生成
  • 元のメディアファイルをダウンロードしてトランスコード
  • バケットにアップロード
  • メタデータをテーブルに更新

3. サーバーサイドエクスポート (lib/export/server.ts, app/api/render/route.ts)

  • ブラウザベースのエクスポートからサーバーベースのFFmpegパイプラインに移行
  • プロジェクトのタイムラインから動画を取得
  • 各クリップをトランスコードして連結
  • 結果をexportsバケットにアップロード
  • 署名付きURLを返す

4. Node.js FFmpegラッパー (lib/ffmpeg/node.ts)

  • fluent-ffmpegを使用したFFmpeg操作のラッパー
  • @ffmpeg-installer/ffmpegでバイナリを自動管理

5. メディアアクション更新 (app/actions/media.ts)

  • メディアアップロード時にプロキシ生成をトリガー
  • getSignedUrlでプロキシURLを優先的に返す(フォールバック付き)

6. エディタクライアント更新 (app/editor/[projectId]/EditorClient.tsx)

  • 新しいサーバーサイドエクスポートAPIを使用

テスト

npm run type-check ✓

次のステップ

  1. 既存のメディアファイルのプロキシを一括生成
  2. レンダラーを拡張して、複数の音声トラックとトランジションに対応

関連リンク

  • Migration: supabase/migrations/006_proxy_workflow.sql
  • DEVELOPMENT_SUMMARY.md: 実装の詳細
  • MASTER_IMPLEMENTATION_PLAN.md: アーキテクチャの説明

- Add migration 006_proxy_workflow.sql: extend media_files with proxy metadata, create media-proxies/exports buckets
- Implement server-side proxy generation (lib/media/proxy.ts) with automatic transcoding to 720p
- Replace browser export with Supabase-backed FFmpeg pipeline (app/api/render/route.ts, lib/export/server.ts)
- Add Node.js FFmpeg wrapper (lib/ffmpeg/node.ts) using fluent-ffmpeg
- Update media actions to trigger proxy generation and prefer proxy URLs for playback
- Update EditorClient to use new server-side export API
- Add dependencies: fluent-ffmpeg, @ffmpeg-installer/ffmpeg, @types/fluent-ffmpeg

Tests:
- npm run type-check ✓

Next steps:
- Backfill proxies for existing media files
- Extend renderer to handle layered audio tracks and transitions
@vercel
Copy link
Copy Markdown

vercel bot commented Oct 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
pro-edit Ready Ready Preview Comment Oct 18, 2025 2:49pm

💡 Enable Vercel Agent with $100 free credit for automated AI reviews

- Add webpack externals configuration for @ffmpeg-installer/ffmpeg and fluent-ffmpeg (next.config.ts:78-101)
- Explicitly set nodejs runtime for render API route (app/api/render/route.ts:6)
- Prevents Next.js from bundling platform-specific FFmpeg binaries
- Resolves MODULE_NOT_FOUND error for @ffmpeg-installer/linux-x64 on Vercel

Verification:
- npm run type-check ✓
- npm run build ✓ (FFmpeg modules now resolve correctly)
@claude
Copy link
Copy Markdown

claude bot commented Oct 18, 2025

PR Review: Server-side FFmpeg Rendering with Proxy Workflow

Summary

This PR implements a critical architectural enhancement by moving video rendering from client-side (FFmpeg.wasm) to server-side (Node.js FFmpeg), along with automatic proxy generation for media files. This is a significant improvement for scalability and performance.

✅ Strengths

1. Architecture Design

  • Excellent separation of concerns: Clean split between proxy generation (lib/media/proxy.ts) and export rendering (lib/export/server.ts)
  • Proper resource management: Consistent use of temporary directories with cleanup in finally blocks
  • Status tracking: Good use of database status fields (proxy_status, export_jobs table) for async operations

2. Database Design

  • Well-structured migration (006_proxy_workflow.sql): Proper use of constraints, RLS policies, and backfill logic
  • Security: Appropriate RLS policies ensuring users only access their own media/exports
  • Appropriate bucket limits: 512MB for proxies, 2GB for exports

3. Code Quality

  • Type safety: Good use of TypeScript interfaces and type guards
  • Error handling: Comprehensive try-catch blocks with proper error propagation
  • Caching: Smart media download caching in processExportJob to avoid redundant downloads

4. User Experience

  • Fire-and-forget proxy generation: Non-blocking with proper error handling
  • Progress tracking: Real-time export progress updates via updateJobProgress
  • Fallback logic: Proxy URL with fallback to original media in getSignedUrl

🔴 Critical Issues

1. Security: Potential Resource Exhaustion (HIGH)

Location: app/api/render/route.ts:45-51, lib/media/proxy.ts:118

Both operations run synchronously in the API route handler, which can cause:

  • Timeout issues: Video processing can take minutes, but API routes typically have 10-60s timeouts
  • Memory exhaustion: Multiple concurrent exports could overwhelm the server
  • DOS vulnerability: Malicious users could trigger multiple exports

Recommendation: Queue jobs for background processing instead of awaiting processExportJob synchronously. Return jobId immediately and implement polling endpoint for status updates.

2. Race Condition in Proxy Generation (MEDIUM)

Location: lib/media/proxy.ts:149-151

The proxy_status check is not atomic. Two concurrent requests could both pass this check and start duplicate transcoding jobs.

Recommendation: Use database-level conditional update with proxy_status = 'pending' in WHERE clause to ensure only one process starts transcoding.

3. Missing Input Validation (MEDIUM)

Location: lib/export/server.ts:277

Project names could contain very long strings, causing filesystem issues. Recommend limiting filename length and adding timestamp for uniqueness.

4. Signed URL Expiration Too Short (MEDIUM)

Location: lib/export/server.ts:299

1-hour expiration may be insufficient for large exports (2GB) on slow connections. Recommend 24-hour expiration.

🟡 Bugs & Edge Cases

5. Missing Cleanup on Concat File (LOW)

Location: lib/export/server.ts:128

If concatClips throws between creating the concat file and running ffmpeg, the .txt file won't be cleaned up. Add try/finally block.

6. Proxy Generation Never Retries (MEDIUM)

Location: lib/media/proxy.ts:171-178

Once a proxy fails, it stays in failed state forever. Consider adding retry logic or UI button to regenerate.

7. Incomplete Error Context (LOW)

Location: lib/export/server.ts:154

Failed job progress updates log to console but continue silently, potentially leaving job status stale.

⚡ Performance Considerations

8. FFmpeg Preset Trade-off

Locations: lib/export/server.ts:93, lib/media/proxy.ts:51

Using veryfast preset is good for speed, but consider monitoring CPU usage and costs under load.

9. No Concurrency Limits

Missing rate limiting for concurrent exports and proxy generations per user and server-wide.

Recommendation: Check active export count before creating new jobs and return 429 if limit exceeded.

10. Memory-Inefficient File Reading (MEDIUM)

Location: lib/export/server.ts:283

Reading entire export file into memory before upload could cause OOM errors for large files. Use streaming upload instead.

🧪 Test Coverage

Missing Tests:

  1. No tests for new server-side export functionality - Critical for such complex logic
  2. No tests for proxy generation - Should test all status transitions
  3. No tests for race conditions - Especially proxy generation concurrency
  4. No tests for FFmpeg error handling - What happens if FFmpeg crashes?

Recommendation: Add unit tests for processExportJob, generateProxyForMedia, and edge cases.

📝 Minor Issues

11. Inconsistent Error Messages

Mix of English and Japanese error messages. Recommend standardizing on English for error messages.

12. Missing JSDoc Comments

Functions like transcodeClip, concatClips lack documentation for parameters.

13. Hardcoded Magic Numbers

Values like 1280x720 (proxy dimensions) and audio bitrates should be constants or configurable.

🎯 Recommendations Summary

Must Fix Before Merge:

  1. Implement background job processing for exports (issue Add Claude Code GitHub Workflow #1)
  2. Fix proxy generation race condition (issue docs: MVP v1.0.0 Documentation Release (Japanese) #2)
  3. Add filename length validation (issue Release: ProEdit MVP v1.0.0 - Complete Video Editor Implementation #3)
  4. Fix memory-inefficient file upload (issue feat(export): Server-side FFmpeg rendering with proxy workflow #10)

Should Fix Soon:

  1. Add basic test coverage for export/proxy logic
  2. Implement concurrency limits
  3. Add proxy regeneration capability
  4. Increase signed URL expiration

Nice to Have:

  1. Standardize error message language
  2. Add comprehensive JSDoc comments
  3. Extract magic numbers to constants

🎉 Overall Assessment

This is a well-architected implementation that moves the project in the right direction. The code is clean, well-organized, and follows good practices. However, the synchronous processing in API routes is a critical blocker for production use.

Grade: B+ (after fixing critical issues, would be A)

Recommendation: Request changes to address critical issues #1, #2, and #10 before merging.


Great work on this foundational feature! The migration to server-side rendering will pay dividends in scalability and reliability. 🚀

Address PR review feedback with production-ready enhancements:

## Core Improvements

### 1. Async Job Processing (Critical Fix #1)
- Add background job queue with concurrency controls (lib/export/queue.ts)
- Implement global (default: 2) and per-user (default: 1) export limits
- Queue state persists across requests using globalThis
- API returns jobId immediately, client polls for status

### 2. Proxy Generation Race Condition Fix (Critical Fix #2)
- Implement atomic DB lock with conditional update (lib/media/proxy.ts:169-192)
- Use .neq('proxy_status', 'processing') to prevent duplicate jobs
- Add retry logic with configurable max attempts (default: 3)
- Support force regeneration via options parameter

### 3. Resource Management
- Stream-based file uploads to prevent OOM (lib/export/server.ts:335-361)
- Proper cleanup of concat files in finally block (lib/export/server.ts:113-126)
- Service role client for background operations (lib/supabase/admin.ts)

### 4. Data Validation & Security
- Sanitize export filenames with length limit (lib/export/server.ts:157-167)
- Extend signed URL expiration to 24h (lib/export/server.ts:15)
- Add user_id to export_jobs with RLS policies (007_export_job_enhancements.sql)
- Rate limiting: return 429 when limits exceeded

### 5. API Enhancements
- New polling endpoint: GET /api/render/[jobId] (app/api/render/[jobId]/route.ts)
- POST /api/render now enqueues jobs instead of blocking
- Fix Next.js 15 async params compatibility
- Export progress tracking with real-time updates

### 6. Configuration
- Externalize magic numbers as env variables:
  - PROXY_TARGET_WIDTH/HEIGHT (default: 1280x720)
  - PROXY_MAX_ATTEMPTS (default: 3)
  - EXPORT_MAX_CONCURRENT (default: 2)
  - EXPORT_MAX_PER_USER (default: 1)
  - EXPORT_SIGNED_URL_TTL (default: 86400)
  - EXPORT_AUDIO_BITRATE (default: 192000)
  - EXPORT_MAX_FILENAME_LENGTH (default: 80)

Tests:
- npm run type-check ✓
- Supabase migration 007 applied ✓

Migration Status:
- 006_proxy_workflow.sql: Applied ✓
- 007_export_job_enhancements.sql: Applied ✓

Next Steps:
1. Set SUPABASE_SERVICE_ROLE_KEY in Vercel environment
2. Test export queue under concurrent load
3. Monitor FFmpeg resource usage in production
@claude
Copy link
Copy Markdown

claude bot commented Oct 18, 2025

Pull Request Review: Server-side FFmpeg Rendering with Proxy Workflow

Summary

This PR implements a significant architectural improvement by migrating from browser-based export (WebCodecs/FFmpeg.wasm) to server-side FFmpeg rendering with automatic proxy generation. The implementation is comprehensive and addresses critical scalability concerns.

Code Quality & Best Practices

✅ Strengths

  1. Excellent Resource Management

    • Proper cleanup in finally blocks (lib/export/server.ts:392, lib/media/proxy.ts:230-236)
    • Stream-based uploads prevent OOM issues (lib/export/server.ts:343)
    • Temporary file cleanup with error handling
  2. Good Security Practices

    • Filename sanitization with NFKD normalization (lib/export/server.ts:157-167)
    • RLS policies properly configured for multi-tenant isolation (migrations 006 & 007)
    • Service role client isolated for background operations (lib/supabase/admin.ts)
    • User ID verification in all API routes
  3. Robust Concurrency Controls

    • Atomic DB locks prevent race conditions (lib/media/proxy.ts:169-192)
    • Queue-based export management with per-user limits (lib/export/queue.ts)
    • Deduplication logic for queued jobs (lib/export/queue.ts:89-94)
  4. Configuration Externalization

    • All magic numbers moved to environment variables with sensible defaults
    • Easy to tune for different deployment environments
  5. Error Handling

    • Comprehensive error messages with context
    • Retry logic for proxy generation (lib/media/proxy.ts:194-238)
    • Graceful degradation (proxy fallback to original)

Potential Issues & Recommendations

🔴 Critical Issues

  1. Memory Leak in Queue State (lib/export/queue.ts:23-40)

    • globalThis queue persists indefinitely across requests
    • No cleanup mechanism for completed jobs
    • Impact: Memory will grow unbounded on long-running servers
    • Fix: Implement TTL-based cleanup or bounded queue size
    // Suggested fix: Add periodic cleanup
    function cleanupCompletedJobs(state: QueueState, maxAge: number = 3600000) {
      // Remove jobs older than maxAge from memory
    }
  2. Race Condition in Proxy Lock (lib/media/proxy.ts:169-192)

    • The .neq('proxy_status', 'processing') with .or() filter might not be atomic
    • Multiple concurrent requests could bypass the lock
    • Impact: Duplicate proxy generation for same media
    • Fix: Use database-level SELECT FOR UPDATE or implement distributed locks
    // Consider using Postgres advisory locks:
    // SELECT pg_advisory_lock(hashtext(media_file_id))
  3. Missing Timeout Protection (lib/export/server.ts:224-394)

    • No timeout on FFmpeg operations
    • Large files or slow transcoding could hang indefinitely
    • Impact: Worker processes stuck, resource exhaustion
    • Fix: Add timeout to FFmpeg commands
    createFfmpegCommand(sourcePath)
      .timeout(300) // 5 minutes
      .on('error', handleTimeout)

🟡 Important Concerns

  1. No Cleanup for Failed Exports (lib/export/server.ts)

    • Failed exports remain in storage forever
    • Fix: Add cleanup job or TTL policy on exports bucket
  2. Missing Input Validation (app/api/render/route.ts:23)

    • quality parameter accepts user input without validation
    • Fix: Validate against EXPORT_PRESETS keys
    if (!['720p', '1080p', '4k'].includes(quality)) {
      return NextResponse.json({ error: "Invalid quality" }, { status: 400 });
    }
  3. Polling Inefficiency (app/editor/[projectId]/EditorClient.tsx:266)

    • 2-second polling interval is wasteful
    • Impact: Unnecessary database load
    • Fix: Use Supabase Realtime subscriptions or exponential backoff
    const subscription = supabase
      .channel(`export_job_${jobId}`)
      .on('postgres_changes', { event: 'UPDATE', schema: 'public', table: 'export_jobs' }, handleUpdate)
      .subscribe()
  4. Error Boundary Missing (app/editor/[projectId]/EditorClient.tsx)

    • Export failures don't clear polling interval in all cases
    • Fix: Ensure clearExportPoll() is called in all error paths

🟢 Minor Issues

  1. Inconsistent Error Logging (lib/export/server.ts:152-154)

    • Job progress update failures are logged but not thrown
    • Could silently fail without user notification
    • Fix: Consider retry logic or alerting
  2. Hard-coded Audio Settings (lib/media/proxy.ts:60-62)

    • Audio bitrate and channels are hard-coded
    • Fix: Extract to environment variables like other settings
  3. Missing Migration Rollback (supabase/migrations/)

    • No down migrations provided
    • Fix: Add rollback SQL for development safety

Performance Considerations

✅ Optimizations

  • CRF values well-chosen for quality/size balance (lib/export/server.ts:9-13)
  • veryfast preset balances speed and compression
  • movflags +faststart enables progressive playback
  • Proper use of concat demuxer for minimal re-encoding

⚠️ Performance Concerns

  1. Proxy Generation on Upload Path (app/actions/media.ts:51-56)

    • Fire-and-forget proxy generation blocks upload response
    • Impact: Slow upload UX if transcoding is slow
    • Fix: Already async, but consider job queue integration
  2. No Caching for Downloaded Media (lib/export/server.ts:42-68)

    • Each export re-downloads media from Supabase
    • Impact: Bandwidth waste, slow exports for repeated media
    • Fix: Implement local cache with LRU eviction
  3. Sequential Clip Processing (lib/export/server.ts:303-330)

    • Clips are transcoded one at a time
    • Impact: Slow exports for long timelines
    • Fix: Process clips in parallel (with concurrency limit)
    await Promise.all(
      videoEffects.map((effect, index) => 
        transcodeClipWithSemaphore(effect, index, semaphore)
      )
    )

Security Concerns

✅ Well Implemented

  • Path traversal protection via sanitization
  • User-scoped storage paths prevent cross-user access
  • Service role key properly secured (not in client code)
  • Signed URLs with appropriate TTL (24h)

⚠️ Risks

  1. Service Role Key Exposure Risk

    • Ensure SUPABASE_SERVICE_ROLE_KEY is not logged or exposed in errors
    • Mitigation: Add env var validation at startup
  2. Filename Injection (lib/export/server.ts:157-167)

    • While sanitized, consider additional length validation
    • Current: MAX_FILENAME_LENGTH is good
    • Verify: Test with extremely long project names
  3. Storage Bucket Limits (migrations/006:24, 64)

    • Proxy bucket: 512MB limit might be too restrictive
    • Exports bucket: 2GB matches Supabase tier limits
    • Recommendation: Add quota monitoring

Test Coverage

❌ Critical Gap

No tests for new functionality:

  • No unit tests for lib/export/server.ts
  • No unit tests for lib/media/proxy.ts
  • No integration tests for export queue
  • No tests for migration scripts

Recommendations:

  1. Add unit tests for core functions:

    • sanitizeFilename() with edge cases
    • transcodeClip() error handling
    • Queue state management
    • Proxy lock acquisition
  2. Add integration tests:

    • End-to-end export flow
    • Concurrent export limiting
    • Proxy generation for various formats
  3. Add migration tests:

    • Verify RLS policies work correctly
    • Test bucket creation and permissions

Architecture Review

✅ Strengths

  • Clean separation of concerns (queue, server, proxy)
  • Proper use of Next.js App Router conventions
  • Good type safety with TypeScript
  • Sensible default configurations

🔄 Suggestions

  1. Consider Event-Driven Architecture

    • Current polling could be replaced with Supabase Realtime
    • More responsive and efficient
  2. Add Observability

    • No metrics for export duration, success rate, etc.
    • Add structured logging for production debugging
    • Consider APM integration (Sentry, DataDog)
  3. Queue Persistence

    • Current in-memory queue lost on server restart
    • Consider Redis or database-backed queue for reliability

Summary & Recommendation

Overall Assessment: ✅ APPROVE WITH MINOR CHANGES

This is a high-quality implementation that addresses critical architectural gaps. The code is well-structured, security-conscious, and production-ready with a few important fixes.

Required Before Merge:

  1. ✅ Fix memory leak in queue state (add cleanup)
  2. ✅ Add FFmpeg timeout protection
  3. ✅ Add quality parameter validation
  4. ✅ Fix export polling cleanup in all error paths

Recommended Follow-ups:

  1. Add comprehensive test coverage (separate PR acceptable)
  2. Implement Realtime subscriptions for exports
  3. Add parallel clip processing for performance
  4. Implement queue persistence for reliability
  5. Add observability/monitoring

Migration Checklist:

  • Verify SUPABASE_SERVICE_ROLE_KEY is set in Vercel
  • Test migrations in staging environment
  • Monitor FFmpeg resource usage in production
  • Set up alerts for failed exports
  • Document proxy regeneration procedure for existing media

Excellent work on this feature! The transition to server-side rendering significantly improves scalability and user experience. The issues identified are addressable and don't block the merge.


Reviewed by: Claude Code
Date: 2025-10-18

@terisuke terisuke merged commit 70949d0 into main Oct 18, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant