Opened 6 years ago
Last modified 18 months ago
#45982 new defect (bug)
PDF thumbnails have a default black background
Reported by: | nebrekab | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | 5.0.3 |
Component: | Media | Keywords: | |
Focuses: | Cc: |
Description
I wouldn't necessarily call this a bug, but when a PDF is uploaded where the page has no solid background colour its thumbnail image background is rendered black. As most paper used is white ( I would hazard a guess... :) ), it feels like the default background colour of thumbnails should be set to white. I have created a fix within /wp-includes/class-wp-image-editor-imagick.php
<?php try { $this->image = new Imagick(); $file_extension = strtolower( pathinfo( $this->file, PATHINFO_EXTENSION ) ); $filename = $this->file; if ( 'pdf' == $file_extension ) { $filename = $this->pdf_setup(); } // Reading image after Imagick instantiation because `setResolution` // only applies correctly before the image is read. $this->image->readImage( $filename ); if ( ! $this->image->valid() ) return new WP_Error( 'invalid_image', __('File is not an image.'), $this->file); // Select the first frame to handle animated images properly if ( is_callable( array( $this->image, 'setIteratorIndex' ) ) ) $this->image->setIteratorIndex(0); $this->mime_type = $this->get_mime_type( $this->image->getImageFormat() ); // !bb modification to default to white backgrounds if ( 'pdf' == $file_extension ) { if ($this->image->getImageAlphaChannel()) { // Remove alpha channel (use 11 if constant does not work... see https://stackoverflow.com/questions/10805122/imagemagick-setimagealphachannel-not-working-php) $this->image->setImageAlphaChannel(Imagick::ALPHACHANNEL_REMOVE); // set image background color to white $this->image->setImageBackgroundColor('#ffffff'); } }// !bb end modification to default to white backgrounds }
Attachments (1)
Change History (24)
#2
in reply to:
↑ 1
@
6 years ago
Thanks for the swift reply!
Yes, it sure doesn't have to be hardcoded - the default could be a constant or set via a function setting? - but generally I see it as more to do with the colour of paper being more likely to be white than any other colour, certainly not black, and that it's not so much to do with the colour of the website's design. Somewhere in the code (Imagick perhaps?) they have made the decision to default to black, and I just feel as though that's not the most logical choice... :)
If it could be set in some way shape or form in WordPress - that would be amazing. How do we go about getting this in place? :)
Replying to subrataemfluence:
Welcome to trac and thanks for the ticket!
My personal opinion is using hard coded specific background color is not a good idea. Different website can have different backgrounds. If a black background hits eyes, then a white background on another color can do the same.
However, if we can somehow make this background matched with that of site's body color dynamically, that probably makes more sense!
#4
follow-up:
↓ 5
@
6 years ago
Hello, I reported the same issue and in my test I fixed it without define any colour by default instead of this I merge all layers with constant LAYERMETHOD_FLATTEN, could be this a good solution?
<?php $ this -> image -> mergeImageLayers ( Imagick :: LAYERMETHOD_FLATTEN );
#5
in reply to:
↑ 4
@
6 years ago
Replying to rdiazg:
Hello, I reported the same issue and in my test I fixed it without define any colour by default instead of this I merge all layers with constant LAYERMETHOD_FLATTEN, could be this a good solution?
<?php $ this -> image -> mergeImageLayers ( Imagick :: LAYERMETHOD_FLATTEN );
We have a legacy thumbnail generation script for uploaded PDFs and this is the approach we use on the command line (with a -flatten parameter on mogrify). It's been working well for several years but it would be nice to be able to switch to native PDF thumbnails rather than fill the media library with additional images. Probably makes sense to put it in the pdf_setup() function at the bottom of the class.
This ticket was mentioned in Slack in #core by radigar. View the logs.
5 years ago
#7
@
5 years ago
For anyone who has this issue in the future, the proposed solutions in this thread didn't work for me.
Instead, I used this workaround, which works perfectly: #39216
@
5 years ago
Hi, I have the same issue. The workaround from #39216 does not work for me, I need to setImageAlphaChannel
. I'm using PHP7.2 with ImageMagick 6.9.10 and ghostscript 9.5. I've attached a patch that adds filters to both the Imagick class and the GD class both passing the $this->image
object as a reference. Then, in a plugin, you can make the fix that works for you. Would that be a solution?
#8
follow-ups:
↓ 9
↓ 12
@
5 years ago
With my patch (see comment above) and below code my pdf's are without black backgrounds:
<?php add_action( 'imagick_load_after_read', function( $image, $filename, $file_extension ) { if ( 'pdf' === $file_extension ) { $image->setImageAlphaChannel( Imagick::ALPHACHANNEL_REMOVE ); } }, 10, 3 );
#9
in reply to:
↑ 8
@
4 years ago
Replying to joostdekeijzer:
With my patch (see comment above) and below code my pdf's are without black backgrounds:
<?php add_action( 'imagick_load_after_read', function( $image, $filename, $file_extension ) { if ( 'pdf' === $file_extension ) { $image->setImageAlphaChannel( Imagick::ALPHACHANNEL_REMOVE ); } }, 10, 3 );
I am using a combination of this and above, I wish the hook was in core already.
<?php /** * Patch for black backgrounds on PDF's. Does require a core modification. * * https://core.trac.wordpress.org/ticket/45982 * * /wp-includes/class-wp-image-editor-magick.php * * Line 166: * do_action_ref_array( 'imagick_load_after_read', array( &$this->image, $filename, $file_extension, $this->mime_type ) ); * * @param object $image * @param string $filename * @param string $file_extension * @param string $mime_type */ add_action('imagick_load_after_read', function (object $image, string $filename, string $file_extension, string $mime_type) { if ('pdf' === $file_extension) { if ($image->getImageAlphaChannel()) { $image->setImageAlphaChannel(Imagick::ALPHACHANNEL_REMOVE); $image->setImageBackgroundColor('#ffffff'); } } }, 10, 4);
#10
@
4 years ago
I had a PDF that always rendered the entire thumbnail as completely black. Being able to set Imagick::ALPHACHANNEL_REMOVE fixed it for me. So thanks for this patch, hopefully it comes to core soon.
#11
@
4 years ago
when will this be fixed? adding the action hook seems the most non controversial way to do it - so please add it to the core.
#12
in reply to:
↑ 8
@
4 years ago
Replying to joostdekeijzer:
With my patch (see comment above) and below code my pdf's are without black backgrounds:
<?php add_action( 'imagick_load_after_read', function( $image, $filename, $file_extension ) { if ( 'pdf' === $file_extension ) { $image->setImageAlphaChannel( Imagick::ALPHACHANNEL_REMOVE ); } }, 10, 3 );
I think this is a great idea... did you add a pull request for the hook @ (https://github.com/WordPress/wordpress-develop) maybe this would help move things along?
#13
follow-up:
↓ 14
@
4 years ago
This sounds great and I welcome the hook/fix.
Still, the thing that irks me with this particular issue is that I do not think we should be having to apply a fix or modify via a hook to remove black backgrounds from PDFs at all, and that they should default to white.
I'd hazard a guess that white has been the most common colour of paper since the Egyptians invented it. :)
#14
in reply to:
↑ 13
@
4 years ago
Replying to nebrekab:
This sounds great and I welcome the hook/fix.
Still, the thing that irks me with this particular issue is that I do not think we should be having to apply a fix or modify via a hook to remove black backgrounds from PDFs at all, and that they should default to white.
I'd hazard a guess that white has been the most common colour of paper since the Egyptians invented it. :)
yes I agree. that its black seems to be a bug and not a feature ;) also it has nothing to do with the website color imho but as you said with the "paper color"
but I'd rather have the hook than a discussion about it.
#16
@
4 years ago
I was facing this exact same issue recently but it seems there is an easy fix for this by providing your own editor class that extends the core WP_Image_Editor_Imagick
class. You can hook into the wp_image_editors
filter and add your editor class as the first one from the list.
Here is an example of a working solution.
First, create a custom editor class that extends the imagick core class:
<?php namespace App\Cms\Media\Editors; final class ExtendedWpImageEditorImagick extends \WP_Image_Editor_Imagick { protected function pdf_load_source() { $loaded = parent::pdf_load_source(); try { $this->image->setImageAlphaChannel(\Imagick::ALPHACHANNEL_REMOVE); $this->image->setBackgroundColor('white'); } catch (\Exception $exception) { error_log($exception->getMessage()); } return $loaded; } }
Next hook into the filter and set your editor as the first implementation like so:
<?php add_filter('wp_image_editors', function (array $editors) { array_unshift($editors, ExtendedWpImageEditorImagick::class); return $editors; });
In my example, I was able to remove the transparent alpha channel of our PDFs to help generate a preview image with a white background. I've overwritten the pdf_load_source
method but you can override any other method based on your needs.
I hope this can also help. It's not that I'm against a hook for background color property only but perhaps a more generic hook to get the $this->image
instance from the imagick core editor would be useful. At least where we're sure it is correctly loaded. In the meantime, the solution above can help people customize the behavior without touching to the core files for their project.
#17
follow-up:
↓ 18
@
4 years ago
If you just want to dump @jlambe's great solution at the end of your themes functions.php, you will get errors about the namespace not being at the start of the file. In this case, the below less elegant version is working for me.
<?php /** * Patch to prevent black PDF backgrounds. * * https://core.trac.wordpress.org/ticket/45982 */ require_once ABSPATH . 'wp-includes/class-wp-image-editor.php'; require_once ABSPATH . 'wp-includes/class-wp-image-editor-imagick.php'; // phpcs:ignore PSR1.Classes.ClassDeclaration.MissingNamespace final class ExtendedWpImageEditorImagick extends WP_Image_Editor_Imagick { /** * Add properties to the image produced by Ghostscript to prevent black PDF backgrounds. * * @return true|WP_error */ // phpcs:ignore PSR1.Methods.CamelCapsMethodName.NotCamelCaps protected function pdf_load_source() { $loaded = parent::pdf_load_source(); try { $this->image->setImageAlphaChannel(Imagick::ALPHACHANNEL_REMOVE); $this->image->setBackgroundColor('#ffffff'); } catch (Exception $exception) { error_log($exception->getMessage()); } return $loaded; } } /** * Filters the list of image editing library classes to prevent black PDF backgrounds. * * @param array $editors * @return array */ add_filter('wp_image_editors', function (array $editors): array { array_unshift($editors, ExtendedWpImageEditorImagick::class); return $editors; });
#18
in reply to:
↑ 17
@
4 years ago
Replying to thatstevensguy:
If you just want to dump @jlambe's great solution at the end of your themes functions.php, you will get errors about the namespace not being at the start of the file. In this case, the below less elegant version is working for me.
THANK YOU so much for that solution of yours for simply using the theme functions.php, thanks to @jlambe and everyone else too. I'd tried some of those but had errors beyond my capability.
But this solution is ideal.
For those with lots of existing PDFs they need to fix, check out this 4 year old plugin:
https://en-gb.wordpress.org/plugins/gs-only-pdf-preview/ which is a quick/easy way to fix those existing ones, plugin is not supported it seems but works perfectly for me.
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
2 years ago
#21
@
21 months ago
Just pitching in. This is still a very real issue with PDF-files having transparent elements on the first page, apparently.. But, the "less elegant solution" mentioned by @thatstevensguy, still gets the job done, in WordPress 6.1
I used "Force Regenerate Thumbnails" on those bulk selected existing PDF's in the media library with black backgrounds.
The script handles new uploads too.
A welcome patch! - but would be great if it wasn't needed.
#22
@
20 months ago
Thanks @jlambe and @thatstevensguy for this solution, it worked for me in WordPress 6.2.
I also needed to regenerate thumbnails for all of my PDFs and found that the old plugin mentioned above (GS Only PDF Preview) didn't work but Regenerate Thumbnails which is still maintained worked perfectly via bulk edits.
Welcome to trac and thanks for the ticket!
My personal opinion is using hard coded specific background color is not a good idea. Different website can have different backgrounds. If a black background hits eyes, then a white background on another color can do the same.
However, if we can somehow make this background matched with that of site's body color dynamically, that probably makes more sense!