Make WordPress Core

Opened 6 weeks ago

Closed 5 weeks ago

Last modified 4 weeks ago

#62305 closed defect (bug) (fixed)

Original HEIC image is not converted to JPEG

Reported by: azaozz's profile azaozz Owned by: azaozz's profile azaozz
Milestone: 6.7 Priority: normal
Severity: normal Version: 6.7
Component: Media Keywords: has-patch has-unit-tests dev-reviewed
Focuses: Cc:

Description (last modified by azaozz)

When uploading a smaller HEIC image, for example 800px wide, the original is not converted to JPEG. That results in WordPress using the .heic image in posts, listing it in the srcset, etc. (The .heic image can only be displayed in Safari, other browsers show a broken image placeholder.)

Attachments (1)

sample-800px.heic (179.4 KB) - added by azaozz 6 weeks ago.

Download all attachments as: .zip

Change History (52)

#1 @azaozz
6 weeks ago

Seems to fix this the HEIC images should always be treated as oversized. When the original image is smaller than the oversized threshold (2560px), it has to be converted to a JPEG with the same dimensions.

Last edited 6 weeks ago by azaozz (previous) (diff)

#2 @azaozz
6 weeks ago

  • Keywords needs-patch added

@azaozz
6 weeks ago

#3 @ironprogrammer
6 weeks ago

Thanks for the report, @azaozz!

Confirming that I see this as well, with the provided sample image and my own cropped and exported HEIC image (using macOS Photos):

<figure class="wp-block-image size-full">
    <img decoding="async" width="800" height="533"
        src="http://wp-src.test/wp-content/uploads/2024/10/sample-800px.heic" alt="" class="wp-image-1921"
        srcset="http://wp-src.test/wp-content/uploads/2024/10/sample-800px.heic 800w,
        http://wp-src.test/wp-content/uploads/2024/10/sample-800px-300x200.jpg 300w,
        http://wp-src.test/wp-content/uploads/2024/10/sample-800px-768x512.jpg 768w"
        sizes="(max-width: 800px) 100vw, 800px">
</figure>

For iOS/macOS users it doesn't seem this should happen often, unless they're cropping in Photos and exporting as HEIC (default is JPEG). Most iOS photos should be above the oversized threshold.

#4 @adamsilverstein
6 weeks ago

Seems to fix this the HEIC images should always be treated as oversized. When the original image is smaller than the oversized threshold (2560px), it has to be converted to a JPEG with the same dimensions.

That seems like a reasonable solution.

This ticket was mentioned in PR #7656 on WordPress/wordpress-develop by @adamsilverstein.


6 weeks ago
#5

  • Keywords has-patch added; needs-patch removed

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core by azaozz. View the logs.


6 weeks ago

This ticket was mentioned in PR #7667 on WordPress/wordpress-develop by @azaozz.


6 weeks ago
#8

Fix converting all heic images to jpeg

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

@azaozz commented on PR #7667:


6 weeks ago
#9

PR may need to be adjusted a bit after https://core.trac.wordpress.org/ticket/62272.

#10 @azaozz
6 weeks ago

Seems https://github.com/WordPress/wordpress-develop/pull/7656 needs a bit more, couldn't get it to work in all cases here.

Made https://github.com/WordPress/wordpress-develop/pull/7667 instead. Uses a slightly different approach, and adds support for converting all HEIF mime types (although results may vary when converting animations/sequences).

Last edited 6 weeks ago by azaozz (previous) (diff)

This ticket was mentioned in Slack in #core by azaozz. View the logs.


6 weeks ago

#12 follow-up: @adamsilverstein
6 weeks ago

https://github.com/WordPress/wordpress-develop/pull/7667 looks good, thanks @azaozz! Left some tiny feedback on the PR, otherwise LGTM.

Would be good to get some unit tests for this although manual testing is probably enough confirmation at this point. I will give it a test.

#14 in reply to: ↑ 12 @azaozz
6 weeks ago

Replying to adamsilverstein:

Thanks!

Would be good to get some unit tests for this although manual testing is probably enough confirmation at this point.

Yep, planning to add few tests for this and for #62272, probably reuse the same sample image. But imho manual testing is more important at this point.

Last edited 6 weeks ago by azaozz (previous) (diff)

#15 @azaozz
6 weeks ago

  • Description modified (diff)

#16 @azaozz
6 weeks ago

  • Owner set to azaozz
  • Resolution set to fixed
  • Status changed from new to closed

In 59317:

Media: Fix converting of all HEIC/HEIF images to JPEGs after uploading regardless of dimensions.

Props ironprogrammer, adamsilverstein, azaozz.
Fixes #62305.

#17 @azaozz
6 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 6.7.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


6 weeks ago

This ticket was mentioned in Slack in #core by stoyangeorgiev. View the logs.


5 weeks ago

This ticket was mentioned in Slack in #core by azaozz. View the logs.


5 weeks ago

#21 @ironprogrammer
5 weeks ago

Test Report

For a pre-backport confidence check, please note that recent tests performed for related ticket #62272 included images that were smaller than the big image threshold:

  • anim-heif-sequence.heic - 256 × 144
  • burst-heif-sequence.heic - 640 × 360
  • static-heif.heic - 1440 × 960
  • static-ios-heic.HEIC - 4032 × 3024 (regression test)

Actual Results

  • ✅ In each case, the images were successfully converted to JPEG.

Supplemental Artifacts

The following markup was generated for each "undersized" uploaded/converted image (HTML comment added for clarity):

<!-- anim-heif-sequence.heic -->
<img decoding="async" width="256" height="144"
  src="http://wp-src.test/wp-content/uploads/2024/10/anim-heif-sequence-256x144.jpg"
  alt="" class="wp-image-1929">
<!-- burst-heif-sequence.heic -->
<img fetchpriority="high" decoding="async" width="1024" height="576" data-id="1930"
  src="http://wp-src.test/wp-content/uploads/2024/10/burst-heif-sequence-1024x576.jpg"
  alt="" class="wp-image-1930"
  srcset="http://wp-src.test/wp-content/uploads/2024/10/burst-heif-sequence-1024x576.jpg 1024w,
    http://wp-src.test/wp-content/uploads/2024/10/burst-heif-sequence-300x169.jpg 300w,
    http://wp-src.test/wp-content/uploads/2024/10/burst-heif-sequence-768x432.jpg 768w,
    http://wp-src.test/wp-content/uploads/2024/10/burst-heif-sequence-1280x720.jpg 1280w"
  sizes="(max-width: 1024px) 100vw, 1024px">
<!-- static-heif.heic -->
<img decoding="async" width="1024" height="683" data-id="1931"
  src="http://wp-src.test/wp-content/uploads/2024/10/static-heif-1024x683.jpg"
  alt="" class="wp-image-1931"
  srcset="http://wp-src.test/wp-content/uploads/2024/10/static-heif-1024x683.jpg 1024w,
    http://wp-src.test/wp-content/uploads/2024/10/static-heif-300x200.jpg 300w,
    http://wp-src.test/wp-content/uploads/2024/10/static-heif-768x512.jpg 768w,
    http://wp-src.test/wp-content/uploads/2024/10/static-heif-1440x960.jpg 1440w"
  sizes="(max-width: 1024px) 100vw, 1024px">

This ticket was mentioned in Slack in #core by chaion07. View the logs.


5 weeks ago

#23 follow-up: @peterwilsoncc
5 weeks ago

@azaozz The changes in r59317 will need a follow up. The docblock for the image_editor_output_format filter uses the variable name $output_format but the new code uses the variable name $default_output_format.

I think $output_format is the better name as by the time a plugin the filter runs in a plugin they may no longer be the defaults. Eg, plugin two in this code sample won't get the defaults.

add_filter( 'image_editor_output_format', 'plugin_one_changes', 10 );
add_filter( 'image_editor_output_format', 'plugin_two_changes', 20 );

#24 in reply to: ↑ 23 @azaozz
5 weeks ago

Replying to peterwilsoncc:

The changes in r59317 will need a follow up.
...
I think $output_format is the better

Ah, nice catch! Sure, $output_format sounds better. Fixing.

#25 @azaozz
5 weeks ago

In 59346:

Media: Better variable name and some docs fixes for the new wp_get_image_editor_output_format().

Props peterwilsoncc, apermo, azaozz.
See #62305.

#26 @azaozz
5 weeks ago

Just a note that both [59317] and [59346] need to be merged to the 6.7 branch (in that order).

#27 follow-up: @mukesh27
5 weeks ago

The [59317] changes caused a unit test error for the Modern Format Image plugin. For more details, please see https://github.com/WordPress/performance/issues/1634.

#28 in reply to: ↑ 27 @azaozz
5 weeks ago

Replying to mukesh27:

The [59317] changes caused a unit test error for the Modern Format Image plugin.

@mukesh27 Thanks for the ping about this!

May be missing something but seems all failing tests there are "negative". I.e. they are to confirm that the originally uploaded image in not converted to the expected format (when smaller than the big image threshold), and only the sub-sizes are converted?

I.e. these tests confirm that when uploading an 1080x720px JPEG image, and the WP_Image_Editor is set to convert JPEG to WEBP, the original image remains JPEG but all sub-sizes are WEBP. This comment on one of the failing tests seems to explain it: https://github.com/WordPress/performance/blob/trunk/plugins/webp-uploads/tests/test-load.php#L56.

If that's the case, thinking these tests can be fixed to reflect that this bug in core is now fixed.

#29 @peterwilsoncc
5 weeks ago

Thanks for the follow up, Ozz.

I reviewed the failing test and I think you are correct that it's a false negative. The sample image used is 1080px wide (so under the -scaled limit) so it makes sense to me that the image is converted.

I've asked Adam to confirm that so I'll wait until tomorrow morning before doing a backport of [59317] and [59346].

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


5 weeks ago

This ticket was mentioned in PR #7733 on WordPress/wordpress-develop by @mukesh27.


5 weeks ago
#31

  • Keywords has-unit-tests added

Testing attachment metadata in trunk and 6.7 branch.

Track ticket: https://core.trac.wordpress.org/ticket/62305

@mukesh27 commented on PR #7733:


5 weeks ago
#32

The [59317] changes make some changes in attachment metadata.

# In 6.7 branch

<prE>Array
(
    [width] => 1920
    [height] => 1080
    [file] => /tmp//33772.jpg
    [filesize] => 176615
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => 33772-300x169.webp
                    [width] => 300
                    [height] => 169
                    [mime-type] => image/webp
                    [filesize] => 55988
                )

            [large] => Array
                (
                    [file] => 33772-1024x576.webp
                    [width] => 1024
                    [height] => 576
                    [mime-type] => image/webp
                    [filesize] => 446208
                )

            [thumbnail] => Array
                (
                    [file] => 33772-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                    [filesize] => 31268
                )

            [medium_large] => Array
                (
                    [file] => 33772-768x432.webp
                    [width] => 768
                    [height] => 432
                    [mime-type] => image/webp
                    [filesize] => 279734
                )

        )

    [image_meta] => Array
        (
            [aperture] => 8
            [credit] => Photoshop Author
            [camera] => DMC-LX2
            [caption] => Photoshop Description
            [created_timestamp] => 1306315327
            [copyright] => Photoshop Copyrright Notice
            [focal_length] => 6.3
            [iso] => 100
            [shutter_speed] => 0.0025
            [title] => Photoshop Document Ttitle
            [orientation] => 1
            [keywords] => Array
                (
                    [0] => beach
                    [1] => baywatch
                    [2] => LA
                    [3] => sunset
                )
        )
)

# In current trunk

<prE>Array
(
    [width] => 1920
    [height] => 1080
    [file] => /tmp/33772-1920x1080.webp
    [filesize] => 865924
    [sizes] => Array
        (
            [medium] => Array
                (
                    [file] => 33772-300x169.webp
                    [width] => 300
                    [height] => 169
                    [mime-type] => image/webp
                    [filesize] => 55988
                )

            [large] => Array
                (
                    [file] => 33772-1024x576.webp
                    [width] => 1024
                    [height] => 576
                    [mime-type] => image/webp
                    [filesize] => 446208
                )

            [thumbnail] => Array
                (
                    [file] => 33772-150x150.webp
                    [width] => 150
                    [height] => 150
                    [mime-type] => image/webp
                    [filesize] => 31268
                )

            [medium_large] => Array
                (
                    [file] => 33772-768x432.webp
                    [width] => 768
                    [height] => 432
                    [mime-type] => image/webp
                    [filesize] => 279734
                )

        )

    [image_meta] => Array
        (
            [aperture] => 8
            [credit] => Photoshop Author
            [camera] => DMC-LX2
            [caption] => Photoshop Description
            [created_timestamp] => 1306315327
            [copyright] => Photoshop Copyrright Notice
            [focal_length] => 6.3
            [iso] => 100
            [shutter_speed] => 0.0025
            [title] => Photoshop Document Ttitle
            [orientation] => 1
            [keywords] => Array
                (
                    [0] => beach
                    [1] => baywatch
                    [2] => LA
                    [3] => sunset
                )
        )
    [original_image] => 33772.jpg
)

Check the file key in array. It use alternate mime type image and add the image sizes in name. Before it was /tmp//33772.jpg now it was /tmp/33772-1920x1080.webp. It cause the issues in plugins that use the file and update the metadata. See https://github.com/WordPress/performance/issues/1634

#33 @flixos90
5 weeks ago

@azaozz @peterwilsoncc I'm not sure the tests are "false negatives".

The assertImageNotHasSource( $attachment_id, 'image/jpeg' ) is to verify that the (full size) image is not available as a JPEG, as for that particular test it shouldn't be - it should only be converted to WebP or AVIF, and the original JPEG image should only be "backed up" under original_image but otherwise there should be no references to JPEG on the attachment.

I'm not sure yet why that behavior changed and whether it's something that needs to be fixed in Core or updated in the plugin to support an intentional Core change. But the assertions themselves still look correct to me.

#34 @flixos90
5 weeks ago

I looked into this more closely and found a few reasons for the problem, probably requiring changes in both places for different things:

  • The bug reported by @mukesh27 above is an actual bug in the Modern Image Formats plugin itself, that as I now discovered has been there for quite a while, but so far was only "exposed" by big images, which we didn't have test coverage for. 😞 More context in https://github.com/WordPress/performance/pull/1635#issuecomment-2460323575. So this needs to be fixed in the plugin.
  • As a separate problem, [59317] does not check which MIME type to convert an image to. The image_editor_output_format can be used with an output array for each MIME type, and the Modern Image Formats plugin may specify the MIME type itself there too. For example, "for JPEG" input, the valid output format is "JPEG" and/or "WebP". If you think about it as a conversion map, this may seem weird, but the filter is about "output format", not "conversion map", so we cannot assume it's only used as the latter. Core may therefore currently try to convert a JPEG to a JPEG, which it shouldn't even attempt. We should only set the $convert flag to true if the output format only contains MIME types that differ from the input MIME type.
Version 0, edited 5 weeks ago by flixos90 (next)

#37 @flixos90
5 weeks ago

In 59366:

Media: Only mark an image as requiring conversion if the output format differs from the input format.

Follow up to [59317] and [59346].

Props adamsilverstein, peterwilsoncc.
See #62305.

#39 @flixos90
5 weeks ago

Requiring backport to 6.7 branch:

  1. [59317]
  2. [59346]
  3. [59366]

This ticket was mentioned in PR #7740 on WordPress/wordpress-develop by @peterwilsoncc.


5 weeks ago
#40

Test group backport to 6.7 brnach

Trac ticket: Core-62305

#41 @peterwilsoncc
5 weeks ago

  • Keywords dev-reviewed added; fixed-major removed

[59317], [59346], [59366] look good for backport.

#42 @peterwilsoncc
5 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 59367:

Media: Fix converting of all HEIC/HEIF images to JPEGs after uploading regardless of dimensions.

This backport includes follow up commits to improve a variable name and improve accuracy of when an image needs to be converted.

Reviewed by peterwilsoncc.
Merges [59317], [59346], [59366] to the 6.7 branch.

Props ironprogrammer, adamsilverstein, azaozz, peterwilsoncc, apermo, flixos90.
Fixes #62305.

@peterwilsoncc commented on PR #7733:


4 weeks ago
#44

I've incorproated these tests in to https://github.com/WordPress/wordpress-develop/pull/7748 with a few modifications to use HEIC testing to allow it to rely on the default core features.

@mukesh27 commented on PR #7733:


4 weeks ago
#45

There was 1 failure:

1) Tests_Media::test_image_converted_to_other_format_has_correct_filename with data set "do not scale image" (false)
The file name is expected to be 33772.webp
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'33772.webp'
+'33772-1920x1080.webp'

@mukesh27 commented on PR #7733:


4 weeks ago
#46

@peterwilsoncc In https://github.com/WordPress/wordpress-develop/pull/7733/commits/76a5c39ab9208e11bb194b44dae3bc98a1566ed6 i incorporate changes from PR #7748 but now it return original image instead WebP mime type.

There was 1 failure:

1) Tests_Media::test_image_converted_to_other_format_has_correct_filename with data set "do not scale image" (false)
The file extension is expected to change.
Failed asserting that '/tmp/33772.jpg' ends not with ".jpg".

@peterwilsoncc commented on PR #7733:


4 weeks ago
#47

@mukeshpanchal27 That looks like it was due to the regex error @felixarntz picked up in https://github.com/WordPress/wordpress-develop/pull/7748#discussion_r1834688508

@mukesh27 commented on PR #7733:


4 weeks ago
#48

@peterwilsoncc Do we need these PR or can we close it?

#49 @peterwilsoncc
4 weeks ago

In 59379:

Media: Remove dimension suffix from full size converted HEIC images.

Removes the dimension suffix, eg -1000x1000 from the file name of full size images automatically converted from HEIC to JPEGs by WordPress. Introduces unit tests for the default conversion of images and customized conversion settings via the image_editor_output_format filter.

Follow up to [58849], [58942], [59317], [59346], [59366]

Props mukesh27, peterwilsoncc, azaozz, apermo, flixos90, ironprogrammer.
Fixes #62359.
See #53645, #62305.

@peterwilsoncc commented on PR #7733:


4 weeks ago
#50

@mukeshpanchal27 This was included in the follow up PR.

#51 @davidbaumwald
4 weeks ago

In 59381:

Media: Remove dimension suffix from full size converted HEIC images.

Removes the dimension suffix, eg -1000x1000 from the file name of full size images automatically converted from HEIC to JPEGs by WordPress. Introduces unit tests for the default conversion of images and customized conversion settings via the image_editor_output_format filter.

Follow up to [58849], [58942], [59317], [59346], [59366].

Reviewed by davidbaumwald.
Merges [59379] and [59380] to the 6.7 branch.

Props mukesh27, peterwilsoncc, azaozz, apermo, flixos90, ironprogrammer.
Fixes #62359.
See #53645, #62305.

Note: See TracTickets for help on using tickets.