Image editing in GD runs image stream functions multiple times

[glynnquelch]

I referenced this on a similar issue
https://core.trac.wordpress.org/ticket/23127#comment:29

When the imagecreatefromstring() function is called in wp-includes/class-wp-image-editor-gd.php, it will often call this function twice. This often results in serious memory issues for larger images and at some formats it doubles the memory used

<?php
// WebP may not work with imagecreatefromstring().
if (
 function_exists( 'imagecreatefromwebp' ) &&
 ( 'image/webp' === wp_get_image_mime( $this->file ) )
) {
 $this->image = @imagecreatefromwebp( $this->file );
} else {
 $this->image = @imagecreatefromstring( $file_contents );
}

// AVIF may not work with imagecreatefromstring().
if (
 function_exists( 'imagecreatefromavif' ) &&
 ( 'image/avif' === wp_get_image_mime( $this->file ) )
) {
 $this->image = @imagecreatefromavif( $this->file );
} else {
 $this->image = @imagecreatefromstring( $file_contents );
}

If I make a change locally to

<?php
// WebP may not work with imagecreatefromstring().
if ( function_exists( 'imagecreatefromwebp' ) &&
        ( 'image/webp' === wp_get_image_mime( $this->file ) )
) {
        $this->image = @imagecreatefromwebp( $this->file );
// AVIF may not work with imagecreatefromstring().
} elseif ( function_exists( 'imagecreatefromavif' ) &&
        ( 'image/avif' === wp_get_image_mime( $this->file ) )
) {
        $this->image = @imagecreatefromavif( $this->file );
} else {
        $this->image = @imagecreatefromstring( $file_contents );
}

I see a reduction in the memory overhead

Filename	Size (W x H)	Filesize	Current GD Peak Memory	Improved Flow Peak Memory
Suru_Bog_10000px.jpg	10000 x 4534	33.08 MB	410.41 MB	266 MB
test.jpg	7500 x 7500	1.75 MB	510.02 MB	308.71 MB
156442-ultra-widescreen-wallpaper.jpg	4500 x 1901	642.36 KB	96.98 MB	92.96 MB
logo.png	1024 x 512	85.47 KB	20.36 MB	10.29 MB
Sample-10345x5531.webp	10345 x 3989	7.49MB	378.79 MB	207.38MB
minion-large.webp	10000 x 5000	2.15MB	419.38 MB	224.05MB
minion-large.gif	25000 x 12500	25.84MB	734.04 MB	424.89MB
SamplePNGImage_30mbmb.png	7345 x 2832	31.39MB	297.11 MB	241.41MB
IMG_20211115_123549.jpg	4624 x 3472	2.57 MB	167.69 MB	132.58MB

Some smaller images see no difference and some gifs use more memory at times, but for images with larger dimensions, we see memory allocation errors with hosts that limit memory at 512mb of ram.

[glynnquelch]

Diff to improve the control flow

[desrosj]

Thanks for this report, @glynnquelch. And welcome to Trac!

Unfortunately, it’s too late to consider this for 6.7, but this sort of thing would be great to improve as soon as we’re able to validate and test properly.

Moving to 6.7.1 for investigation.

[adamsilverstein]

Good catch @glynnquelch and thanks for the new ticket - the logic there does look incorrect. It was fine when we added the original conditional when WebP was introduced. When we added AVIF support we should added the else logic as you have it. Marking this as having originated in 6.5.

Trac ticket: https://core.trac.wordpress.org/ticket/62331

[adamsilverstein]

Tests for patch running on ​https://github.com/WordPress/wordpress-develop/pull/7705 (also adds consolidating comment lines).

It would be nice to add a unit test for this, ideally asserting that imagecreatefromstring is only called once.

This is a pretty significant bug for GD users and a very small change to fix: we may want to consider back porting to 6.5+.

[adamsilverstein]

It would be nice to add a unit test for this, ideally asserting that imagecreatefromstring is only called once.

Not sure this is possible with our testing framework. Fortunately, the fix is clear looking at the code logic. Previous to the this change, imagecreatefromstring is called twice.

[glynnquelch]

Thanks for running with this @adamsilverstein 🚀🙌

[desrosj]

Because of a few bug reports opened since 6.7 was released, the Core team is evaluating the need for a short 6.7.1 cycle (possibly next week).

While this issue was not introduced during 6.7 (which technically means it now falls outside of the focus for 6.7.1 as currently defined), I think this is worth including. As @adamsilverstein said, this is a pretty significant bug for GD users, and there are likely some undiscovered edge cases from running WebP images through imagecreatefromstring).

[desrosj]

In 59413:

Media: Avoid running expensive logic twice using GD.

Support for uploading AVIF was added in [57524]. A new block of conditional logic was added determine which function should be used to create the new image file that resulted in these expensive functions being run twice.

This combines the two conditional logic to ensure the appropriate function is only run once regardless of format.

Props adamsilverstein, glynnquelch.
Fixes #62331.

[desrosj]

Reopening to consider backporting.

[desrosj]

Properly marking for backport review.

[adamsilverstein]

Looks good for backporting @desrosj - thanks!

[desrosj]

Thank you!

[desrosj]

In 59423:

Media: Avoid running expensive logic twice using GD.

Support for uploading AVIF was added in [57524]. A new block of conditional logic was added determine which function should be used to create the new image file that resulted in these expensive functions being run twice.

This combines the two conditional logic to ensure the appropriate function is only run once regardless of format.

Reviewed by adamsilverstein.
Merges [59413] to the 6.7 branch.

Props adamsilverstein, glynnquelch.
Fixes #62331.