Skip to content

Feature/s3 support - #108

Open
Mike-Solar wants to merge 20 commits into
elonen:masterfrom
Mike-Solar:feature/s3-support
Open

Feature/s3 support#108
Mike-Solar wants to merge 20 commits into
elonen:masterfrom
Mike-Solar:feature/s3-support

Conversation

@Mike-Solar

Copy link
Copy Markdown
Contributor

Add S3 support for the project. This version does not contains Chinese support.

@Mike-Solar

Copy link
Copy Markdown
Contributor Author

@elonen

…view

This removes rustfmt formatting noise that was mixed with the
S3 feature changes, making the PR diff focus on actual code changes.

Changes:
- Revert formatting-only changes
- Restore Cargo.lock to version control (for reproducible builds)
- Pin AWS SDK versions compatible with Rust 1.87

The S3 feature functionality is unchanged - this is purely a diff
cleanup to make code review easier (~700 lines vs ~12,000).
@elonen

elonen commented Nov 30, 2025

Copy link
Copy Markdown
Owner

Thanks for this, S3 storage is a great addition!
I'm working through the PR and will be submitting patches to address a few things, as this is a rather major change.

Diff Cleanup

I've pushed a commit (d7e025b) that separates formatting changes from the actual S3 feature code. The original PR had ~12,000 lines changed in server/src/, but most were rustfmt reformatting mixed with semantic changes. The cleanup:

  • Reverts formatting-only changes to preserve the original code style
  • Restores Cargo.lock to version control (for reproducible builds)
  • Pins AWS SDK versions compatible with Rust 1.87

S3 functionality is unchanged; this just makes the diff reviewable (~1800 lines vs ~12000).

My Concerns & Probable Changes

Browser Compatibility

Transcoding exists primarily because of limited video playback support in browsers, not for bandwidth/storage. I'm thinking:

  • Add a config option to disable allowing users to skip transcoding
  • Add validation when "Skip transcode" is selected to check that user know what they are doing: warn about formats that won't play in browsers (ProRes, AVI, etc.)
  • Audio and image files should still be processed since they require transcoding to become playable
  • Update the client UI label to clarify the risks of skipping transcoding

Configuration & Security

  • ✅ DONE - Add environment variable support for S3 credentials via clap's env attribute by letting AWS SDK handle auth in the many ways it supports (avoiding secrets in CLI args/process lists)

Code Quality

  • Probably refactor S3 operations to use async proper instead of creating a blocking runtime
  • ✅ DONE - Add integration tests for the S3 storage path (using MinIO or similar)

Media management

  • What does / should this do when a video is trashed?

Dependencies

The AWS SDK versions (aws-sdk-s3 1.115.0, aws-sdk-sts 1.94.0) require Rust 1.88+. The pinned versions in Cargo.lock (aws-sdk-s3 1.103.0) work with Rust 1.87. I'll keep 1.87 support for now, and pin lower version. Maybe update later.

What Looks Good

  • Clean StorageBackend abstraction
  • No breaking changes to existing installations (folder structure, DB schema, gRPC API intact - can maybe reveal S3 to Organizers later)
  • basic_folders plugin remains compatible without modification
  • Progress reporting and multipart upload handling are well-implemented

I'll submit patches as they're ready, feel free to comment and discuss.

@elonen

elonen commented Nov 30, 2025

Copy link
Copy Markdown
Owner

Thoughts on user-specified transcoding mode

Bug: Default behavior changed from Auto to Force

Currently the checkbox defaults to checked (true), which sends "true"TranscodePreference::Force. This would mean all HTTP uploads are transcoded by default, even browser-compatible H.264/MP4 files that previously weren't transcoded (unless too high bitrate).

First, let's make this an option in server config, disabled by default.
When enabled, this would fix the abovementioned problem:

1. Client: Replace checkbox with dropdown (Auto/Force/Skip)

A dropdown restores Auto as the default while offering all three options:

<script>
    let transcodeOption: "auto" | "force" | "skip" = $state("auto");
</script>

<div class="flex justify-end items-center text-sm mb-2 gap-2">
    <select
        bind:value={transcodeOption}
        class="bg-slate-800 border border-slate-600 text-gray-300 text-xs rounded px-2 py-1 focus:outline-none focus:border-slate-500"
    >
        <option value="auto">Auto transcode</option>
        <option value="force">Always transcode</option>
        <option value="skip">Skip transcoding</option>
    </select>
    {#if transcodeOption === "skip"}
        <span class="text-amber-400 text-xs">⚠ Without transcoding, file will only play if it's already browser-compatible</span>
    {/if}
</div>

Update the header to send the string value:

ajax.setRequestHeader("X-CLAPSHOT-TRANSCODE", transcodeOption);  // "auto", "force", or "skip"

(The cookie can be removed since the header is authoritative.)

2. Server: Update header parsing to match

In file_upload.rs:

let transcode_preference = hdrs
    .get("x-clapshot-transcode")
    .and_then(|v| v.to_str().ok())
    .map(|s| match s.to_ascii_lowercase().as_str() {
        "force" => TranscodePreference::Force,
        "skip" => TranscodePreference::Skip,
        _ => TranscodePreference::Auto,
    })
    .unwrap_or(TranscodePreference::Auto);

3. Server: Soft validation + notification when Skip is used on (potentially) incompatible formats

In video_pipeline/mod.rs, around line 430:

let requested_transcode = match md.transcode_preference {
    TranscodePreference::Force => {
        Some(("user requested transcoding".to_string(), target_bitrate))
    }
    TranscodePreference::Skip => {
        // Soft validate: warn if format may not be browser-compatible
        if let Some((reason, _)) = auto_transcoding_need(md, target_bitrate) {
            user_msg_tx
                .send(UserMessage {
                    topic: UserMessageTopic::Ok,
                    msg: "Transcoding skipped as requested".to_string(),
                    details: Some(format!(
                        "Note: This file may not play in all browsers ({}). \
                         If playback fails, re-upload with transcoding enabled.",
                        reason
                    )),
                    user_id: Some(md.user_id.clone()),
                    media_file_id: Some(media_id.to_string()),
                    ..Default::default()
                })
                .ok();
        }
        None
    }
    TranscodePreference::Auto => auto_transcoding_need(md, target_bitrate),
};

=> give users full control while providing clear feedback when they upload something that might not play.

@Mike-Solar

Copy link
Copy Markdown
Contributor Author

#106

…its, flaky integration tests)

  Don't persist tokio runtime in ObjectStorageBackend. Instead, create
  temporary runtimes for client init and each upload operation. This
  avoids "cannot drop runtime in async context" panics when storage is
  dropped inside api_server's tokio runtime during shutdown

  Trade-off: slightly less efficient (no connection pooling), but safe.
  A proper fix would make uploads truly async.
@elonen

elonen commented Dec 1, 2025

Copy link
Copy Markdown
Owner

⚠️ This also really needs playback authentication, S3 pre-signed endpoints and/or proxy through Clapshot server (secure and foolproof but would make Server a bandwidth bottleneck).

@elonen

elonen commented Dec 1, 2025

Copy link
Copy Markdown
Owner

Stretch goal: Make this able to co-exist with local files.
Idea: store S3 info in <videoid>/S3.json, which would signify that the video is not locally stored, but in object storage.

(Maybe do #109 while at it)

@Mike-Solar

Copy link
Copy Markdown
Contributor Author

⚠️ This also really needs playback authentication, S3 pre-signed endpoints and/or proxy through Clapshot server (secure and foolproof but would make Server a bandwidth bottleneck).

In my design, S3 bucket is a public bucket and there's no authentication. But this actually leads security issues. I just realized this.

@Mike-Solar

Copy link
Copy Markdown
Contributor Author

Hey, I think we can return a temporary link for S3 backend. I may do this these days. Authentication is still required.

@elonen
elonen force-pushed the master branch 3 times, most recently from 9d6823d to 884ce3e Compare June 3, 2026 18:35
… i18n, and expand tests

Server:
- Add StorageBackend::media_url() and presigned_url() for S3 presigned redirects
- Add authenticated GET /api/media/{media_id}/{path} endpoint returning HTTP 302
- Make --s3-public-url optional/deprecated and add --s3-presigned-url-expiry
- Update MediaFile/Subtitle URL generation and filename parsing for presigned flow
- Add S3 unit/integration tests (media_url, presigned URL, upload, redirect endpoint)
- Improve progress-reader polling and test robustness for fast transcodes

Client:
- Complete Chinese (zh) translations for newly extracted i18n keys
- Wire NavBar, App, VideoPlayer, EDLImport, ExportDialog, SubtitleCard to $t()
- Add localStorage mock for i18n test setup

Docs:
- Update README and nginx configs for presigned S3 flow
# Conflicts:
#	.gitignore
#	Dockerfile.server
#	Makefile
#	client/debian/additional_files/clapshot+htadmin.nginx.conf
#	client/package-lock.json
#	client/src/App.svelte
#	client/src/i18n.ts
#	client/src/lib/NavBar.svelte
#	client/src/lib/player_view/SubtitleCard.svelte
#	client/src/lib/tools/EDLImport.svelte
#	client/src/lib/tools/comment-export/ExportDialog.svelte
#	server/Cargo.lock
#	server/Cargo.toml
#	server/src/lib.rs
#	server/src/tests/integration_test.rs
- Restore client/src/i18n.ts re-export shadowed by empty file after merge
- Fix server UserSessionData/IncomingFile missing fields from upstream proto changes
- Fix cs_main_test! macro invocations missing url_base parameter
- Pass StorageBackend into init_and_spawn_workers in test macro
- Remove duplicate title span and duplicate i18n imports from merge
- Convert remaining dot-notation i18n keys to gettext source strings
- Update i18n extractor to handle imperative get(t)(...) calls
- Translate document.title and ISO 639 aria-label for zh/fi
- Regenerate compiled client catalogs (zh/fi 100% translated)
- Switch ExportDialog format selector to native select for testability
- Include upstream translation wrapping for UserMessage, FolderListing, VideoTile, CommentInput, EDLImport
- Add ExportDialog test timing fix with await tick()
…fig docs

- Add --s3-region CLI arg and wire it through to StorageBackend::s3().
- Refactor ObjectStorageBackend upload path into async core + sync wrapper.
- Use Handle::try_current() to avoid nested runtime panics from async callers.
- Wrap put_object/create_multipart_upload/upload_part/complete_multipart_upload
  in exponential-backoff retry helper.
- Use tokio::task::spawn_blocking for subtitle uploads from async WS handlers.
- Add S3 configuration section to clapshot-server.conf example.
- Update integration tests for new StorageBackend::s3() signature.

Tests: all 7 S3 integration tests pass with local MinIO.
Cover guess_content_type, local/S3 accessors, key_for_path,
media_url, presigned_url local error path, and upload no-ops.
Also add async variant test for upload_with_progress_async.
When the project directory and /tmp are on different filesystems,
mediainfo scanning falls back to a symlink. The symlink was created
with the original relative source path, which resolves to a non-existent
file from the temp directory and causes 'No media tracks found'.
Use fs::canonicalize() to create an absolute symlink target.
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.

2 participants