From c81ae28da203356bc622f7a9e3ef1bc8d3e31803 Mon Sep 17 00:00:00 2001 From: mARTin-B78 Date: Mon, 14 Sep 2026 17:39:57 +0200 Subject: [PATCH] feat(magazine): add figure/chart/table description generation and spoken narration Adds a "Describe" action per figure in article review: sends the cropped image to the configured vision engine (with fallback) for a type-aware one- or two-sentence description -- a chart gets a prompt about what it measures and its trend, a table about headings and what stands out, etc. Editable before approval. A new "Read figure descriptions" toggle in the reading controls speaks these where they occur; purely additive, off by default, and a figure with no description behaves exactly as before. New core/magazine.py describe_visual_region()/_chain() reuse the same reliability guards as page OCR (repetition-loop detection, sentence- boundary truncation). New routes: POST .../images/{id}/describe (background job), PATCH .../images/{id} (manual edit), descriptions=1 on the article-text endpoint. Live-verified end to end against the ingested c't KI Wissen 2026 magazine. Also found and documented a real, config-dependent limitation: the currently-configured primary vision engine is transcription-only and returns empty content for any descriptive prompt (confirmed with three different prompts) -- not a code defect, but description generation needs a genuinely caption-capable engine reachable as the fallback. 251 tests passing. Co-Authored-By: Claude Sonnet 5 --- CHANGELOG.md | 12 +++ Manual.md | 26 ++++- PROJECT_MAP.md | 14 ++- TODO.md | 9 +- VERSION | 2 +- core/magazine.py | 161 ++++++++++++++++++++++++++++++- routes/magazine.py | 112 +++++++++++++++++++++- static/js/magazine.js | 82 +++++++++++++++- static/sections/s-magazine.html | 4 + static/style.css | 14 +++ tests/test_magazine.py | 148 ++++++++++++++++++++++++++++ tests/test_magazine_routes.py | 164 ++++++++++++++++++++++++++++++++ 12 files changed, 733 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01e6c7a..5255de0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,18 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi --- +## [1.29.0] — 2026-09-14 + +### Added +- **Magazine figure descriptions.** In article review, "Describe" under a figure/chart/table/code block sends it to the configured vision engine (with fallback) for a one- or two-sentence description in plain language, type-aware (a chart gets a "what it measures and its trend" prompt, a table gets "headings and what stands out," etc.). Editable before approval. A new "Read figure descriptions" toggle in the reading controls speaks generated descriptions where they occur in the article; purely additive — off by default, and a figure with no generated description behaves exactly as before. +- New `core/magazine.py` functions `describe_visual_region()`/`describe_visual_region_chain()` (same reliability guards as page OCR: repetition-loop detection, sentence-boundary truncation), `POST /api/magazine/{doc}/images/{id}/describe` (background job) and `PATCH /api/magazine/{doc}/images/{id}` (manual edit) routes, `descriptions=1` query param on the article-text endpoint. + +### Verified +- Live end-to-end against the ingested c't KI Wissen 2026 magazine (a real chart, article-008): ingest → Describe → persisted in layout.json → appears as a `figure_description` block in article text only when requested. +- Found and documented a real config-dependent limitation, not a code defect: the currently-configured primary vision engine (baidu/Unlimited-OCR) is transcription-only — it returns empty content for any descriptive prompt regardless of phrasing (confirmed with three different prompts) — so description generation currently depends on a caption-capable fallback engine being configured and running. See Manual.md's Errors table and PROJECT_MAP.md's Known limitations. + +--- + ## [1.28.2] — 2026-09-14 ### Fixed diff --git a/Manual.md b/Manual.md index bb81d82..6649c86 100644 --- a/Manual.md +++ b/Manual.md @@ -63,13 +63,25 @@ turned into a podcast discussion between two or three voices you pick. engine missed a region entirely, **"Draw it"** lets you click-drag a box directly on the page image and add it as text (typed or OCRed) or as a new figure. Confirm the boundaries, correct the title, then **Approve**. + Optionally, click **"Describe"** under a figure, chart, table or code block + to have the configured vision engine generate a one- or two-sentence + description for listeners who cannot see the image — e.g. what a chart + measures and its general trend, or a table's row/column headings and + what stands out. You can edit the generated text, or write your own from + scratch, before approving the article. Generating a description changes + nothing about existing narration by itself — it is only spoken if you + also turn on "Read figure descriptions" in step 4. 4. **Listen.** An approved article can be narrated with the default voice, or sent to podcast mode. Use **Play all articles** (on the contents screen) to listen through every approved article back to back without returning to this screen between them. A figure from the page is shown automatically as playback reaches it. **Word highlighting** can be turned off if you find it distracting — it is an estimate based on word length, not a measurement, so - it will not always land exactly on the spoken word. **Driving mode** + it will not always land exactly on the spoken word. Turn on **"Read figure + descriptions"** in the reading controls to have any figure descriptions + generated during review spoken aloud where they occur in the article; + figures without a generated description are silently skipped, exactly as + before this option existed. **Driving mode** (the button next to the article title) switches to two large controls and nothing else, for listening without looking at the screen. @@ -113,14 +125,18 @@ unreviewed boundaries may include a neighbouring article or an advert. | "Article has too little text" | Under 200 characters — usually a stub. | Check the boundaries in review. | | A figure box sits over the wrong part of the page | The OCR engine grounded the figure loosely. | Press **"Fix figure boundaries"**; if the graphic is too pale to detect, draw the region by hand. | | "Fix figure boundaries" reports nothing corrected | Every box already matched its page, or none could be placed with confidence. | Nothing is wrong; correct any remaining box by drawing it. | +| "No vision OCR engine configured" (Describe) | `vision_ocr_url`/`vision_ocr_model` are empty. | Set a vision engine in Settings. | +| "The vision engine returned nothing usable for this figure" | The configured engine (and fallback, if set) returned an empty or degenerate response for this image. Some vision models are transcription-only and cannot produce open-ended descriptions at all, however the prompt is phrased — confirmed with a pure-OCR model that answers its own "Text Recognition:" prompt but returns nothing for any descriptive one. | Configure a genuinely caption-capable vision model as the fallback (`vision_ocr_fallback_url`/`vision_ocr_fallback_model`), or write the description by hand. | ### Privacy Ingestion, OCR and storage are entirely local to the container. Podcast mode is -the exception: **the article's text is sent to whatever LLM `llm_url` points -at.** If that is a hosted API rather than a model on your own network, the -article leaves your machine. Narration uses your local TTS stack and sends -nothing outward. +one exception: **the article's text is sent to whatever LLM `llm_url` points +at.** Generating a figure description is the other: **the cropped image is +sent to whatever `vision_ocr_url` (and, on failure, `vision_ocr_fallback_url`) +points at.** If either is a hosted API rather than a model on your own +network, that content leaves your machine. Narration uses your local TTS +stack and sends nothing outward. ### Accessibility and mobile diff --git a/PROJECT_MAP.md b/PROJECT_MAP.md index 8f5a659..2329061 100644 --- a/PROJECT_MAP.md +++ b/PROJECT_MAP.md @@ -90,6 +90,13 @@ which is the writable volume. real measured audio duration, not true forced alignment. - No formal cross-engine OCR accuracy benchmark exists (two engines wired: ocr-baidu-unlimited-ocr primary, KDL Frontier Parser nano fallback). +- Figure description generation (Magazine review, "Describe") needs a + genuinely caption-capable vision model — confirmed live that the primary + engine (baidu/Unlimited-OCR) is transcription-only and returns empty + content for any descriptive prompt, regardless of phrasing (three prompts + tried). Falls through to `vision_ocr_fallback_url`/`_model`, which worked + correctly once its container was started — that container + (`ocr-kdl-frontier-parser-nano`) is not running by default. - Figure boundaries are re-derived from the page render (`snap_visual_bboxes`) because vision OCR grounds figures loosely; it corrects roughly 100 of the 147 figure regions on `c't KI Wissen 2026` and deliberately declines the @@ -108,6 +115,7 @@ which is the writable volume. ## Next logical step -Magazine P2 per TODO.md: image descriptions, table/chart handling, EPUB 3 -Media Overlay export, and a formal OCR-engine benchmark (see -architecture/magazine-ingestion.md). +Magazine P2 per TODO.md: image descriptions shipped; remaining — table/chart +handling depth, EPUB 3 Media Overlay export, and a formal OCR-engine +benchmark (see architecture/magazine-ingestion.md). Then Character system, +then Audiobook speaker attribution, per the user's stated priority order. diff --git a/TODO.md b/TODO.md index 17f87b6..f1e6ea8 100644 --- a/TODO.md +++ b/TODO.md @@ -400,7 +400,14 @@ casting. The original PDF remains the visual source of truth while approved arti - [~] Improve cross-page continuation detection and automatic sidebar/advertisement exclusion. *(sidebar exclusion done via mark_sidebars, a narrow-column geometry heuristic; cross-page continuation still only uses the forward "continued on page N" reference, not "continued from")* -- [ ] Add image descriptions, chart/table handling, and optional spoken figure summaries. +- [x] Add image descriptions, chart/table handling, and optional spoken figure summaries. + *(per-figure "Describe" in review generates a type-aware description — chart/table/code/figure each get their + own prompt — via the configured vision engine with fallback; editable before approval; spoken only if "Read + figure descriptions" is enabled. Live-verified against a real chart: the configured primary vision engine + turned out to be a pure-OCR model that returns empty content for any non-transcription prompt regardless of + phrasing — confirmed with three different prompts — so description generation only works when a genuinely + caption-capable model is reachable as the fallback; documented as a config-dependent limitation in Manual.md, + not a code defect.)* - [~] Add word-level karaoke timing and EPUB 3 Media Overlay export. *(word-level highlighting done as a character-length estimate against the real measured audio duration, not true forced alignment; no EPUB 3 export)* diff --git a/VERSION b/VERSION index d079232..72f3c1d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.28.2 \ No newline at end of file +1.29.0 \ No newline at end of file diff --git a/core/magazine.py b/core/magazine.py index 4552088..36193a8 100644 --- a/core/magazine.py +++ b/core/magazine.py @@ -1140,6 +1140,140 @@ def vision_ocr_page_chain(png_path: Path, return None +# One line per visual type -- a chart needs "what trend", a table needs +# "what's listed", not the generic "what's shown" that suits a photo. Every +# variant still asks for one or two plain sentences: this is read aloud +# between the surrounding paragraphs, not a caption someone reads at their +# own pace, so it has to stay short regardless of how much detail the image +# could support. +_DESCRIBE_PROMPTS = { + BLOCK_CHART: ( + "This image is a chart or graph from a magazine article. In one or " + "two plain sentences, for a listener who cannot see it, describe " + "what kind of chart it is, what it measures, and the general trend " + "or most notable values. Do not invent a number you cannot actually " + "read in the image. No markdown, no preamble -- the description " + "text only." + ), + BLOCK_TABLE: ( + "This image is a table from a magazine article. In one or two " + "plain sentences, for a listener who cannot see it, summarise what " + "it lists -- do not read out individual cells. No markdown, no " + "preamble -- the description text only." + ), + BLOCK_CODE: ( + "This image is a code listing or code box from a magazine article. " + "In one plain sentence, for a listener who cannot see it, describe " + "what it appears to show (the language and general purpose) -- do " + "not read the code line by line. No markdown, no preamble -- the " + "description text only." + ), + BLOCK_FIGURE: ( + "This image is a photo or illustration from a magazine article. In " + "one or two plain sentences, for a listener who cannot see it, " + "describe what it shows, including any text visible in the image " + "itself. No markdown, no preamble -- the description text only." + ), +} + + +def describe_visual_region(image_path: Path, region_type: str, base_url: str, + model: str, timeout: float = 90.0) -> str | None: + """Describe one already-cropped figure/chart/table/code image in plain + language, for optional inclusion in an article's spoken narration. + + A deliberately different task from vision_ocr_page(): that prompt + explicitly forbids inventing a description ("Images, figures, and + charts may have empty transcription; do not invent descriptions") -- + correct for transcription, wrong here, where a description is exactly + what is wanted. Reuses the same engine (already vision-capable and + already configured) with its own prompt instead, and the same + reliability guards -- a repeated-phrase decoding loop is exactly as + possible describing a busy chart as transcribing one. + + Returns None on any failure (unreachable engine, timeout, empty or + degenerate response) so the caller can try a fallback engine or leave + the figure without a description -- never partway through one. + """ + try: + image_b64 = base64.b64encode(image_path.read_bytes()).decode("ascii") + except OSError: + return None + + base_url = base_url.strip().rstrip("/") + if not urlsplit(base_url).path: + base_url += "/v1" + + prompt = _DESCRIBE_PROMPTS.get(region_type, _DESCRIBE_PROMPTS[BLOCK_FIGURE]) + unlimited_ocr = "unlimited-ocr" in model.lower() + payload = { + "model": model, + "messages": [{"role": "user", "content": [ + {"type": "text", "text": prompt}, + {"type": "image_url", "image_url": { + "url": f"data:image/png;base64,{image_b64}", + }}, + ]}], + "max_tokens": 300, + "temperature": 0.2, + } + if unlimited_ocr: + # Same anti-repetition settings as vision_ocr_page -- same model, + # same decoding-loop risk, regardless of the prompt asked of it. + payload.update({ + "skip_special_tokens": False, + "ngram_size": 35, + "window_size": 128, + "images_config": {"image_mode": "gundam"}, + }) + try: + resp = request_with_retries( + "POST", f"{base_url.rstrip('/')}/chat/completions", + json=payload, timeout=timeout, attempts=2, + ) + resp.raise_for_status() + content = resp.json()["choices"][0]["message"]["content"] + except requests.exceptions.RequestException as exc: + status = getattr(getattr(exc, "response", None), "status_code", None) + logging.getLogger(__name__).warning("Figure description request failed (%s%s)", + type(exc).__name__, f", HTTP {status}" if status else "") + return None + except (ValueError, KeyError, IndexError, TypeError): + logging.getLogger(__name__).warning("Figure description returned an invalid completion response") + return None + if not isinstance(content, str): + return None + text = content.strip().strip("`").strip() + if not text or len(text) < 8: + return None + if _looks_like_repetition_loop(text): + logging.getLogger(__name__).warning("Figure description returned a repeated phrase loop; discarding response") + return None + # A long response is either the model ignoring "one or two sentences" or + # a loop this pass's own repetition check didn't happen to catch -- + # either way, narration wants a short aside, not a paragraph. Cut at a + # sentence boundary rather than mid-word. + limit = 600 + if len(text) > limit: + cut = text.rfind(". ", 0, limit) + text = text[:cut + 1] if cut > limit // 2 else text[:limit].rstrip() + "…" + return text + + +def describe_visual_region_chain(image_path: Path, region_type: str, + engines: Sequence[tuple[str, str]], + timeout: float = 90.0) -> str | None: + """Try each configured vision engine in order, first usable description + wins -- same shape as vision_ocr_page_chain.""" + for url, model in engines: + if not (url and model): + continue + text = describe_visual_region(image_path, region_type, url, model, timeout=timeout) + if text: + return text + return None + + def build_vision_blocks(regions: list[dict[str, Any]], page_no: int, scale_x: float, id_prefix: str) -> list[Block]: """Turn one page's vision OCR regions into Blocks. @@ -2438,12 +2572,26 @@ def load_articles(doc_dir: Path) -> list[dict[str, Any]]: return [] +BLOCK_FIGURE_DESCRIPTION = "figure_description" + + def article_text(layout: dict[str, Any], article: dict[str, Any], - include_captions: bool = False) -> list[dict[str, Any]]: + include_captions: bool = False, + include_descriptions: bool = False) -> list[dict[str, Any]]: """The narratable text of one article, in reading order. Returned per block so the player can highlight the source region and jump to the originating PDF page. + + A figure/chart/table/code region has no text block of its own to + narrate -- see describe_visual_region() -- so an already-described + image belonging to this article is synthesised into a + BLOCK_FIGURE_DESCRIPTION entry at that image's own reading position when + `include_descriptions` is set. Optional and off by default: generating + a description costs a vision-model call the user may not have wanted + for every figure, and a description read aloud mid-article changes the + listening experience enough that it should be an explicit choice, not a + silent side effect of one being available. """ wanted = set(article.get("blocks") or []) types = set(NARRATABLE) | ({BLOCK_CAPTION} if include_captions else set()) @@ -2452,5 +2600,16 @@ def article_text(layout: dict[str, Any], article: dict[str, Any], for b in layout.get("blocks", []) if b["id"] in wanted and b["type"] in types ] + if include_descriptions: + wanted_images = set(article.get("images") or []) + for img in layout.get("images", []): + description = (img.get("description") or "").strip() + if img["id"] not in wanted_images or not description: + continue + blocks.append({ + "id": f"desc-{img['id']}", "page": img["page"], "bbox": img["bbox"], + "type": BLOCK_FIGURE_DESCRIPTION, "text": description, + "reading_order": img.get("reading_order", 0), + }) blocks.sort(key=lambda b: (b["page"], b["reading_order"])) return blocks diff --git a/routes/magazine.py b/routes/magazine.py index 27e7abb..acf6f46 100644 --- a/routes/magazine.py +++ b/routes/magazine.py @@ -422,7 +422,7 @@ async def magazine_update_article(doc_id: str, article_id: str, request: Request @router.get("/api/magazine/{doc_id}/articles/{article_id}/text") async def magazine_article_text(doc_id: str, article_id: str, - captions: int = 0): + captions: int = 0, descriptions: int = 0): """The narratable text of one article, per block, in reading order.""" d, _ = _require_magazine(doc_id) article_id = _article_id(article_id) @@ -431,7 +431,8 @@ async def magazine_article_text(doc_id: str, article_id: str, if target is None: raise HTTPException(404, "Article not found") blocks = mag.article_text(mag.load_layout(d), target, - include_captions=bool(captions)) + include_captions=bool(captions), + include_descriptions=bool(descriptions)) return {"article": target, "blocks": blocks} @@ -842,6 +843,113 @@ async def magazine_image(doc_id: str, name: str): return FileResponse(path, media_type="image/png") +# ── Figure descriptions ─────────────────────────────────────────────────────── + +def _image_id(image_id: str) -> str: + if not re.fullmatch(r"[A-Za-z0-9_-]{1,64}", image_id or ""): + raise HTTPException(400, "Bad image id") + return image_id + + +def _describe_image_sync(doc_id: str, job_id: str, image_id: str, + vision_ocr_url: str, vision_ocr_model: str, + vision_ocr_fallback_url: str | None, + vision_ocr_fallback_model: str | None) -> None: + """Describe one figure/chart/table/code image, for optional inclusion in + the article's spoken narration (see article_text's include_descriptions). + Restart-safe for the same reason "OCR again" is: a vision-model call can + take tens of seconds. + """ + d = _LIB / doc_id + try: + layout = mag.load_layout(d) + if not layout: + raise FileNotFoundError("No layout yet -- run ingestion first") + image = next((i for i in layout.get("images", []) if i["id"] == image_id), None) + if image is None: + raise FileNotFoundError("Image not found") + png = d / "images" / str(image.get("filename") or f"{image_id}.png") + if not png.is_file(): + raise FileNotFoundError("Figure image not found on disk") + background_job_update(job_id, stage="describe", detail="Describing figure") + engines = [(vision_ocr_url, vision_ocr_model), + (vision_ocr_fallback_url, vision_ocr_fallback_model)] + text = mag.describe_visual_region_chain(png, image.get("type", mag.BLOCK_FIGURE), engines) + if not text: + raise RuntimeError("The vision engine returned nothing usable for this figure") + image["description"] = text + mag.save_layout(d, layout) + background_job_update(job_id, stage="done", done=True, + detail="Figure described", result={"image_id": image_id, "description": text}) + except Exception as exc: # noqa: BLE001 - surfaced to UI + background_job_update(job_id, stage="failed", done=True, error=str(exc)[:1000]) + + +@router.post("/api/magazine/{doc_id}/images/{image_id}/describe") +async def magazine_describe_image(doc_id: str, image_id: str, request: Request): + """Describe one figure/chart/table/code image with the configured vision + engine, in plain language for a listener who cannot see it. Purely + additive: the description is stored alongside the image and only ever + spoken if the reader explicitly turns on "Read figure descriptions" -- + generating one changes nothing about existing narration on its own. + """ + d, _ = _require_magazine(doc_id) + image_id = _image_id(image_id) + layout = mag.load_layout(d) + if not layout or not any(i["id"] == image_id for i in layout.get("images", [])): + raise HTTPException(404, "Image not found") + + body = {} + try: + body = await request.json() + except Exception: + pass + settings = _load_settings() + vision_ocr_url = str(body.get("vision_ocr_url") or settings.get("vision_ocr_url") or "").strip() + vision_ocr_model = str(body.get("vision_ocr_model") or settings.get("vision_ocr_model") or "").strip() + if not (vision_ocr_url and vision_ocr_model): + raise HTTPException(400, "No vision OCR engine configured -- set one in Settings") + vision_ocr_fallback_url = str( + body.get("vision_ocr_fallback_url") or settings.get("vision_ocr_fallback_url") or "").strip() or None + vision_ocr_fallback_model = str( + body.get("vision_ocr_fallback_model") or settings.get("vision_ocr_fallback_model") or "").strip() or None + + job_id = uuid.uuid4().hex + background_job_create(job_id, "magazine-describe-image", fingerprint=f"{doc_id}:{image_id}") + asyncio.create_task( + asyncio.to_thread(_describe_image_sync, doc_id, job_id, image_id, + vision_ocr_url, vision_ocr_model, + vision_ocr_fallback_url, vision_ocr_fallback_model) + ) + return {"job_id": job_id} + + +@router.patch("/api/magazine/{doc_id}/images/{image_id}") +async def magazine_update_image_description(doc_id: str, image_id: str, request: Request): + """Set or clear a figure's description by hand -- trusted verbatim, the + same as any other text the user types directly (see add_manual_region). + Not for correcting a bad boundary, only the spoken description text. + """ + d, _ = _require_magazine(doc_id) + image_id = _image_id(image_id) + layout = mag.load_layout(d) + if not layout: + raise HTTPException(404, "Document not ingested yet") + image = next((i for i in layout.get("images", []) if i["id"] == image_id), None) + if image is None: + raise HTTPException(404, "Image not found") + try: + body = await request.json() + except Exception: + raise HTTPException(400, "Invalid JSON body") + if "description" not in body: + raise HTTPException(400, "Nothing to update -- expected a 'description' field") + description = str(body.get("description") or "").strip()[:2000] + image["description"] = description + mag.save_layout(d, layout) + return {"image_id": image_id, "description": description} + + # ── Podcast mode ───────────────────────────────────────────────────────────── _PODCAST_SYSTEM = ( diff --git a/static/js/magazine.js b/static/js/magazine.js index 92f6481..8185918 100644 --- a/static/js/magazine.js +++ b/static/js/magazine.js @@ -822,6 +822,7 @@ function renderReview() { '' + (owner ? 'Belongs to Article ' + owner.index + ': ' + esc(owner.title) : on ? 'Shown during playback for this article.' : 'Not linked to any article on this page.') + '' + '' + + magDescribeMarkup(r.id, r.data.description) + ''; return; } @@ -878,6 +879,79 @@ function magReviewToggleImage(id, checked) { } } +// ── Figure descriptions ────────────────────────────────────────────────── +// Purely additive: generating or editing a description never changes +// whether the image itself is shown or included (see magReviewToggleImage +// above) -- it only supplies the text "Read figure descriptions" narrates +// if the reader turns that on. Never spoken by itself. +function magDescribeMarkup(id, description) { + const safeId = esc(id); + const has = !!(description || '').trim(); + return '
' + + '' + + '' + + '' + + '' + + '
'; +} + +async function magDescribeImage(id) { + const btn = $('mag-describe-btn-' + id); + const statusEl = $('mag-describe-status-' + id); + if (btn) btn.disabled = true; + if (statusEl) statusEl.textContent = 'Describing…'; + let jobId; + try { + const r = await api('/api/magazine/' + state.doc.id + '/images/' + id + '/describe', { method: 'POST' }); + jobId = r.job_id; + } catch (e) { + if (btn) btn.disabled = false; + if (statusEl) statusEl.textContent = ''; + return say('Could not start: ' + e.message, 'err'); + } + let result = null, failed = false; + for (;;) { + let job; + try { job = await api('/api/jobs/' + jobId); } catch (_) { break; } + if (job.done) { + if (job.error) { failed = true; say('Description failed: ' + job.error, 'err'); } + else { result = job.result; say('Figure described', 'ok'); } + break; + } + await new Promise((r) => setTimeout(r, 2000)); + } + if (btn) { btn.disabled = false; btn.innerHTML = ' Redo description'; } + if (statusEl) statusEl.textContent = ''; + if (failed || !result) return; + const textarea = $('mag-describe-text-' + id); + if (textarea) textarea.value = result.description || ''; + const layoutImg = (state.layout?.images || []).find((i) => i.id === id); + if (layoutImg) layoutImg.description = result.description || ''; +} + +async function magSaveImageDescription(id, textarea) { + const statusEl = $('mag-describe-status-' + id); + if (statusEl) statusEl.textContent = 'Saving…'; + try { + await api('/api/magazine/' + state.doc.id + '/images/' + id, { + method: 'PATCH', headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ description: textarea.value }), + }); + const layoutImg = (state.layout?.images || []).find((i) => i.id === id); + if (layoutImg) layoutImg.description = textarea.value; + if (statusEl) statusEl.textContent = 'Saved'; + setTimeout(() => { if (statusEl) statusEl.textContent = ''; }, 1500); + } catch (e) { + if (statusEl) statusEl.textContent = ''; + say('Could not save description: ' + e.message, 'err'); + } +} + function magReviewClearCurrentPage() { if (state.reviewPage == null) return; if (!_magReviewChecked) _magReviewChecked = new Set(state.article.blocks || []); @@ -1333,7 +1407,8 @@ async function magRead(articleId, opts) { await ensureLayout(); try { const data = await api('/api/magazine/' + state.doc.id + '/articles/' + a.id + '/text?captions=' + - ($('mag-read-captions')?.checked ? 1 : 0)); + ($('mag-read-captions')?.checked ? 1 : 0) + '&descriptions=' + + ($('mag-read-descriptions')?.checked ? 1 : 0)); state.sentences = splitSentences(data.blocks || []); } catch (e) { return say('Could not load article text: ' + e.message, 'err'); @@ -1359,7 +1434,8 @@ function renderArticle() { host.innerHTML = state.sentences.map((s, i) => { const tag = s.type === 'title' ? 'h2' : s.type === 'deck' ? 'p class="mag-deck"' : s.type === 'byline' ? 'p class="mag-byline"' : s.type === 'caption' - ? 'p class="mag-caption"' : 'span'; + ? 'p class="mag-caption"' : s.type === 'figure_description' + ? 'p class="mag-figure-description"' : 'span'; const open = tag.split(' ')[0]; // Each word gets its own span so playback can highlight it individually // (see estimateWordTimings) -- sentence-level highlighting alone doesn't @@ -1852,6 +1928,8 @@ root.magReviewToggleBox = magReviewToggleBox; root.magReocrPage = magReocrPage; root.magRefineFigures = magRefineFigures; root.magReviewToggleImage = magReviewToggleImage; +root.magDescribeImage = magDescribeImage; +root.magSaveImageDescription = magSaveImageDescription; root.magReviewToggleDraw = magReviewToggleDraw; root.magReviewCancelDrawPanel = magReviewCancelDrawPanel; root.magReviewSubmitDraw = magReviewSubmitDraw; diff --git a/static/sections/s-magazine.html b/static/sections/s-magazine.html index 4546c0d..4f28682 100644 --- a/static/sections/s-magazine.html +++ b/static/sections/s-magazine.html @@ -129,6 +129,10 @@ Word highlighting +
diff --git a/static/style.css b/static/style.css index 988716d..d218c85 100644 --- a/static/style.css +++ b/static/style.css @@ -4995,6 +4995,14 @@ input[list="lang-codes"] { background: var(--mauve); color: #fff; font-size: 10px; font-weight: 700; } .mag-block-image { cursor: default; grid-template-columns: auto auto 90px 1fr; } .mag-block-text { font-size: 14px; line-height: 1.45; } +/* A 5th grid child in the 4-column .mag-block-image row -- span the whole + row below the thumbnail rather than being squeezed into column 1 alone. */ +.mag-describe { grid-column: 1 / -1; display: flex; flex-wrap: wrap; align-items: center; gap: 8px; + margin-top: 4px; padding-top: 8px; border-top: 1px dashed var(--border); } +.mag-describe-label { flex-basis: 100%; font-size: 11px; color: var(--subtext); } +.mag-describe-text { flex: 1 1 100%; min-width: 0; min-height: 44px; resize: vertical; + background: var(--panel); border: 1px solid var(--border); color: var(--text); + border-radius: var(--radius); padding: 6px 8px; font-size: 13px; font-family: inherit; } .mag-review-foot { display: flex; gap: 8px; justify-content: flex-end; margin-top: 16px; position: sticky; bottom: 0; padding: 12px 0; background: var(--bg); } @media (max-width: 980px) { @@ -5013,6 +5021,12 @@ input[list="lang-codes"] { .mag-article .mag-deck { font-size: 19px; color: var(--subtext); margin: 0 0 12px; } .mag-article .mag-byline { font-size: 14px; color: var(--subtext); margin: 0 0 20px; } .mag-article .mag-caption { font-size: 14px; color: var(--subtext); font-style: italic; } +/* A synthesised aside, not part of the article's own written text -- set + off visually the same way a caption is, so a reader following along + silently (word-highlighting off) can tell it apart from the real prose. */ +.mag-article .mag-figure-description { font-size: 15px; color: var(--subtext); font-style: italic; + padding-left: 10px; border-left: 2px solid var(--border); margin: 4px 0 14px; } +.mag-article .mag-figure-description::before { content: '\1F5BC '; font-style: normal; } .mag-article [data-i] { cursor: pointer; border-radius: 3px; } .mag-article [data-i]:hover { background: rgba(37,99,235,.07); } .mag-article [data-i]:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; } diff --git a/tests/test_magazine.py b/tests/test_magazine.py index 5b41d4a..7f4967c 100644 --- a/tests/test_magazine.py +++ b/tests/test_magazine.py @@ -23,6 +23,8 @@ from core.magazine import ( _looks_like_repetition_loop, page_ink_grid, BLOCK_FIGURE, + BLOCK_CHART, + BLOCK_TABLE, looks_like_real_title, Article, BLOCK_AD, @@ -63,6 +65,9 @@ from core.magazine import ( score_contents_page, vision_ocr_page, vision_ocr_page_chain, + describe_visual_region, + describe_visual_region_chain, + BLOCK_FIGURE_DESCRIPTION, ) PAGE_W, PAGE_H = 595.0, 842.0 # A4 in points @@ -863,6 +868,95 @@ class VisionOcrPageCallTests(unittest.TestCase): self.assertIsNone(vision_ocr_page(pathlib.Path(__file__), "http://engine/v1", "qwen-vl")) +class DescribeVisualRegionTests(unittest.TestCase): + """describe_visual_region() is a deliberately different task from + vision_ocr_page(): that prompt forbids inventing a description, this + one asks for exactly that -- but shares the same reliability guards.""" + + def _resp(self, status=200, json_body=None): + import requests + resp = mock.Mock(status_code=status) + resp.raise_for_status = mock.Mock( + side_effect=None if status < 400 else requests.exceptions.HTTPError(f"HTTP {status}")) + resp.json = mock.Mock(return_value=json_body or {}) + return resp + + def test_successful_call_returns_the_description(self): + body = {"choices": [{"message": {"content": "A bar chart showing sales rising over five years."}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)): + got = describe_visual_region(pathlib.Path(__file__), BLOCK_CHART, "http://engine/v1", "m") + self.assertEqual(got, "A bar chart showing sales rising over five years.") + + def test_prompt_is_chosen_by_region_type(self): + body = {"choices": [{"message": {"content": "A description long enough to pass."}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)) as request: + describe_visual_region(pathlib.Path(__file__), BLOCK_TABLE, "http://engine/v1", "m") + prompt = request.call_args.kwargs["json"]["messages"][0]["content"][0]["text"] + self.assertIn("table", prompt.lower()) + self.assertNotIn("chart or graph", prompt.lower()) + + def test_unknown_region_type_falls_back_to_the_figure_prompt(self): + body = {"choices": [{"message": {"content": "A description long enough to pass."}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)) as request: + describe_visual_region(pathlib.Path(__file__), "something-odd", "http://engine/v1", "m") + prompt = request.call_args.kwargs["json"]["messages"][0]["content"][0]["text"] + self.assertIn("photo or illustration", prompt.lower()) + + def test_a_repeated_phrase_loop_is_rejected(self): + body = {"choices": [{"message": {"content": "chart chart chart " * 20}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)): + self.assertIsNone(describe_visual_region(pathlib.Path(__file__), BLOCK_CHART, "http://engine/v1", "m")) + + def test_too_short_a_response_is_rejected(self): + body = {"choices": [{"message": {"content": "Yes."}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)): + self.assertIsNone(describe_visual_region(pathlib.Path(__file__), BLOCK_FIGURE, "http://engine/v1", "m")) + + def test_empty_response_is_rejected(self): + body = {"choices": [{"message": {"content": " "}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)): + self.assertIsNone(describe_visual_region(pathlib.Path(__file__), BLOCK_FIGURE, "http://engine/v1", "m")) + + def test_a_long_response_is_cut_at_a_sentence_boundary(self): + # Genuinely varied sentences -- a repeated one would (correctly) + # trip the repetition-loop guard instead of exercising truncation. + sentences = [f"This is plausible sentence number {i} about the figure." + for i in range(20)] + body = {"choices": [{"message": {"content": " ".join(sentences)}}]} + with mock.patch("core.magazine.request_with_retries", + return_value=self._resp(json_body=body)): + got = describe_visual_region(pathlib.Path(__file__), BLOCK_FIGURE, "http://engine/v1", "m") + self.assertLessEqual(len(got), 601) + self.assertTrue(got.endswith(".")) + + def test_unreachable_engine_returns_none(self): + import requests + with mock.patch("core.magazine.request_with_retries", + side_effect=requests.exceptions.ConnectionError()): + self.assertIsNone(describe_visual_region(pathlib.Path(__file__), BLOCK_FIGURE, "http://engine/v1", "m")) + + def test_missing_file_returns_none(self): + self.assertIsNone(describe_visual_region(pathlib.Path("/no/such/file.png"), + BLOCK_FIGURE, "http://engine/v1", "m")) + + def test_chain_falls_back_to_the_second_engine(self): + with mock.patch("core.magazine.describe_visual_region", + side_effect=[None, "A usable description of the figure."]) as mocked: + got = describe_visual_region_chain(pathlib.Path(__file__), BLOCK_FIGURE, + [("http://a/v1", "m1"), ("http://b/v1", "m2")]) + self.assertEqual(got, "A usable description of the figure.") + self.assertEqual(mocked.call_count, 2) + + def test_chain_with_no_configured_engines_returns_none(self): + self.assertIsNone(describe_visual_region_chain(pathlib.Path(__file__), BLOCK_FIGURE, [("", ""), (None, None)])) + + class UnlimitedOcrRegionTests(unittest.TestCase): # The URL/error-path tests below remain deliberately adjacent to the # native-marker tests; share the same mocked OpenAI response helper. @@ -979,6 +1073,60 @@ class ArticleTextTests(unittest.TestCase): include_captions=True)] self.assertEqual(with_caps, ["first", "second", "a caption"]) + def test_described_image_is_narrated_at_its_reading_position(self): + layout = { + "blocks": [ + {"id": "b1", "page": 1, "type": BLOCK_TITLE, "reading_order": 1, + "text": "Headline", "bbox": [0, 0, 1, 1]}, + {"id": "b2", "page": 1, "type": BLOCK_BODY, "reading_order": 3, + "text": "Body after the figure", "bbox": [0, 0, 1, 1]}, + ], + "images": [ + {"id": "img1", "page": 1, "bbox": [0, 0, 1, 1], "reading_order": 2, + "description": "A photo of a cat."}, + ], + } + article = {"blocks": ["b1", "b2"], "images": ["img1"]} + got = article_text(layout, article, include_descriptions=True) + self.assertEqual([b["text"] for b in got], + ["Headline", "A photo of a cat.", "Body after the figure"]) + self.assertEqual(got[1]["type"], BLOCK_FIGURE_DESCRIPTION) + self.assertEqual(got[1]["page"], 1) + + def test_descriptions_are_off_by_default(self): + layout = { + "blocks": [{"id": "b1", "page": 1, "type": BLOCK_BODY, "reading_order": 1, + "text": "Body", "bbox": [0, 0, 1, 1]}], + "images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1], + "reading_order": 0, "description": "A photo."}], + } + article = {"blocks": ["b1"], "images": ["img1"]} + got = article_text(layout, article) + self.assertEqual([b["text"] for b in got], ["Body"]) + + def test_an_undescribed_image_is_silently_skipped(self): + """No description yet (or an empty one) -- nothing to narrate, not + an empty sentence.""" + layout = { + "blocks": [{"id": "b1", "page": 1, "type": BLOCK_BODY, "reading_order": 1, + "text": "Body", "bbox": [0, 0, 1, 1]}], + "images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1], "reading_order": 0}], + } + article = {"blocks": ["b1"], "images": ["img1"]} + got = article_text(layout, article, include_descriptions=True) + self.assertEqual([b["text"] for b in got], ["Body"]) + + def test_a_described_image_belonging_to_another_article_is_excluded(self): + layout = { + "blocks": [{"id": "b1", "page": 1, "type": BLOCK_BODY, "reading_order": 1, + "text": "Body", "bbox": [0, 0, 1, 1]}], + "images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1], + "reading_order": 0, "description": "Not this article's figure."}], + } + article = {"blocks": ["b1"], "images": []} + got = article_text(layout, article, include_descriptions=True) + self.assertEqual([b["text"] for b in got], ["Body"]) + def test_listening_estimate_counts_only_narratable_words(self): blocks = [ Block(id="b1", page=1, bbox=[0, 0, 1, 1], type=BLOCK_BODY, diff --git a/tests/test_magazine_routes.py b/tests/test_magazine_routes.py index 27bb873..be951f0 100644 --- a/tests/test_magazine_routes.py +++ b/tests/test_magazine_routes.py @@ -291,3 +291,167 @@ class IngestPreviewHtmlTests(unittest.TestCase): from routes.magazine import _render_ingest_preview_html self.assertEqual(_render_ingest_preview_html([], None), "") self.assertEqual(_render_ingest_preview_html([], []), "") + + +class DescribeImageJobTests(unittest.TestCase): + """Figure descriptions are additive: generating one never touches the + article's own blocks, only the image record's own description field.""" + + def _layout(self): + return { + "images": [ + {"id": "img1", "page": 1, "bbox": [0, 0, 10, 10], "filename": "img1.png", "type": "figure"}, + ], + } + + def test_successful_description_is_saved(self): + from unittest.mock import patch + from routes.magazine import _describe_image_sync + layout = self._layout() + with patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine.mag.describe_visual_region_chain', + return_value='A photo of a mountain.') as chain, \ + patch('routes.magazine.mag.save_layout') as save, \ + patch('routes.magazine.background_job_update') as update, \ + patch('pathlib.Path.is_file', return_value=True): + _describe_image_sync('a' * 32, 'job', 'img1', 'http://e/v1', 'm', None, None) + save.assert_called_once() + self.assertEqual(layout['images'][0]['description'], 'A photo of a mountain.') + chain.assert_called_once() + self.assertEqual(chain.call_args.args[1], 'figure') # region type passed through + done = [c.kwargs for c in update.call_args_list if c.kwargs.get('done')] + self.assertEqual(done[-1]['stage'], 'done') + self.assertEqual(done[-1]['result'], {'image_id': 'img1', 'description': 'A photo of a mountain.'}) + + def test_no_layout_reports_failure(self): + from unittest.mock import patch + from routes.magazine import _describe_image_sync + with patch('routes.magazine.mag.load_layout', return_value=None), \ + patch('routes.magazine.mag.save_layout') as save, \ + patch('routes.magazine.background_job_update') as update: + _describe_image_sync('a' * 32, 'job', 'img1', 'http://e/v1', 'm', None, None) + save.assert_not_called() + done = [c.kwargs for c in update.call_args_list if c.kwargs.get('done')] + self.assertEqual(done[-1]['stage'], 'failed') + self.assertIn('No layout', done[-1]['error']) + + def test_unknown_image_id_reports_failure(self): + from unittest.mock import patch + from routes.magazine import _describe_image_sync + with patch('routes.magazine.mag.load_layout', return_value=self._layout()), \ + patch('routes.magazine.mag.save_layout') as save, \ + patch('routes.magazine.background_job_update') as update: + _describe_image_sync('a' * 32, 'job', 'missing', 'http://e/v1', 'm', None, None) + save.assert_not_called() + done = [c.kwargs for c in update.call_args_list if c.kwargs.get('done')] + self.assertEqual(done[-1]['stage'], 'failed') + self.assertIn('not found', done[-1]['error'].lower()) + + def test_engine_returning_nothing_usable_is_a_failure_not_a_blank_save(self): + from unittest.mock import patch + from routes.magazine import _describe_image_sync + layout = self._layout() + with patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine.mag.describe_visual_region_chain', return_value=None), \ + patch('routes.magazine.mag.save_layout') as save, \ + patch('routes.magazine.background_job_update') as update, \ + patch('pathlib.Path.is_file', return_value=True): + _describe_image_sync('a' * 32, 'job', 'img1', 'http://e/v1', 'm', None, None) + save.assert_not_called() + self.assertNotIn('description', layout['images'][0]) + done = [c.kwargs for c in update.call_args_list if c.kwargs.get('done')] + self.assertEqual(done[-1]['stage'], 'failed') + + +class DescribeImageRouteTests(unittest.IsolatedAsyncioTestCase): + async def test_unconfigured_vision_engine_is_rejected(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_describe_image + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine._load_settings', return_value={}): + request = AsyncMock() + request.json.return_value = {} + with self.assertRaises(HTTPException) as err: + await magazine_describe_image('a' * 32, 'img1', request) + self.assertEqual(err.exception.status_code, 400) + + async def test_unknown_image_id_is_a_404(self): + from unittest.mock import patch + from routes.magazine import magazine_describe_image + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout): + with self.assertRaises(HTTPException) as err: + await magazine_describe_image('a' * 32, 'missing', None) + self.assertEqual(err.exception.status_code, 404) + + async def test_valid_request_starts_a_background_job(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_describe_image + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine._load_settings', + return_value={'vision_ocr_url': 'http://e/v1', 'vision_ocr_model': 'm'}), \ + patch('routes.magazine.background_job_create') as create, \ + patch('routes.magazine.asyncio.to_thread', new=lambda *a, **k: None), \ + patch('asyncio.create_task') as create_task: + request = AsyncMock() + request.json.return_value = {} + result = await magazine_describe_image('a' * 32, 'img1', request) + self.assertIn('job_id', result) + create.assert_called_once() + create_task.assert_called_once() + + +class UpdateImageDescriptionRouteTests(unittest.IsolatedAsyncioTestCase): + async def test_sets_the_description_verbatim(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_update_image_description + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine.mag.save_layout') as save: + request = AsyncMock() + request.json.return_value = {'description': ' A hand-typed description. '} + result = await magazine_update_image_description('a' * 32, 'img1', request) + self.assertEqual(result['description'], 'A hand-typed description.') + self.assertEqual(layout['images'][0]['description'], 'A hand-typed description.') + save.assert_called_once() + + async def test_unknown_image_id_is_a_404(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_update_image_description + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout): + request = AsyncMock() + request.json.return_value = {'description': 'x'} + with self.assertRaises(HTTPException) as err: + await magazine_update_image_description('a' * 32, 'missing', request) + self.assertEqual(err.exception.status_code, 404) + + async def test_missing_description_field_is_rejected(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_update_image_description + layout = {"images": [{"id": "img1", "page": 1, "bbox": [0, 0, 1, 1]}]} + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout): + request = AsyncMock() + request.json.return_value = {} + with self.assertRaises(HTTPException) as err: + await magazine_update_image_description('a' * 32, 'img1', request) + self.assertEqual(err.exception.status_code, 400) + + async def test_no_layout_is_a_404(self): + from unittest.mock import AsyncMock, patch + from routes.magazine import magazine_update_image_description + with patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=None): + request = AsyncMock() + request.json.return_value = {'description': 'x'} + with self.assertRaises(HTTPException) as err: + await magazine_update_image_description('a' * 32, 'img1', request) + self.assertEqual(err.exception.status_code, 404)