Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#48202 closed defect (bug) (invalid)

Media: rotation of JPEG files may fail

Reported by: azaozz's profile azaozz Owned by:
Milestone: Priority: high
Severity: normal Version:
Component: Media Keywords: has-patch reporter-feedback
Focuses: Cc:

Description

Reported by @tmatsuur at https://core.trac.wordpress.org/ticket/14459#comment:98.

In an environment where GD is used, JPEG files could not be rotated.

This is due to improper processing of JPEG images that do not support the alpha channel.

class-wp-image-editor-gd.php: 344 (5.3 Beta 2)

public function rotate( $angle ) {
	if ( function_exists( 'imagerotate' ) ) {
		$transparency = imagecolorallocatealpha( $this->image, 255, 255, 255, 127 );
		$rotated      = imagerotate( $this->image, $angle, $transparency );

		if ( is_resource( $rotated ) ) {
			imagealphablending( $rotated, true );
			imagesavealpha( $rotated, true );
			imagedestroy( $this->image );
			$this->image = $rotated;
			$this->update_size();
			return true;
		}
	}
	return new WP_Error( 'image_rotate_error', __( 'Image rotate failed.' ), $this->file );
}

I confirmed that the JPEG image can be rotated normally by correcting as follows.

public function rotate( $angle ) {
	if ( function_exists( 'imagerotate' ) ) {
		if ( 'image/png' === $this->mime_type || 'image/gif' === $this->mime_type ) {
			$transparency = imagecolorallocatealpha( $this->image, 255, 255, 255, 127 );
			$rotated      = imagerotate( $this->image, $angle, $transparency );

			if ( is_resource( $rotated ) ) {
				imagealphablending( $rotated, true );
				imagesavealpha( $rotated, true );
				imagedestroy( $this->image );
				$this->image = $rotated;
				$this->update_size();
				return true;
			}
		} else {
			$rotated      = imagerotate( $this->image, $angle );
			if ( is_resource( $rotated ) ) {
				$this->image = $rotated;
				$this->update_size();
				return true;
			}
		}
	}
	return new WP_Error( 'image_rotate_error', __( 'Image rotate failed.' ), $this->file );
}

The PHP version that confirmed this behavior is 7.2.3.

Attachments (1)

48202.diff (1.8 KB) - added by azaozz 5 years ago.

Download all attachments as: .zip

Change History (7)

#1 @azaozz
5 years ago

Thanks for the bug report!

Yes, the rotate() function in WP_Image_Editor_GD needs updating.

  • JPEGs don't support transparency so there's no need to do imagealphablending() and imagesavealpha() for them (the latter is only needed for PNGs).

However testing the current code with several different JPEG images in PHP 7.3.2 with GD version bundled (2.1.0 compatible) seems to work as expected.

I'll make a patch. Would be great if you can confirm it works as expected.

#2 @azaozz
5 years ago

Looking at #30596 (where this code was added) and at https://www.php.net/manual/en/function.imagealphablending.php it seems that imagealphablending( $rotated, true ); was added in error. When true, imagealphablending() removes transparency and "blends" the pixels making them opaque.

It is also set to false when loading the image, seemingly for the same reason.

The only bit that's needed is imagesavealpha( $rotated, true ); which is used only for PNGs.

Patch coming up.

@azaozz
5 years ago

#3 follow-up: @azaozz
5 years ago

  • Keywords has-patch reporter-feedback added

In 48202.diff:

  • Use imagecolorallocate() instead of imagecolorallocatealpha() when rotating JPEGs.
  • Do not set imagealphablending() and imagesavealpha() for JPEGs.

Testing here with PHP 7.3, this patch doesn't make any difference. Rotated PNGs are tansparent, GIFs are ...not really. JPEGs work as expected.

@tmatsuur can you test (the soonest -- the better) if that fixes it for you?

Version 0, edited 5 years ago by azaozz (next)

#4 in reply to: ↑ 3 @tmatsuur
5 years ago

Replying to azaozz:

In 48202.diff:

  • Use imagecolorallocate() instead of imagecolorallocatealpha() when rotating JPEGs.
  • Do not set imagealphablending() and imagesavealpha() for JPEGs.

Testing here with PHP 7.3, this patch doesn't make any difference. Rotated PNGs are tansparent, GIFs are ...not really. JPEGs work as expected.

@tmatsuur can you test (the sooner -- the better) if that fixes it for you?

Thanks @azaozz

As you pointed out, it was wrong to specify GIF as a condition in an if statement.

#5 follow-up: @tmatsuur
5 years ago

@azaozz , I'm very sorry.

I seem to have made a big mistake.

I used a photo that was taken with an iPhone a while ago, but the rotate data was inappropriate.

I checked again using the photos I took today.

Of course, we have confirmed that the version of the patch we created works without problems. And there was no problem with the previous version.

I am sorry for taking your time.

#6 in reply to: ↑ 5 @azaozz
5 years ago

  • Milestone 5.3 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Replying to tmatsuur:

I used a photo that was taken with an iPhone a while ago, but the rotate data was inappropriate.

I checked again using the photos I took today.

Thanks for clearing that out! Yeah, mistakes happen, not a problem. I'm glad the (old) code works as expected.

Note: See TracTickets for help on using tickets.