Preflight — an AI-generated PNG that bloats the JPEG it replaces

The transparency default got wider, and all three of its consumers inherited that — including one you already decided about

From Devin, both verified by measurement · publish-size thread · background-removal thread

Read this one first, because it may unmake the choice you already made. You chose “revert just the publish call site to sampled”, and the comment in 632e5b1a7 reasons that the sampling “will usually miss [stray pixels] and allow the image to be converted to the smaller JPEG format”. That describes master’s corner-only sampling. But the sampled mode itself was widened underneath it in d1d02a4aa — corner block plus ~100 scattered points. So reverting the call did not restore the behaviour; it restored a call to a predicate that now answers differently.

Measured. A photograph with a 30×30 knock-out in the middle, put through AdjustImageForDisplay the way the publication code calls in:

Both consequences are real, and they pull opposite ways: the knock-out now survives publication (master flattened it — an improvement), and the book ships larger (which is the bloat this ticket exists to remove). It applies to every book, not just AI-edited ones, from a branch whose title is about the AI image editor.

And it exposes a limit in my own test. AdjustImageForDisplay_PhotoPngWithOneStrayTransparentPixel_StillConvertsToJpeg hunts for a pixel the sampler misses, so by construction it can only pin the direction where conversion still happens. It cannot see this case, and I should not have let it read as though the publish path were covered.

The second consumer, same root cause:

RuntimeImageProcessor.MakePngBackgroundTransparent opens with if (ImageUtils.HasTransparency(originalImage.Image)) return …Clone() — the default sampling mode, and when it answers true the background is left alone. On origin/master that default is corner-only, a 15×15 block. This branch widened it in d1d02a4aa to fold in the ~100 jittered points. So for every image that is opaque in its corners but has one sampled interior pixel below alpha 255, the background now silently isn’t removed.

What makes it a decision rather than a shrug: on the ImageTransparencyMode.Force path the caller has explicitly asked for the white background to go, and it now quietly doesn’t. No error, no log — the image comes back as it went in. Nothing in the suite covers that path, which is why six full runs never mentioned it.

This is the third time this branch has widened a shared predicate and hit a consumer nobody was looking at. !needsResize stopped resized photos converting at all; the exhaustive scan blocked conversion for a single stray pixel; now the scattered sample suppresses background removal. Each of the first two looked obviously right at the time, which is exactly why I have not quietly picked a fix for this one.

One choice settles both. Options: (1, recommended — and this is a change of mind) narrow the widening itself: keep the scattered sample for the AI import path, which is what asked for it, and let every other caller have master’s corner-only default. Before running the measurement I had this ranked last for blast radius; the measurement moved it, because it is the only option that confines a global behaviour change to the scope named in the PR title. (2) Accept both consequences deliberately — better transparency fidelity, larger books, background removal quietly declining on some images — and record that in the code so nobody rediscovers it as a bug. If you take this, Force should at least log rather than silently no-op. (3) Fix them one at a time: give just the background-removal caller a corner-only test, and separately decide the publish path. More commits, and it leaves BookThumbNailer:283 — the third consumer — unexamined.

What I still have not measured: how often real book images carry transparency away from the corner. My measurement proves the mechanism on a constructed image; it says nothing about frequency, and frequency is what decides whether option 2 is fine or expensive. A look at a handful of real collections would settle it, and I would not guess.

Reused book images: the reason for excluding them is gone

From John, and independently from Devin · open thread

Unchanged and still waiting. The conversion runs only for a freshly generated or uploaded result; its stated justification was that re-encoding a reused image would lose its embedded credits, which is exactly what we fixed. So the exclusion rests on scope alone. Devin’s addition stands: resizeIfNeeded:false just above rests on the same obsolete reasoning.

The full C# suite is only intermittently green — filed as BL-16722, nothing to decide

Found by this run · kept here because it colours how you read the test row, not because it needs an answer

AGENTS.md says the suite is expected green through the wrapper. Across seven full runs today it was green twice, with 1–2 failures each time otherwise, spread over six different and unrelated tests — AI editor, epub audio, credits page, TeamCollection status, BloomPUB compression, device xmatter.

Correction to the first version of this item: I wrote that every failure shared one signature. It does not — I had read the exception messages from two runs and the test names from the others. Three of the six share it; the rest have separate causes, listed below. The mechanism still holds for those three, and they are filed as BL-16722.

Those three are each a DirectoryNotFoundException on a path inside the run’s own isolated temp root, raised from inside RobustFile after its retries — …\BookTests\book\book.htm, …\BookTests\book\epub-thumbnail.png, …\ePUB_export\0\…\lang-qr-code.png. A directory that should exist, vanishing mid-test.

The mechanism. BookTestsBase.Setup() is a [SetUp] — so it runs before every test — and does new TemporaryFolder(GetTestFolderName()), which returns the literal "BookTests", then nests "book" inside it. TemporaryFolder deletes an existing folder of that name before creating it. Eight fixtures derive from BookTestsBase and only BookThumbnailerTests overrides the name, so they all share one path. Any work still touching BookTests\book when the next test’s setup wipes it gets exactly the exception above. That fits the symptoms precisely: intermittent, timing dependent, and clustered in the publishing fixtures — the ones most likely to have something outstanding, which is the same hazard behind this repo’s “tests retire their BloomServers rather than disposing them” rule. ePUB_export is worse: it is a production constant (EpubMaker.kEPUBExportFolder), so every epub test shares one staging root.

The correlation is what makes this more than a guess: all three of those failures are in fixtures that inherit the shared name (ExportEpubTests twice, BloomPubMakerTests once).

The other three failures are not this bug, and I am not claiming them: InCreditsPage_LicenseUrlAndISBN_AreRemoved could not find customBookStyles.css under the shared output\browser — a build artefact, not a temp folder; GetStatus_BuildNeeded… in RabAppProjectTests (which does not inherit the name) reported RAB importing 1 book where 2 were expected; and my own ConvertPngToJpeg_UndecodableFile… lost a file from its fixture’s own per-test folder, which remains unexplained.

Filed as BL-16722 (Bug, Bloom 6.6, Incoming) rather than fixed here — it is test-infrastructure work across nine fixtures, unrelated to this ticket. The fix has a precedent in the tree: BookThumbnailerTests and BookCompressorTests already override GetTestFolderName(), and deriving it from GetType().Name in the base class would make the problem structurally impossible. Worth doing: a suite that goes red two runs in three for reasons unrelated to your change trains everyone to re-run until green, which is how a real regression gets waved through — and AGENTS.md tells people the suite is expected green, so they bisect their own work instead. Which is what I did, for about half an hour.

Two things I got wrong on the way here, both worth knowing. I first blamed my own orphan-cleanup fix, on the strength of a single green run with it reverted; the next run failed elsewhere with the fix restored, so that was wrong. And two of the red runs overlapped a git worktree add/remove I started myself, so I was polluting my own measurements for part of it. The root cause above does not depend on either of those.

Settled

• Two passes on one line of mine. Devin reviewed the metadata re-attach twice and was right twice: first that it lacked CopyCoreMetadata’s read-failure guard (64a16b4c9), then that the guard still let an empty model be written — the normal case for a generated result — which could strip what a verbatim copy had preserved (dae14c121). Worth noticing about my own work: the line exists to stop credits being lost, and both defects were ways it could cause the loss. The licence test passing after each change is what shows the real case still works.
• The metadata guard I was missing — Devin noticed that the re-attach in 688cb7d0a wrote the source’s metadata unconditionally, while CopyCoreMetadata skips when libpalaso couldn’t read it. So for a source with unparseable tags my fix could have stripped metadata that a verbatim copy had preserved — a line whose whole purpose is not losing credits, causing the loss. Guarded in 64a16b4c9. No test: reaching it needs bytes that decode as an image but whose tags fail to parse, which I could not construct reliably, and the existing undecodable-file test fails earlier at the decode. Said so in the commit rather than implying it is covered.
• The tRNS test on Linux — Devin asked whether it passes on both CI platforms. There is no second platform: BloomTests.csproj targets net8.0-windows, so libgdiplus never sees it. Its second point was right though, and is why the test exists: if GDI+ ever stopped surfacing tRNS, the AI editor would flatten such an image and delete the original. Refuted and resolved.
• The imported upload’s licence — fixed in 688cb7d0a, and my diagnosis of it was wrong twice. I reported that a GraphicsMagick import resize loses the credits, and that this was pre-existing breakage in shared import code. Neither held. Every other caller re-attaches the metadata afterwards (PageEditingModel.ChangePicture does it in the next statement), so only the path this PR added lost anything — and what it lost was not the credits, because libpalaso writes those twice, as PNG tEXt keys and as XMP. GraphicsMagick keeps the tEXt and drops the XMP, where the licence alone lives. So the test I wrote to prove the bug passed; adding one licence assertion turned it red. The real symptom is nastier than the one I described: an uploaded photo arrived with its copyright and creator intact and its Creative Commons licence silently reduced to NullLicense — healthy-looking, and wrong. Fixed with the call the other caller already makes. Bearing on the item below: the reuse exclusion’s credits justification is now definitively gone, since metadata is re-attached whichever path processing took.
• The wasted GraphicsMagick pass on every published JPEG — you chose a ticket of its own, filed as BL-16721 (Bug, Bloom 6.6, Incoming). It carries the diagnosis rather than the symptom — including that isJpeg is already computed nine lines above the condition, so the fix is plausibly one term — and records what I did not establish: the real per-publish cost, and how often the 50%-smaller test actually succeeds. The fix is not free, so that is worth knowing first. Thread resolved.
• The stray alpha pixel — you chose option 2, in the working tree: AdjustImageForDisplay is back to sampled detection and the AI editor keeps the exhaustive scan, committed as 632e5b1a7 with your reasoning in the comment. The part I could not supply is the part that decided it: this code sampled for years without a complaint, which beats a measurement over a handful of images. Recorded on the thread and resolved. I then pinned it with a test — the revert broke nothing, which is exactly how the exhaustive scan reached that call site unnoticed in the first place, and how !needsResize got in before it. Also fixed one comment the change made untrue.
• A truecolour PNG whose transparency is in a tRNS chunk — Devin has listed this for several rounds and it had no thread and no recorded outcome, so I measured it: GDI+ does hand us such a PNG with an alpha channel, so HasTransparency catches it and nothing gets flattened. Same shape as the palette bug, which is why it was worth checking rather than assuming; now pinned by a test with a 198-byte generated image.
• The orphaned reserved JPEG — the catch could walk away from a JPEG GraphicsMagick had already written (the throw can come from disposing the PalasoImage, after the conversion succeeded). Real, and now cleaned up there too.
• The empty metadata write — Devin asked whether copying metadata from a source with none leaves the JPEG asserting a default licence. Measured: the raw conversion and the copied one are identical (IsEmpty=true, NullLicense), and the ask token is already on the source. No change; resolved.
• The scan’s cost — kept, on your decision. Resolved.
• John’s rename and accessibility points — both committed in 947871e8a, each with a reply saying what changed. His threads await only his own disposition.
• The transparency sampling gap, empty/placeholder slots, and the severe publish-size regression — all closed in earlier rounds.

Why the Reviewable count kept climbing back. Every thread this workflow posts to GitHub — each mirrored Devin finding, and each PR-level comment, which lands in the -top discussion — is mirrored into Reviewable as a discussion that lists me as owing a disposition, and resolving it on GitHub does not set that. So the check would read “4 discussions left” while GitHub showed nothing unresolved. I now set those dispositions in the same step as posting: satisfied for anything fixed or refuted and closed, discussing for the one item deliberately left open. As of the final commit only that open item (decision 1) still shows against my name. John’s own threads stay his to flip.