fix(pptx): apply a blip's a:lum brightness and contrast - #352
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All contributors have signed the CLA — thank you! ✍️ ✅ Posted by the CLA bot. |
Greptile SummaryThis PR parses DrawingML
Confidence Score: 4/5The PR should not merge until luminance effects are also preserved for ordinary shape picture fills. The new transform is consistently implemented in both rendering backends, but the parser and layout pipeline silently drop the same Files Needing Attention: crates/pptx-parse/src/drawing.rs, crates/pptx-parse/src/model.rs, crates/pptx-render/src/layout.rs
|
| Filename | Overview |
|---|---|
| crates/pptx-parse/src/drawing.rs | Parses signed luminance attributes for pictures, but does not preserve them for ordinary shape picture fills. |
| crates/pptx-parse/src/model.rs | Adds the unresolved luminance effect variant; PictureFill still cannot carry effects. |
| crates/pptx-render/src/layout.rs | Resolves luminance values into display-list effects, while shape picture fills continue to emit an empty effect list. |
| crates/pptx-render/src/image_effects.rs | Applies the bounded luminance ramp per RGB channel while preserving alpha. |
| packages/pptx/src/render/canvas.ts | Implements the corresponding Canvas luminance ramp within the existing private-surface recolouring pipeline. |
| crates/betteroffice-pptx/tests/render.rs | Verifies parsing-to-raster behavior and representative output pixels for p:pic images. |
| packages/pptx/src/render/canvas.test.ts | Pins the TypeScript luminance ramp to the same representative values as the Rust backend. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
XML["DrawingML a:blip / a:lum"] --> Parse["PPTX parser"]
Parse --> Pic["p:pic effects"]
Parse -. "effects currently discarded" .-> Fill["shape a:blipFill"]
Pic --> Layout["ImageEffect::Luminance"]
Layout --> Rust["Rust raster ramp"]
Layout --> JSON["Display-list JSON"]
JSON --> Canvas["Canvas ramp"]
Rust --> Output["Rendered image"]
Canvas --> Output
Reviews (1): Last reviewed commit: "fix(pptx): apply a blip's a:lum brightne..." | Re-trigger Greptile
| "lum" => Some(BlipEffect::Luminance { | ||
| brightness: fixed_percentage_attribute(child, "bright").unwrap_or(0.0), | ||
| contrast: fixed_percentage_attribute(child, "contrast").unwrap_or(0.0), | ||
| }), |
There was a problem hiding this comment.
a:lum is parsed only for p:pic nodes. When it appears in an ordinary shape's stretched a:blipFill, parse_picture_fill ignores the blip's child effects and picture_filled emits an empty effect list. As a result, that image renders without its specified brightness and contrast. Preserve these effects in PictureFill and pass them through the layout path.
|
Shape fills drop luminance — the reading is right, the reach is nil. Confirmed structurally: What I measured before deciding: across all 48 decks, zero I have left it out rather than widen a mergeable change on reasoning alone, and recorded it as its |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TL;DR:
Before/After:
LibreOffice (reference),
main, and this branch renderingcrates/pptx-render/tests/fixtures/blip-lum.pptx. The five rows are the control, PowerPoint'sWashout pair (
bright="70000" contrast="-70000"), brightness only, contrast only, and thebright="3000" contrast="77000"pair the corpus also carries. Onmainevery row is the control.The same two images are committed as
blip-lum.before.pngandblip-lum.after.png.Repro file:
crates/pptx-render/tests/fixtures/blip-lum.pptx, added here. It is synthetic and publishable: onesix-swatch bitmap drawn five times, once per
a:lumsetting, with the sample coordinates and theirexpected colours written down in
blip-lum.mdnext to it. Every corpus deck that reproduces thisis a third-party presentation and is not publishable.
Summary:
Parse
a:blip'sa:luminto a newBlipEffect::Luminance { brightness, contrast }, in documentorder with the effects fix(pptx): apply a blip's duotone, biLevel and colour-change effects #312 added, and resolve it into
ImageEffect::Luminancein the layoutpass.
brightandcontrastareST_FixedPercentage, so they need a signed reader; the existingpercentage_attributerejects negatives, and the corpus is full ofcontrast="-70000".Apply it in both backends as a 256-entry ramp per channel, alpha untouched, over straight alpha
before the premultiply, exactly where the other effects run.
The curve is measured, not read off a spec. ECMA-376 and MS-OI29500 both say only "the
percentage to change the brightness/contrast". It was fitted to LibreOffice 26.8 by rendering 121
bright×contrastpairs over a 16-step grey ramp throughpptx-pdfand reading the pixels:with
b,cfractions in-1..=1. That reproduces the reference to within one level on everypair sampled, per channel, with one exception below.
Clamp both attributes to
-100 %..=100 %at parse and again in layout.contrast = 100 %is thepole of the slope expression; past it the ramp inverts.
Pin the identical sample points in a Rust unit test and a
buntest so the two backends cannotdrift, a parse test for the signed percentages, a layout test for the clamp, and an end-to-end
pixel test over the new fixture.
Where this deliberately differs from the reference:
bright="70000" contrast="-70000"— exactly, and only, that pair — is the one place the formuladisagrees with LibreOffice, by 10–11 levels (black lands on
CDCDCDhere andD8D8D8there).LibreOffice special-cases MSO's Washout preset into its own watermark colour mode: its immediate
neighbours (69 %/−70 %, 71 %/−70 %, 70 %/−69 %, 70 %/−71 %) all follow the formula above, so this
is a LibreOffice compatibility shim rather than the meaning of the element, and reproducing it
would bake one renderer's quirk into the contract. The gap is far inside the harness's 40-level
per-pixel threshold and no measured slide changes verdict because of it.
Measured, whole decks re-rendered against the LibreOffice reference, baseline built from this same
worktree with the commit reverted:
duotone+lumlumblipsEvery one of the first deck's 32 slides improves, by −8.80 to −19.40; the decorative strip its
masters draw goes from a solid orange band to the near-white the reference draws. In the second
deck the two slides that carry a slide-level
lummove −0.16 and −0.19 and nothing else moves; itsthird occurrence is on a layout whose media is an
.emf, which the decoder skips before any effectcould apply, so that slide is byte-identical. No slide in either deck moved the wrong way. Neither
deck is publishable, which is why the screenshots above are of the committed fixture.
No deck schema bump:
a:lumadds no key. A snapshot built onmainfrom alumdeck stores noblipEffectsJsonfor those pictures, so the existing schema-17 backfill restores them the firsttime the source is reattached, and the restored snapshot equals a fresh one.
Test plan:
cargo fmt --all --checkandcargo clippy --workspace --all-targets -- -D warnings.cargo testonbetteroffice-pptx-parse,betteroffice-pptx-render,betteroffice-pptx-raster,betteroffice-pptx-editandbetteroffice-pptx --features raster: 463 passed.cargo check --manifest-path apps/native-viewer/Cargo.toml --no-default-features --features pptxandcargo checkinbindings/python-pptx, both separate workspaces.bun run build:pptx-wasm(each.wasmstarts0061736d),bun run typecheck:packages, and absolute-pathbun testonpackages/pptxandpackages/pptx-react: 186 passed.origin/mainand on this branch with the same fonts: 111 slides byte-identical, and the one differing slide differs only by the four newLuminanceeffects.origin/mainall five fixture rows render as the control (000000,03A7DF, …); on this branch they renderCDCDCD/E1E1E1/CEFFFF,808080,404040/C0C0C0/4194B0and949494, matchingblip-lum.md.lumparse arm; dropping the parse clamp; perturbing either branch of the Rust slope; dropping the layout clamp; returningNonefrom the layout arm; perturbing the TypeScript slope.origin/mainfrom the fixture carries no bitmap effects, gains exactly the fourLuminanceeffects when its source is reattached on this branch, equals a fresh snapshot, and saves byte-identically per ZIP part.create-demo-deck,create-demo-workbook,check:seeds, and a cleangit diffon both committed assets.Closes #351