Generate only WebP images by default for JPEG and WebP uploads #527

eugene-manuilov · 2022-09-13T09:01:12Z

Summary

Fixes #524

Relevant technical choices

Changes in other files are required to fix issues that have been found after removing the image/jpeg mime type from the list of allowed transformations for jpeg images.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

…ort only webp versions.

felixarntz

@eugene-manuilov The root change here is of course rather simple, but I see this is leading to all sorts of problems now. I believe some of your fixes are a bit preliminary though. We should not focus on fixing the tests, we should focus more on fixing the expected behavior, and it is very well possible that the tests are an issue here. At a high level, I think the biggest overarching problem is the following:

We have all this logic here to generate additional MIME type files.
That is however mainly for the case that the input MIME type needs more than one output MIME type.
With this PR, we're changing both image/jpeg and image/webp to have only one output MIME type though, so most of the code in the module shouldn't be run if that's the case (early returns are needed). For the single MIME type behavior, the only function that is critical is webp_uploads_filter_image_editor_output_format() (the image_editor_output_format callback), and that will have core handle everything. The only other thing that still needs to run from our plugin is to populate the sources, but not generate any additional images, since core will already generate all the necessary images out of the box (only in e.g. WebP even though you upload a JPEG).

There are potentially a few smaller problems here or there where we assume that the uploaded image MIME type is also the MIME type that WordPress has already generated files for, which is not necessarily the case. But really, by adding early returns in most entry points here we should be able to fix almost all the problems in a more appropriate way.

modules/images/webp-uploads/helper.php

modules/images/webp-uploads/image-edit.php

modules/images/webp-uploads/load.php

felixarntz · 2022-09-13T21:22:57Z

modules/images/webp-uploads/load.php

-		if ( empty( $properties['sources'][ $current_mime ] ) ) {
+		if (
+			empty( $properties['sources'][ $current_mime ] ) &&
+			in_array( $current_mime, $valid_mime_transforms[ $mime_type ], true )


Why is this needed? If the size already has a file of $current_mime, whatever the MIME type is WordPress would only have generated it if it should. If without this condition we get a problem, then I think the bug is somewhere else.

If I remember correctly, the problem was that we kept generating source information for the image/jpeg mime type despite the fact that we were no jpeg images after converting it to webp. This fix ensures that we don't add a mime type to the sources list if it doesn't exists in the transformations list.

@eugene-manuilov

we kept generating source information for the image/jpeg mime type despite the fact that we were no jpeg images after converting it to webp.

This is one of the problems I was trying to convey further above. We need to fix that, rather than add this condition here I think.

I tested this locally, and I still think the extra condition here should be reverted. With the configuration of a single MIME type, $current_mime will be set to image/webp, as that is the MIME type of any existing image size WP core generates in that scenario. All this condition does is set the sources information for that existing WebP image. Checking whether it is in the list of allowed transform MIME types is unnecessary - mostly since that file already exists. If the file already exists, we don't need to check whether our filters allow for it. It's already generated, so we can just populate the information for it.

The MIME transforms filter is critical only to decide whether to transform an image of a certain format to a certain format, but for an image that already exists we can just add its information. In fact, we need to remove a similar condition around line 58 as well, since otherwise it runs into the problem that for the original JPEG no sources value is added, just because we don't want to generate it.

@eugene-manuilov I fixed this in 670b63f. There are currently unit test failures, but those are only happening because WordPress still keeps the full JPEG image. Simply not putting it into sources should not be a fix for those tests, since that means the image is still there.

What I think we want to do in the case that only WebP should be generated is actually override the original file with our generated "full" size WebP. We can use _wp_image_meta_replace_original() for that. And in that case, we can then also remove that original image from sources.

@eugene-manuilov I found the actual problem that was causing the test to fail, and fixed it in 8825df7 (see code comment on what was causing the problem).

The extra condition here is indeed not necessary and was only avoiding the failure rather than fixing the problem which was more in the test itself.

modules/images/webp-uploads/load.php

…mages.

…-webp-for-jpeg

… the allowed image transformations.

…WebP support (by secondary editor class).

…only mode, and add coverage for both use-cases.

felixarntz · 2022-10-05T03:06:12Z

@eugene-manuilov In 9581c53, I implemented a fix to ensure that, when the MIME transforms are set to only generate WebP, the original/full size image is also replaced accordingly. See data below what that changes.

Attachment metadata before

array(7) {
  ["width"]=>
  int(1800)
  ["height"]=>
  int(1350)
  ["file"]=>
  string(15) "2022/10/bar.jpg"
  ["filesize"]=>
  int(326585)
  ["sizes"]=>
  array(5) {
    ["medium"]=>
    array(6) {
      ["file"]=>
      string(16) "bar-300x225.webp"
      ["width"]=>
      int(300)
      ["height"]=>
      int(225)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(19644)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "bar-300x225.webp"
          ["filesize"]=>
          int(19644)
        }
      }
    }
    ["large"]=>
    array(6) {
      ["file"]=>
      string(17) "bar-1024x768.webp"
      ["width"]=>
      int(1024)
      ["height"]=>
      int(768)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(102508)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(17) "bar-1024x768.webp"
          ["filesize"]=>
          int(102508)
        }
      }
    }
    ["thumbnail"]=>
    array(6) {
      ["file"]=>
      string(16) "bar-150x150.webp"
      ["width"]=>
      int(150)
      ["height"]=>
      int(150)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(7614)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "bar-150x150.webp"
          ["filesize"]=>
          int(7614)
        }
      }
    }
    ["medium_large"]=>
    array(6) {
      ["file"]=>
      string(16) "bar-768x576.webp"
      ["width"]=>
      int(768)
      ["height"]=>
      int(576)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(68598)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "bar-768x576.webp"
          ["filesize"]=>
          int(68598)
        }
      }
    }
    ["1536x1536"]=>
    array(5) {
      ["file"]=>
      string(18) "bar-1536x1152.webp"
      ["width"]=>
      int(1536)
      ["height"]=>
      int(1152)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(179844)
    }
  }
  ["image_meta"]=>
  array(12) {
    ["aperture"]=>
    string(1) "0"
    ["credit"]=>
    string(0) ""
    ["camera"]=>
    string(0) ""
    ["caption"]=>
    string(0) ""
    ["created_timestamp"]=>
    string(1) "0"
    ["copyright"]=>
    string(0) ""
    ["focal_length"]=>
    string(1) "0"
    ["iso"]=>
    string(1) "0"
    ["shutter_speed"]=>
    string(1) "0"
    ["title"]=>
    string(0) ""
    ["orientation"]=>
    string(1) "0"
    ["keywords"]=>
    array(0) {
    }
  }
  ["sources"]=>
  array(2) {
    ["image/jpeg"]=>
    array(2) {
      ["file"]=>
      string(7) "bar.jpg"
      ["filesize"]=>
      int(326585)
    }
    ["image/webp"]=>
    array(2) {
      ["file"]=>
      string(12) "bar-jpg.webp"
      ["filesize"]=>
      int(315124)
    }
  }
}

Attachment metadata after

array(8) {
  ["width"]=>
  int(1800)
  ["height"]=>
  int(1200)
  ["file"]=>
  string(20) "2022/10/pie-jpg.webp"
  ["filesize"]=>
  int(92266)
  ["sizes"]=>
  array(5) {
    ["medium"]=>
    array(6) {
      ["file"]=>
      string(16) "pie-300x200.webp"
      ["width"]=>
      int(300)
      ["height"]=>
      int(200)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(4912)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "pie-300x200.webp"
          ["filesize"]=>
          int(4912)
        }
      }
    }
    ["large"]=>
    array(6) {
      ["file"]=>
      string(17) "pie-1024x683.webp"
      ["width"]=>
      int(1024)
      ["height"]=>
      int(683)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(20432)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(17) "pie-1024x683.webp"
          ["filesize"]=>
          int(20432)
        }
      }
    }
    ["thumbnail"]=>
    array(6) {
      ["file"]=>
      string(16) "pie-150x150.webp"
      ["width"]=>
      int(150)
      ["height"]=>
      int(150)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(2256)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "pie-150x150.webp"
          ["filesize"]=>
          int(2256)
        }
      }
    }
    ["medium_large"]=>
    array(6) {
      ["file"]=>
      string(16) "pie-768x512.webp"
      ["width"]=>
      int(768)
      ["height"]=>
      int(512)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(13368)
      ["sources"]=>
      array(1) {
        ["image/webp"]=>
        array(2) {
          ["file"]=>
          string(16) "pie-768x512.webp"
          ["filesize"]=>
          int(13368)
        }
      }
    }
    ["1536x1536"]=>
    array(5) {
      ["file"]=>
      string(18) "pie-1536x1024.webp"
      ["width"]=>
      int(1536)
      ["height"]=>
      int(1024)
      ["mime-type"]=>
      string(10) "image/webp"
      ["filesize"]=>
      int(40008)
    }
  }
  ["image_meta"]=>
  array(12) {
    ["aperture"]=>
    string(1) "0"
    ["credit"]=>
    string(0) ""
    ["camera"]=>
    string(0) ""
    ["caption"]=>
    string(0) ""
    ["created_timestamp"]=>
    string(1) "0"
    ["copyright"]=>
    string(0) ""
    ["focal_length"]=>
    string(1) "0"
    ["iso"]=>
    string(1) "0"
    ["shutter_speed"]=>
    string(1) "0"
    ["title"]=>
    string(0) ""
    ["orientation"]=>
    string(1) "0"
    ["keywords"]=>
    array(0) {
    }
  }
  ["sources"]=>
  array(1) {
    ["image/webp"]=>
    array(2) {
      ["file"]=>
      string(12) "pie-jpg.webp"
      ["filesize"]=>
      int(92266)
    }
  }
  ["original_image"]=>
  string(7) "pie.jpg"
}

The original image is now backed up, similarly to how core does it when scaling it down. This way we keep the JPEG available in case it's needed, but we fully use WebP, as we should when that is the only output MIME type from the webp_uploads_get_upload_image_mime_transforms() function.

…ite the image similar to how WordPress core does.

felixarntz

@eugene-manuilov This is now good to go from my end. Since I made additional changes, please review and LMKWYT.

mehulkaklotar

Changes look good to me! 🚀

We will need to add some conditions in this code for the checkbox integration for site settings to generate both mime types from this PR which is being worked on right now #537

Updated the default mime types for jpeg images transformation to supp…

9eb37cd

…ort only webp versions.

eugene-manuilov added [Type] Enhancement A suggestion for improvement of an existing feature [Focus] Images [Plugin] Modern Image Formats Issues for the Modern Image Formats plugin (formerly WebP Uploads) labels Sep 13, 2022

eugene-manuilov added this to the 1.5.0 milestone Sep 13, 2022

eugene-manuilov requested review from adamsilverstein, felixarntz and mitogh as code owners September 13, 2022 09:01

eugene-manuilov mentioned this pull request Sep 13, 2022

Modify WebP default behavior to follow latest decision (generate only WebP for JPEG) #524

Closed

eugene-manuilov added 2 commits September 13, 2022 20:28

Fix broken tests and incorrect behaviour.

f4a7aea

Fix the remaining issues.

63c7602

felixarntz requested changes Sep 13, 2022

View reviewed changes

eugene-manuilov added 2 commits September 14, 2022 14:18

Remove image/jpeg option from allowed transformations list for webp i…

10b0030

…mages.

Merge remote-tracking branch 'origin/trunk' into enhancement/524-only…

1bd3b76

…-webp-for-jpeg

eugene-manuilov requested a review from felixarntz September 14, 2022 14:14

felixarntz modified the milestones: 1.5.0, 1.6.0 Sep 14, 2022

Merge remote-tracking branch 'origin/trunk' into enhancement/524-only…

2dae125

…-webp-for-jpeg

This was referenced Sep 21, 2022

Add checkbox to Settings > Media to control whether to generate JPEG in addition to WebP #537

Merged

Add checkbox to Media screen to enable multi MIME type output (WebP and JPEG) #525

Closed

eugene-manuilov and others added 8 commits September 30, 2022 20:51

Merge remote-tracking branch 'origin/trunk' into enhancement/524-only…

03ab177

…-webp-for-jpeg

Revert a few changes.

95cbb93

Add an early return.

93e17d2

Merge remote-tracking branch 'origin/trunk' into enhancement/524-only…

95d1fc0

…-webp-for-jpeg

Remove unnecessary checks for whether existing image files are within…

670b63f

… the allowed image transformations.

Introduce opt_in_to_jpeg_and_webp() method to simplify tests.

166f5b4

Fix test problem that was causing WordPress to generate JPEG despite …

8825df7

…WebP support (by secondary editor class).

Ensure even the full image is a WebP when the module is used in WebP-…

9581c53

…only mode, and add coverage for both use-cases.

Remove unnecessary file_exists check, as we should always save/overwr…

02fda1e

…ite the image similar to how WordPress core does.

felixarntz approved these changes Oct 5, 2022

View reviewed changes

mehulkaklotar approved these changes Oct 5, 2022

View reviewed changes

jjgrainger approved these changes Oct 5, 2022

View reviewed changes

mukeshpanchal27 approved these changes Oct 5, 2022

View reviewed changes

mehulkaklotar merged commit 950daa7 into trunk Oct 5, 2022

mehulkaklotar deleted the enhancement/524-only-webp-for-jpeg branch October 5, 2022 11:30

felixarntz changed the title ~~Updated the default mime types for jpeg images transformation to support only webp versions.~~ Generate only WebP images by default for JPEG and WebP uploads Oct 5, 2022

adamsilverstein mentioned this pull request Nov 9, 2022

Tweak: Make Elementor compatible to WebP [ED-7895] elementor/elementor#19558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Generate only WebP images by default for JPEG and WebP uploads #527

Generate only WebP images by default for JPEG and WebP uploads #527

Uh oh!

eugene-manuilov commented Sep 13, 2022 •

edited

Loading

Uh oh!

felixarntz left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz Sep 13, 2022

Uh oh!

eugene-manuilov Sep 14, 2022

Uh oh!

felixarntz Sep 14, 2022

Uh oh!

felixarntz Oct 4, 2022

Uh oh!

felixarntz Oct 4, 2022

Uh oh!

felixarntz Oct 5, 2022

Uh oh!

Uh oh!

felixarntz commented Oct 5, 2022

Uh oh!

felixarntz left a comment

Uh oh!

mehulkaklotar left a comment

Uh oh!

Uh oh!

Generate only WebP images by default for JPEG and WebP uploads #527

Generate only WebP images by default for JPEG and WebP uploads #527

Uh oh!

Conversation

eugene-manuilov commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Relevant technical choices

Checklist

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

felixarntz Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

eugene-manuilov Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

felixarntz Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

felixarntz Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

felixarntz Oct 4, 2022

Choose a reason for hiding this comment

Uh oh!

felixarntz Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felixarntz commented Oct 5, 2022

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

mehulkaklotar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eugene-manuilov commented Sep 13, 2022 •

edited

Loading