The 2/4 finding is right: after writeImagePart started passing an explicit
filename stem, initImageFilename had no caller in video.go, and its
"conditioning frame" doc no longer described its one remaining user
(lipsync.go's avatar image). A one-line wrapper that survives only to be
misdescribed is not indirection worth keeping.
lipsync now calls imageFilename(mime, "frame") directly, and imageFilename's
doc lists the real bases — including WHY the video keyframes need distinct
ones: a backend that stages uploads by filename would otherwise have the
second overwrite the first.
Not taken: consolidating the first/last-frame rationale to a single canonical
site. The copies address different readers — the wire encoding (provider), the
contract's undetectable-support caveat (videogen), and the mode table (README)
— and last round's finding was a doc pointing at a note that did not exist.
Trading duplication for cross-references is what produced that.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
- README documented only t2v/i2v. Now a table of the four keyframe
combinations, plus the undetectable-support caveat, which is the one thing a
caller cannot work out for itself.
- The LastImage doc comment said "see the note on LastImage support in
provider/llamaswap" — there was no such note. A pointer to something that
does not exist is worse than no pointer; the comment is now self-contained.
- Generate's doc described only input_reference; it now names
input_reference_last and explains why an unsupporting backend returns a clip
rather than an error.
The 2/4 finding (writeImagePart reusing the "frame" base for both parts) was
already fixed in dbc9689 — from the receiving end, where the consequence is
concrete rather than stylistic: ComfyUI stages uploads by FILENAME with
overwrite=true, so a shared name means the second clobbers the first and both
keyframes resolve to one image.
Not taken: initImageFilename's name is no longer misleading (writeImagePart
stopped calling it), and it is still used by lipsync.go so it is not dead.
The empty-LastImage test stays standalone — it mirrors the existing standalone
empty-InitImage coverage rather than a table this file does not have.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
Caught while writing the receiving end. Distinct multipart field names are not
sufficient: backends stage an uploaded frame under a name derived from the
FILENAME, and our own ComfyUI shim posts to /upload/image with overwrite=true.
Both parts were sending initImageFilename(mime) — literally "frame.png" for
each — so the second upload would have clobbered the first and BOTH keyframe
inputs would have resolved to the same stored image.
The failure mode is the worst kind: a clip pinned at both ends to the same
frame renders cleanly, returns 200, and looks like the feature not working
rather than like a bug. Nothing upstream or downstream would report a fault.
writeImagePart now takes the filename stem (frame / frame_last), and the test
asserts the two arrive under different filenames.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
videogen.Request gains LastImage alongside InitImage, so one Request covers
t2v, i2v and FL2V without a mode flag. With InitImage it pins both ends of the
clip; alone it pins the destination and lets the backend invent the approach.
The llamaswap provider sends it as a SEPARATE `input_reference_last` part
rather than a second `input_reference`. Multipart permits repeated names, but
then which frame is first and which is last depends on part ORDER — an
ordering contract invisible in the payload, that nothing notices breaking. A
backend that does not know the new name ignores the part, the same degradation
as any other unknown field.
Both parts go through one writeImagePart helper so their encoding cannot
drift, and an empty LastImage is rejected up front exactly as InitImage
already is.
Support is per-model and deliberately NOT advertised in this contract: a
backend that ignores a trailing keyframe returns an ordinary clip, which is
indistinguishable from success. The doc comment says so, because a caller that
needs to know whether the pin took effect has to establish that out of band —
and the mort side gates on a convar for exactly this reason.
Motivated by mort's #1567 (long-form video): with both ends pinned, drift
becomes structurally bounded inside each shot instead of compounding across an
autoregressive chain.
Tests break-checked: sending the last frame under the shared name fails both
the distinct-name assertion and the last-alone case.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
Gadfly on #23, blocking, 2/2 agreement — and it is the exact defect this
whole line of work has been about: a call that succeeds while handing back
the wrong bytes.
sniffImageMIME falls back to image/png when detection is inconclusive, and
the guard only consulted Content-Type. A response with NO Content-Type
therefore skipped the check entirely and was labelled a PNG. The shim answers
JSON on a semantic miss (no face found in the source or target), which is
precisely the body that would have sailed through as a successful image.
The check now validates the BYTES — http.DetectContentType must say image/ —
and the reported MIME prefers the server's own label only when that label is
itself an image type. Break-checked by restoring the header-only condition,
which fails the new test.
Also from that review:
- index is documented as ignored under all=true, so a negative one is no
longer rejected there; it is still rejected when it would actually be
sent, and both halves are tested.
- initImageFilename (video.go) was imageFilename with the base fixed to
"frame" and now delegates to it — two copies of one extension table is
how they drift.
- DetectedFace carried Width/Height alongside Box, two sources of truth for
one fact that can disagree after any transform. Now a Size() method
derived from Box.
- a dead `apiErr` in the test (declared, then `_ = apiErr`) was an
abandoned errors.As check; it is wired up and now asserts callers can
classify the error.
- swapImg duplicated editInit verbatim; removed.
Not taken: adding a FaceSwapProvider/ModelOption surface to match the other
optional imagegen capabilities (single-model finding). There are no options
to carry yet, and inventing an empty option type to look symmetrical would be
API surface with nothing behind it. Worth revisiting when a real knob exists.
- SubmitChain rejects NaN/±Inf segment seconds with ErrUnsupported
(previously an obscure json.Marshal error; NaN fails every comparison
and +Inf passed the >= 0 check).
- ChainStatus skips segment entries with no usable id — JSON null
(which no-op-unmarshals into a string, previously appending ""),
empty strings, and id-less objects; the unfiltered list survives in
Raw. ChainJob.SegmentIDs doc now also says ChainSegmentResult takes
the segment index, not an id string.
- jobPath rejects '%' in job ids — %2F/%2E%2E percent-escapes decode
back into path structure server-side, bypassing the literal check on
this upstream-echoed value.
- singleVideoResult moves to video.go next to videoMIME, and the two
remaining hand-rolled copies of the video-result validation
(videoModel.Generate, Interpolate) now use it — one validation, one
message shape.
- videogen.LipSyncer renamed to videogen.Lipsyncer for consistency with
the rest of the surface's Lipsync* naming (LipsyncProvider,
LipsyncModel, LipsyncRequest); not yet consumed downstream, so the
rename is free now and never again.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
maxVideoResponseBytes (512MB) replaces the shared 64MB JSON cap on the
/v1/videos/sync read path (doRaw now takes the cap per call) — 3/6
models flagged that a legitimate long/high-bitrate clip would be
discarded after minutes of GPU work. Plus a stale stable-diffusion
comment in initImageFilename and a test-handler early return.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
- videoMIME no longer hard-falls-back to video/mp4: a 2xx body that is
neither declared nor sniffable as video (JSON job envelope, HTML error
page) is now an APIError instead of a 'successful' corrupt clip.
- Resolution rides the wire as width/height AND the OpenAI-style size
string, so either upstream convention honors an explicit request.
- writeFormFields + mimeFromContentType shared helpers replace the
copied multipart loop (audio.go/video.go) and Content-Type branch.
- ADR-0019 indexed in docs/adr/README.md; README gains the videogen
section + support-matrix mention (docs-parity rule).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
New videogen/ contract package (ADR-0019): Request/Result/Model/Provider
with the imagegen conventions. Text-to-video and image-to-video are one
surface (Request.InitImage, nil = t2v) since hybrid checkpoints like
Wan 2.2 TI2V serve both from one model; Result carries a single clip.
provider/llamaswap gains VideoModel(id) targeting the blocking
POST {base}/v1/videos/sync (multipart, model-routed by the fork's new
video routes): vLLM-Omni parameter names, OpenAI-style input_reference
file part, optional fields stay off the wire so per-model launch-flag
defaults apply. CLAUDE.md package map picks up audio/ (missed in #12)
and videogen/.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steveandClaude Opus 5