feat(animation): derive a GIF from a video and fix the clip palette - #317
Merged
Conversation
The shared GIF palette was median-cut, which allocates entries by pixel population. On a clip with a large textured area the background claimed nearly every slot and small saturated marks -- overlay glyphs, thin paths, labels -- collapsed to the nearest muddy neighbour. Quantising for colour coverage keeps them: on a texture-heavy test clip the marks moved from 100-180 RGB distance off their intended colours to under 12, in a smaller file, with whole-frame and background error better or unchanged. Adds gif_from_video, which derives a GIF from a video already on disk. Drawing is far more expensive than encoding -- hours, for a long clip -- so the frames are rendered once and every other format read back off that file rather than re-rendered. The palette machinery moves out of the writer into build_clip_palette and quantize_to_palette so both paths quantise identically, rather than the derived GIF growing a second, divergent quantiser. Chroma subsampling caps what any of this can achieve: yuv420p, which save_animation writes by default, discards colour resolution before the palette ever runs and holds a derived GIF at ~50 RGB distance against ~5 from yuv444p. gif_from_video warns when handed such a source, and the docs say to render the intermediate at yuv444p when a GIF will come off it. Closes #315 Closes #308
Takes the module to 100% line and branch coverage. The gaps were an unreported pixel format, the NV planar formats, and a source that decodes to no frames -- each reachable only by exercising the helpers directly rather than through a rendered clip.
build_clip_palette and quantize_to_palette are public but had no runnable examples, and gif_from_video only showed the happy path. Adds worked examples for the reserved black/white entries, a reduced colour budget, a colour that appears only in a late frame, the shared-table guarantee, width scaling, and the missing-source error.
…mple The coverage-based palette only helped marks about five pixels across. The montage that fed it downsampled each frame to a third, and an interpolating resize blends a small mark into its background before the quantiser can see it: a 3x3 mark still landed 92 away from its intended colour and a 1x1 mark 173, against 175 for the median cut the change was meant to replace. Nothing about the fix reached the sizes that matter -- satellites, orbit paths and labels are one to three pixels. The palette source is now the set of colours the clip contains, collected once each through a presence bitmap at six bits per channel, with each bucket keeping a colour that genuinely occurs so exact hues arrive unshifted. Coverage over distinct colours does not care how many pixels a colour covers, so marks now reproduce exactly at every size tested, including single pixels. It is also faster than the montage it replaces -- 2.5s against 8.2s over 120 frames of 720p -- and drops the resize entirely, so frames need not share a size. The trade is a marginally coarser background, RMSE 7.5 to 8.3 on the test clip, since entries go to colours the clip contains rather than what it contains most of. The docs said the file is always smaller; that was only true of the texture-heavy case, so they now say it moves either way. The palette test was pinned to 5x5 marks, the one size that passed. It now sweeps 5x5, 3x3 and 1x1.
Decoding every frame into a list before quantising made peak memory proportional to the clip's length: 150 frames of 720p held 817 MB, and the hours-long masters this function exists to serve would need tens of gigabytes. The source is now decoded twice and streamed both times, once to learn the clip's colours and once to quantise and write. Neither pass keeps a frame after reading it, and Pillow consumes append_images lazily, so peak memory is flat in the clip's length -- the same clip now costs 7 MB against 99 MB buffered. Decoding twice is far cheaper than the RAM it saves. The decoder was also never closed. It is a generator wrapping an ffmpeg child, so a caller that stopped early, or an exception raised mid-iteration, left the child running -- reachable from this module's own chroma warning under -W error. Iteration now happens inside a try/finally that closes it however the caller leaves.
Four ways to get an unhelpful failure, or none at all: build_clip_palette became public with no validation. An empty clip raised IndexError from inside the colour census, and a budget above 254 either let the reserved black and white overwrite chosen entries or reached Pillow as a bare "invalid palette size". Both now raise a ValueError naming the limit. gif_from_video never checked its output extension. Pillow selects an encoder from the suffix, so a .png path silently produced an APNG and .webp a WebP -- neither being what a caller of a function named gif_from_video asked for. fps reached a division unchecked, so fps=0 surfaced as a bare ZeroDivisionError from the writer, and a negative loop as a bare struct.error from Pillow packing an unsigned short. A rate above 100 fps rounded the GIF delay to zero. The field is in hundredths of a second, and viewers discard a zero delay and replay at their own default -- slower than the caller asked for rather than faster. The delay now floors at the 10 ms the format can actually express, and at 1 ms for WebP, which keeps millisecond timing.
Writing a video honours matplotlib's animation.ffmpeg_path and a system FFmpeg on PATH, falling back to the bundled binary only when neither resolves. Reading one went straight to imageio-ffmpeg's bundled copy, so the same process could decode with one FFmpeg and encode with another, and a pinned custom build was silently ignored on the way in. Decoding now goes through the same resolver and exports the result for imageio-ffmpeg to pick up, leaving an explicit IMAGEIO_FFMPEG_EXE alone. The documented exception was wrong too: it named a missing package, when the error that actually reaches the caller is a missing binary.
The chroma check treated the whole nv family as 4:2:0, so nv24 and nv42 -- both 4:4:4 -- were warned about for losing colour they never lost. It also missed the semi-planar high-bit-depth formats entirely: p010, p016 and p012 are 4:2:0 and p210 is 4:2:2, and none of them start with yuv or nv, so a 10-bit HDR master went through without the warning that matters most for it. Each family is now read the way FFmpeg defines it, with the p-formats spelling their sampling in the first digit. The yuvj element of the prefix tuple was dead -- yuvj420p already starts with yuv -- so it is gone. The tests used invented format strings; they now use names taken from ffmpeg -pix_fmts, including the 4:4:4 members of each family that must not warn.
… ceiling build_clip_palette and quantize_to_palette went out public with bare `list` parameters and no return types, while the rest of the module is fully annotated. They now say what they take -- any iterable of frames, since the streaming path hands them a generator -- and what they give back. The 254-entry ceiling was named _CLIP_PALETTE_COLORS but is the default of two public functions, so mkdocstrings rendered a private name in their signatures. It is now CLIP_PALETTE_COLORS. The module declares __all__, matching the convention the watermark module already follows, and the docs name quantize_to_palette alongside the builder so the pair is discoverable to a downstream package writing its own frames. One doctest imported warnings without using it.
The same-width test only checked the output dimensions, which cannot tell a skipped resample from one that resampled to the same size -- and resampling a frame to its own size still costs time and softens it. Image.resize is now wrapped and must not be called at all. The no-warning test escalated every UserWarning to an error, so an unrelated warning from anywhere in the stack would have failed it for the wrong reason. It now records warnings and asserts only that the chroma one is absent. The duration cases all divided 1000 exactly, so they never exercised the truncation. Adding the default 12 fps and 3 fps showed the delay field is in hundredths of a second: 83 ms is stored as 80 and 333 as 330. The expectations are what round-trips. The decoder stub imported imageio_ffmpeg inside the test body, which the repo's style rules forbid, and the source MP4 was re-encoded for every test that asked for it though none of them mutate it -- the fixture is now module-scoped.
Running ruff format over the whole test file rewrapped four assertions in the odd-dimension and palette-stability tests, neither of which this branch touches. They are back as they were, so the diff shows only the feature.
Selecting the palette for colour coverage keeps small saturated marks, but it spends entries on colours the clip merely contains rather than on what it contains most of, so a smooth photographic clip with nothing small at stake renders its background slightly more coarsely -- RMSE 7.5 to 8.3 on the test clip. That case now has a way out. quantize_method takes a key of QUANTIZE_METHODS -- coverage, the default, median for Pillow's population-weighted cut, or octree between them -- and is accepted by save_animation and gif_from_video alike, so the rendered and derived paths offer the same choice. An unknown key names the accepted ones. A test asserts the knob actually changes the outcome rather than being decorative: on a texture-heavy clip the default keeps the marks and median cut loses them.
gif_from_video reads a file, which the boundary heuristic's first question rules out, and SCOPE.md carried only the tiles/reference carve-out. Nothing recorded why reading a video was acceptable, so the shipped API and the stated scope disagreed. Adds an "Animation output" section giving the reason -- rendering frames costs hours where encoding them costs seconds, so producing every format from a live FuncAnimation means re-rendering per format -- and the limit that still holds: the input must be cleopatra's own output, not an arbitrary user video, and this is not licence to become a media-conversion tool.
…ly weights build_clip_palette gained a method parameter with no worked example. Adds one listing the accepted keys. Writing it corrected the description. The palette is built from a census that holds each colour once, so median cut never sees pixel counts: it splits the colour cube by how densely the clip populates it, weighting by distinct colours rather than by area. The practical effect on a photographic clip is the same -- the crowded regions a background occupies win the table -- but "weights by pixel population" was not what the code does.
The census reshaped each frame to three columns without checking its mode, so an RGBA, L or P frame was reinterpreted byte-wise whenever its pixel count happened to divide by three, yielding a palette of pure misalignment artifacts. Frames are converted to RGB first. build_clip_palette is public and documented for reuse, so this was reachable from outside. Colours were also bucketed to six bits with the last writer winning, which contradicted the promise that the palette reproduces a clip's colours exactly. Keys are now the full 24-bit triple, which removes the representative table entirely: the bitmap is 16 MB whatever the clip, the survey is chunked so a 4K frame is not briefly copied at four bytes a pixel, and padding the census to a square tiles the colours instead of repeating one -- repetition handed that colour a share the strategies read as prominence. The memory claim was wrong. Streaming keeps the decoded RGB frames from ever being resident together, which is the larger cost, but Pillow's GIF encoder accumulates every quantised frame before writing a byte, so peak memory stays proportional to the clip: 154 MB measured for 150 frames of 720p, not flat. The docstring said a long master need not fit in RAM. It does. Median cut was described as weighting by pixel population in three places. The census holds each colour once, so no strategy here sees pixel counts at all -- they weight by distinct colours. The behaviour is unchanged; the description was not true of it. Also: the ffmpeg binary is no longer exported permanently, only while the decoder starts, so it cannot go stale or leak into unrelated code; an empty frame stream raises instead of a bare StopIteration; quantize_method is validated before the render rather than at a point WebP and single-frame GIFs never reach; a single-frame GIF now gets the shared palette and its reserved black and white; packed 4:2:2 formats are recognised; NaN and infinite rates are rejected; a zero-pixel frame says so; scaling moved into the ffmpeg filter chain rather than per frame in Pillow; and the Resampling enum, the annotations and the return types satisfy mypy and ruff, both of which this file had started failing.
…aths The source MP4 was shared across the module, so all 22 tests that used it hung off one ffmpeg encode: a single flaky run turned into twenty simultaneous errors rather than one failure, which matches a transient 18-error run seen during development and hides which test actually broke. It is per-test again; the encode is worth the seconds. Adds cover for what the source fixes changed: non-RGB frames round-tripping to their own colour, exact colour reproduction, census padding not favouring one colour, a zero-pixel frame, an empty frame stream, a single-frame GIF keeping reserved black, the environment variable being restored both when it was unset and when the caller had pinned their own, an unknown strategy rejected before rendering, NaN and infinite rates, packed 4:2:2 formats, and the writer taking a generator -- the lazy path the video route depends on and nothing exercised. Three docstrings still described the montage that no longer exists.
The reference page repeated the wrong claims: that median cut weights by pixel population, and implicitly that memory is bounded. Adds a Memory note giving the real shape -- roughly width x height x frames at peak, because Pillow buffers the quantised frames -- and points at width for a long master. Benchmark figures were quoted with nothing behind them. The mark-fidelity numbers now name the test that asserts them; the background-RMSE and file-size figures are marked as measured-not-asserted rather than presented as guarantees. SCOPE.md claimed cleopatra reads only files it wrote, which the code does not enforce and a check would not improve. It now states the intended input as purpose, and rests the boundary on what the helper does not do. Raises the imageio-ffmpeg floor to the 0.6.0 the suite actually runs against; reading frames back with output_params and a post-filter size was never tested against the old 0.4.9. Glyph.save_animation's forwarded-kwargs list gains quantize_method.
save_animation had grown past the cognitive-complexity limit by accumulating option checks inline; resolving the output format and validating the options against it moves to a helper, which is where that logic belonged anyway since the checks depend on which format the extension selected. The rate guard was written as `not fps > 0` to catch NaN, which reads as an inverted comparison; math.isfinite says what it means and covers infinity too. The ffmpeg rcParam name was spelled out at three call sites and is now a constant. Two exception tests built their argument inside the pytest.raises block, so a failure there would have been mistaken for the error under test.
The iterator was constructed inside pytest.raises, so a failure creating it would have been mistaken for the error under test.
SCOPE.md conflicted: main's watermark work and this branch both added an exception to the data-I/O rule. Kept both, alongside the existing tiles/reference carve-out, and aligned the boundary heuristic's first question with the three the section now lists.
CI verifies uv.lock matches pyproject.toml before installing anything, so the raised floor failed the run at setup rather than at the tests.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Two changes to the GIF path, kept together because the second is only worth having once the first
lands.
The shared GIF palette was picked by pixel population. Median cut allocates entries to whatever
covers the most pixels, so on a clip with a large textured area the background claimed nearly the
whole table and small saturated marks — overlay glyphs, thin orbit paths, labels — collapsed to the
nearest muddy neighbour. The palette is now chosen for colour coverage over the set of colours
the clip contains, gathered by a presence bitmap at full 8-bit precision. Coverage over distinct
colours does not care how many pixels a colour covers, so a mark survives at any size: on the
texture-heavy test clip the four marks moved from 100–180 RGB distance off their intended colours to
reproducing them exactly, including single-pixel marks.
gif_from_videoderives a GIF from a video already on disk, without re-rendering. Drawing isfar more expensive than encoding — hours, for a long clip — so the frames are rendered once and
every other format read back off that file. It runs through the same
build_clip_paletteassave_animation, so a derived GIF and a rendered one quantise identically rather than the derivedpath growing a second quantiser.
Chroma subsampling caps what any of this can do, and is now surfaced.
save_animationwritesyuv420pby default, which stores colour at reduced resolution — that loss happens before the GIFpalette ever runs and no quantiser can undo it. On the test clip a
yuv420pintermediate holds aderived GIF at ~50 RGB distance against ~5 from
yuv444p.gif_from_videoemits aUserWarningwhen handed a subsampled source, so the feature cannot silently under-deliver on its most likely
input.
Memory, stated honestly. The source is decoded twice rather than held, which keeps the decoded
RGB frames from ever being resident together — the larger cost. It does not make memory flat in
the clip's length: Pillow's GIF encoder accumulates every quantised frame before writing a byte, so
peak is roughly
width × height × frames(154 MB measured for 150 frames of 720p). The docs say soand point at
widthfor a long master.Escape hatch.
quantize_method("coverage"default,"median","octree") is accepted byboth
save_animationandgif_from_video, for a smooth photographic clip with no small marks atstake where spending the table on the crowded regions of colour space renders the background a
little more finely.
Dependencies: none added. The
imageio-ffmpegfloor is raised0.4.9 → 0.6.0— the version thesuite actually exercises, since reading frames back with
output_paramsand a post-filter size wasnever tested against the old floor.
Scope:
SCOPE.mdgains an "Animation output" section.gif_from_videoreads a file, which theboundary heuristic's first question rules out, and nothing recorded why that was acceptable. It
states the reason (render once, derive many) and the limit that still holds (this does not become a
media-conversion tool).
Issues
Type of change
Check relevant points.
How Has This Been Tested?
Run the animation suite:
tests/test_animation.py— 171 passed, module at 100 % line and branch coverage(232 statements, 82 branches, 0 missing).
TestClipPaletteQuality), parametrised over5×5, 3×3 and 1×1. The size sweep is the point: a palette built from a spatially downsampled
clip passes at 5×5 and fails at 1×1, and 1×1 is the size that matters for satellites and orbit
paths. Verified the guard bites by restoring median cut — it fails at 165/178/144/101.
gif_from_video— GIF magic bytes, one clip-wide palette, marks surviving the round trip,fpsframe count,widthscaling, the chroma warning firing onyuv420pand staying silenton
yuv444p, missing source, non-GIF extension, andmax_colors/fps/width/loopvalidation.
being reinterpreted byte-wise, and colours are reproduced exactly.
quantize_methodknob is proven to do something — on the texture-heavy clip thedefault keeps the marks and
"median"loses them; a decorative parameter would score alike.IMAGEIO_FFMPEG_EXErestored both whenpreviously unset and when the caller had pinned their own binary.
pytest --doctest-modules src/cleopatra/glyphs/base/animation.py).mypyclean andruff checkclean on the changed files.this PR: 5 issues, all fixed in code.
Measurements quoted above come from a synthetic clip built for the tests, not from the satellite
footage that prompted the issues — the effect sizes are large, but a run against the real showcase
clip before merging is worth doing.
Checklist: