ersatztv/docs/superpowers/specs/2026-07-21-external-logo-download-on-save-design.md

# 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.