# 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. **Uploads are budget-checked too (decided: fold in).** Direct **uploads** (`UploadArtworkHandler`) are not budget-checked today. Once URL logos become uploads, they inherit that gap on any subsequent re-upload — an inconsistency this change would *create* (same bytes, same cache, enforcement depending only on arrival path). The `RemoteImageDecodeBudget` component is being built regardless, so `UploadArtworkHandler` calls it too. One consistent rule: **anything entering the logo cache is budget-checked, however it arrived.** A budget failure returns a `400` from the upload endpoint the same way it does from the channel save. (Risk is admin-only, like #511's SSRF stance, but the failure mode — cache succeeds, render OOMs concurrent streams later — is exactly the fail-late pattern this redesign exists to kill, so it is closed here rather than deferred.) ### 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**. - #1 (generated-initials `localhost`) and #510 (deco path) remain untouched. ## Resolved decisions 1. **`UploadArtworkHandler` decode validation is folded into this PR**, not deferred — see *Decode validation*. The component exists either way and the inconsistency is created by this change. 2. **`IRemoteImageFetcher` stays in `Core/Interfaces/Streaming/`.** It is still used by the streaming path (YAML image elements), and a namespace move is churn against `git blame` for weak naming-accuracy benefit. Trivial standalone rename if ever wanted.