Skip to content

feat(thumbnailer): add sandbox-safe rootless ffmpeg video thumbnailer - #1226

Closed
castrojo wants to merge 9 commits into
mainfrom
feat/ffmpeg-thumbnailer
Closed

castrojo wants to merge 9 commits into
mainfrom
feat/ffmpeg-thumbnailer

Conversation

@castrojo

@castrojo castrojo commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Human Parts

This basically replaces the gnome thumbnailer with an ffmpeg container and fixes all thumbnails. It's just quadlets lol. Dakota has nothing and fedora always ships a broken one, one less rpmfusion dep or wherever you get it from. Distro agnostic too!

I've been running this for about a month on my new machine and can confirm it works. Would love more testing on super large folders of videos!

thumbnailer

What does this change?

Adds a sandbox-safe, resource-governed ffmpeg video thumbnailer daemon and in-sandbox shim for GNOME on image-based systems (Bluefin, Bluefin LTS, Dakota).

Why?

Stock video thumbnailers in GNOME (totem-video-thumbnailer / gst-video-thumbnailer) fail on many modern codecs (AV1, HEVC, ProRes, 10-bit), crash under memory pressure, or are completely absent on minimal desktop images like Dakota.

Architecture & Container Selection

  • Automatic Container Variant Selection via systemd Quadlets:
    • ffmpeg-thumbnailer.container: Uses docker.io/jrottenberg/ffmpeg:9.0-alpine320@sha256:2c3d7158... (lean 111 MB multi-arch Alpine image, software decode with native libdav1d, libx264, libx265, libvpx). Guarded by ConditionPathExists=!/sys/module/nvidia/version.
    • ffmpeg-thumbnailer-nvidia.container: Uses docker.io/jrottenberg/ffmpeg:9.0-nvidia2404@sha256:76b1f321... with AddDevice=-nvidia.com/gpu=all (CDI). Guarded by ConditionPathExists=/sys/module/nvidia/version.
    • Both quadlets produce a container named ffmpeg-thumbnailer, so the host daemon targets a single container name regardless of hardware.
  • Sandbox Boundary & File Descriptor Passing:
    • GNOME runs thumbnailers inside a bubblewrap sandbox with --unshare-all and seccomp filters blocking user namespaces.
    • The in-sandbox pure-Python shim opens the input video and output PNG and passes open descriptors over SCM_RIGHTS via a UNIX socket in ~/.cache/gnome-desktop-thumbnailer/gstreamer-1.0/ (the only writable host directory bound into the sandbox).
    • The host daemon executes podman exec --preserve-fd=N,M ffmpeg-thumbnailer timeout -s KILL 15 ffmpeg -threads 1 ... directly on /proc/self/fd/N.
  • Resource Governance (Anti-Pegging):
    • Both container and daemon run under Slice=background.slice with CPUWeight=20, IOWeight=20, MemoryHigh=512M, MemoryMax=1024M, TasksMax=512, and ManagedOOMMemoryPressure=kill.
    • In-container execution is hard-capped by timeout -s KILL 15 and -threads 1.
    • Host daemon throttles concurrency via semaphore (FFT_MAX_INFLIGHT, default $\min(4, \text{cpu_count} // 2)$).
  • Desktop Precedence:
    • 10-ffmpeg-thumbnailer.conf in /usr/lib/environment.d prepends /usr/share/ublue-os/thumbnailer to XDG_DATA_DIRS.
    • Because libgnome-desktop scans XDG_DATA_DIRS in order and first registration per MIME wins, ffmpeg-thumbnailer.thumbnailer cleanly overrides /usr/share/thumbnailers/gst-video-thumbnailer.thumbnailer.

Checklist

  • PR title follows Conventional Commits (fix:, feat:, docs:, ci:, refactor:, etc.)
  • just check passes
  • pre-commit run --all-files passes
  • Skill doc updated if the change affects agent-facing conventions or behavior
  • AGENTS.md / docs/SKILL.md / docs/skills/ links remain valid
  • CI is green after push: gh run list --repo projectbluefin/common --limit 5

AI attribution

Assisted-by: Claude Opus via GitHub Copilot
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

Replaces unaccelerated/unsupported video thumbnailing in GNOME with a rootless
containerized ffmpeg daemon running under background.slice.

- Dual quadlet (.container) files for automatic variant selection:
  - Non-NVIDIA (AMD/Intel/software): docker.io/jrottenberg/ffmpeg:9.0-alpine320 (lean 111 MB)
  - NVIDIA: docker.io/jrottenberg/ffmpeg:9.0-nvidia2404 with CDI gpu=all
- In-sandbox pure-Python shim passes input/output file descriptors over SCM_RIGHTS
  via UNIX domain socket in the GStreamer cache dir.
- Daemon invokes 'podman exec --preserve-fd' with in-container timeout(1) and single-thread decode.
- Resource-governed under background.slice with CPUWeight=20, IOWeight=20, MemoryHigh/Max and TasksMax.
- Precedence over /usr/share/thumbnailers/gst-video-thumbnailer guaranteed via environment.d prepending /usr/share/ublue-os/thumbnailer to XDG_DATA_DIRS.
- Unit tests added in tests/test_ffmpeg_thumbnailer.py and registered in Justfile + unit-tests.yml.

Assisted-by: Claude Opus via GitHub Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: castrojo <castrojo@users.noreply.github.com>
- Raise TasksMax from 32 to 512 in daemon.service to support concurrent podman exec clients.
- Fix NVIDIA CDI AddDevice specification (drop leading hyphen).
- Fix aarch64 NVIDIA handling: ConditionArchitecture=x86-64 on NVIDIA unit, triggering ConditionArchitecture=|!x86-64 and ConditionPathExists=|!/sys/module/nvidia/version on Alpine unit.
- Validate passed file descriptors in daemon: ensure regular file type and output owner matches uid.
- Return ERR_UNAVAILABLE when container is stopped/pulling; shim falls back to local ffmpeg rather than failing permanently.

Assisted-by: Claude Opus via GitHub Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: castrojo <castrojo@users.noreply.github.com>
@castrojo

Copy link
Copy Markdown
Contributor Author

Automated Review Summary & Hardening (PR #1226)

This branch has been audited and updated to address all findings from parallel code and security reviews:

  1. TasksMax Capacity:

    • Raised TasksMax from 32 to 512 in ffmpeg-thumbnailer-daemon.service. Each podman exec client spawns ~17 threads; 32 tasks was causing thread starvation and failure under concurrent thumbnail requests.
  2. NVIDIA CDI Device Specification:

    • Fixed AddDevice=nvidia.com/gpu=all in ffmpeg-thumbnailer-nvidia.container. Quadlet's leading - syntax performs an os.Stat on the device string, which dropped the CDI device reference.
  3. Multi-Architecture Matrix & Alpine Fallback:

    • jrottenberg/ffmpeg:9.0-nvidia2404 is amd64-only. Added ConditionArchitecture=x86-64 to the NVIDIA unit, and triggering conditions ConditionArchitecture=|!x86-64 and ConditionPathExists=|!/sys/module/nvidia/version to the Alpine unit so aarch64 NVIDIA hosts use the lean multi-arch Alpine container.
  4. SCM_RIGHTS File Descriptor Validation:

    • Hardened handle_request in ffmpeg-thumbnailer-daemon via os.fstat: validates that both input and output descriptors are regular files (S_ISREG) and that the output file owner matches the running user's UID (st_uid == os.getuid()), preventing arbitrary file truncation attacks.
  5. Graceful Degradation (ERR_UNAVAILABLE):

    • When the container is pulling, offline, or stopped, the daemon returns ERR_UNAVAILABLE. The shim treats this as a transient condition and falls back to local ffmpeg if installed, rather than causing GNOME to write a permanent fail marker.
  6. In-Container Execution Guardrails:

    • In-container execution uses timeout -s KILL 15 and -threads 1, ensuring stuck decodes are reaped cleanly by RunInit (catatonit) and do not saturate host CPU cores.

…ementarity, and ERR_UNAVAILABLE

- Add test_daemon_handle_request_invalid_fd_type: asserts daemon rejects non-regular fds (sockets).
- Add test_daemon_returns_unavailable_when_container_missing: asserts daemon returns ERR_UNAVAILABLE when container is absent (exit 125).
- Add test_shim_falls_back_on_err_unavailable: asserts shim invokes local ffmpeg fallback on ERR_UNAVAILABLE.
- Add test_quadlet_condition_complementarity: asserts identical ContainerName, RunInit=true, and NVIDIA CDI presence.
- Ensure fallback ffmpeg invocation in shim uses explicit -f image2 -c:v png.

Assisted-by: Claude Opus via GitHub Copilot

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: castrojo <castrojo@users.noreply.github.com>
@castrojo castrojo added the hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2026
@castrojo

Copy link
Copy Markdown
Contributor Author

Operational Verification & Unexercised Environmental Matrix

1. Exercised Verification (Proved Locally):

  • Happy Path End-to-End: A real user quadlet container (docker.io/jrottenberg/ffmpeg:9.0-alpine320@sha256:2c3d...) started with --entrypoint=/bin/sleep --init --network none --read-only --userns keep-id --device /dev/dri. The daemon accepted open descriptors over UNIX domain socket, executed podman exec --preserve-fd=... timeout -s KILL 15 ffmpeg -threads 1 ..., and the shim produced a valid 256px PNG thumbnail from a test MKV video.
  • Graceful Degradation (ERR_UNAVAILABLE): Container stopped mid-operation $\to$ daemon detected podman exit 125 and returned ERR_UNAVAILABLE $\to$ shim invoked local host ffmpeg fallback and produced a valid thumbnail.
  • Signal Handling & Shutdown: podman stop completed in 0.06s without hanging or SIGKILL escalation due to RunInit=true (catatonit).
  • Security Invariant Validation: SCM_RIGHTS handler rejected socket file descriptors with ERR: invalid file descriptor type.
  • Test Suite: 10 pytest tests in tests/test_ffmpeg_thumbnailer.py passing in 0.03s.

2. Unexercised Environmental Matrix (Requires Full Image Build / Hardware):

  • Desktop Session Precedence: Tested XDG directory loading spec against libgnome-desktop source, but live GNOME Shell / Nautilus session with 10-ffmpeg-thumbnailer.conf prepending XDG_DATA_DIRS has not run inside an ostree boot session.
  • SELinux Enforcing Host: The workstation has SELinux disabled. The containers use SecurityLabelDisable=true, but /proc/self/fd reopen behavior under strict targeted enforcing mode on a live Bluefin ostree boot should be verified in staging.
  • NVIDIA Hardware with CDI: The NVIDIA quadlet specifies AddDevice=nvidia.com/gpu=all. Requires a physical machine with proprietary NVIDIA drivers and /etc/cdi/nvidia.yaml to verify NVDEC decode path.
  • aarch64 Real Device: Multi-arch Alpine image inspected, but runtime on aarch64 hardware remains unexercised.

@Danathar

Copy link
Copy Markdown
Contributor

I ran this end to end on Bluefin 44 (GNOME 50, SELinux enforcing), using GNOME's own thumbnail code (GnomeDesktop.DesktopThumbnailFactory, gnome-desktop 44.5) instead of calling the shim by hand. The container path works. The first video went from GNOME's sandbox to the shim, the daemon, podman exec and ffmpeg, and came back as a thumbnail, with SELinux enforcing and label=disable. The XDG_DATA_DIRS prepend also works: with the new directory first, GNOME picked ffmpeg-thumbnailer.thumbnailer over gst-video-thumbnailer and ffmpegthumbnailer. But two things stop the container from being used on Bluefin today.

1. GNOME deletes the daemon's socket after every thumbnail. After each thumbnailer run, gnome-desktop empties ~/.cache/gnome-desktop-thumbnailer/gstreamer-1.0/ except for gstreamer-1.0.registry. That's clean_gst_registry_dir(), called at the end of gnome_desktop_thumbnail_script_exec() in libgnome-desktop/gnome-desktop-thumbnail-script.c. It takes the socket with it. The daemon keeps running, listening on a path that no longer exists, so systemd never restarts it.

Here's what happened. I hid host ffmpeg from the sandbox so the fallback couldn't mask the result:

Step Result Socket afterwards
video 1 (webm) thumbnail OK, through the container deleted: Left-over file 'ffmpeg-thumbnailer.sock' ... deleting
video 2 (mp4) failed: ffmpeg-thumbnailer: failed to thumbnail still gone
video 3 (the webm from #1207) failed still gone
restart the daemon, then one PNG through the stock glycin thumbnailer thumbnail OK deleted

So any thumbnail at all cuts the link, even a picture. After that, video thumbnails only work if the host has its own ffmpeg for the fallback. Where it doesn't (the Dakota case in the description), they fail. Nautilus normally records a failed thumbnail and doesn't retry until the file changes.

2. On podman 5.8 the quadlets never load. Only podman 6.0 and later read rootless quadlets from /usr/share/containers/systemd/users/. Fedora 44 ships podman 5.8.x (5.8.7 is the current F44 update). Its user generator only looks in these places (my scratch paths shortened):

$ /usr/libexec/podman/quadlet -v -dryrun -user
No files parsed from [$XDG_RUNTIME_DIR/containers/systemd ~/.config/containers/systemd /etc/containers/systemd/users /etc/containers/systemd/users/1000]

In podman's source, getRootlessDirs in v5.8.7 has no /usr/share users path. v6.0.0 added basePaths := []string{UnitDirAdmin, UnitDirDistro}. So on today's GNOME 50 Bluefin image the container never exists, the daemon gets exit 125 on every request, and everything goes to the fallback. That might explain why it works on your machine: if you're on podman 6, problem 2 doesn't happen there, and host ffmpeg would cover problem 1.

Smaller things:

  • tests/test_ffmpeg_thumbnailer.py is in the Justfile but not in .github/workflows/unit-tests.yml, so CI never runs it. The Unit Tests run on b7a71a2 shows 73 passed in its pytest step, the same number as main. The 10 new tests do pass locally.
  • Nothing sets FFT_HWACCEL, so it stays none. NVIDIA machines would download the CUDA image (637 MiB compressed for amd64, once per user, since rootless storage is per user) and then decode in software anyway.
  • The Justfile change also registers tests/test_bluefin_countme.bats. Main registered that file in ci: run the suite-registration drift gate on every PR #1192 (merged today), so a merge with current main lists it twice (Justfile lines 14 and 22) and just test runs it twice. That line can be dropped.
  • This PR conflicts with feat(wallpaper): ship original looping movie for Hidamari #1207 on the Justfile pytest line. Whichever one lands second needs a rebase.

What I didn't cover: NVIDIA, aarch64, and a real login session. The /usr/libexec path doesn't exist on my machine, so I pointed a copy of the .thumbnailer entry at a copy of the shim. Everything else was this branch's code, run in a scratch HOME against a container I started by hand with the flags from quadlet -dryrun.

@hanthor hanthor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on the sandboxing, but this needs changes and a maintainer decision before it can land:

  • Default-on everywhere. user-preset/01-ffmpeg-thumbnailer.preset, the static graphical-session.target.wants symlink and WantedBy=default.target in users/*.container turn this on for every user on every image. Each login then pulls docker.io/jrottenberg/ffmpeg, which is about 111 MB, or multi-GB for the CUDA variant on nvidia. It's digest-pinned, but it lives in an individual's Docker Hub namespace and nothing tracks updates to it. Either rebuild and sign it under ghcr.io/projectbluefin, or make the feature opt-in.
  • ffmpeg-thumbnailer*.container: SecurityLabelDisable=true removes SELinux confinement from the process whose whole job is parsing untrusted media. Please keep the label, or document why it has to be dropped.
  • environment.d/10-ffmpeg-thumbnailer.conf makes the shim own every video MIME type even when the daemon or container is down. The fallback is host ffmpeg, which Dakota doesn't ship, so thumbnails disappear instead of falling back to gst.
  • The description says the daemon runs with MemoryMax=1024M, but the unit sets 512M.

@castrojo

Copy link
Copy Markdown
Contributor Author

Status update on ffmpeg-thumbnailer:
Review by @hanthor and @Danathar flagged key design blockers:

  1. Default-on enablement in user-preset enables the service unconditionally across all desktop installs, whereas ffmpeg-thumbnailer was designed as an opt-in or separate container appliance.
  2. In-sandbox execution under SELinux enforcing mode and interaction with GNOME's thumbnail factory require verification.
  3. PR feat(thumbnailer): add opt-in ujust toggle for the ffmpeg thumbnailer daemon #1234 depends on this PR.

Holding open as an author/maintainer design decision regarding default vs opt-in service enablement.

- Drop SecurityLabelDisable=true from both container units so the
  untrusted-media-parsing process keeps SELinux confinement (keeps the
  label rather than dropping it).
- Raise the host daemon.service MemoryHigh/MemoryMax to 512M/1024M to
  match the container units, removing the 512M-vs-1024M inconsistency.
@castrojo

Copy link
Copy Markdown
Contributor Author

Two of the four items were clear code fixes; I've pushed those. The other two are maintainer decisions, and I want to flag them before touching code.

Fixed (pushed cce2e80):

#2 SELinux label — dropped SecurityLabelDisable=true from both .container units. The process parses untrusted media, so keeping the SELinux label is the secure default. No reason to drop it.

#4 MemoryMax — the host daemon unit was the outlier (MemoryHigh=256M/MemoryMax=512M); both containers already run 512M/1024M. Raised ffmpeg-thumbnailer-daemon.service to 512M/1024M to match.

Need a maintainer decision (coupled):

#1 Default-on everywhere. This is the real one. Two honest paths:

  • Opt-in: remove the user-preset enable, the static graphical-session.target.wants symlink, and WantedBy=default.target in the containers. Lowest risk, no new infra, directly kills the "pull a per-login image on every image" concern.
  • First-party image: rebuild + sign ffmpeg under ghcr.io/projectbluefin and track updates. More durable, but it's a whole new build/sign pipeline and a larger PR.

I'd start opt-in and build the signed ghcr image as a follow-up, but flipping the rollout is a product call, not a code fix — flagging rather than doing it unilaterally.

#3 environment.d MIME ownership. This is coupled to #1. The shim already returns ERR_UNAVAILABLE and falls back to local ffmpeg when the container is down, but GNOME won't retry gst because the prepended XDG_DATA_DIRS makes ffmpeg win every video MIME. Restoring the gst fallback requires not shadowing gst, which is the same decision as #1. So this one resolves together with #1.

Want me to implement the opt-in path for #1 (and #3 falls out), or would you rather go straight to the signed ghcr image?

🐝 Hive Agent: contributor | SHA: cce2e80

@Danathar

Copy link
Copy Markdown
Contributor

Thanks for the SELinux and MemoryMax fixes.

On the rollout, I'd go opt-in first: no preset enable, no static graphical-session.target.wants link, no WantedBy=default.target. The signed ghcr image can come later as its own PR.

One thing from my earlier review is still open at cce2e80 and isn't in the list above: the daemon's socket still sits in ~/.cache/gnome-desktop-thumbnailer/gstreamer-1.0/. gnome-desktop empties that directory after every thumbnail (clean_gst_registry_dir()), so the first thumbnail of any kind deletes the socket and every later video falls back to host ffmpeg or fails. The socket needs a path that gnome-desktop's bwrap sandbox can still reach and that it doesn't clean. I haven't checked which paths it binds besides that cache directory, so that's the first thing to find out. Recreating the socket in the daemon whenever it disappears is a fallback if no such path exists. This has to be fixed whichever rollout we pick.

…reamer cache

GNOME's gstreamer clean_gst_registry_dir() periodically deletes the
contents of ~/.cache/gnome-desktop-thumbnailer, including the daemon's
socket path. The old listener kept serving on the now-dead path, so
every new thumbnailer connection failed until a full daemon restart.

serve_recovering() polls for the missing socket and rebinds at the same
path (unlinking the stale socket first so the new listener owns a fresh
inode) without a restart. Request handling still runs in per-request
threads, so the poll loop never blocks on a long ffmpeg job.

Adds a regression test that deletes the socket mid-run and verifies the
daemon rebinds and answers PING again.
…p path

gnome-desktop's clean_gst_registry_dir() runs g_remove() on every
top-level entry of ~/.cache/gnome-desktop-thumbnailer/gstreamer-1.0
except gstreamer-1.0.registry after each thumbnailer run. The socket
lived at the top level, so every thumbnail deleted it. The rebind loop
added in 3596c80 only notices after up to 0.5s, while Nautilus starts
the next thumbnailer almost immediately, so back-to-back video
thumbnails still missed the socket and failed on hosts without ffmpeg.

Move the socket into a ffmpeg-thumbnailer/ subdirectory. remove() cannot
delete a non-empty directory, so the directory and the socket inode
survive the cleanup. The path keeps the old length so AF_UNIX's 108-byte
limit is not reached for any home directory the old path supported. The
rebind loop stays as a fallback for a wiped ~/.cache and now recreates
the subdirectory.

Also drop the duplicate test_bluefin_countme.bats line from the Justfile
test recipe; main already registers it.

Assisted-by: Claude via omp
Signed-off-by: castrojo <castrojo@users.noreply.github.com>

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed 907b843 to fix the socket-lifetime problem Danathar reported. 3596c80 did not fully fix it.

What was wrong at 3596c80

After every thumbnailer run, gnome-desktop's clean_gst_registry_dir() (libgnome-desktop/gnome-desktop-thumbnail-script.c) calls g_remove() on each top-level entry of ~/.cache/gnome-desktop-thumbnailer/gstreamer-1.0/ except gstreamer-1.0.registry. The socket sat at that top level, so every thumbnail deleted it.

serve_recovering() notices the missing path within 0.5s and rebinds. But Nautilus starts the next thumbnailer almost immediately, so the next shim finds no socket, and on a host without ffmpeg it fails.

I reproduced this with the real daemon (fake podman writing a PNG), the real shim with no ffmpeg on PATH, and a faithful copy of the cleanup (libc remove() on top-level entries) after each call. Twenty back-to-back thumbnails gave 2 successes and 18 failures.

Fix (907b843)

  • ffmpeg-thumbnailer-daemon:40-43 and ffmpeg-video-thumbnailer-shim:27-33: the socket moves to gstreamer-1.0/ffmpeg-thumbnailer/sock. remove() cannot delete a non-empty directory, so the subdirectory and the socket inode survive. Only the bind in that cache directory is user-writable (/usr, /etc/ld.so.cache, /etc/alternatives and the fontconfig cache are root-owned read-only binds), so a subdirectory there is the only place that is both reachable and not cleaned.
  • The new path is the same length as the old one, so the 108-byte AF_UNIX limit is no tighter for long /var/home/<user> paths.
  • GNOME now logs "Failed to delete left-over thumbnailing '.../ffmpeg-thumbnailer'" once per thumbnail. Before, it logged "Left-over file ... deleting" once per thumbnail, so the log volume is the same.
  • make_server() now recreates the subdirectory, so the rebind loop still handles a user wiping ~/.cache.
  • After the fix, the same repro gave 20 successes and 0 failures.
  • New test test_socket_survives_gnome_gst_registry_cleanup runs the cleanup against the daemon's real socket layout. It asserts that the shim and daemon resolve the same path, that the socket inode is unchanged, and that PING succeeds with no wait. It fails on 3596c80 and passes on 907b843.
  • Justfile: I dropped the third bats tests/test_bluefin_countme.bats line this PR added; main already registers it.

CI at 907b843 is green: validate, both image builds, Unit Tests (54 passed in the validators step, including the new test), and PR E2E on GNOME 50.

Status of @hanthor's review

  • SELinux label: fixed in cce2e80; SecurityLabelDisable=true is gone from both .container units. This still needs a re-test on an enforcing host. Danathar's end-to-end run used label=disable, and with the label on, ffmpeg in container_t reopens /proc/self/fd/N for host-labelled files, which container-selinux may deny.
  • MemoryMax mismatch: fixed in cce2e80. ffmpeg-thumbnailer-daemon.service now has MemoryHigh=512M and MemoryMax=1024M, matching the containers.
  • Default-on, and the image in an individual's Docker Hub namespace: open, needs a maintainer decision. Opt-in (remove the user-preset, the graphical-session.target.wants symlink and WantedBy=default.target) is the smallest path. Danathar has also said he prefers it.
  • environment.d MIME ownership when the daemon or container is down: open. It depends on the same decision.

Also still open from Danathar's review, not changed here

  • Rootless quadlets under /usr/share/containers/systemd/users/ are only read by podman 6.0 or later; he reports Fedora 44 ships 5.8.x.
  • FFT_HWACCEL is never set, so NVIDIA hosts pull the CUDA image and still decode in software.

This PR still needs @hanthor to re-review and resolve the CHANGES_REQUESTED review after the rollout decision, plus two approvals from maintainers other than me.

@Danathar Danathar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review at 907b843 (hold label left in place). I ran tests/test_ffmpeg_thumbnailer.py here: 12 passed. When I moved the socket back to the top of the gst cache dir in both daemon and shim, test_socket_survives_gnome_gst_registry_cleanup failed with FileNotFoundError, so the new test guards the fix. CI (unit tests, both builds, validate) is green; the GNOME 50 E2E for this head was still running when I looked.

hanthor's points:

  • Default-on everywhere: not fixed. The user preset, the graphical-session.target.wants link and WantedBy=default.target in both .container files are unchanged. This is the maintainer decision you asked for, and it's still open.
  • SecurityLabelDisable=true: fixed. It's gone from both .container files.
  • environment.d taking every video MIME type: not fixed. The file is unchanged; you tied it to the rollout choice.
  • MemoryMax: fixed. The daemon is now 512M/1024M, the same as the containers.

My earlier points:

  1. GNOME deleting the socket: fixed. clean_gst_registry_dir() calls g_remove() on each top-level entry (gnome-desktop-thumbnail-script.c:1216), and that can't remove a non-empty directory. I ran that cleanup after each of 20 requests, 10 ms apart, against this head's serve_recovering: 20/20 reached the daemon. On 3596c80 the same test gave 1 of 10 at 50 ms gaps. One side effect: that g_remove() now fails on every run, so gnome-desktop will log Failed to delete left-over thumbnailing '.../ffmpeg-thumbnailer' after each thumbnail. It's only journal noise.
  2. Podman 5.8: not fixed, and it's worse than I said before. In podman v5.8.7, getRootDirs in pkg/systemd/quadlet/unitdirs.go walks all of /usr/share/containers/systemd, users/ included, with no filter. So both .container files become system services, started as root by PID 1 at boot. The GNOME 50 E2E for 3596c80 (run 36257836867) shows it: systemd[1]: Starting ffmpeg-thumbnailer.service, generated into /run/systemd/generator/, then a failed Docker Hub pull because DNS isn't up yet. The rootless daemon's podman exec can't reach a root container anyway. Fedora 44's current podman is 5.8.7 (Bodhi, 2026-09-19), and the quadlet files are unchanged at 907b843. They need a location that 5.8's user generator reads, or they should stay out of the image until podman 6.
  3. Test not in CI: fixed.
  4. FFT_HWACCEL never set: not fixed. The daemon still defaults to none, so NVIDIA hosts would pull the CUDA image and still decode in software.
  5. Duplicate test_bluefin_countme.bats in the Justfile: fixed.

New, small: ManagedOOMMemory=kill isn't a systemd key. The E2E log warns Unknown key 'ManagedOOMMemory' in section [Service], ignoring for the daemon and both containers. ManagedOOMMemoryPressure=kill is probably what was meant.

What's left: the rollout decision (with the MIME ownership that depends on it), the podman 5.8 quadlet location, and FFT_HWACCEL.

@castrojo

Copy link
Copy Markdown
Contributor Author

Status at head 907b843: blocked on a maintainer rollout decision, so I'm not dismissing hanthor's CHANGES_REQUESTED (R5319623719).

  • Decision needed, owner @hanthor: default-on vs opt-in. Opt-in means dropping the user preset, the graphical-session.target.wants link, and WantedBy=default.target in both .container files. Danathar prefers opt-in. The environment.d MIME-ownership point resolves together with this choice. feat(thumbnailer): add opt-in ujust toggle for the ffmpeg thumbnailer daemon #1234 (the ujust toggle) also waits on it.

  • Open technical points, per Danathar at 907b843:

    • The quadlet location isn't read by podman 5.8.7's user generator. Its system generator picks the files up and runs them as root services (E2E run 36257836867).
    • FFT_HWACCEL is never set.
    • ManagedOOMMemory=kill is not a systemd key; ManagedOOMMemoryPressure=kill is the likely intent.

    These go in one push once the rollout is decided, because the unit layout depends on it.

  • Already fixed: SELinux label and MemoryMax (cce2e80), GNOME socket cleanup (907b843), CI registration of the tests, and the duplicate Justfile line.

castrojo is the author, so this needs hanthor's CR cleared plus two approvals from people other than castrojo.

Addresses review feedback on PR #1226 (hanthor CHANGES_REQUESTED,
Danathar review).

Opt-in rollout (hanthor's default-on concern #1): the thumbnailer no
longer turns on for every user on every image. Removing the three
default-on mechanisms Danathar specified:
- delete user-preset/01-ffmpeg-thumbnailer.preset (no auto-enable),
- delete the static graphical-session.target.wants symlink,
- drop WantedBy=default.target from both *.container units, leaving
  them static so neither container starts at boot.

ManagedOOM key (Danathar): ManagedOOMMemory=kill is not a systemd key
and warned on every unit. Renamed to ManagedOOMMemoryPressure=kill in
the two container units and the daemon service.

Still open, flagged to maintainers (need a decision or hardware/
version testing, not guessable here):
- environment.d MIME shadowing (hanthor #3): the global XDG_DATA_DIRS
  prepend stays and still hides gst for non-opt-in users; gating it
  behind opt-in is a separate decision.
- FFT_HWACCEL is never set (daemon defaults to "none"); NVIDIA decode
  path is untested without hardware.
- podman 5.8 quadlet location: /usr/share/containers/systemd/users is
  parsed as root system units on 5.8; needs a 5.8-readable shipped
  location or a version gate.
- Opt-in enablement: the daemon lacks Wants= on the container units, so
  the ujust toggle (#1234) must start the container unit too.
@castrojo

Copy link
Copy Markdown
Contributor Author

Pushed dd9be26 addressing the parts of the review that are code fixes. Two of the open items still need a maintainer decision or hardware/version testing, so I am flagging them rather than guessing (details below).

Fixed (dd9be26):

  1. ManagedOOM key (Danathar). ManagedOOMMemory=kill is not a systemd key and warned Unknown key ... ignoring on every unit. Renamed to ManagedOOMMemoryPressure=kill in both *.container units and ffmpeg-thumbnailer-daemon.service.

  2. Default-on rollout (hanthor CR Add common system files from bluefin #1, Danathar's opt-in spec). Removed the three default-on mechanisms so no image is pulled on every login:

    • deleted user-preset/01-ffmpeg-thumbnailer.preset (no auto-enable),
    • deleted the static graphical-session.target.wants/ffmpeg-thumbnailer-daemon.service symlink,
    • dropped WantedBy=default.target from both *.container [Install] sections, leaving the containers static so neither starts at boot.

The daemon service keeps its own [Install] — it only creates symlinks when explicitly enabled, so with the preset and static link gone it no longer turns the feature on by default. hanthor's other two points (SecurityLabelDisable dropped in cce2e80, MemoryMax reconciled in cce2e80) are already in.

Flagged — need a maintainer decision or hardware/version testing (not guessable from here):

  • environment.d MIME shadowing (hanthor CR Switch to podman and buildah for container builds #3). This stays. The XDG_DATA_DIRS prepend is global and still puts ffmpeg-thumbnailer ahead of gst-video-thumbnailer for every user, so a non-opt-in user on a host without host ffmpeg can't fall back to gst. Opt-in (daemon off) does not remove the shadowing, so gating the environment.d + .thumbnailer behind opt-in is a separate decision — I did not fold it into this push.
  • FFT_HWACCEL never set (Danathar). The daemon defaults to none, so NVIDIA hosts would decode in software. Setting it to cuda/vaapi/auto is hardware-specific and untested without a machine; I did not guess.
  • podman 5.8 quadlet location (Danathar). Under 5.8.7 /usr/share/containers/systemd/users/ is parsed as root system units, not rootless. Needs a 5.8-readable shipped location or a version gate — no clean fix from here.
  • Opt-in enablement. The daemon has After= but no Wants= on the container units, so enabling the daemon does not start the container. The ujust toggle (feat(thumbnailer): add opt-in ujust toggle for the ffmpeg thumbnailer daemon #1234) will need to start/enable the container unit too when it is built.

I am the PR author, so my commit does not count as an approval. @hanthor, @Danathar — the code points above are pushed; flagging the four items that need a maintainer call or hardware. Ready for re-review on what I changed.

🐝 Hive Agent: contributor | SHA: dd9be26

@Danathar

Copy link
Copy Markdown
Contributor

Re-checked at dd9be26 against my review at 907b843. tests/test_ffmpeg_thumbnailer.py passes here (12 passed, on a local merge with current main), and CI on dd9be26 is green, including the GNOME 50 E2E.

  • Opt-in: done the way I suggested. The preset, the graphical-session.target.wants link and both WantedBy=default.target lines are gone. The dd9be26 E2E serial log has no ffmpeg-thumbnailer lines; the 3596c80 one had PID 1 starting ffmpeg-thumbnailer.service and failing a Docker Hub pull.
  • ManagedOOMMemoryPressure=kill: fixed in all three units. systemd-analyze verify no longer flags the key.
  • Podman 5.8 location: still open. Nothing starts at boot now, but podman 5.8's rootless generator still never sees these files, so opting in can't work on Fedora 44 yet. One location 5.8.7 does read for users is /etc/containers/systemd/users/: getRootlessDirs walks it, and the root generator skips it (GetUserLevelFilter). That would avoid waiting for podman 6.
  • FFT_HWACCEL: still open. An NVIDIA user who opts in would pull the CUDA image and still decode in software. If NVDEC can't be tested yet, shipping only the Alpine container for now would avoid that.
  • The PR body still lists ManagedOOMMemory=kill and TasksMax=64 for the daemon. It's ManagedOOMMemoryPressure=kill and 512 now, and the body becomes the squash commit message.

The environment.d point and the CHANGES_REQUESTED are hanthor's, so I'll leave those with him.

@castrojo

Copy link
Copy Markdown
Contributor Author

Status check: Blocked on maintainer architectural rollout decision (@hanthor):

  1. Rootless Podman quadlet drop-in location (/usr/share/containers/systemd/users vs system generator execution).
  2. Docker Hub container image hosting vs rebuild/sign under ghcr.io/projectbluefin.
  3. Resolution of upstream architectural decision before unblocking downstreams (feat(thumbnailer): add opt-in ujust toggle for the ffmpeg thumbnailer daemon #1234).

@castrojo

Copy link
Copy Markdown
Contributor Author

Updated the PR body per your review: the daemon resource-governance line now reads ManagedOOMMemoryPressure=kill (was ManagedOOMMemory=kill) and TasksMax=512 (was 64), matching the pushed units. The body becomes the squash message, so this keeps it accurate.

The remaining technical items you raised (podman 5.8 quadlet location at /etc/containers/systemd/users/, FFT_HWACCEL, and the environment.d MIME shadowing) still need a maintainer call or hardware/version testing, so I am leaving those flagged rather than guessing. Ready for another look on the body fix.

🐝 Hive Agent: contributor | SHA: unknown

Resolve the Justfile pytest-line conflict by keeping both
tests/test_render_hidamari_movie.py (main) and
tests/test_ffmpeg_thumbnailer.py (this branch).

Assisted-by: Claude via omp
@castrojo

Copy link
Copy Markdown
Contributor Author

Merged origin/main into the branch (d30d404) to clear the conflict. The only conflict was the Justfile pytest line: main added tests/test_render_hidamari_movie.py, this branch added tests/test_ffmpeg_thumbnailer.py; both are kept. Locally pytest tests/test_ffmpeg_thumbnailer.py 12 passed and bats tests/test_suite_registration.bats is clean. Required checks at d30d404 are green (validate, test, both image builds, Trivy).

No behaviour change. Still blocked on @hanthor's CHANGES_REQUESTED and the open maintainer calls listed above (podman 5.8 user-quadlet location, Docker Hub image vs a signed ghcr rebuild, environment.d MIME ownership when disabled). castrojo is the author, so merging needs 2 approvals from other maintainers.

@Danathar

Copy link
Copy Markdown
Contributor

Checked d30d404. It's a merge of main, and the PR's own files are unchanged since dd9be26 (pytest 12 pass, CI green). The body now matches the units. Still open from my 09-27 review: the two .container files are still under /usr/share/containers/systemd/users/, not a location podman 5.8 reads for user quadlets, and FFT_HWACCEL is still never set, so the daemon defaults to none (daemon:48). Also, related to #1234: the daemon unit has only After= on the containers, no Wants=, so they won't come back after a login unless something else starts them.

@castrojo castrojo closed this Oct 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

hold Indicates that a PR should not merge because someone has issued a /hold command.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants