From 0a73eb4678e0b996762c9ab0144c744f012f6fcd Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 09:39:41 +0200 Subject: [PATCH 1/4] fix(svm): draw through a graphics state stack, and stop the fill killing the stroke `PUSH` and `POP` were skipped like any unimplemented action, so the state stack did not exist: a colour, font or map mode set inside a push leaked out of it and contaminated everything after. That is the most common thing we got wrong - of 1125 metafiles harvested from the odt/ods fixtures, 1124 push, 22317 times in all. The flags matter too: 20919 of those pushes save five of the fourteen state groups, so restoring everything on a pop would be wrong far more often than restoring nothing. Four more, all in what we already drew: - **The fill killed the stroke.** A rectangle or polygon wrote the line style and then the fill style, and the fill style ended in `stroke:none`, so a filled shape never had an outline. The pen outlines what the brush fills. - **A poly-polygon is one shape.** It was one `` per sub-polygon, so a donut, a ring and the counter of an "o" came out solid. Now one `` with `fill-rule:evenodd`, which is what vcl fills with - LibreOffice's own svg export puts that rule on its root element. - **A line's `LineInfo` was read and dropped.** Width, dash pattern and join now reach the stroke; only a default `LineInfo` keeps the hairline. - **The font size did not scale.** Coordinates went through the map mode and `font-size` did not, so text kept its size while the drawing around it shrank. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01CmCr22NW6wPQKiQidk96bq --- CHANGELOG.md | 8 + src/odr/internal/svm/PLAN.md | 63 +++++- src/odr/internal/svm/svm_format.cpp | 15 ++ src/odr/internal/svm/svm_format.hpp | 29 +++ src/odr/internal/svm/svm_to_svg.cpp | 338 +++++++++++++++++++++------- test/src/internal/svm/svm_test.cpp | 209 +++++++++++++++-- 6 files changed, 542 insertions(+), 120 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82312629a..004d9461c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ The release run heads these entries with the version and opens a fresh - A spreadsheet decodes in less memory: 626 MB peak instead of 914 MB on a 297 MB `content.xml`. Rendered output is unchanged. +- A StarView metafile draws through a graphics state stack, so a colour, font + or map mode set inside a `PUSH` no longer leaks out of it and contaminates + the rest of the drawing. Nearly every metafile a document carries uses one. + +- A filled shape in a StarView metafile keeps its outline, its poly-polygons + cut their holes out, a line takes the width, dashing and join it carries, and + the font size scales with the drawing. + - Text in a StarView metafile is escaped into the svg it renders as. An `&`, `<` or `>` in a label made the svg malformed, and a malformed svg renders as nothing. diff --git a/src/odr/internal/svm/PLAN.md b/src/odr/internal/svm/PLAN.md index 25ded3a6b..e8ab5764b 100644 --- a/src/odr/internal/svm/PLAN.md +++ b/src/odr/internal/svm/PLAN.md @@ -19,21 +19,60 @@ Each stage is one pull request, stacked on the one before it. #772's defects 1 (escaping, but see stage 3 for the encoding half of it), 9 (style dispatch) and 10 (silence). 2. **Fixes to what we already emit.** The graphics state stack (`PUSH`/`POP`), - poly-polygon fill rule, the font size and map-mode unit in the transform, - `LineInfo`. #772 defects 2, 3, 5, 6, 8. -3. **Text.** `TEXTALIGN`, the `TEXTARRAY` dx array, `TEXTRECT`, the #95 font - attributes (bold, italic, underline, strikeout, family), and decoding a - non-`UCS2` string instead of passing its bytes through — until then a - latin-1 label emits invalid utf-8, which costs the image exactly as an - unescaped `&` did. -4. **Primitives.** `PIXEL`, `POINT`, `LINE`, `ROUNDRECT`, `ELLIPSE`, `ARC`, - `PIE`, `CHORD` — one `svgwriter.cxx` case each. + the fill that killed the stroke, the poly-polygon fill rule, the font size + in the transform, `LineInfo`. #772 defects 2, 3, 5, 8. +3. **Text.** `TEXTALIGN`, the `TEXTARRAY` dx array, the `STRETCHTEXT` width, + `TEXTRECT`, the #95 font attributes (bold, italic, underline, strikeout, + family), and decoding a non-`UCS2` string instead of passing its bytes + through — until then a latin-1 label emits invalid utf-8, which costs the + image exactly as an unescaped `&` did. +4. **Clipping.** `CLIPREGION`, `ISECTRECTCLIPREGION`, + `ISECTREGIONCLIPREGION`, `MOVECLIPREGION`. 5. **Bitmaps** (#194). See the shortcut below. -6. **Fills, clipping, transparency.** `GRADIENT`, `GRADIENTEX`, `HATCH`, - `WALLPAPER`, the `CLIPREGION` family, `TRANSPARENT`, `FLOATTRANSPARENT`. -7. **Stretch.** Bézier flags (#772 defect 4), the `EPS` substitute metafile, +6. **Primitives.** `PIXEL`, `POINT`, `LINE`, `ROUNDRECT`, `ELLIPSE`, `ARC`, + `PIE`, `CHORD` — one `svgwriter.cxx` case each. +7. **Fills and transparency.** `GRADIENT`, `GRADIENTEX`, `HATCH`, + `WALLPAPER`, `TRANSPARENT`, `FLOATTRANSPARENT`. +8. **The map mode's unit** (#772 defect 6), see below. +9. **Stretch.** Bézier flags (#772 defect 4), the `EPS` substitute metafile, and version-1 (pre-`VCLMTF`) files via `SvmConverter.cxx`. +The order follows what files actually contain, not the action list. Over 1125 +metafiles harvested from the `odt`/`ods` fixtures: + +| action | occurrences | files (of 1125) | +| --- | --- | --- | +| `PUSH` / `POP` | 22317 each | 1124 | +| `TEXTALIGN` | 21534 | 1117 | +| `STRETCHTEXT` | 20335 | 1097 | +| `ISECTRECTCLIPREGION` | 1124 | 1123 | +| `RECT` | 845 | 324 | +| `TEXTARRAY` | 764 | 20 | +| `POLYLINE` | 477 | 13 | +| `POLYPOLYGON` | 247 | 14 | +| `LINE` | 47 | 2 | +| `BMPEXSCALE` | 1 | 1 | + +`ELLIPSE`, `ARC`, `PIE`, `CHORD`, `ROUNDRECT`, `POINT`, `PIXEL`, `GRADIENT`, +`HATCH`, `TRANSPARENT` and `EPS` do not occur at all, which is why they come +after clipping rather than before it. The one bitmap is the whole data area of +`odr-private/svm/Vyplaty.svm` — 1.59 MB of `BMPEXSCALE` in a 1.63 MB file, and +the reason that chart renders as an empty frame today. + +## The map mode + +Deferred, and not just an oversight — the units are one part of a bigger +question. `MetaMapModeAction::Execute` calls `OutputDevice::SetMapMode`, which +*replaces* the map mode, **except** where the new one's unit is +`MapUnit::MapRelative` (13): then its scales multiply the current ones and its +origin offsets the current one. We replace unconditionally, and we ignore the +unit, so a `MAPMODE` action that switches from 100th mm to twips is off by a +factor of 1.76. + +It is rare — 4 `MAPMODE` actions in 1125 files, one of them relative — and +getting it right means following `vcl/source/outdev/map.cxx` rather than +guessing, so it is its own stage. + ## Shortcuts worth taking - **Bitmaps are `.bmp` files already.** `SvmReader` reads them with diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index cc6d41ec7..19ef3a525 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -287,6 +287,11 @@ svm::MapMode svm::read_map_mode(std::istream &in) { result.scale_y = read_int_pair(in); read_primitive(in, result.simple); + // the scales are fractions, and every coordinate goes through them + if (result.scale_x.y == 0 || result.scale_y.y == 0) { + throw MalformedSvmFile(); + } + return result; } @@ -487,6 +492,16 @@ svm::read_text_rectangle_action(std::istream &in, const VersionLength &vl, return result; } +std::uint16_t svm::read_push_action(std::istream &in, const VersionLength &vl) { + if (vl.length < sizeof(std::uint16_t)) { + return PUSH_ALL; + } + + std::uint16_t result; + read_primitive(in, result); + return result; +} + svm::TextLineAction svm::read_text_line_action(std::istream &in, const VersionLength &vl) { TextLineAction result; diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index 177787360..a8117fbc5 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -18,6 +18,33 @@ enum TextEncoding { RTL_TEXTENCODING_UCS2 = 0xFFFF, }; +/// `LineStyle`, what a `LineInfo` draws with. +enum MetaLineStyle { + LINE_NONE = 0, + LINE_SOLID = 1, + LINE_DASH = 2, +}; + +/// `vcl::PushFlags`: what a `PUSH` saves and its `POP` restores. Anything not +/// named survives the pop. +enum PushFlags : std::uint16_t { + PUSH_LINECOLOR = 0x0001, + PUSH_FILLCOLOR = 0x0002, + PUSH_FONT = 0x0004, + PUSH_TEXTCOLOR = 0x0008, + PUSH_MAPMODE = 0x0010, + PUSH_CLIPREGION = 0x0020, + PUSH_RASTEROP = 0x0040, + PUSH_TEXTFILLCOLOR = 0x0080, + PUSH_TEXTALIGN = 0x0100, + PUSH_REFPOINT = 0x0200, + PUSH_TEXTLINECOLOR = 0x0400, + PUSH_TEXTLAYOUTMODE = 0x0800, + PUSH_TEXTLANGUAGE = 0x1000, + PUSH_OVERLINECOLOR = 0x2000, + PUSH_ALL = 0xffff, +}; + enum MetaActionType { META_NULL_ACTION = 0, META_PIXEL_ACTION = 100, @@ -252,5 +279,7 @@ TextRectangleAction read_text_rectangle_action(std::istream &in, const VersionLength &vl, TextEncoding encoding); TextLineAction read_text_line_action(std::istream &in, const VersionLength &vl); +/// The `PushFlags` of a `PUSH`. A version that carries none saves everything. +std::uint16_t read_push_action(std::istream &in, const VersionLength &vl); } // namespace odr::internal::svm diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index 2a9a4f1e3..5bef56084 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -7,53 +7,75 @@ #include #include +#include +#include #include +#include +#include namespace odr::internal::svm { namespace { -/// Which of the graphics state's colours a shape draws with. -enum class StyleKind { - line, ///< stroke, no fill - fill, ///< fill, no stroke - text, ///< the text colour as fill, plus the font -}; +/// A file nests three or four deep; a thousand is a broken file, not a drawing. +constexpr std::size_t max_push_depth = 1024; std::string action_name(const ActionHeader &action_header) { return std::string(action_type_name(action_header.type)) + "(" + std::to_string(action_header.type) + ")"; } -struct Context final { - svg::SvgWriter *out{}; - const Logger *logger{}; - +/// The drawing state an action reads and a `PUSH` saves, one field group per +/// `PushFlags` bit we model. +struct GraphicsState final { MapMode map_mode; - TextEncoding encoding{}; Font font; - TextLineAction text_line; - std::uint32_t fill_rgb{}; - bool fill_rgb_set{}; + TextEncoding encoding{RTL_TEXTENCODING_ASCII_US}; std::uint32_t line_rgb{}; bool line_rgb_set{}; - std::uint32_t over_line_rgb{}; + std::uint32_t fill_rgb{}; + bool fill_rgb_set{}; std::uint32_t text_rgb{}; std::uint32_t text_fill_rgb{}; bool text_fill_rgb_set{}; + std::uint32_t over_line_rgb{}; +}; + +struct SavedState final { + GraphicsState state; + /// What the `PUSH` asked to have restored; the rest survives its `POP`. + std::uint16_t flags{}; +}; + +struct Context final { + svg::SvgWriter *out{}; + const Logger *logger{}; + + GraphicsState state; + std::vector stack; }; -double transform(const std::int32_t coordinate, const std::int32_t origin, - const IntPair scale) { - return (origin + coordinate) * static_cast(scale.x) / scale.y; +double scale(const IntPair fraction) { + return static_cast(fraction.x) / fraction.y; } double transform_x(const std::int32_t x, const Context &context) { - return transform(x, context.map_mode.origin.x, context.map_mode.scale_x); + const MapMode &map_mode = context.state.map_mode; + return (map_mode.origin.x + x) * scale(map_mode.scale_x); } double transform_y(const std::int32_t y, const Context &context) { - return transform(y, context.map_mode.origin.y, context.map_mode.scale_y); + const MapMode &map_mode = context.state.map_mode; + return (map_mode.origin.y + y) * scale(map_mode.scale_y); +} + +/// A length carries no origin, only the scale. +double transform_width(const std::int32_t width, const Context &context) { + return width * scale(context.state.map_mode.scale_x); +} + +double transform_height(const std::int32_t height, const Context &context) { + return height * scale(context.state.map_mode.scale_y); } std::string get_svg_color_string(const std::uint32_t color) { @@ -64,47 +86,104 @@ std::string get_svg_color_string(const std::uint32_t color) { std::to_string(blue) + ")"; } -/// The colour, or `-opacity:0` where the state sets none. -void write_color_style(svg::SvgWriter &out, const std::string &property, +void write_color_style(svg::SvgWriter &out, const std::string_view property, const std::uint32_t color, const bool set) { - if (set) { - out.write_style(property, get_svg_color_string(color)); - } else { - out.write_style(property + "-opacity", "0"); + out.write_style(property, set ? get_svg_color_string(color) : "none"); +} + +/// Nothing set: a hairline, drawn solid. +bool is_default(const LineInfo &line_info) { + return line_info.width == 0 && line_info.line_style != LINE_DASH; +} + +/// `LineInfo::GetDotDashArray`: every dash, then every dot, each followed by +/// the distance to the next. +std::string get_dash_array_string(const LineInfo &line_info, + const Context &context) { + std::string result; + const auto append = [&](const std::int32_t length) { + if (!result.empty()) { + result += ","; + } + result += svg::format_number(transform_width(length, context)); + result += ","; + result += svg::format_number(transform_width(line_info.distance, context)); + }; + + for (std::uint16_t i = 0; i < line_info.dash_count; ++i) { + append(line_info.dash_length); } + for (std::uint16_t i = 0; i < line_info.dot_count; ++i) { + append(line_info.dot_length); + } + + return result; } -void write_line_style(svg::SvgWriter &out, const Context &context) { - write_color_style(out, "stroke", context.line_rgb, context.line_rgb_set); - out.write_style("vector-effect", "non-scaling-stroke"); - out.write_style("fill", "none"); +/// `basegfx::B2DLineJoin`. +std::string_view get_line_join_string(const std::uint16_t line_join) { + switch (line_join) { + case 2: + return "bevel"; + case 4: + return "round"; + default: + return "miter"; + } +} + +/// The pen: the state's line colour, and the width, join and dashing of the +/// action's own `LineInfo` where it carries one. +void write_stroke_style(svg::SvgWriter &out, const Context &context, + const LineInfo *line_info) { + const GraphicsState &state = context.state; + write_color_style(out, "stroke", state.line_rgb, state.line_rgb_set); + + if (line_info == nullptr || is_default(*line_info)) { + // a hairline is one device pixel wide however far the drawing is scaled + out.write_style("vector-effect", "non-scaling-stroke"); + return; + } + + out.write_style("stroke-width", transform_width(line_info->width, context)); + out.write_style("stroke-linejoin", + get_line_join_string(line_info->line_join)); + if (line_info->line_style == LINE_DASH) { + if (const std::string dash_array = + get_dash_array_string(*line_info, context); + !dash_array.empty()) { + out.write_style("stroke-dasharray", dash_array); + } + } } +/// The brush. Sub-polygons of one shape are holes in it, which is what +/// `evenodd` cuts out. void write_fill_style(svg::SvgWriter &out, const Context &context) { - write_color_style(out, "fill", context.fill_rgb, context.fill_rgb_set); - out.write_style("stroke", "none"); + write_color_style(out, "fill", context.state.fill_rgb, + context.state.fill_rgb_set); + out.write_style("fill-rule", "evenodd"); } void write_text_style(svg::SvgWriter &out, const Context &context) { - write_color_style(out, "fill", context.text_rgb, true); - out.write_style("font-family", context.font.family_name); - out.write_style("font-size", context.font.size.y); + const GraphicsState &state = context.state; + write_color_style(out, "fill", state.text_rgb, true); + out.write_style("stroke", "none"); + out.write_style("font-family", state.font.family_name); + // the size is a length in the drawing and scales with it + out.write_style("font-size", + std::abs(transform_height(state.font.size.y, context))); } -void write_style(svg::SvgWriter &out, const Context &context, - const StyleKind kind) { - switch (kind) { - case StyleKind::line: - write_line_style(out, context); - break; - case StyleKind::fill: - // TODO the fill overrides the stroke just written - write_line_style(out, context); +/// A shape is outlined by the pen and filled by the brush; a line is only +/// drawn. +void write_shape_style(svg::SvgWriter &out, const Context &context, + const bool fill, const LineInfo *line_info = nullptr) { + write_stroke_style(out, context, line_info); + if (fill) { write_fill_style(out, context); - break; - case StyleKind::text: - write_text_style(out, context); - break; + } else { + out.write_style("fill", "none"); } } @@ -118,25 +197,58 @@ void write_rectangle(const Rectangle &rect, const Context &context) { transform_x(rect.left, context)); out.write_attribute("height", transform_y(rect.bottom, context) - transform_y(rect.top, context)); - write_style(out, context, StyleKind::fill); + write_shape_style(out, context, true); out.write_element_end(); } -void write_polygon(const std::string &tag, const std::vector &points, - const bool fill, const Context &context) { - svg::SvgWriter &out = *context.out; +/// `svgwriter.cxx`'s `GetPathString`: one `M`, the rest of the points as one +/// `L` run, closed with `Z` where the shape is closed. +std::string +get_path_data_string(const std::vector> &polygons, + const bool close, const Context &context) { + std::string result; + + const auto append_point = [&](const IntPair point) { + result += svg::format_number(transform_x(point.x, context)); + result += ","; + result += svg::format_number(transform_y(point.y, context)); + }; + + for (const auto &polygon : polygons) { + if (polygon.size() < 2) { + continue; + } - std::string points_attribute; - for (const auto [x, y] : points) { - points_attribute += svg::format_number(transform_x(x, context)); - points_attribute += ","; - points_attribute += svg::format_number(transform_y(y, context)); - points_attribute += " "; + if (!result.empty()) { + result += " "; + } + result += "M "; + append_point(polygon.front()); + result += " L"; + for (const IntPair point : polygon | std::views::drop(1)) { + result += " "; + append_point(point); + } + // a polyline that ends where it began is a closed shape after all + if (close || (polygon.front().x == polygon.back().x && + polygon.front().y == polygon.back().y)) { + result += " Z"; + } } - out.write_element_begin(tag); - out.write_attribute("points", points_attribute); - write_style(out, context, fill ? StyleKind::fill : StyleKind::line); + return result; +} + +/// Every polygon of a poly-polygon goes into **one** path: they are one shape, +/// and only then does the fill rule cut its holes out. +void write_path(const std::vector> &polygons, + const bool fill, const LineInfo *line_info, + const Context &context) { + svg::SvgWriter &out = *context.out; + + out.write_element_begin("path"); + out.write_attribute("d", get_path_data_string(polygons, fill, context)); + write_shape_style(out, context, fill, line_info); out.write_element_end(); } @@ -147,73 +259,122 @@ void write_text(const IntPair &point, const std::string &text, out.write_element_begin("text"); out.write_attribute("x", transform_x(point.x, context)); out.write_attribute("y", transform_y(point.y, context)); - write_style(out, context, StyleKind::text); + write_text_style(out, context); out.write_text(text); out.write_element_end(); } +void push_state(const std::uint16_t flags, Context &context) { + if (context.stack.size() >= max_push_depth) { + throw MalformedSvmFile(); + } + context.stack.push_back({context.state, flags}); +} + +/// Restores what the matching `PUSH` asked for, and leaves the rest as the +/// actions inside it left it. +void pop_state(Context &context) { + if (context.stack.empty()) { + ODR_WARNING(*context.logger, "pop without a push, ignoring"); + return; + } + + const SavedState saved = std::move(context.stack.back()); + context.stack.pop_back(); + + GraphicsState &state = context.state; + if (saved.flags & PUSH_LINECOLOR) { + state.line_rgb = saved.state.line_rgb; + state.line_rgb_set = saved.state.line_rgb_set; + } + if (saved.flags & PUSH_FILLCOLOR) { + state.fill_rgb = saved.state.fill_rgb; + state.fill_rgb_set = saved.state.fill_rgb_set; + } + if (saved.flags & PUSH_FONT) { + state.font = saved.state.font; + state.encoding = saved.state.encoding; + } + if (saved.flags & PUSH_TEXTCOLOR) { + state.text_rgb = saved.state.text_rgb; + } + if (saved.flags & PUSH_MAPMODE) { + state.map_mode = saved.state.map_mode; + } + if (saved.flags & PUSH_TEXTFILLCOLOR) { + state.text_fill_rgb = saved.state.text_fill_rgb; + state.text_fill_rgb_set = saved.state.text_fill_rgb_set; + } + if (saved.flags & PUSH_OVERLINECOLOR) { + state.over_line_rgb = saved.state.over_line_rgb; + } +} + void translate_action(const ActionHeader &action_header, std::istream &in, Context &context) { + GraphicsState &state = context.state; + switch (action_header.type) { + case META_PUSH_ACTION: + push_state(read_push_action(in, action_header.vl), context); + break; + case META_POP_ACTION: + pop_state(context); + break; case META_FILLCOLOR_ACTION: - read_primitive(in, context.fill_rgb); - read_primitive(in, context.fill_rgb_set); + read_primitive(in, state.fill_rgb); + read_primitive(in, state.fill_rgb_set); break; case META_LINECOLOR_ACTION: - read_primitive(in, context.line_rgb); - read_primitive(in, context.line_rgb_set); + read_primitive(in, state.line_rgb); + read_primitive(in, state.line_rgb_set); break; case META_OVERLINECOLOR_ACTION: - read_primitive(in, context.over_line_rgb); + read_primitive(in, state.over_line_rgb); break; case META_TEXTCOLOR_ACTION: - read_primitive(in, context.text_rgb); + read_primitive(in, state.text_rgb); break; case META_TEXTFILLCOLOR_ACTION: - read_primitive(in, context.text_fill_rgb); - read_primitive(in, context.text_fill_rgb_set); + read_primitive(in, state.text_fill_rgb); + read_primitive(in, state.text_fill_rgb_set); break; case META_FONT_ACTION: - context.font = read_font(in); - context.encoding = static_cast(context.font.charset); + state.font = read_font(in); + state.encoding = static_cast(state.font.charset); break; - case META_TEXTLINE_ACTION: - context.text_line = read_text_line_action(in, action_header.vl); + case META_MAPMODE_ACTION: + state.map_mode = read_map_mode(in); break; case META_RECT_ACTION: { const Rectangle action = read_rectangle(in); write_rectangle(action, context); } break; - case META_MAPMODE_ACTION: { - context.map_mode = read_map_mode(in); - } break; case META_POLYLINE_ACTION: { auto [points, line_info] = read_poly_line_action(in, action_header.vl); - write_polygon("polyline", points, false, context); + write_path({std::move(points)}, false, &line_info, context); } break; case META_POLYGON_ACTION: { auto [points] = read_polygon_action(in, action_header.vl); - write_polygon("polygon", points, true, context); + write_path({std::move(points)}, true, nullptr, context); } break; case META_POLYPOLYGON_ACTION: { auto [polygons] = read_poly_polygon_action(in, action_header.vl); - for (const auto &p : polygons) { - write_polygon("polygon", p, true, context); - } + write_path(polygons, true, nullptr, context); } break; case META_TEXT_ACTION: { const TextAction action = - read_text_action(in, action_header.vl, context.encoding); + read_text_action(in, action_header.vl, state.encoding); write_text(action.point, action.text, context); } break; case META_TEXTARRAY_ACTION: { const TextArrayAction action = - read_text_array_action(in, action_header.vl, context.encoding); + read_text_array_action(in, action_header.vl, state.encoding); write_text(action.point, action.text, context); } break; case META_STRETCHTEXT_ACTION: { const StretchTextAction action = - read_stretch_text_action(in, action_header.vl, context.encoding); + read_stretch_text_action(in, action_header.vl, state.encoding); write_text(action.point, action.text, context); } break; default: @@ -243,8 +404,7 @@ void svm::translate_to_svg(const SvmFile &file, std::ostream &out, const Header header = read_header(in); - context.encoding = RTL_TEXTENCODING_ASCII_US; - context.map_mode = header.map_mode; + context.state.map_mode = header.map_mode; writer.write_element_begin("svg"); writer.write_attribute("xmlns", "http://www.w3.org/2000/svg"); @@ -270,6 +430,10 @@ void svm::translate_to_svg(const SvmFile &file, std::ostream &out, } } + if (!context.stack.empty()) { + ODR_WARNING(logger, context.stack.size() << " pushes were never popped"); + } + writer.write_element_end(); } diff --git a/test/src/internal/svm/svm_test.cpp b/test/src/internal/svm/svm_test.cpp index 64d06f6f9..3e2ba6cc2 100644 --- a/test/src/internal/svm/svm_test.cpp +++ b/test/src/internal/svm/svm_test.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -50,6 +51,33 @@ class SvmBuilder final { return i32(left).i32(top).i32(right).i32(bottom); } + SvmBuilder & + polygon(const std::vector> &points) { + u16(static_cast(points.size())); + for (const auto &[x, y] : points) { + point(x, y); + } + return *this; + } + + /// The font the text actions below draw with, at @p size. + SvmBuilder &font(const std::string &family, const std::int32_t size) { + action(svm::META_FONT_ACTION) + .begin() + .ascii_string(family) + .ascii_string("") + .point(0, size) + .u16(11); // charset: ascii + for (int i = 0; i < 9; ++i) { + u16(0); // family, pitch, weight, underline, strikeout, italic, + // language, width, orientation + } + for (int i = 0; i < 4; ++i) { + u8(0); // wordline, outline, shadow, kerning + } + return end().end(); + } + /// A pascal string, as `read_uint16_prefixed_ascii_string` reads it. SvmBuilder &ascii_string(const std::string &value) { u16(static_cast(value.size())); @@ -114,6 +142,15 @@ class SvmBuilder final { std::vector m_open; }; +std::size_t count_of(const std::string &haystack, const std::string &needle) { + std::size_t result = 0; + for (std::size_t at = haystack.find(needle); at != std::string::npos; + at = haystack.find(needle, at + 1)) { + ++result; + } + return result; +} + std::string translate(const std::string &data) { const svm::SvmFile file(std::make_shared(data)); std::ostringstream out; @@ -181,27 +218,7 @@ TEST(SvmToSvg, text_is_escaped) { TEST(SvmToSvg, font_family_is_escaped) { const std::string svg = translate(SvmBuilder() - .action(svm::META_FONT_ACTION) - .begin() - .ascii_string("a\"b") - .ascii_string("") - .point(0, 10) - .u16(11) // charset - .u16(0) // family - .u16(0) // pitch - .u16(0) // weight - .u16(0) // underline - .u16(0) // strikeout - .u16(0) // italic - .u16(0) // language - .u16(0) // width - .u16(0) // orientation - .u8(0) // wordline - .u8(0) // outline - .u8(0) // shadow - .u8(0) // kerning - .end() - .end() + .font("a\"b", 10) .action(svm::META_TEXT_ACTION) .point(0, 0) .ascii_string("x") @@ -235,3 +252,153 @@ TEST(SvmToSvg, string) { EXPECT_LT(0, out.str().size()); } + +/// The pen outlines what the brush fills - a shape that sets both draws both. +TEST(SvmToSvg, a_shape_is_stroked_and_filled) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_LINECOLOR_ACTION) + .u32(0x00ff00) + .u8(1) + .end() + .action(svm::META_FILLCOLOR_ACTION) + .u32(0x0000ff) + .u8(1) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("stroke:rgb(0,255,0)")); + EXPECT_NE(std::string::npos, svg.find("fill:rgb(0,0,255)")); +} + +/// A colour the state does not set is not drawn at all. +TEST(SvmToSvg, an_unset_colour_draws_nothing) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_LINECOLOR_ACTION) + .u32(0x00ff00) + .u8(0) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("stroke:none")); +} + +/// A pop restores the state the push saved. +TEST(SvmToSvg, pop_restores_what_the_push_saved) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_FILLCOLOR_ACTION) + .u32(0xff0000) + .u8(1) + .end() + .action(svm::META_PUSH_ACTION) + .u16(svm::PUSH_FILLCOLOR) + .end() + .action(svm::META_FILLCOLOR_ACTION) + .u32(0x0000ff) + .u8(1) + .end() + .action(svm::META_POP_ACTION) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("fill:rgb(255,0,0)")); +} + +/// ...and only that: what the flags do not name outlives the pop. Every +/// second push in the corpus saves this subset, so restoring everything would +/// be wrong far more often than not. +TEST(SvmToSvg, pop_keeps_what_the_push_did_not_save) { + const std::string svg = translate(SvmBuilder() + .action(svm::META_PUSH_ACTION) + .u16(svm::PUSH_LINECOLOR) + .end() + .action(svm::META_FILLCOLOR_ACTION) + .u32(0x0000ff) + .u8(1) + .end() + .action(svm::META_POP_ACTION) + .end() + .action(svm::META_RECT_ACTION) + .rectangle(0, 0, 10, 10) + .end() + .file()); + + EXPECT_NE(std::string::npos, svg.find("fill:rgb(0,0,255)")); +} + +/// One shape, one path: a second polygon is a hole in the first, and only the +/// fill rule over a single path cuts it out. +TEST(SvmToSvg, a_poly_polygon_is_one_path) { + const std::string svg = + translate(SvmBuilder() + .action(svm::META_POLYPOLYGON_ACTION, 2) + .u16(2) // polygons + .polygon({{0, 0}, {10, 0}, {10, 10}, {0, 10}}) + .polygon({{2, 2}, {8, 2}, {8, 8}, {2, 8}}) + .u16(0) // complex polygons + .end() + .file()); + + EXPECT_EQ(1, count_of(svg, " Date: Sun, 30 Aug 2026 10:17:12 +0200 Subject: [PATCH 2/4] test: advance the reference output to the graphics state stack The 23 private and 6 public files this branch changes, regenerated. --- test/data.cmake | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/data.cmake b/test/data.cmake index 1905450a1..ac55b605b 100644 --- a/test/data.cmake +++ b/test/data.cmake @@ -17,9 +17,9 @@ odr_test_data( odr_test_data( PATH "reference-output/odr-public" URL "https://github.com/opendocument-app/OpenDocument.test.output.git" - REVISION "54fe51a0e28d95287fa3ca5d1112825706511067") + REVISION "7f139cdd6494523da598ab3625081ae0584f2e7a") odr_test_data( PATH "reference-output/odr-private" URL "https://github.com/opendocument-app/OpenDocument.test-private.output.git" - REVISION "420e0669f520aba75be4eb966f306ef53829dccd") + REVISION "9847681458e5400f3e1e668b0b6f4bfc9074e46e") From 7049fdc5485c3ac79f3aebf2b85b1b069cfff071 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 11:09:09 +0200 Subject: [PATCH 3/4] docs(svm): trim the comments to the house standard No restatement and no narrative: the seven test doc comments each said what their own test name says, and the source comments that told a story are cut to the point they carry. The one fact worth keeping - that half the pushes in the corpus save a subset of `PushFlags`, which is why a `POP` must restore only what its `PUSH` named - moves to `AGENTS.md`, where rationale belongs. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/svm/AGENTS.md | 3 +++ src/odr/internal/svm/svm_format.cpp | 2 +- src/odr/internal/svm/svm_format.hpp | 4 ++-- src/odr/internal/svm/svm_to_svg.cpp | 30 +++++++++++------------------ test/src/internal/svm/svm_test.cpp | 11 ----------- 5 files changed, 17 insertions(+), 33 deletions(-) diff --git a/src/odr/internal/svm/AGENTS.md b/src/odr/internal/svm/AGENTS.md index 46ce9af80..d676252a5 100644 --- a/src/odr/internal/svm/AGENTS.md +++ b/src/odr/internal/svm/AGENTS.md @@ -51,6 +51,9 @@ The format is not specified anywhere. The references, best first: - **An unimplemented action is skipped by its length, never guessed at.** The loop checks how far the reader got: short means the rest is ignored (logged), past the end means the file is malformed and we throw. +- **A `POP` restores only what its `PUSH` named.** Half the pushes in the + corpus save a subset of `PushFlags`, so restoring the whole state would be + wrong more often than right. - **Everything unhandled is logged.** A metafile we cannot draw looks exactly like one we drew correctly — a blank rectangle raises no error anywhere — so the log is the only way to tell. Anything reached by `default:` says so. diff --git a/src/odr/internal/svm/svm_format.cpp b/src/odr/internal/svm/svm_format.cpp index 19ef3a525..54353e8a9 100644 --- a/src/odr/internal/svm/svm_format.cpp +++ b/src/odr/internal/svm/svm_format.cpp @@ -287,7 +287,7 @@ svm::MapMode svm::read_map_mode(std::istream &in) { result.scale_y = read_int_pair(in); read_primitive(in, result.simple); - // the scales are fractions, and every coordinate goes through them + // every coordinate divides by these if (result.scale_x.y == 0 || result.scale_y.y == 0) { throw MalformedSvmFile(); } diff --git a/src/odr/internal/svm/svm_format.hpp b/src/odr/internal/svm/svm_format.hpp index a8117fbc5..1b1e5bfd4 100644 --- a/src/odr/internal/svm/svm_format.hpp +++ b/src/odr/internal/svm/svm_format.hpp @@ -25,8 +25,8 @@ enum MetaLineStyle { LINE_DASH = 2, }; -/// `vcl::PushFlags`: what a `PUSH` saves and its `POP` restores. Anything not -/// named survives the pop. +/// `vcl::PushFlags`: what a `PUSH` saves; anything not named survives the +/// `POP`. enum PushFlags : std::uint16_t { PUSH_LINECOLOR = 0x0001, PUSH_FILLCOLOR = 0x0002, diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index 5bef56084..aac38613d 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -17,7 +17,7 @@ namespace odr::internal::svm { namespace { -/// A file nests three or four deep; a thousand is a broken file, not a drawing. +/// Beyond this the nesting is a broken file, not a drawing. constexpr std::size_t max_push_depth = 1024; std::string action_name(const ActionHeader &action_header) { @@ -25,8 +25,7 @@ std::string action_name(const ActionHeader &action_header) { std::to_string(action_header.type) + ")"; } -/// The drawing state an action reads and a `PUSH` saves, one field group per -/// `PushFlags` bit we model. +/// What an action reads and a `PUSH` saves, grouped by `PushFlags` bit. struct GraphicsState final { MapMode map_mode; Font font; @@ -43,7 +42,7 @@ struct GraphicsState final { struct SavedState final { GraphicsState state; - /// What the `PUSH` asked to have restored; the rest survives its `POP`. + /// What the `PUSH` named for restoring. std::uint16_t flags{}; }; @@ -132,15 +131,15 @@ std::string_view get_line_join_string(const std::uint16_t line_join) { } } -/// The pen: the state's line colour, and the width, join and dashing of the -/// action's own `LineInfo` where it carries one. +/// The state's line colour, plus the width, join and dashing of the action's +/// own `LineInfo` where it carries one. void write_stroke_style(svg::SvgWriter &out, const Context &context, const LineInfo *line_info) { const GraphicsState &state = context.state; write_color_style(out, "stroke", state.line_rgb, state.line_rgb_set); if (line_info == nullptr || is_default(*line_info)) { - // a hairline is one device pixel wide however far the drawing is scaled + // one device pixel wide however far the drawing is scaled out.write_style("vector-effect", "non-scaling-stroke"); return; } @@ -157,8 +156,7 @@ void write_stroke_style(svg::SvgWriter &out, const Context &context, } } -/// The brush. Sub-polygons of one shape are holes in it, which is what -/// `evenodd` cuts out. +/// `evenodd` is what cuts a shape's sub-polygons out as holes. void write_fill_style(svg::SvgWriter &out, const Context &context) { write_color_style(out, "fill", context.state.fill_rgb, context.state.fill_rgb_set); @@ -170,13 +168,10 @@ void write_text_style(svg::SvgWriter &out, const Context &context) { write_color_style(out, "fill", state.text_rgb, true); out.write_style("stroke", "none"); out.write_style("font-family", state.font.family_name); - // the size is a length in the drawing and scales with it out.write_style("font-size", std::abs(transform_height(state.font.size.y, context))); } -/// A shape is outlined by the pen and filled by the brush; a line is only -/// drawn. void write_shape_style(svg::SvgWriter &out, const Context &context, const bool fill, const LineInfo *line_info = nullptr) { write_stroke_style(out, context, line_info); @@ -201,8 +196,7 @@ void write_rectangle(const Rectangle &rect, const Context &context) { out.write_element_end(); } -/// `svgwriter.cxx`'s `GetPathString`: one `M`, the rest of the points as one -/// `L` run, closed with `Z` where the shape is closed. +/// `svgwriter.cxx`'s `GetPathString`: one `M`, one `L` run, `Z` where closed. std::string get_path_data_string(const std::vector> &polygons, const bool close, const Context &context) { @@ -229,7 +223,7 @@ get_path_data_string(const std::vector> &polygons, result += " "; append_point(point); } - // a polyline that ends where it began is a closed shape after all + // a polyline that ends where it began is closed if (close || (polygon.front().x == polygon.back().x && polygon.front().y == polygon.back().y)) { result += " Z"; @@ -239,8 +233,7 @@ get_path_data_string(const std::vector> &polygons, return result; } -/// Every polygon of a poly-polygon goes into **one** path: they are one shape, -/// and only then does the fill rule cut its holes out. +/// One path for all of them: the fill rule only cuts holes within a path. void write_path(const std::vector> &polygons, const bool fill, const LineInfo *line_info, const Context &context) { @@ -271,8 +264,7 @@ void push_state(const std::uint16_t flags, Context &context) { context.stack.push_back({context.state, flags}); } -/// Restores what the matching `PUSH` asked for, and leaves the rest as the -/// actions inside it left it. +/// Restores only what the matching `PUSH` named. void pop_state(Context &context) { if (context.stack.empty()) { ODR_WARNING(*context.logger, "pop without a push, ignoring"); diff --git a/test/src/internal/svm/svm_test.cpp b/test/src/internal/svm/svm_test.cpp index 3e2ba6cc2..6eb6438ba 100644 --- a/test/src/internal/svm/svm_test.cpp +++ b/test/src/internal/svm/svm_test.cpp @@ -253,7 +253,6 @@ TEST(SvmToSvg, string) { EXPECT_LT(0, out.str().size()); } -/// The pen outlines what the brush fills - a shape that sets both draws both. TEST(SvmToSvg, a_shape_is_stroked_and_filled) { const std::string svg = translate(SvmBuilder() .action(svm::META_LINECOLOR_ACTION) @@ -273,7 +272,6 @@ TEST(SvmToSvg, a_shape_is_stroked_and_filled) { EXPECT_NE(std::string::npos, svg.find("fill:rgb(0,0,255)")); } -/// A colour the state does not set is not drawn at all. TEST(SvmToSvg, an_unset_colour_draws_nothing) { const std::string svg = translate(SvmBuilder() .action(svm::META_LINECOLOR_ACTION) @@ -288,7 +286,6 @@ TEST(SvmToSvg, an_unset_colour_draws_nothing) { EXPECT_NE(std::string::npos, svg.find("stroke:none")); } -/// A pop restores the state the push saved. TEST(SvmToSvg, pop_restores_what_the_push_saved) { const std::string svg = translate(SvmBuilder() .action(svm::META_FILLCOLOR_ACTION) @@ -312,9 +309,6 @@ TEST(SvmToSvg, pop_restores_what_the_push_saved) { EXPECT_NE(std::string::npos, svg.find("fill:rgb(255,0,0)")); } -/// ...and only that: what the flags do not name outlives the pop. Every -/// second push in the corpus saves this subset, so restoring everything would -/// be wrong far more often than not. TEST(SvmToSvg, pop_keeps_what_the_push_did_not_save) { const std::string svg = translate(SvmBuilder() .action(svm::META_PUSH_ACTION) @@ -334,8 +328,6 @@ TEST(SvmToSvg, pop_keeps_what_the_push_did_not_save) { EXPECT_NE(std::string::npos, svg.find("fill:rgb(0,0,255)")); } -/// One shape, one path: a second polygon is a hole in the first, and only the -/// fill rule over a single path cuts it out. TEST(SvmToSvg, a_poly_polygon_is_one_path) { const std::string svg = translate(SvmBuilder() @@ -353,7 +345,6 @@ TEST(SvmToSvg, a_poly_polygon_is_one_path) { EXPECT_NE(std::string::npos, svg.find("fill-rule:evenodd")); } -/// A polyline carries its own pen, and its width is a length in the drawing. TEST(SvmToSvg, a_poly_line_takes_its_line_info) { const std::string svg = translate(SvmBuilder() .action(svm::META_POLYLINE_ACTION, 2) @@ -376,8 +367,6 @@ TEST(SvmToSvg, a_poly_line_takes_its_line_info) { EXPECT_NE(std::string::npos, svg.find("stroke-dasharray:6,3")); } -/// The font size is a length in the drawing, so the map mode scales it like -/// every coordinate. TEST(SvmToSvg, the_font_size_is_scaled) { const std::string svg = translate(SvmBuilder() .action(svm::META_MAPMODE_ACTION) From 488de9169f2bf6afb4ecb6d106d65823bc835078 Mon Sep 17 00:00:00 2001 From: Andreas Stefl Date: Sun, 30 Aug 2026 11:16:46 +0200 Subject: [PATCH 4/4] refactor(svm): pass a path's polygons as a span `write_path({std::move(points)}, ...)` moved nothing: a braced-init-list is a `std::initializer_list`, whose elements are `const`, so each call copied the whole point vector. A span carries the one polygon where it already is, and the poly-polygon case is unchanged. Also records why a stroke width and a dash go through the x scale alone, which reads like an oversight and is not: `svgwriter.cxx` maps them with `ImplMap(sal_Int32)`, which builds a square and takes its `Width()`. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01XQpLmJpJ87qbKoG8B7kbLY --- src/odr/internal/svm/svm_to_svg.cpp | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/odr/internal/svm/svm_to_svg.cpp b/src/odr/internal/svm/svm_to_svg.cpp index aac38613d..c99c258cf 100644 --- a/src/odr/internal/svm/svm_to_svg.cpp +++ b/src/odr/internal/svm/svm_to_svg.cpp @@ -9,6 +9,7 @@ #include #include +#include #include #include #include @@ -68,7 +69,9 @@ double transform_y(const std::int32_t y, const Context &context) { return (map_mode.origin.y + y) * scale(map_mode.scale_y); } -/// A length carries no origin, only the scale. +/// A length carries no origin, only the scale. The x scale even for a stroke +/// width or a dash, which lie along no axis: `svgwriter.cxx` maps those +/// through `ImplMap(sal_Int32)`, which takes the `Width()` of a square. double transform_width(const std::int32_t width, const Context &context) { return width * scale(context.state.map_mode.scale_x); } @@ -198,7 +201,7 @@ void write_rectangle(const Rectangle &rect, const Context &context) { /// `svgwriter.cxx`'s `GetPathString`: one `M`, one `L` run, `Z` where closed. std::string -get_path_data_string(const std::vector> &polygons, +get_path_data_string(const std::span> polygons, const bool close, const Context &context) { std::string result; @@ -234,7 +237,7 @@ get_path_data_string(const std::vector> &polygons, } /// One path for all of them: the fill rule only cuts holes within a path. -void write_path(const std::vector> &polygons, +void write_path(const std::span> polygons, const bool fill, const LineInfo *line_info, const Context &context) { svg::SvgWriter &out = *context.out; @@ -343,15 +346,16 @@ void translate_action(const ActionHeader &action_header, std::istream &in, write_rectangle(action, context); } break; case META_POLYLINE_ACTION: { - auto [points, line_info] = read_poly_line_action(in, action_header.vl); - write_path({std::move(points)}, false, &line_info, context); + const auto [points, line_info] = + read_poly_line_action(in, action_header.vl); + write_path({&points, 1}, false, &line_info, context); } break; case META_POLYGON_ACTION: { - auto [points] = read_polygon_action(in, action_header.vl); - write_path({std::move(points)}, true, nullptr, context); + const auto [points] = read_polygon_action(in, action_header.vl); + write_path({&points, 1}, true, nullptr, context); } break; case META_POLYPOLYGON_ACTION: { - auto [polygons] = read_poly_polygon_action(in, action_header.vl); + const auto [polygons] = read_poly_polygon_action(in, action_header.vl); write_path(polygons, true, nullptr, context); } break; case META_TEXT_ACTION: {