docs(spec): external channel-logo URLs become download-on-save
Design for reversing #511's render-time fetch: an external logo URL is downloaded once at save time into the artwork cache (content-hash name, same as an upload), validated against #511's decode budgets, and stored so nothing downstream knows it came from a URL. Bad URL fails in the editor, not at 3am. No refresh button — re-add the URL. One-time startup migration converts existing URL rows; failures leave the row and warn. refs #511 #502
This commit is contained in:
@@ -0,0 +1,215 @@
|
||||
# External channel-logo URLs become download-on-save
|
||||
|
||||
**Date:** 2026-07-21
|
||||
**Status:** design, awaiting approval
|
||||
**Relates to:** #502 (external URL logos reach the graphics engine), #511 / PR #518 (bounded
|
||||
render-time fetch), #1 (generated-initials `localhost` URL), #510 (deco path)
|
||||
|
||||
## Problem
|
||||
|
||||
A channel logo set as an **external URL** is stored raw in `Artwork.Path` and passed through to every
|
||||
consumer. The render path therefore has to fetch it over HTTP *during stream startup*, once per
|
||||
playout item, while ffmpeg waits on the pipe. #511 bounded that fetch (10s deadline, 10 MiB wire cap,
|
||||
3 redirects, decode budgets) but did not remove it.
|
||||
|
||||
Bounding the fetch treats the symptom. The fetch itself is the problem:
|
||||
|
||||
- **Failure is invisible and late.** A dead, slow, oversized or non-image URL surfaces as a log line
|
||||
at render time. The operator who typed the URL is long gone.
|
||||
- **No preview.** The editor cannot show the bug for an external URL, so the operator cannot tell
|
||||
whether it will work until a stream runs.
|
||||
- **Repeated work.** The same image is re-fetched on every playout item transition.
|
||||
- **Third-party dependency inside stream startup.** A logo host having a bad day degrades tuning.
|
||||
|
||||
## Goal
|
||||
|
||||
**An external logo URL becomes an input method, not a storage format.** Entering a URL downloads the
|
||||
image once, at save time, into the existing artwork cache — after which it is indistinguishable from
|
||||
an uploaded logo. Nothing downstream knows the logo ever came from a URL.
|
||||
|
||||
To refresh a changed image, the operator re-enters the URL. There is no refresh button and no
|
||||
staleness tracking; that is a deliberate simplification, not an oversight.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- **No refresh button, no TTL, no ETag/Last-Modified tracking.** Re-add the URL.
|
||||
- **No change to `ImageGraphicsElement`** (operator-authored YAML `image:`), which may still point at
|
||||
a URL and still fetches at render time through the hardened `IRemoteImageFetcher`. Removing that is
|
||||
an unrelated feature removal.
|
||||
- **No change to the generated-initials fallback** (#1) or the deco path (#510).
|
||||
- **No new artwork storage mechanism.** Reuses `IImageCache` exactly as the upload path does.
|
||||
|
||||
## Design
|
||||
|
||||
### Save path
|
||||
|
||||
All three handlers that persist a channel logo share one code path today and will share the new one:
|
||||
|
||||
| Handler | Current logo logic |
|
||||
|---|---|
|
||||
| `UpdateChannelHandler.ApplyUpdateRequest` | `UpdateChannelHandler.cs:79-123` |
|
||||
| `CreateChannelHandler` | `CreateChannelHandler.cs:63-65` |
|
||||
| `CreateChannelFromLineupHandler` | `CreateChannelFromLineupHandler.cs:360-362` |
|
||||
|
||||
New behavior when the incoming logo path is an absolute `http(s)` URL:
|
||||
|
||||
1. Fetch it with **`IRemoteImageFetcher`** — the primitive #511 already built and hardened (bounded
|
||||
deadline covering headers and body, 10 MiB wire cap, 3 redirects, content-type check, pooled
|
||||
client).
|
||||
2. **Validate the decode budgets** against the downloaded bytes (see *Decode validation* below).
|
||||
3. `IImageCache.SaveArtworkToCache(stream, ArtworkKind.Logo)` → an opaque content-hash name.
|
||||
4. Store that name in `Artwork.Path`, stamp `DateAdded`/`DateUpdated`, exactly as the upload path does.
|
||||
|
||||
Any failure **rejects the save** with a validation error naming the cause. The channel is not
|
||||
persisted and the field stays editable.
|
||||
|
||||
### Naming: content hash, not GUID
|
||||
|
||||
The request was "a random name/guid". This design uses the **existing content hash** that
|
||||
`SaveArtworkToCache` already returns (MD5 of the bytes, stored as `{hash}` with the file at
|
||||
`{LogoCacheFolder}/{hash[..2]}/{hash}`).
|
||||
|
||||
Rationale — it satisfies the intent (opaque, generated, not the URL) while being *strictly better*
|
||||
than a GUID here:
|
||||
|
||||
- It is byte-for-byte the same mechanism as an uploaded logo, so there is one storage convention
|
||||
rather than two.
|
||||
- Re-adding an **unchanged** URL is a natural no-op (same bytes → same hash → same file).
|
||||
- Re-adding a **changed** URL naturally produces a new name, which is exactly the refresh semantic.
|
||||
|
||||
A GUID would deviate from the established convention for no benefit, which the deviation policy in
|
||||
`docs/contributing.md` §10 asks us not to do.
|
||||
|
||||
### Downstream consumers: no code change
|
||||
|
||||
Because `Artwork.Path` now holds a cache name, every consumer already does the right thing:
|
||||
|
||||
| Consumer | Result |
|
||||
|---|---|
|
||||
| M3U (`ChannelPlaylist.cs:63-70`) | `{scheme}://{host}{baseUrl}/iptv/logos/{hash}.jpg` |
|
||||
| XMLTV (`RefreshChannelListHandler.cs:85-95`, `_channel.sbntxt:29-35`) | `{RequestBase}/iptv/logos/{hash}.jpg` |
|
||||
| SPA/API mapper (`Channels/Mapper.cs:129-166`) | `iptv/logos/{hash}` |
|
||||
| Render (`WatermarkSelector.cs:301-325`) | resolves via `imageCache.GetPathForImage`, existence-checked |
|
||||
|
||||
This is the intended outcome: clients stop depending on the third-party host, and the
|
||||
`IsExternalUrl` branches in those consumers become unreachable *for channel logos*. Those branches
|
||||
are **left in place** — `Artwork` is shared with other artwork kinds and with rows that failed
|
||||
migration.
|
||||
|
||||
### Render path
|
||||
|
||||
`WatermarkSelector.ChannelLogoWatermarkOptions` stops treating a URL as renderable. For a logo path
|
||||
that is still a URL (only possible for a row that failed migration), it logs a warning naming the
|
||||
channel and returns `None` — no fetch, no bug, stream unaffected.
|
||||
|
||||
`ImageElementBase.LoadImage` keeps its remote branch for `ImageGraphicsElement`. #511's fetch and
|
||||
decode budgets stay exactly as merged.
|
||||
|
||||
### Decode validation (important)
|
||||
|
||||
Today `ImageElementBase` exempts **local** images from the decode budgets, on the reasoning that a
|
||||
local file is something an operator put on disk rather than bytes an arbitrary host returned. This
|
||||
design invalidates that reasoning for logos: a downloaded URL *becomes* a local file, so without a
|
||||
check at save time the decode bomb simply relocates from the render path to the cache.
|
||||
|
||||
Therefore the save path must validate before caching:
|
||||
|
||||
- Reuse #511's budgets — dimensions, `width × height × frames ≤ 50 MP`, `≤ 600` frames — enforced the
|
||||
same way (`DecoderOptions.MaxFrames` + post-decode re-verification against the decoded image,
|
||||
because header frame counts lie).
|
||||
- To share them, extract the budget helpers currently on `ImageElementBase`
|
||||
(`EnsureDimensionsAffordable`, `AffordableFrames`, `EnsureDecodeAffordable`) into a single reusable
|
||||
component. Proposed: `ErsatzTV.Core/Images/RemoteImageDecodeBudget.cs`, with `ImageElementBase` and
|
||||
the save path both calling it. The retention budget (`EnsureScaledFramesAffordable`) stays in
|
||||
`ImageElementBase` — it depends on render-time scale and has no meaning at save time.
|
||||
|
||||
**Pre-existing hole, flagged for a decision:** direct **uploads** (`UploadArtworkHandler`) are not
|
||||
budget-checked either. Once URL logos become uploads, they inherit that gap on any subsequent
|
||||
re-upload. Applying the same validation to `UploadArtworkHandler` is a small addition using the same
|
||||
component and is *recommended*, but it is a scope increase and is called out rather than assumed.
|
||||
|
||||
### Migration of existing rows
|
||||
|
||||
A one-time migration walks `Artwork` rows whose `Path` is an absolute `http(s)` URL and whose kind is
|
||||
`Logo`:
|
||||
|
||||
- fetch through the same hardened fetcher → validate → `SaveArtworkToCache` → rewrite `Path`, bump
|
||||
`DateUpdated`;
|
||||
- **on failure, leave the row untouched** and log a warning naming the channel and the reason, so the
|
||||
operator gets an actionable list rather than silent breakage.
|
||||
|
||||
Run as a **startup task**, not an EF migration: it performs network I/O and must be resilient and
|
||||
restartable, which does not belong in a schema migration (and would have to be written twice for
|
||||
SQLite and MySql). It follows the existing precedent of `LocalFolderScanner.RefreshArtwork`
|
||||
(`LocalFolderScanner.cs:132-200`), which already does fetch → `*ArtworkToCache` → persist.
|
||||
|
||||
Idempotent by construction: after a successful pass the row's `Path` is no longer a URL, so it is not
|
||||
selected again.
|
||||
|
||||
### Editor UX
|
||||
|
||||
- Save is **synchronous**: the `PUT` performs the download and returns `400` with a specific message
|
||||
on failure (e.g. *"Could not download logo: host did not respond within 10s"*, *"Logo is 41 MB;
|
||||
the limit is 10 MB"*, *"URL returned text/html, not an image"*). Worst case latency is the fetch
|
||||
deadline.
|
||||
- On success the response carries a normal cached logo, so **the preview works with no special
|
||||
casing** — the `&& !externalUrlLogo` suppression at `ChannelEditScreen.tsx:817` is deleted, as is
|
||||
the help text claiming external URLs cannot drive the bug.
|
||||
- The external-URL field is an *input*: after a successful save it clears and the uploaded-logo
|
||||
preview shows the cached image. The existing mutual-exclusion logic
|
||||
(`ChannelEditScreen.tsx:119-123`) is simplified accordingly — the two fields can no longer disagree
|
||||
because only one storage form now exists.
|
||||
- Help text states that changing the remote image requires re-entering the URL.
|
||||
|
||||
## Error handling
|
||||
|
||||
| Case | Behavior |
|
||||
|---|---|
|
||||
| Host unreachable / times out | Save rejected, message names the timeout |
|
||||
| Non-2xx | Save rejected, message names the status |
|
||||
| Not an image content type | Save rejected |
|
||||
| Over the wire cap | Save rejected, message names actual vs limit |
|
||||
| Over a decode budget | Save rejected, message names dimensions/frames vs limit |
|
||||
| Cache write fails | Save rejected, `BaseError` surfaced |
|
||||
| Migration failure | Row untouched, warning logged, channel keeps rendering without a bug |
|
||||
|
||||
## Testing
|
||||
|
||||
- **Handler tests** (`ErsatzTV.Tests`): URL → fetch → cache → `Artwork.Path` is the hash; each failure
|
||||
mode rejects the save and persists nothing; a non-URL path is unchanged; re-adding identical bytes
|
||||
is a no-op.
|
||||
- **Decode budget tests**: move/extend the existing `RemoteImageDecodeLimitTests`, keeping the APNG
|
||||
regression coverage (a default `Identify` throws on most APNGs; header frame counts lie).
|
||||
- **Migration tests**: URL row is converted; failing row is left intact and warned about; a second run
|
||||
is a no-op.
|
||||
- **`WatermarkSelector` tests**: extend `WatermarkSelectorChannelLogoTests` — a cached path renders; a
|
||||
leftover URL path returns `None` with a warning and never fetches.
|
||||
- **SPA tests**: `ChannelEditScreen.test.tsx` — preview renders after a URL save; the removed
|
||||
suppression is not reintroduced; error surfaces inline on a rejected save.
|
||||
- **Golden nets**: `ChannelPlaylistGoldenTests` / `ChannelGuideGoldenTests` should be *unchanged* for
|
||||
uploaded logos, and a channel whose logo came from a URL should now emit an `/iptv/logos/` URL.
|
||||
|
||||
## Docs to update in the same PR
|
||||
|
||||
- `docs/decisions.md` — new entry; explicitly supersedes the "not cached, re-fetched per element
|
||||
init" paragraph of the #511 entry and narrows #502's "external artwork passes through" to the
|
||||
client-facing consumers it still describes.
|
||||
- `docs/channels.md` — replace the stale limitation text (this supersedes PR #522, which should be
|
||||
closed unmerged).
|
||||
- `docs/api-conventions.md` — `PUT /api/v1/channels/{id}` can now fail on logo download; note the new
|
||||
400 cases. Regenerate `v1.json` + `endpoint-index.md` if any response shape changes.
|
||||
|
||||
## Out of scope / follow-ups
|
||||
|
||||
- `ArtworkController.RedirectArtwork` (`ArtworkController.cs:37-57`) builds `"/iptv/logos/" + Path`
|
||||
unconditionally, producing a malformed redirect when `Path` is a URL. Pre-existing, unrelated to
|
||||
this change, and largely mooted by it for logos — **file separately**.
|
||||
- Applying decode budgets to `UploadArtworkHandler` (see *Decode validation*).
|
||||
- #1 (generated-initials `localhost`) and #510 (deco path) remain untouched.
|
||||
|
||||
## Open questions
|
||||
|
||||
1. Should `UploadArtworkHandler` get the same decode validation in this PR, or a follow-up?
|
||||
2. Should `IRemoteImageFetcher` move from `Core/Interfaces/Streaming/` to `Core/Interfaces/Images/`
|
||||
now that its primary consumer is the save path rather than the streaming path? Cosmetic; a
|
||||
namespace move only.
|
||||
Reference in New Issue
Block a user