Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#53667 closed defect (bug) (fixed)

Support `wp_editor_set_quality` for both loaded and saved `mime_type`s

Reported by: kirasong's profile kirasong Owned by: azaozz's profile azaozz
Milestone: 5.8.1 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

WordPress supports setting image quality via a filter, wp_editor_set_quality.

This filter provides the mime_type, which implies that the setting could conditionally apply to a certain MIME type only -- like in the example filter from this post:

// Use a quality setting of 75 for WebP images.
function filter_webp_quality( $quality, $mime_type ) {
  if ( 'image/webp' === $mime_type ) {
     return 75;
  }
  return $quality;
}
add_filter( 'wp_editor_set_quality', 'filter_webp_quality', 10, 2 );

However, set_quality() applies on load, but not on save. This means that if a different type is loaded than is saved (loading a JPEG, and saving WebP, for instance), that the filter would not apply to the latter mime_type.

WordPress should apply the filter's details when saving as well.

Attachments (1)

53667.diff (1.2 KB) - added by desrosj 4 years ago.

Download all attachments as: .zip

Change History (24)

#1 @desrosj
4 years ago

  • Milestone changed from Awaiting Review to 5.8

Going to move this to the milestone to see if we can figure out a solution before RC4.

I don't think this issue would be a huge deal to ship with 5.8, but it's definitely not ideal.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


4 years ago

#3 @kirasong
4 years ago

We talked about this in a media meeting today.

There are more details there, but to leave what I've tried so far:

One possible solution for this would be to run set_quality() during the save process if the output mime_type is different, then again (to set it to the previous type's values) after the process is finished — a bit the way that Imagick currently does with image formats.

This should also be doable with GD.

I haven’t, as of yet, been able to get this exactly right / to an elegant solution, and I think it'd be a big change in set_quality() behavior so close to release.

Ideally, this would have a good set of tests to make sure quality per mime type applies in both Imagick and GD properly during saving, and in the priority (#53669) expected.

@desrosj
4 years ago

#4 follow-up: @desrosj
4 years ago

  • Keywords has-patch added; needs-patch removed

In the media chat, these were the steps agreed upon for 5.8 to address this issue and #53668:

  • Leave the filters in as is.
  • Update the inline docs for the filter to explain that it is a bit experimental, and there are two known potential issues when using the filter to change image formats for sub sizes.
  • Post a developer note detailing these two edge cases.
  • Follow up in 5.8.x releases to fix these edge cases.

53667.diff is an adjustment to the filter's inline documentation to make developer's aware of these two edge cases, and that the filter should generally be considered experimental.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


4 years ago

#6 in reply to: ↑ 4 @ianmjones
4 years ago

Replying to desrosj:

53667.diff is an adjustment to the filter's inline documentation to make developer's aware of these two edge cases, and that the filter should generally be considered experimental.

Looks good to me.

#7 @desrosj
4 years ago

In 51442:

Media: Document edge cases with the new image_editor_output_format filter.

More testing has revealed that the image_editor_output_format filter has some interesting edge cases that developers should be aware of when electing to use this filter (see #53667 and #53668).

Because this is a new filter that was intended to be used for experimenting with different ways to handle generating image sizes and has not yet been adopted in the wild, expanding the inline documentation is an acceptable temporary solution while these edge cases are explored further and addressed.

Props mikeschroder, antpb, desrosj, adamsilverstein, ianmjones.
See #5366, #53668, #35725.

#8 @desrosj
4 years ago

  • Keywords dev-feedback fixed-major commit added

Marking [51442] for backport consideration.

#9 @jorbin
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

[51442] looks good to backport

#10 @desrosj
4 years ago

In 51444:

Media: Document edge cases with the new image_editor_output_format filter.

More testing has revealed that the image_editor_output_format filter has some interesting edge cases that developers should be aware of when electing to use this filter (see #53667 and #53668).

Because this is a new filter that was intended to be used for experimenting with different ways to handle generating image sizes and has not yet been adopted in the wild, expanding the inline documentation is an acceptable temporary solution while these edge cases are explored further and addressed.

Props mikeschroder, antpb, desrosj, adamsilverstein, ianmjones.
Merges [51442] to the 5.8 branch.
See #53667, #53668, #35725.

#11 @desrosj
4 years ago

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

#12 @kirasong
4 years ago

Should we keep this ticket open to track that the wp_editor_set_quality filter doesn't apply properly, since it'll still need to be fixed, even though we've marked image_editor_output_format as experimental?

#13 @desrosj
4 years ago

  • Milestone changed from 5.8 to 5.8.1
  • Resolution fixed deleted
  • Status changed from closed to reopened

Yes we should. Sorry, I closed on accident when I meant to move to 5.8.1.

#14 @desrosj
4 years ago

  • Keywords needs-patch added; has-patch fixed-major commit dev-reviewed removed

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


4 years ago

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


4 years ago
#16

  • Keywords has-patch has-unit-tests added; needs-patch removed

Introduce $output_mime_type in WP_Image_Editor and set it on saving the image if different from the original image's mime-type. Add tests.

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

azaozz commented on PR #1622:


4 years ago
#17

Also introduces WP_Image_Editor::get_default_quality( $mime_type ) to be able to have different default quality for different image formats.

Currently the quality is (re)set and the wp_editor_set_quality filter runs only once when the output image type is different from the input image type. Can be changed to run every time on saving an image, for example when creating image sub-sizes.

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


4 years ago

#19 @azaozz
4 years ago

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

In 51704:

Media: apply the wp_editor_set_quality filter not only when loading an image in the editor but also when saving an converted image, after the mime-type of the output image has changed.

Props mikeschroder, desrosj, azaozz.
Fixes #53667.

#20 @azaozz
4 years ago

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

Reopen for 5.8.1.

#21 @desrosj
4 years ago

  • Version set to 5.8

[51704] looks good in my review and testing. Let's get it into 5.8.1 RC.

#22 @desrosj
4 years ago

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

In 51712:

Media: apply the wp_editor_set_quality filter not only when loading an image in the editor but also when saving an converted image, after the mime-type of the output image has changed.

Props mikeschroder, desrosj, azaozz.
Merges [51704] to the 5.8 branch.
Fixes #53667.

Note: See TracTickets for help on using tickets.