What batch D's review left behind #153

New issue

Closed

opened 2026-08-16 17:50:42 +02:00 by Claude · 1 comment

Claude commented

2026-08-16 17:50:42 +02:00

Collaborator

The three runners have no test

illustrate_from_the_media, illustrate_the_linked and
move_trickplay_of_every_library are the parts that actually run in
production, and nothing exercises them. The pieces under each are tested —
media_images::wanted, by_name::copied, linked_rows_to_illustrate,
describe_existing, move_sheets, save_one_image — but the loops that page
the library, call ffmpeg and file the rows are covered by nothing.

The reason is structural: the strong scan tests live in jellyfin-app and the
runners live in jellyfin-server, which has no fixture harness. That is worth
solving once rather than per runner — a jellyfin-server test that stands up a
database, a temporary metadata directory and a stub for the ffmpeg calls would
reach all three, and the artefact worker's own sweeps as well.

`still.rs` does not model two of upstream's inputs

FFmpegImgExtractPerfTradeoff (MediaEncoder.cs:688) both adds
-skip_frame nokey and drops thumbnail=n=24. It is a server setting this
server has no field for, so every extraction runs as though it were off.
ChapterImageResolution reaches GetImageResolutionParameter; both call
sites here pass resolution: None, which is right only for the MatchSource
default.

Neither changes an answer on a default install. Both are a picture that differs
on one that is not.

A disc rip is handed to ffmpeg as a folder

media_images::wanted refuses VideoType == "Dvd", which is what
VideoImageProvider.GetImage refuses. Upstream then reaches
GetInputPathArgument → GetInputArgument, which for a BluRay or an ISO
concatenates the playlist into one input argument; this server would hand
ffmpeg the folder path and get nothing. The same machinery is why -i here
lacks upstream's file: prefix, which matters for a path ffmpeg would read as
a protocol.

Two kinds fall out of the extraction

Kind::from_type_name answers None for MusicVideo and Trailer, so
wanted refuses both, where upstream treats them as Video and extracts. Only
reachable on a database this server was handed rather than one it scanned, but
that is a case the project supports.

The fixture still cannot measure any of it

Registration disables every image fetcher per library and per type, so upstream
draws no picture in the default snapshot either, and nothing in the media
carries cover art or an attached picture. Measuring the three extractors
against a pair takes a library provisioned with those fetchers on, a track with
cover art, and a container with an attached picture. This belongs with the
fixture work rather than here — reference/README.md says it beside the
trickplay note.

Recorded rather than fixed

Upstream's "import existing tiles" branch (TrickplayManager.cs:449-474)
writes a row with the configured width and the tile count as
ThumbnailCount. It is unreachable once DiscoverExistingTrickplayAsync has
run except for a sheet folder holding files that are not .jpg, where
upstream's own GetImageSize would throw. Not ported, deliberately.
A drawn picture is never redrawn here. BaseDynamicImageProvider re-draws
whatever it made itself; this leaves it alone, because drawing again is a file
whose date moved and a row a rescan rewrites for an identical picture. The
comment beside the check says so.

House rules

Tests run under cargo nextest; upstream's source is the checkout in
upstream/ at the pin (v12.0-rc5, 6d501ba4), not the clone beside the
repository. Generated artifacts are regenerated, never edited. A comment says
what the code does, not how it got there.

What batch D's review round found and the round did not fix. Each is small, none blocks anything, and they are together because they are all the same kind of thing: a place the picture machinery reaches ffmpeg or the fixture and the port is narrower than upstream. ## The three runners have no test `illustrate_from_the_media`, `illustrate_the_linked` and `move_trickplay_of_every_library` are the parts that actually run in production, and nothing exercises them. The pieces under each are tested — `media_images::wanted`, `by_name::copied`, `linked_rows_to_illustrate`, `describe_existing`, `move_sheets`, `save_one_image` — but the loops that page the library, call ffmpeg and file the rows are covered by nothing. The reason is structural: the strong scan tests live in `jellyfin-app` and the runners live in `jellyfin-server`, which has no fixture harness. That is worth solving once rather than per runner — a `jellyfin-server` test that stands up a database, a temporary metadata directory and a stub for the ffmpeg calls would reach all three, and the artefact worker's own sweeps as well. ## `still.rs` does not model two of upstream's inputs - **`FFmpegImgExtractPerfTradeoff`** (`MediaEncoder.cs:688`) both adds `-skip_frame nokey` and drops `thumbnail=n=24`. It is a server setting this server has no field for, so every extraction runs as though it were off. - **`ChapterImageResolution`** reaches `GetImageResolutionParameter`; both call sites here pass `resolution: None`, which is right only for the `MatchSource` default. Neither changes an answer on a default install. Both are a picture that differs on one that is not. ## A disc rip is handed to ffmpeg as a folder `media_images::wanted` refuses `VideoType == "Dvd"`, which is what `VideoImageProvider.GetImage` refuses. Upstream then reaches `GetInputPathArgument` → `GetInputArgument`, which for a BluRay or an ISO concatenates the playlist into one input argument; this server would hand ffmpeg the folder path and get nothing. The same machinery is why `-i` here lacks upstream's `file:` prefix, which matters for a path ffmpeg would read as a protocol. ## Two kinds fall out of the extraction `Kind::from_type_name` answers `None` for `MusicVideo` and `Trailer`, so `wanted` refuses both, where upstream treats them as `Video` and extracts. Only reachable on a database this server was handed rather than one it scanned, but that is a case the project supports. ## The fixture still cannot measure any of it Registration disables every image fetcher per library and per type, so upstream draws no picture in the default snapshot either, and nothing in the media carries cover art or an attached picture. Measuring the three extractors against a pair takes a library provisioned with those fetchers on, a track with cover art, and a container with an attached picture. This belongs with the fixture work rather than here — `reference/README.md` says it beside the trickplay note. ## Recorded rather than fixed - **Upstream's "import existing tiles" branch** (`TrickplayManager.cs:449-474`) writes a row with the *configured* width and the tile count as `ThumbnailCount`. It is unreachable once `DiscoverExistingTrickplayAsync` has run except for a sheet folder holding files that are not `.jpg`, where upstream's own `GetImageSize` would throw. Not ported, deliberately. - **A drawn picture is never redrawn here.** `BaseDynamicImageProvider` re-draws whatever it made itself; this leaves it alone, because drawing again is a file whose date moved and a row a rescan rewrites for an identical picture. The comment beside the check says so. ### House rules Tests run under `cargo nextest`; upstream's source is the checkout in `upstream/` **at the pin** (`v12.0-rc5`, `6d501ba4`), not the clone beside the repository. Generated artifacts are regenerated, never edited. A comment says what the code does, not how it got there.

Claude referenced this issue

2026-08-16 18:23:03 +02:00

The three folder image providers #110 did not name #152

Claude commented

2026-08-16 18:44:35 +02:00

Author

Collaborator

Four of the five landed in 647c33b and 1e5e3dc. The fifth is the fixture's
and has moved to #148.

The two settings

ChapterImageResolution is encoding.image_extraction_height, and the name is
deliberate: despite upstream's, it sizes every picture a video is read for and
not only a chapter's — the frame an item with no poster is given goes through
the same -s. A track's cover art never does.

FFmpeg:imgextractperftradeoff is encoding.image_extraction_perf_tradeoff,
and both halves of what it does are asserted: it adds -skip_frame nokey and
drops the thumbnail=n=24 sampling, so either alone would be a different
picture.

The path

-i now carries upstream's file: prefix. The chapter stills had been missing
it too — this was never only the new extraction's.

One thing worth writing down: upstream builds a single command line and wraps
the path in quotes, which .NET strips before the process sees it. These
arguments are handed over one at a time, so the quotes would become part of the
filename. It is file:/path here and file:"/path" there, and ffmpeg is given
the same bytes either way — checked against a real binary, which cut a
byte-identical picture before and after.

The two kinds

MusicVideo and Trailer are videos upstream extracts for and neither is a
Kind here, on purpose — nothing else in this server branches on either. So
wanted branches on what the file plays as rather than on the row's type,
which is nearer upstream's item is Video than reading a type name was.

The disc rip

A Blu-ray and an ISO are now refused alongside the DVD upstream refuses, and
the comment says why: upstream reads those two through the files their playlist
names, this server enumerates a playlist for nothing but a transcode, and
handing ffmpeg the folder produced nothing at all. Refusing is a divergence,
but it is the one that does not pretend.

The runners

This was the part called "the largest thing on the list", and it turned out to
need less than it looked: a directory that removes itself, a real jpeg, and a
stand-in for ffmpeg that copies it to whatever path it was asked for. That last
one is what makes the extraction reachable with no media file and no encoder.

Four runs are held now — an album given its first track's cover, a playlist
drawn while an unpinned box set is passed over, a film given a still while the
one that already has a picture is left alone and neither loses its streams,
and the sheets moving as a library's setting changes. Each was broken to watch
it go red; the film-and-streams one is the regression the review round found,
now pinned where it happens rather than only in the database.

What moved to #148

Measuring any of this against a pair still takes a library provisioned with the
in-box image fetchers on, a track carrying cover art, and a container carrying
an attached picture. That is fixture work and wants the same rebuild #148 is
already doing.

Four of the five landed in `647c33b` and `1e5e3dc`. The fifth is the fixture's and has moved to #148. ## The two settings `ChapterImageResolution` is `encoding.image_extraction_height`, and the name is deliberate: despite upstream's, it sizes every picture a *video* is read for and not only a chapter's — the frame an item with no poster is given goes through the same `-s`. A track's cover art never does. `FFmpeg:imgextractperftradeoff` is `encoding.image_extraction_perf_tradeoff`, and both halves of what it does are asserted: it adds `-skip_frame nokey` *and* drops the `thumbnail=n=24` sampling, so either alone would be a different picture. ## The path `-i` now carries upstream's `file:` prefix. The chapter stills had been missing it too — this was never only the new extraction's. One thing worth writing down: upstream builds a single command line and wraps the path in quotes, which .NET strips before the process sees it. These arguments are handed over one at a time, so the quotes would become part of the filename. It is `file:/path` here and `file:"/path"` there, and ffmpeg is given the same bytes either way — checked against a real binary, which cut a byte-identical picture before and after. ## The two kinds `MusicVideo` and `Trailer` are videos upstream extracts for and neither is a `Kind` here, on purpose — nothing else in this server branches on either. So `wanted` branches on what the file *plays as* rather than on the row's type, which is nearer upstream's `item is Video` than reading a type name was. ## The disc rip A Blu-ray and an ISO are now refused alongside the DVD upstream refuses, and the comment says why: upstream reads those two through the files their playlist names, this server enumerates a playlist for nothing but a transcode, and handing ffmpeg the folder produced nothing at all. Refusing is a divergence, but it is the one that does not pretend. ## The runners This was the part called "the largest thing on the list", and it turned out to need less than it looked: a directory that removes itself, a real jpeg, and a stand-in for ffmpeg that copies it to whatever path it was asked for. That last one is what makes the extraction reachable with no media file and no encoder. Four runs are held now — an album given its first track's cover, a playlist drawn while an unpinned box set is passed over, a film given a still while the one that already has a picture is left alone *and neither loses its streams*, and the sheets moving as a library's setting changes. Each was broken to watch it go red; the film-and-streams one is the regression the review round found, now pinned where it happens rather than only in the database. ## What moved to #148 Measuring any of this against a pair still takes a library provisioned with the in-box image fetchers on, a track carrying cover art, and a container carrying an attached picture. That is fixture work and wants the same rebuild #148 is already doing.

Claude closed this issue

2026-08-16 18:44:35 +02:00