Skip to content

Conversation

@MacDue
Copy link
Member

@MacDue MacDue commented Oct 19, 2025

While working on #26292, I noticed that while fills/strokes can be Gfx::PaintStyles in LibPDF, currently they never are, which confused me.

It turns out that a few years ago, tiled patterns were implemented in PR #22197, but were partially reverted due to crashes in #22364, leaving around a partial implementation.

It appears the author was going to look into the crashes, but never got around to it. So, this patch resolves all the crashes on 0000.zip, mainly by checking types and not assuming dictionary keys exist.

Summary of ./Meta/test_pdf.py ~/Downloads/0000:

Stacks:
0 crashes (0.0%)
0 distinct crash stacks

7 failed to open (0.7%)
    /home/macdue/Downloads/0000/0000346.pdf
    /home/macdue/Downloads/0000/0000202.pdf
    /home/macdue/Downloads/0000/0000399.pdf
    /home/macdue/Downloads/0000/0000421.pdf
    /home/macdue/Downloads/0000/0000480.pdf
    /home/macdue/Downloads/0000/0000920.pdf
    /home/macdue/Downloads/0000/0000819.pdf

3 files with password (0.3%)

909 files without issues (90.9%)

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 19, 2025
@MacDue
Copy link
Member Author

MacDue commented Oct 19, 2025

patterns.pdf, working again:
Screenshot from 2025-10-19 16-56-02

@LucasChollet
Copy link
Member

I think that we usually modify the original commits themselves, rather than relanding and adding a fixup commit.

@nico
Copy link
Contributor

nico commented Oct 20, 2025

Thanks for the PR!

Happy to merge this (but I agree with @LucasChollet's comment – having HEAD working at all comments is nice in that it makes bisecting easier, etc. It was useful to see what changed, but github makes you choose either a clean history or nice diffs between PR revisions, and serenity chooses the former.)

However, I kind of feel like maybe painting patterns to bitmaps and tiling those might be a better approach than the current approach. That'd be more how other features work, is probably easier to get fast (once we get to that step), interacts nicer with clip masks, type 2 patterns, etc.

(But it's fine to merge the current approach, it's still a step forward.)

Given we have Rendering of feature not supported: Pattern color spaces not yet implemented, in 46 files, on 285 pages, 5478 times before this patch, I'm surprised that the number of files without issues doesn't increase. Looks like we get a bunch of new diags:

Rendering of feature not supported: Unsupported pattern type 2, in 23 files, on 201 pages, 414 times
Malformed PDF file: Pattern resource dictionary missing, in 9 files, on 52 pages, 52 times
Internal error while processing PDF file: Gfx::Bitmap backing store size is empty, in 6 files, on 10 pages, 72 times
Malformed PDF file: Pattern resource dictionary not DictObject, in 3 files, on 8 pages, 2116 times
Malformed PDF file: XObject resource dictionary missing, in 2 files, on 2 pages, 90 times
Malformed PDF file: XObject resource dictionary does not contain Img481, in 1 files, on 1 pages, 37 times

That's a pretty big number of "Malformed PDF"; maybe the code isn't quite right yet. (This isn't a regression, so figuring this out isn't a blocker for merging this, and most of it needs investigating independent of the pattern rendering approach.)

Also, the one real-life example of these patterns that I've consciously seen doesn't seem to improve much with this PR:
Screenshot 2025-10-20 at 7 59 08 AM

@MacDue
Copy link
Member Author

MacDue commented Oct 20, 2025

I don't think this implementation is really correct. I'm guessing just enough was implemented to get patterns.pdf to work but not much more, there's various things around the painting and transforms that look fishy, but I've not looked into things in great detail yet.

My thinking was just to reland the initial patches (fixing up the crashes) and use that as a starting point, as they're at least somewhat along the right lines.

@nico
Copy link
Contributor

nico commented Oct 21, 2025

(Just to reiterate: Sounds great, but please amend the third commit into the first two :))

X-yl and others added 2 commits October 21, 2025 21:25
This reverts commit 6032c06.

This relands the original commit with fixups to make `PatternColorSpace`
more robust to unexpected inputs.

While working on SerenityOS#26292, I noticed that while fills/strokes can be
Gfx::PaintStyles in LibPDF, currently they never are, which confused me.

It turns out that a few years ago, tiled patterns were implemented in
PR SerenityOS#22197, but were partially reverted due to crashes in SerenityOS#22364, leaving
around a partial implementation.

It appears the author was going to look into the crashes, but never got
around to it. So, this patch resolves all the crashes on `0000.zip`,
mainly by checking types and not assuming dictionary keys exist.

Summary of `./Meta/test_pdf.py ~/Downloads/0000`:

```
Stacks:
0 crashes (0.0%)
0 distinct crash stacks

7 failed to open (0.7%)
    /home/macdue/Downloads/0000/0000346.pdf
    /home/macdue/Downloads/0000/0000202.pdf
    /home/macdue/Downloads/0000/0000399.pdf
    /home/macdue/Downloads/0000/0000421.pdf
    /home/macdue/Downloads/0000/0000480.pdf
    /home/macdue/Downloads/0000/0000920.pdf
    /home/macdue/Downloads/0000/0000819.pdf

3 files with password (0.3%)

909 files without issues (90.9%)
```

Co-authored-by: MacDue <[email protected]>
@MacDue
Copy link
Member Author

MacDue commented Oct 21, 2025

Done 👍 I've squashed the fixups into the first commit (as I don't think there are any crashes directly caused by the second commit).

@nico nico merged commit bde4651 into SerenityOS:master Oct 21, 2025
12 checks passed
@github-actions github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Oct 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants