WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 2 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:
PR Number:

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
                        
                        
                }

Change History (7)

#1 follow-up: @subrataemfluence
10 months ago

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!

#2 in reply to: ↑ 1 @nebrekab
10 months ago

Thanks for the swift reply!

Yes, it sure doesn't 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!

Version 0, edited 10 months ago by nebrekab (next)

#3 @swissspidy
9 months ago

#46318 was marked as a duplicate.

#4 follow-up: @rdiazg
9 months 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 @mikenolan
9 months 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 months ago

#7 @madejackson
2 months 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

Note: See TracTickets for help on using tickets.