#62305 closed defect (bug) (fixed)
Original HEIC image is not converted to JPEG
Reported by: | azaozz | Owned by: | 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 )
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)
Change History (52)
#3
@
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
@
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
Trac ticket: https://core.trac.wordpress.org/ticket/62305
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
6 weeks ago
#9
PR may need to be adjusted a bit after https://core.trac.wordpress.org/ticket/62272.
#10
@
6 weeks ago
Seems https://github.com/WordPress/wordpress-develop/pull/7656 needs a bit more, couldn't get ti 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).
This ticket was mentioned in Slack in #core by azaozz. View the logs.
6 weeks ago
#12
follow-up:
↓ 14
@
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.
@adamsilverstein commented on PR #7656:
6 weeks ago
#13
Closing in favor of https://github.com/WordPress/wordpress-develop/pull/7667
#14
in reply to:
↑ 12
@
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.
#16
@
6 weeks ago
- Owner set to azaozz
- Resolution set to fixed
- Status changed from new to closed
In 59317:
#17
@
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.
5 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
@
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:
↓ 24
@
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
@
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.
#27
follow-up:
↓ 28
@
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
@
4 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
@
4 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.
4 weeks ago
This ticket was mentioned in PR #7733 on WordPress/wordpress-develop by @mukesh27.
4 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:
4 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
@
4 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
@
4 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 more minor problem, [59317] does not check which MIME type to convert an image to. The
image_editor_output_format
can receive the same output as input MIME type. The Modern Image Formats plugin for example specifies it dynamically, depending on the configuration. It's definitely possible for the filter to include a map entry fromimage/jpeg
toimage/jpeg
for example. 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 totrue
if the output format only contains MIME types that differ from the input MIME type.
This ticket was mentioned in PR #7736 on WordPress/wordpress-develop by @flixos90.
4 weeks ago
#35
Minor follow up fix to https://core.trac.wordpress.org/changeset/59317, see https://core.trac.wordpress.org/ticket/62305#comment:34.
Trac ticket: https://core.trac.wordpress.org/ticket/62305
This ticket was mentioned in PR #7736 on WordPress/wordpress-develop by @flixos90.
4 weeks ago
#36
Minor follow up fix to https://core.trac.wordpress.org/changeset/59317, see https://core.trac.wordpress.org/ticket/62305#comment:34.
Trac ticket: https://core.trac.wordpress.org/ticket/62305
@flixos90 commented on PR #7736:
4 weeks ago
#38
Committed in https://core.trac.wordpress.org/changeset/59366
This ticket was mentioned in PR #7740 on WordPress/wordpress-develop by @peterwilsoncc.
4 weeks ago
#40
Test group backport to 6.7 brnach
Trac ticket: Core-62305
@peterwilsoncc commented on PR #7740:
4 weeks ago
#43
@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?
@peterwilsoncc commented on PR #7733:
4 weeks ago
#50
@mukeshpanchal27 This was included in the follow up PR.
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.