WordPress.org
Get WordPress

Make WordPress Core

Opened 8 years ago

Closed 16 months ago

Last modified 7 months ago

#39216 closed defect (bug) (fixed)

PDFs with non-opaque alpha channels can result in previews with black backgrounds.

Reported by: gitlost's profile gitlost Owned by: antpb's profile antpb
Milestone: 6.4 Priority: normal
Severity: normal Version: 4.7
Component: Media Keywords: dev-feedback 2nd-opinion has-patch has-unit-tests early
Focuses: Cc:

Description

If a PDF has a non-opaque alpha channel it can result in a black background when rendering the preview. Attached is a real-world example and also a simple blank(ish) test pdf.

One way around this is to use the Imagick::ALPHACHANNEL_REMOVE mode of setImageAlphaChannel(), as in the attached patch, added as an extra pdf_process() method in view of #38832. The unittest requires the blank test pdf to be uploaded to the test data images directory.

The blank test pdf was generated using TCPDF (​https://tcpdf.org):

<?php
$dirname = dirname( __FILE__ );
$tcpdf_dirname = $dirname . '/tcpdf';
require $tcpdf_dirname . '/tcpdf.php';

$pdf = new TCPDF();
$pdf->AddPage();
$pdf->SetAlpha(0);
$pdf->Output( $dirname . '/test_alpha.pdf', 'F' );

Attachments (8)

contents-hs-issue-41.pdf​ (257.9 KB) - added by gitlost 8 years ago.
Real-world example.
test_alpha.pdf​ (7.0 KB) - added by gitlost 8 years ago.
Simple blank test pdf.
39216.patch​ (3.7 KB) - added by gitlost 8 years ago.
Patch to remove alpha channel. Unittest requires blank test pdf.
test_cmyk.pdf​ (7.4 KB) - added by gitlost 8 years ago.
Test CMYK PDF.
39216.2.patch​ (4.9 KB) - added by gitlost 8 years ago.
Refresh of patch with unsharpMaskImage not called for CMYK.
39216.3.patch​ (2.9 KB) - added by gitlost 8 years ago.
Refresh of original patch without the unsharpMaskImage() change.
39216.4.patch​ (2.9 KB) - added by gitlost 7 years ago.
Refresh, comment changes only.
39216-5.patch​ (3.4 KB) - added by antpb 16 months ago.
Updating coding standards and conflicts

Download all attachments as: .zip

Change History (46)

@gitlost
8 years ago

Real-world example.

@gitlost
8 years ago

Simple blank test pdf.

@gitlost
8 years ago

Patch to remove alpha channel. Unittest requires blank test pdf.

#1 @gitlost
8 years ago

There's also an issue with PDFs with CMYK color spaces, as the resulting "full" jpegs will also have CMYK color spaces, and it turns out that Imagick::unsharpMaskImage() (as called in WP_Image_Editor_Imagick::thumbnail_image()) messes them up. In particular black text becomes fuzzy and over-black, and colors change. This can be seen using the attached "test_cmyk.pdf", generated using TCPDF:

<?php
$dirname = dirname( __FILE__ );
$tcpdf_dirname = $dirname . '/tcpdf';
require $tcpdf_dirname . '/tcpdf.php';

$pdf = new TCPDF();
$pdf->AddSpotColor( 'spot_black', 0, 0, 0, 100 );
$pdf->AddSpotColor( 'spot_dark_blue', 89, 42, 0, 48 );
$pdf->AddPage();
$pdf->SetFontSize( 30 );
$pdf->SetTextSpotColor( 'spot_black' );
$pdf->Text( 20, 20, 'Spot Black' );
$pdf->SetFillSpotColor( 'spot_dark_blue' );
$pdf->Rect( 20, 40, 30, 30, 'DF' );
$pdf->SetFontSize( 18 );
$pdf->SetTextSpotColor( 'spot_dark_blue' );
$pdf->Text( 20, 70, 'Dark Blue' );
$pdf->Output( $dirname . '/test_cmyk.pdf', 'F' );

Not calling Imagick::unsharpMaskImage() if the color space is CMYK fixes the fuzzy black text and severe color change issues, but still results in noticeable color change. This could be ameliorated by adding ICC profiles in the proposed pdf_process() method, but it's messy, and anyway I now think that a much better solution to all these issues is to use GhostScript directly - see #39262.

FWIW the following is a refreshed patch not calling Imagick::unsharpMaskImage() if the color space is CMYK. The unit test requires "test_cmyk.pdf" to be uploaded to the test data images directory.

@gitlost
8 years ago

Test CMYK PDF.

@gitlost
8 years ago

Refresh of patch with unsharpMaskImage not called for CMYK.

This ticket was mentioned in ​Slack in #core-media by joemcgill. ​View the logs.
8 years ago

#3 @joemcgill
8 years ago

  • Keywords dev-feedback 2nd-opinion has-patch has-unit-tests added

Hi @gitlost,

Thanks for digging into this. I'm hoping to get feedback from @mikeschroder and/or @markoheijnen on the approach, but in the mean time I have an initial question. Unless I'm mistaken, it seems to me that the CMYK issue is distinct from the alpha channel issue. Is that correct? If so, we should probably separate that issue into a separate ticket.

This ticket was mentioned in ​Slack in #core-media by mike. ​View the logs.
8 years ago

#5 @gitlost
8 years ago

Hi, thanks for responding. Yes you're right the Imagick::unsharpMaskImage() CMYK issue is distinct from the alpha channel issue, as it applies to any jpeg with a CMYK color space, so I'll split it off into a new ticket and refresh the patch here without it. The noticeable color change for CMYK PDFs issue still remains though.

Was messing about with ImageMagick's "delegate.xml" ("/etc/ImageMagick-6/delegate.xml" on Ubuntu 16.10) and found that if on the decode="ps:cmyk" line you change -sDEVICE=pam to -sDEVICE=jpeg then both the CMYK and the alpha issues disappear and you get perfect previews.

Similarly if on the decode="ps:alpha line you change -sDEVICE=pngalpha to -sDEVICE=jpeg and add -dUseCIEColor then the alpha channel issue disappears for the "test_alpha.pdf" uploaded here.

So there may be a way through Imagick to get the behaviour from ImageMagick that one gets by calling GhostScript directly (where none of these issues arises), but it's not easy to see. It's a bit like fishing through a keyhole.

@gitlost
8 years ago

Refresh of original patch without the unsharpMaskImage() change.

#6 @launchinteractive
7 years ago

I've been getting the black issue and this patch fixes it for me. Is there a reason why it hasn't been added to WordPress core?

@gitlost
7 years ago

Refresh, comment changes only.

#7 @gitlost
7 years ago

On investigating #40537, found out this only affects systems with Ghostscript versions >= 9.14, probably due to the change in color management ​https://www.ghostscript.com/doc/9.21/History9.htm#Version9.14.

The new patch is just a refresh with comment changes only. It doesn't try to determine the Ghostscript version (and act accordingly) as it's messy to do in a system-independent way (and would require an exec()).

(Pssttt! For all your PDF preview needs - at very competitive prices - see ​https://wordpress.org/plugins/gs-only-pdf-preview/.)

#8 follow-up: @emirpprime
6 years ago

We've just upgraded ~20 servers to Debian 9 from Deb7/8 and been bitten by this. Unfortunately in all the testing no-one happened to upload a PDF with transparency in it.

So far, modifying the decode="ps:alpha line in /etc/ImageMagick-6/delegates.xml from -sDEVICE=pngalpha to -sDEVICE=jpeg has fixed it.

Original error shown in wp-admin (when using Regenerate Thumbnails plugin, otherwise only error is black images):

ERROR: FailedToExecuteCommand `'gs' -sstdout=%stderr -dQUIET -dSAFER -dBATCH -dNOPAUSE -dNOPROMPT -dMaxBitmap=500000000 -dAlignToPixels=0 -dGridFitTT=2 '-sDEVICE=pngalpha' -dTextAlphaBits=4 -dGraphicsAlphaBits=4 '-r128x128' -dFirstPage=1 -dLastPage=1 '-sOutputFile=/tmp/magick-7338Pz_rSv55v5nG%d' '-f/tmp/magick-7338lWRCG1kb4w9Z' '-f/tmp/magick-7338WA7HNhRpf3Uj (-1) @ error/delegate.c/ExternalDelegateCommand/462

System info:

Debian 9, PHP7.2, Apache 2.4

Media Debugging Info:

Active Editor	WP_Image_Editor_Imagick
Imagick Module Number	1687
ImageMagick Version	ImageMagick 6.9.7-4 Q16 x86_64 20170114 http://www.imagemagick.org
GD Version	2.2.5
Ghostscript Version	9.26
Memory Limit	256M
Max Execution Time	30
Max Input Time	60
Upload Max Filesize	20M
Post Max Size	20MImagick Resource Limits
Imagick area limit	122 MB
Imagick disk limit	1073741824
Imagick file limit	6144
Imagick map limit	512 MB
Imagick memory limit	256 MB
Imagick thread limit	2

#9 follow-ups: @mwtsn
6 years ago

Hi all,

I've been faced with the black PDF thumbnail image issue, and I've come up with a solution. Mine is specific to Imagick.

In the file wp-includes/class-wp-image-editor-imagick.php after line 153 ($this->image->readImage( $filename );) you can put in the code:

$this->image->setImageBackgroundColor('white');
$this->image = $this->image->flattenImages();

This fixes my issue. So I was wondering if it would be useful for us to add an action into the code at this point, so developers can easily add code into the solution to alter the image output as they need?

Let me know if you think this approach is workable, and I will submit a patch.

If not, I'll create a new version of WP_Image_Editor_Imagick and use the 'wp_image_editors' filter to pull this in.

#10 in reply to: ↑ 9 @ceer
6 years ago

Let me know if you think this approach is workable, and I will submit a patch.

I've tested without success.
Furthermore, I have this notice:

Deprecated: Imagick::flattenImages method is deprecated and it's use should be avoided in /www/wp-includes/class-wp-image-editor-imagick.php on line 157

The only working solution for me yet is the @gitlost one: https://core.trac.wordpress.org/attachment/ticket/39216/39216.4.patch

Last edited 6 years ago by ceer (previous) (diff)

#11 in reply to: ↑ 9 ; follow-up: @maysi
5 years ago

Replying to mwtsn:

In the file wp-includes/class-wp-image-editor-imagick.php after line 153 ($this->image->readImage( $filename );) you can put in the code:

$this->image->setImageBackgroundColor('white');
$this->image = $this->image->flattenImages();

This worked for me, too.

#12 in reply to: ↑ 11 @madejackson
5 years ago

Replying to maysi:

Replying to mwtsn:

In the file wp-includes/class-wp-image-editor-imagick.php after line 153 ($this->image->readImage( $filename );) you can put in the code:

$this->image->setImageBackgroundColor('white');
$this->image = $this->image->flattenImages();

This worked for me, too.

Can confirm, This worked for me, too.

The two other Solutions proposed in ticket #45982 did break the thumbnail-generation for me.

#13 @ceer
5 years ago

Hi @mwtsn, I confirm that your solution (ticket:39216#comment:9) now worked for me too! (ImageMagick 6.8.9-9, Ubuntu 16.04.6, PHP 7.0.33).

#14 in reply to: ↑ 8 @6adminit
5 years ago

Replying to emirpprime:

We've just upgraded ~20 servers to Debian 9 from Deb7/8 and been bitten by this. Unfortunately in all the testing no-one happened to upload a PDF with transparency in it.

So far, modifying the decode="ps:alpha line in /etc/ImageMagick-6/delegates.xml from -sDEVICE=pngalpha to -sDEVICE=jpeg has fixed it.

I can confirm this very simple mod do the trick for us too

Configuration:
Debian 10
PHP 7.2.27
Ghostscript 9.27

Command:

sed -i.back -e 's#pngalpha#jpeg#g' /etc/ImageMagick-6/delegates.xml

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
2 years ago

#16 @antpb
2 years ago

  • Milestone changed from Awaiting Review to 6.2

This ticket was discussed in a recent bug scrub. We are moving this ticket to 6.2 for investigation since it has been so long. Would any folks watching this ticket be able to provide an update if this is still impacting and if the fix is still valid?

#17 @antpb
2 years ago

Also noting #46318 and #45982 as related

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
21 months ago

#19 @antpb
21 months ago

  • Milestone changed from 6.2 to Future Release

Theres still investigation that needs to happen here so we'll need to move this to a future release to allow time to investigate and propose a fix.

#20 @antpb
21 months ago

  • Milestone changed from Future Release to 6.2

#21 @sabernhardt
21 months ago

  • Milestone changed from 6.2 to 6.3

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
18 months ago

#23 @antpb
18 months ago

  • Owner set to antpb
  • Status changed from new to assigned

Just a high level test of the patch I can confirm it fixes the issue. Theres some formatting that needs to be adjusted in the patch and I need to investigate an issue with applying the tests in the patch but aside from that this should be good to go.

Assigning to myself to make those updates.

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
17 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
17 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
17 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
16 months ago

@antpb
16 months ago

Updating coding standards and conflicts

This ticket was mentioned in ​PR #4745 on ​WordPress/wordpress-develop by ​antpb.
16 months ago #28

Creating this draft to check that the added files in these tests are okay.

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
16 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
16 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
16 months ago

This ticket was mentioned in ​Slack in #core by chaion07. ​View the logs.
16 months ago

#33 @chaion07
16 months ago

Thanks @gitlost for reporting this. We reviewed this ticket once during a recent bug-scrub session and based on the feedback we are hoping to update the milestone to 6.4 unless @antpb is available to take a look in the next few hours. Cheers!

Props to @costdev & @oglekler

This ticket was mentioned in ​Slack in #core-media by oglekler. ​View the logs.
16 months ago

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
16 months ago

#36 @antpb
16 months ago

  • Keywords early added
  • Milestone changed from 6.3 to 6.4

Moving 6.4 and adding early. this has had a lot of review and ready to commit

#37 @antpb
16 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 56271:

Media: Adjust PDF upload handling to remove non-opaque alpha channels from previews.

Previously, Imagick uploads of PDF files with non-opaque alpha channels would result in a black background replacing alpha in the generated thumbnail. This patch adds a remove_pdf_alpha_channel() function in the Imagick classes to use a white background instead.

Props gitlost, joemcgill, joedolson, launchinteractive, emirpprime, mwtsn, ceer, maysi, madejackson, 6adminit, costdev, oglekler.
Fixes #39216.

This ticket was mentioned in ​Slack in #core-media by antpb. ​View the logs.
7 months ago

Note: See TracTickets for help on using tickets.