fix(pptx): replay metafile gradients and source-free blits - #357
Conversation
- GRADIENTFILL becomes flat-shaded bands, one display-list shape per band, for both rectangle modes and Gouraud triangles. - BITBLT is honoured for the raster operations that name no source bitmap: PATCOPY paints the current brush, BLACKNESS and WHITENESS paint their constant, DSTCOPY draws nothing. Anything else still rejects the metafile. - Vertex counts, index ranges and record sizes are bounded and checked, so a malformed record produces no geometry. 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 adds bounded replay support for EMF gradient fills and source-free BITBLT operations, with a synthetic PPTX fixture and integration coverage.
Confidence Score: 4/5The PR should not merge until triangle gradients preserve color variation across both dimensions of a general Gouraud triangle. The source-free blit, rectangle-gradient, validation, and budget paths are supported, but the triangle algorithm paints each dominant-axis slice with one color and therefore renders valid independently colored vertices incorrectly. Files Needing Attention: crates/pptx-render/src/metafile.rs
|
| Filename | Overview |
|---|---|
| crates/pptx-render/src/metafile.rs | Adds bounded GRADIENTFILL and source-free BITBLT replay, but triangle gradients reduce a generally two-dimensional Gouraud field to uniformly colored one-dimensional slices. |
| crates/pptx-render/tests/metafile_gradient.rs | Verifies fixture-level gradient bands and PATCOPY output, although its triangle colors vary only in one dimension and therefore miss the general Gouraud case. |
| crates/pptx-render/tests/fixtures/metafile-gradient.md | Documents the synthetic EMF records, expected primitive counts, and visual-reference limitations. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[EMF record] --> B{Record type}
B -->|GRADIENTFILL| C[Validate mode, counts, arrays, and indexes]
C --> D{Gradient mode}
D -->|Rectangle| E[Emit overlapping horizontal or vertical bands]
D -->|Triangle| F[Choose dominant channel axis]
F --> G[Clip triangle into overlapping flat-color bands]
B -->|BITBLT| H{Source bitmap declared?}
H -->|Yes| I[Reject metafile]
H -->|No| J{Raster operation}
J -->|DSTCOPY| K[No operation]
J -->|PATCOPY| L[Fill with selected brush]
J -->|BLACKNESS or WHITENESS| M[Fill with constant color]
J -->|Other| I
E --> N[Display-list shapes]
G --> N
L --> N
M --> N
Reviews (1): Last reviewed commit: "fix(pptx): replay metafile gradients and..." | Re-trigger Greptile
| let dominant = (0..3) | ||
| .max_by(|a, b| spread(*a).total_cmp(&spread(*b))) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
Triangle gradients lose variation
A valid Gouraud triangle can have independent colors at all three corners, such as red, green, and blue. This code chooses one color channel as the slicing axis and assigns one uniform sampled color to each complete slice. Color variation along each slice is therefore erased, so these general two-dimensional gradients render incorrectly. The added fixture varies color only along the slicing direction and does not cover this case.
Knowledge Base Used: PPTX rendering and WebAssembly
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Triangle gradients lose variation — correct, and it is an approximation rather than a bug I can You have read it right. The constraint is the display list. What made banding the right trade for this change specifically: every So I have left it, and written it up rather than left it implied — the pull request body and the |
TL;DR:
Before/After:
LibreOffice, before, after, on the committed fixture. LibreOffice's EMF import draws the
rectangle-mode gradient and the pattern blit but leaves triangle-mode gradients blank, so the top
half is empty in the reference pane and shaded in ours; PowerPoint shades it.
Repro file:
crates/pptx-render/tests/fixtures/metafile-gradient.pptx, added here withmetafile-gradient.mddescribing its records. Every corpus slide that reproduces the bug is athird-party presentation and cannot be published, so the fixture is synthetic: one 200x200 unit EMF
holding a triangle-mode gradient, a rectangle-mode gradient, a
PATCOPYblit and aDSTCOPYblit.Summary:
EMR_GRADIENTFILLas flat-shaded bands, onePrimitive::Shapeper band, for bothrectangle modes and for Gouraud triangles. Colour is affine over a triangle, so the bands are
slices taken perpendicular to the gradient of the widest-varying channel, each filled with the
colour sampled at its own midpoint. Band count follows the colour spread and is capped at 64, so
a two-level gradient costs two shapes.
neighbouring bands overlap. Abutting bands leave an antialiased crack on every boundary; this was
visible as white hairlines across the whole gradient before the change.
EMR_BITBLTfor the raster operations that name no source bitmap:DSTCOPYdrawsnothing,
PATCOPYfills the destination with the selected brush,BLACKNESSandWHITENESSfill it with their constant. A blit that declares source bits, or any other ROP, still rejects
the metafile — those genuinely need a raster the display list cannot carry.
single 100-byte no-op blit was deleting an otherwise fully supported picture. That is the actual
user-visible defect; the gradients are the smaller half.
allocation, the record must be long enough for the arrays it declares, and an out-of-range vertex
index rejects the record rather than being clamped. Malformed shapes are tested next to the happy
path, as in the rest of the file.
Where the metric disagrees:
The change moves exactly one corpus slide, in a deck that cannot be named here: fine 14.51 → 14.20,
coarse 9.72 → 9.38,
skipped_images1 → 0. Its EMF was rejected for a singleDSTCOPYrecord andnow draws a small figure icon that matches the reference. Every other slide in the two decks that
own the affected parts is byte-identical.
Nothing else moves, for two reasons worth stating plainly:
PR does not touch (75, 67, 30, 29, 28, 58, 82, 84). The one corpus part carrying
GRADIENTFILLis among them, so no corpus slide shows a metafile gradient even after this change. The
gradient half is carried by the fixture and the unit tests, not by the metric.
a:blip/a:duotone, andrender_picturesends any picture with blip effects down thePrimitive::Imagepath, so itkeeps being skipped. That is the pre-existing interaction between metafiles and blip effects, not
a regression here.
Test plan:
cargo test -p betteroffice-pptx-rendercargo test -p betteroffice-pptx-edit(schema migrations, unaffected: no model change)cargo clippy -p betteroffice-pptx-render --all-targetsandcargo fmt --all -- --checkcargo checkinsidebindings/python-pptx(separate workspace)crates/pptx-render/tests/fixtures/metafile-gradient.pptxand confirmskipped_imagesis 0 and both gradients are smoothCloses #356