WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 10 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)

image-library-load-filters.diff (2.0 KB) - added by joostdekeijzer 2 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?

Download all attachments as: .zip

Change History (19)

#1 follow-up: @subrataemfluence
3 years 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
3 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!

Last edited 3 years ago by nebrekab (previous) (diff)

#3 @swissspidy
3 years ago

#46318 was marked as a duplicate.

#4 follow-up: @rdiazg
3 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 @mikenolan
3 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.


2 years ago

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

@joostdekeijzer
2 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: @joostdekeijzer
2 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 @thatstevensguy
14 months 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);
Last edited 14 months ago by thatstevensguy (previous) (diff)

#10 @gmariani405
13 months 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 @tofuSCHNITZEL
11 months 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 @tofuSCHNITZEL
11 months 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: @nebrekab
11 months 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. :)

Last edited 11 months ago by nebrekab (previous) (diff)

#14 in reply to: ↑ 13 @tofuSCHNITZEL
11 months 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.

Last edited 11 months ago by tofuSCHNITZEL (previous) (diff)

#15 @nebrekab
11 months ago

Amen to that.

An easy solution is a welcome solution.

#16 @jlambe
10 months 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: @thatstevensguy
10 months 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;
});

Last edited 10 months ago by thatstevensguy (previous) (diff)

#18 in reply to: ↑ 17 @richardshea
10 months 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.

Last edited 10 months ago by richardshea (previous) (diff)
Note: See TracTickets for help on using tickets.