From f33808b9779f5a6ede2cd33e746aea8cad415d9e Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Fri, 3 Jul 2026 19:41:29 -0400 Subject: [PATCH] fix(pixiv): capture ugoira frame timings in the post record (ordering bug) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The core writes the post record BEFORE extract_media, but the ugoira frame delays were only memoized DURING extract_media — so write_post_record never saw them and ugoira_frames was always empty in the record. Extract a memoized _ugoira_meta (frames + zip url share ONE /v1/ugoira/metadata call regardless of order) and inject client.fetch_ugoira_frames into the downloader (mirrors Patreon's content_fetcher) so write_post_record populates the frames itself. Zero extra API calls — the fetch is shared/memoized with extract_media. A recapture now backfills the timings onto existing ugoira posts. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CDgx8bQS5YrGRK76v8HUnM --- backend/app/services/pixiv_client.py | 55 +++++++++++++++++++----- backend/app/services/pixiv_downloader.py | 16 ++++++- backend/app/services/pixiv_ingester.py | 4 ++ tests/test_pixiv_client.py | 32 ++++++++++++++ tests/test_pixiv_downloader.py | 34 +++++++++++++++ 5 files changed, 130 insertions(+), 11 deletions(-) diff --git a/backend/app/services/pixiv_client.py b/backend/app/services/pixiv_client.py index 3635ca4..e6b5fea 100644 --- a/backend/app/services/pixiv_client.py +++ b/backend/app/services/pixiv_client.py @@ -435,24 +435,59 @@ class PixivClient: ) ] - def _ugoira_media(self, work: dict, pid: str) -> list[MediaItem]: - """The ugoira frame zip (gallery-dl's default non-original mode): - `/v1/ugoira/metadata` → zip_urls.medium with the 600x600→1920x1080 - swap. Frame delays are memoized onto the work so the post record - captures them (a future ugoira→video conversion needs the timings — - the zip alone has none). A metadata failure downgrades to 'no media' - with a warning (matching gallery-dl) instead of failing the walk — - except auth failures, which stay loud.""" + def _ugoira_meta(self, work: dict, pid: str) -> dict | None: + """Fetch + memoize the ugoira metadata (frames + zip urls) for a work. + + Idempotent and cached on the work dict, so the post record and the + media extraction share ONE `/v1/ugoira/metadata` call regardless of + which runs first (the core writes the post record BEFORE it extracts + media). Returns None — and caches the miss — on a non-auth failure + (matching gallery-dl's downgrade); auth failures stay loud.""" + if "_ugoira_meta" in work: + return work["_ugoira_meta"] try: body = self._call("/v1/ugoira/metadata", {"illust_id": pid}) meta = body["ugoira_metadata"] - zip_url = meta["zip_urls"]["medium"] except PixivAuthError: raise except (PixivAPIError, KeyError, TypeError) as exc: log.warning("Pixiv ugoira metadata failed for %s: %s", pid, exc) - return [] + work["_ugoira_meta"] = None + return None + work["_ugoira_meta"] = meta + # Frame delays: a future ugoira→video conversion needs the timings (the + # zip alone has none), so the post record captures them. work["_ugoira_frames"] = meta.get("frames") or [] + return meta + + def fetch_ugoira_frames(self, post: dict) -> None: + """Populate `post['_work']['_ugoira_frames']` for an ugoira post (no-op + otherwise). The core writes the post record BEFORE extract_media, so + without this the frame timings would never reach the record; this + fetches (and memoizes, so extract_media reuses it) the metadata. Injected + into the downloader by the ingester, mirroring Patreon's content_fetcher. + Auth errors propagate; other failures leave frames unset.""" + work = post.get("_work") or {} + if work.get("type") != "ugoira": + return + pid = str(post.get("id") or "") + if pid: + self._ugoira_meta(work, pid) + + def _ugoira_media(self, work: dict, pid: str) -> list[MediaItem]: + """The ugoira frame zip (gallery-dl's default non-original mode): + `/v1/ugoira/metadata` → zip_urls.medium with the 600x600→1920x1080 + swap. A metadata failure downgrades to 'no media' with a warning + (matching gallery-dl) instead of failing the walk — except auth + failures, which stay loud.""" + meta = self._ugoira_meta(work, pid) + if meta is None: + return [] + try: + zip_url = meta["zip_urls"]["medium"] + except (KeyError, TypeError) as exc: + log.warning("Pixiv ugoira zip url missing for %s: %s", pid, exc) + return [] url = zip_url.replace("_ugoira600x600", "_ugoira1920x1080", 1) return [ MediaItem( diff --git a/backend/app/services/pixiv_downloader.py b/backend/app/services/pixiv_downloader.py index 24e1226..532398c 100644 --- a/backend/app/services/pixiv_downloader.py +++ b/backend/app/services/pixiv_downloader.py @@ -102,11 +102,16 @@ class PixivDownloader(BaseNativeDownloader): validate: bool = True, rate_limit: float = 0.0, session: requests.Session | None = None, + ugoira_frames_fetcher: Callable[[dict], None] | None = None, ): super().__init__( images_root, cookies_path, platform="pixiv", validate=validate, rate_limit=rate_limit, session=session, ) + # Injected by the ingester (client.fetch_ugoira_frames) so write_post_record + # can populate frame timings — which extract_media memoizes, but the core + # writes the post record FIRST. Mirrors Patreon's content_fetcher. + self._ugoira_frames_fetcher = ugoira_frames_fetcher if session is None: # i.pximg.net 403s any GET without the app Referer; mirror the # client's full app-header profile (gallery-dl serves media off @@ -248,7 +253,16 @@ class PixivDownloader(BaseNativeDownloader): "account": user.get("account"), "name": user.get("name"), } - # Memoized by extract_media's ugoira branch; the zip has no timings. + # Ugoira frame timings. extract_media memoizes these, but the core writes + # the post record BEFORE extracting media, so fetch them here (shared + + # idempotent via the client's memoization) so the record actually keeps + # them — the zip carries no timings. + if ( + work.get("type") == "ugoira" + and not work.get("_ugoira_frames") + and self._ugoira_frames_fetcher is not None + ): + self._ugoira_frames_fetcher(post) frames = work.get("_ugoira_frames") if frames: data["ugoira_frames"] = frames diff --git a/backend/app/services/pixiv_ingester.py b/backend/app/services/pixiv_ingester.py index 663f292..8930de1 100644 --- a/backend/app/services/pixiv_ingester.py +++ b/backend/app/services/pixiv_ingester.py @@ -81,6 +81,10 @@ class PixivIngester(Ingester): if downloader is not None else PixivDownloader( self.images_root, cookies_path, validate=validate, rate_limit=rate_limit, + # write_post_record runs before extract_media in the core, so it + # fetches ugoira frame timings via the SAME client (shared, + # memoized) — else the record's ugoira_frames stays empty. + ugoira_frames_fetcher=resolved_client.fetch_ugoira_frames, ) ) super().__init__( diff --git a/tests/test_pixiv_client.py b/tests/test_pixiv_client.py index 8419159..1c60443 100644 --- a/tests/test_pixiv_client.py +++ b/tests/test_pixiv_client.py @@ -200,6 +200,38 @@ def test_extract_media_ugoira_zip_swap(client, page1, monkeypatch): assert post["_work"]["_ugoira_frames"] == frames +def test_fetch_ugoira_frames_memoizes_and_shares_one_call(client, page1, monkeypatch): + # fetch_ugoira_frames (called by write_post_record, which the core runs + # BEFORE extract_media) populates frames; extract_media then REUSES the + # memoized metadata — exactly one /v1/ugoira/metadata call total. + frames = [{"file": "000000.jpg", "delay": 90}] + calls = [] + + def fake_call(endpoint, params): + calls.append(endpoint) + return {"ugoira_metadata": { + "zip_urls": {"medium": ( + "https://i.pximg.net/img-zip-ugoira/x/333_ugoira600x600.zip" + )}, + "frames": frames, + }} + + monkeypatch.setattr(client, "_call", fake_call) + post = _post_for(client, page1, 333) + client.fetch_ugoira_frames(post) + assert post["_work"]["_ugoira_frames"] == frames + items = client.extract_media(post, {}) # reuses memoized meta + assert items[0].media_id == "ugoira" + assert calls == ["/v1/ugoira/metadata"] # ONE fetch, not two + + +def test_fetch_ugoira_frames_noop_for_non_ugoira(client, page1, monkeypatch): + monkeypatch.setattr(client, "_call", lambda e, p: pytest.fail("should not fetch")) + post = _post_for(client, page1, 222) # a single-page illust + client.fetch_ugoira_frames(post) + assert "_ugoira_frames" not in post["_work"] + + def test_extract_media_ugoira_metadata_failure_downgrades( client, page1, monkeypatch ): diff --git a/tests/test_pixiv_downloader.py b/tests/test_pixiv_downloader.py index ed45603..fef95eb 100644 --- a/tests/test_pixiv_downloader.py +++ b/tests/test_pixiv_downloader.py @@ -268,6 +268,40 @@ def test_write_post_record_includes_ugoira_frames(tmp_path): assert data["ugoira_frames"] == frames +def test_write_post_record_fetches_ugoira_frames_when_absent(tmp_path): + # The core writes the post record BEFORE extract_media, so frames aren't + # memoized yet — the injected fetcher must populate them so the record keeps + # the timings (regression: they were silently always empty). + post = _post_only(333) + assert "_ugoira_frames" not in post["_work"] + frames = [{"file": "000000.jpg", "delay": 120}] + calls = [] + + def fetcher(p): + calls.append(p) + p["_work"]["_ugoira_frames"] = frames + + dl = PixivDownloader( + tmp_path, validate=False, session=_FakeSession(), + ugoira_frames_fetcher=fetcher, + ) + outcome = dl.write_post_record(post, "artist-a") + data = json.loads(outcome.path.read_text()) + assert data["ugoira_frames"] == frames + assert len(calls) == 1 + + +def test_write_post_record_non_ugoira_does_not_call_fetcher(tmp_path): + post = _post_only(111) # a plain multi-page illust + calls = [] + dl = PixivDownloader( + tmp_path, validate=False, session=_FakeSession(), + ugoira_frames_fetcher=lambda p: calls.append(p), + ) + dl.write_post_record(post, "artist-a") + assert calls == [] + + def test_write_post_record_without_id(tmp_path): dl = _downloader(tmp_path) outcome = dl.write_post_record({"id": None, "attributes": {}}, "artist-a")