Skip to content

Conversation

@X-yl
Copy link
Contributor

@X-yl X-yl commented Dec 7, 2023

LibPDF vs PDF.js :)

image

So I've added some basic pattern rendering to LibPDF, mainly targeting Adobe's example on p176 of the spec (pictured above). There's a couple structural-y changes that I'm not too sure about:

  • Changing all the ColorSpaces to output PaintStyles instead of Colors: This is so the pattern space can output a PaintStyle that generates the bitmap, but it is a bit overkill for the normal device spaces that have to output a SolidColorPaintStyle.
  • The PatternColorSpace sort of blends in with the Renderer code a bit since it has to render the pattern, so I decided to make it a friend class and pass in a reference to Renderer. Not too sure how to structure it tbh.

Also my C++ is a bit rusty (in more ways than one) so let me know if there's better ways to do things :)

@github-actions github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Dec 7, 2023
@X-yl
Copy link
Contributor Author

X-yl commented Dec 7, 2023

Hmm discovered by tweaking the text matrix of he file I can stop the text clipping:
image

With a change like this:

--- a/Tests/LibPDF/pattern.pdf
+++ b/Tests/LibPDF/pattern.pdf
@@ -23,7 +23,7 @@ stream
 BT % Begin text object
 0.0 -0.05 TD % Move text position
 /F1 1 Tf % Set text font and size
-55 0 0 55 7.1771 2.4414 Tm % Set text matrix
+55 0 0 55 7.1771 10.4414 Tm % Set text matrix
 0 Tc % Set character spacing
 0 Tw % Set word spacing
 1.0 0.0 0.0 rg % Set nonstroking colour to red

But I'm wondering if that's a bug or if we expect the text matrix to be different because of different fonts or something?

Copy link
Contributor

@nico nico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is is super cool!

The PaintStyle change looks extremely clean. However, we use the ColorSpace machinery for transforming images too (cf Renderer::load_image() towards the end) and I worry a bit that creating one PaintStyle object per call to style() will make performance even harder to optimize.

Maybe we should instead just special-case Pattern in Renderer more and do a bunch of is pattern then call style else call color or something like that?

Also, do you think we could pass Renderer to create() and have the Pattern object remember it, instead of passing it to style() all the time?

Finally, the "preferences" object is for user-settable things I think. Maybe we can add a parameter for the bg color?

@X-yl
Copy link
Contributor Author

X-yl commented Dec 8, 2023

Perhaps we could have the style function return a Variant<Color, RefPtr<PaintStyle>> (typedef'd to something less clunky ideally) and then branch on whether it produced a RefPtr<PaintStyle> or a Color?

X-yl added 3 commits December 9, 2023 14:54
This is in anticipation of Pattern color space support which does not
yield a simple color.
RepeatingBitmapPaintStyle tiles a bitmap passed on passed in parameters
and paints according to that.

OffsetPaintStyle offsets an existing paint style and paints with that.

These two will be useful in the PDF renderer.
@X-yl
Copy link
Contributor Author

X-yl commented Dec 9, 2023

Made changes based on feedback :)

I do recognise the difference in semantics between checking if the ColorSpace is pattern and then calling style() or else color() versus having style() returning Variant<Color, PaintStyle> but I think it's a bit cleaner (as opposed to stubbing out style() for almost every ColorSpace) and performance wise is they should be exactly the same since the branch predictor would quite easily predict what each ColorSpace returns?

Copy link
Contributor

@nico nico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

lgtm as-is; some pontificating below (but feel free to ignore; up to you).

@nico
Copy link
Contributor

nico commented Dec 9, 2023

Also, I don't remember if Serenity's PDF viewer supports page zoom (MacPDF doesn't yet). Might be worth thinking about if the pattern rasterization applies zoom at the right layer, so that zooming in doesn't make the pattern pixelated.

@X-yl
Copy link
Contributor Author

X-yl commented Dec 10, 2023

Also, I don't remember if Serenity's PDF viewer supports page zoom (MacPDF doesn't yet). Might be worth thinking about if the pattern rasterization applies zoom at the right layer, so that zooming in doesn't make the pattern pixelated.

So the way I decided on the size for the bitmap for rasterization was to take the pattern bounding box and transform it to device space with the CTM (ETA: after first mapping it to user space with the pattern matrix). This seems to handle scaling nicely, and intuitively I think it's right, but frankly I did a lot of throwing random matrices at the wall to see what stuck so I could be missing something.

crisp wee letters

@nico nico added ✅ pr-community-approved PR has been approved by a community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Dec 10, 2023
@awesomekling awesomekling merged commit 60c7ff9 into SerenityOS:master Dec 10, 2023
@github-actions github-actions bot removed the ✅ pr-community-approved PR has been approved by a community member label Dec 10, 2023
MacDue added a commit to MacDue/serenity that referenced this pull request Oct 19, 2025
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%)
```
MacDue added a commit to MacDue/serenity that referenced this pull request Oct 21, 2025
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]>
nico pushed a commit that referenced this pull request Oct 21, 2025
This reverts commit 6032c06.

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

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%)
```

Co-authored-by: MacDue <[email protected]>
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.

3 participants