Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't paint the background if the PDF has transparency #2804

Merged
merged 1 commit into from
May 21, 2022

Conversation

DarthSim
Copy link
Contributor

Hey again!

Some PDF pages require the background to be transparent to be rendered properly. Luckily, FPDFPage_HasTransparency returns TRUE for such pages so we can easily determine if we should fill the canvas before rendering. PDFium sample tool does the same check BTW: https://pdfium.googlesource.com/pdfium/+/refs/heads/main/samples/pdfium_test.cc#864.

Also, I used FPDFBitmap_FillRect to fill the background of a single page instead of a whole canvas. And I changed pdf->ink type so it can be used with FPDFBitmap_FillRect, hope it's ok.

Unfortunately, I can't share a sample I have as it was provided by a customer of our customer so I definitely have no right to share it anywhere.

@jcupitt
Copy link
Member

jcupitt commented May 16, 2022

Hey @DarthSim, this looks good.

I remember this coming up before: #995

Here's the test image from that issue: test.pdf

I tried with poppler:

vips copy "test.pdf[background=0 128 0 128]" x.png

To make:

x

ie. a transparent green background.

I'll have a go with your branch.

@jcupitt
Copy link
Member

jcupitt commented May 16, 2022

(poppler background is premultiplied BGRA, confusingly, we should fix this too)

@jcupitt
Copy link
Member

jcupitt commented May 16, 2022

I'm not sure this change is a good idea. Try this with the master branch and pdfium enabled:

vips copy "test.pdf[background=128 0 0 128]" x.png

To make:

x

(it should be a red background, we should fix this)

Or like this:

vips copy test.pdf[background=0] x.png

To make:

x

(try opening that in an image viewer to see the transparent background)

With this patch, the first example breaks since the transparent blue background is never applied.

Could you solve your user's problem by converting with background=0?

@jcupitt
Copy link
Member

jcupitt commented May 16, 2022

... I pushed something to master to fix the byte ordering of background.

jcupitt added a commit that referenced this pull request May 16, 2022
the `background` param was BGRA rather than vips-style RGBA

see #2804
@DarthSim DarthSim force-pushed the fix/dont-fill-transparent-pdf branch from 851e454 to d10eb45 Compare May 16, 2022 18:25
@DarthSim DarthSim force-pushed the fix/dont-fill-transparent-pdf branch from d10eb45 to 6ad217e Compare May 16, 2022 18:28
@DarthSim
Copy link
Contributor Author

The color in FPDFBitmap_FillRect should be ARGB. Fixed.

vips copy test.pdf[background=0] x.png

pdf3_t

vips copy "test.pdf[background=0 128 0 128]" x.png

pdf3_g

vips copy "test.pdf[background=128 0 0 128]" x.png

pdf3_r

@jcupitt
Copy link
Member

jcupitt commented May 20, 2022

I tried adding:

                if( !(pdf->page = FPDF_LoadPage( pdf->doc, page_no )) ) {
                        g_mutex_unlock( vips_pdfium_mutex );
                        vips_pdfium_error();
                        vips_error( class->nickname,
                                _( "unable to load page %d" ), page_no );
                        return( -1 );
                }
                pdf->current_page = page_no;

                printf( "HasTransparency = %d\n",
                        FPDFPage_HasTransparency(pdf->page) ) ;

to vips_foreign_load_pdf_get_page() and I get:

$ vipsheader ~/pics/test.pdf
HasTransparency = 0
/home/john/pics/test.pdf: 612x792 uchar, 4 bands, srgb, pdfload

Even though that image obviously has a transparent background. I searched my collection of PDFs and I found one which does report transparency:

http://www.rollthepotato.net/~john/bizz_2176220.pdf

There's a single pixel at the bottom right where the alpha is 254 rather than 255.

I don't think FPDFPage_HasTransparency() is reporting anything useful :(

@DarthSim
Copy link
Contributor Author

Even though that image obviously has a transparent background

It is, but FPDFPage_HasTransparency reports if the page background SHOULD be transparent. I mean, if it returns 0, we should fill the background, otherwise, we should. As I mentioned in the PR description, PDFium's sample tool does the same. Also, if we'll take a look at FPDFPage_HasTransparency implementation, we'll see that it just returns pPage->BackgroundAlphaNeeded() which name actually has more sense in the context of this discussion. And if we take a look at what changes this value, we'll see that it depends only on blend mode.

Most raw-text PDFs don't defy their background yet should not be rendered transparent. For example https://code.visualstudio.com/shortcuts/keyboard-shortcuts-windows.pdf.

@jcupitt
Copy link
Member

jcupitt commented May 21, 2022

Ah I understand, thanks for the explanation. Yes, BackgroundAlphaNeeded() seems like a better name.

OK, let's merge, and thank you for pushing for this improvement!

@jcupitt jcupitt merged commit 985fb1d into libvips:master May 21, 2022
@jcupitt
Copy link
Member

jcupitt commented May 21, 2022

I changed it to use vips__bgra2rgba() -- it should work with any byte order.

I suppose this means that the poppler PDF loader is wrong. I'll look for something like FPDFPage_HasTransparency() in their API.

@DarthSim
Copy link
Contributor Author

Great, thanks!

I changed it to use vips__bgra2rgba() -- it should work with any byte order.

Ink in FPDFBitmap_FillRect() should be ARGB: https://pdfium.googlesource.com/pdfium/+/refs/heads/main/public/fpdfview.h#1093

@jcupitt
Copy link
Member

jcupitt commented May 21, 2022

Yes, vips__bgra2rgba swaps B and A, so it also converts RGBA to BGRA (it's a self inverse).

@jcupitt
Copy link
Member

jcupitt commented May 21, 2022

... and ARGB is BGRA but written LSB first.

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.

None yet

2 participants