WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#34931 closed enhancement (wontfix)

Adding a filter to modify ratio differs when generating img srcset

Reported by: mbcreation Owned by: joemcgill
Milestone: Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch close
Focuses: Cc:
PR Number:

Description

Instead of debating about how accurate the current ratio is (0.002) why not just add a filter ?
Since this is my first contribution attempt, please be nice :) Or explain how to do better.
Have a nice 4.4 day.

Attachments (1)

srcset_max_diff_ratio.diff (629 bytes) - added by mbcreation 4 years ago.

Download all attachments as: .zip

Change History (14)

#1 @mbcreation
4 years ago

  • Keywords has-patch added

#2 @swissspidy
4 years ago

  • Type changed from feature request to enhancement

Hi there, welcome to trac then!

I'll leave it to others to decide whether this filter makes sense.

From a code point of view I'd suggest you to follow the inline documentation standards for filters. Perhaps putting $ratio_diff = apply_filters( 'max_srcset_image_diff_ratio', 0.002 ) on a new line for increased readability.

Last edited 4 years ago by swissspidy (previous) (diff)

#3 @mbcreation
4 years ago

Thanks Pascal !

Should I edit my ticket with a modified patch ?

Benoit.

#4 @joemcgill
4 years ago

  • Keywords reporter-feedback added
  • Owner set to joemcgill
  • Status changed from new to reviewing

Thanks @mbcreation!

The way we're calculating ratio variance in wp_calculate_image_srcset() could probably be improved, but I hesitate to add a user filter, because that would open up the probability that images in a srcset are completely different crops than the original image, which would not be good.

Are you encountering a use case where this filter would be helpful?

Related: #34810.

#5 @mbcreation
4 years ago

Hi Joe and thanks for your feedback.

Just did one update (to 4.4) this morning and noticed that around half of my thumbnail didn't include any srcset.
By getting into the source code i found out that this "diff ratio" was responsible of that. Then wondering why there was not filter like max_srcset_image_width (for instance)

Not a big deal of course but the actual figure seems to be quite demanding. (excuse my english...)

#6 @joemcgill
4 years ago

@mbcreation can you share the image dimensions of an original image that wasn't getting a srcset applied, and tell me what other intermediate sizes are available (and their respective dimensions) for that image by checking the output of wp_get_attachment_metadata( )? Thanks.

#7 follow-up: @mbcreation
4 years ago

@joemcgill

These are dump of $image and ratio from wp_calculate_image_srcset loop.
Original is 3008*2000 so it's filtered out by max_srcset_image_width

<?php
                        array(3) {
  ["file"]=>
  string(20) "desserts-150x150.jpg"
  ["width"]=>
  string(3) "150"
  ["height"]=>
  string(3) "150"
}
float(0.33333333333333)
array(3) {
  ["file"]=>
  string(20) "desserts-300x199.jpg"
  ["width"]=>
  string(3) "300"
  ["height"]=>
  string(3) "199"
}
float(0.0033333333333333)
array(3) {
  ["file"]=>
  string(21) "desserts-1024x680.jpg"
  ["width"]=>
  string(4) "1024"
  ["height"]=>
  string(3) "680"
}
float(0.0026041666666666)
array(3) {
  ["file"]=>
  string(20) "desserts-595x231.jpg"
  ["width"]=>
  string(3) "595"
  ["height"]=>
  string(3) "231"
}
float(0.27843137254902)
array(3) {
  ["file"]=>
  string(19) "desserts-123x82.jpg"
  ["width"]=>
  string(3) "123"
  ["height"]=>
  string(2) "82"
}
float(0)

#8 in reply to: ↑ 7 ; follow-up: @joemcgill
4 years ago

Replying to mbcreation:

Thanks again. Which image is being shown in the src attribute in your example?

#9 in reply to: ↑ 8 @mbcreation
4 years ago

Replying to joemcgill:

Replying to mbcreation:

Thanks again. Which image is being shown in the src attribute in your example?

123*82 (as expected) since this is a post_thumbnail call with a custom size parameter

#10 follow-up: @joemcgill
4 years ago

Got it. This does look like it may be a bug with the way the variance is calculated. I'd prefer to handle this edge case in the function itself first before introducing another filter for now.

#11 in reply to: ↑ 10 @mbcreation
4 years ago

Replying to joemcgill:

Got it. This does look like it may be a bug with the way the variance is calculated. I'd prefer to handle this edge case in the function itself first before introducing another filter for now.

Well thanks for the feedback !

#12 @joemcgill
4 years ago

  • Keywords close added; reporter-feedback removed

Looks like #34955 is a related issue. Since the purpose of this ticket, as I understand it, is requesting that a filter be added, I'm going to leave this as is and handle the bugfix on the other ticket and suggest that this be closed when we come up with the proper fix.

#13 @joemcgill
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from reviewing to closed

Closing now that #34955 is fixed as I don't think we want to add a filter for the ratios, since it would likely be used to generate invalid markup, and cases where a filter would be helpful are likely bugs that would be better to fix than provide a workaround for.

Note: See TracTickets for help on using tickets.