Do not upscale/overize in filter_image_downsize

drzraf commented 3 years ago

Timmy apparently lacks the checks about whether cropping/resizing would actually reduce (contrary to increase) an image during the image_downsize filter (Since neither filter_image_downsize nor resize() contain the oversize guards which only live inside get_image_params()). That's one such trace I grabbed:

  25 => 
  array (
    'function' => 'post_get_meta_field',
    'type' => 'dynamic',
    'class' => 'Timber\\Integrations\\ACF',
    'file' => '/web/wp/wp-includes/class-wp-hook.php',
    'line' => 289,
    'params' => 
    array (
      'value' => '\'27721\'',
      'post_id' => '27716',
      'field_name' => '\'headerImageMobile\'',
    ),
  ),
  26 => 
  array (
    'function' => 'get_field',
    'file' => '/web/app/plugins/timber-library/lib/Integrations/ACF.php',
    'line' => 30,
    'params' => 
    array (
      'selector' => '\'headerImageMobile\'',
      'post_id' => '27716',
      'format_value' => '???',
    ),
  ),
[.... .... ...]
  33 => 
  array (
    'function' => 'format_value',
    'type' => 'dynamic',
    'class' => 'acf_field_image',
    'file' => 'web/wp/wp-includes/class-wp-hook.php',
    'line' => 287,
    'params' => 
    array (
      'value' => '\'27721\'',
      'post_id' => '27716',
      'field' => '[\'ID\' => 0, \'key\' => \'field_5f7ddcfa8c43d\', \'label\' => \'Header Image Mobile\', \'name\' => \'headerImageMobile\', \'prefix\' => \'acf\', \'type\' => \'image\', \'value\' => NULL, \'menu
_order\' => 1, \'instructions\' => \'com)\'
, \'required\' => 0, \'id\' => \'\', \'class\' => \'\', \'conditional_logic\' => 0, \'parent\' => \'group_5ba3c42472043\', \'wrapper\' => [\'width\' => \'\', \'class\' => \'\', \'id\' => \'\'], \'return_format\' 
=> \'array\', \'preview_size\' => \'thumbnail\', \'library\' => \'all\', \'min_width\' => 1666, \'min_height\' => 3000, \'min_size\' => \'\', \'max_width\' => 1666, \'max_height\' => 3000, \'max_size\' => \'\', \
'mime_types\' => \'jpg,jpeg\', \'_name\' => \'headerImageMobile\', \'_valid\' => 1]',
    ),
  ),
  34 => 
  array (
    'function' => 'acf_get_attachment',
    'file' => 'web/app/plugins/advanced-custom-fields-pro/includes/fields/class-acf-field-image.php',
    'line' => 337,
    'params' => 
    array (
      'attachment' => '27721',
    ),
  ),
  35 => 
  array (
    'function' => 'wp_get_attachment_image_src',
    'file' => 'web/app/plugins/advanced-custom-fields-pro/includes/api/api-helpers.php',
    'line' => 3189,
    'params' => 
    array (
      'attachment_id' => '27721',
      'size' => '\'content-default\'',
      'icon' => '???',
    ),
  ),
  36 => 
  array (
    'function' => 'image_downsize',
    'file' => 'web/wp/wp-includes/media.php',
    'line' => 953,
    'params' => 
    array (
      'id' => '27721',
      'size' => '\'content-default\'',
    ),
  ),
  37 => 
  array (
    'function' => 'apply_filters',
    'file' => 'web/wp/wp-includes/media.php',
    'line' => 207,
    'params' => 
    array (
      'tag' => '\'image_downsize\'',
      'value' => 'FALSE',
      1 => '27721',
      2 => '\'content-default\'',
    ),
  ),
  38 => 
  array (
    'function' => 'apply_filters',
    'type' => 'dynamic',
    'class' => 'WP_Hook',
    'file' => '/web/wp/wp-includes/plugin.php',
    'line' => 212,
    'params' => 
    array (
      'value' => 'FALSE',
      'args' => '[0 => FALSE, 1 => 27721, 2 => \'content-default\']',
    ),
  ),
  39 => 
  array (
    'function' => 'filter_image_downsize',
    'type' => 'dynamic',
    'class' => 'Timmy\\Timmy',
    'file' => '/web/wp/wp-includes/class-wp-hook.php',
    'line' => 287,
    'params' => 
    array (
      'return' => 'FALSE',
      'attachment_id' => '27721',
      'size' => '\'content-default\'',
    ),
  ),
  40 => 
  array (
    'function' => 'resize',
    'type' => 'static',
    'class' => 'Timmy\\Timmy',
    'file' => '/vendor/mindkomm/timmy/lib/Timmy.php',
    'line' => 496,
    'params' => 
    array (
      'img_size' => '[\'resize\' => [0 => 3000], \'srcset\' => [0 => [...], 1 => [...], 2 => [...], 3 => [...], 4 => [...], 5 => [...], 6 => [...], 7 => [...], 8 => [...], 9 => [...], 10 => [...], 11 => [...]], \'sizes\' => \'(max-width: 1200px) 100vw, 1200px\', \'show_in_ui\' => TRUE, \'oversize\' => [\'allow\' => FALSE, \'style_attr\' => FALSE], \'name\' => \'Content - Default\']',
      'file_src' => '\'https://bar.mx/app/uploads/2021/02/foo-3-mx.jpg\'',
      'width' => '3000',
      'height' => '0',
      'crop' => '\'default\'',
      'force' => 'FALSE',
    ),
  ),
  41 => 
  array (
    'function' => 'resize',
    'type' => 'static',
    'class' => 'Timber\\ImageHelper',
    'file' => '/vendor/mindkomm/timmy/lib/Timmy.php',
    'line' => 698,
    'params' => 
    array (
      'src' => '\'https://bar.mx/app/uploads/2021/02/foo-3-mx.jpg\'',
      'w' => '3000',
      'h' => '0',
      'crop' => '\'default\'',
      'force' => 'FALSE',
    ),
  ),

uploads/2021/02/foo-3-mx.jpg is actually 1666x3000 and Timmy must know about this. Then it must not try to create an image larger than the initial one since it's has all the chances to end-up badly:

Because the resulting image bring no quality (Browser can upscale image if told so and do it quicker and better)
Because triggering imagick to upscale often lead to memory problems (Among those ran by web/wp/wp-includes/class-wp-image-editor-imagick.php many filters would consume a bunch of memory on bigger images).

gchtr commented 3 years ago

Yep, you’re totally right. Timmy can easily know about whether it should upscale an image or not. I started working on this, but it’s taking a little longer than expected.

drzraf commented 2 years ago

Any news regarding this?

gchtr commented 2 years ago

Hey @drzraf

I’m in the process of refactoring a lot of things, because it became harder to maintain the code with a function based approach where I repeat myself quite often. There will be a new 1.0 version, including the fix for this. I made good progress in the last two weeks, so you can expect a result soon.

Sorry for keeping you waiting :)

gchtr commented 2 years ago

I just added the first beta for Timmy v1.0.0 that includes a fix for this: https://github.com/mindkomm/timmy/releases/tag/1.0.0-beta.1.

drzraf commented 2 years ago

I tested it and it seems to work! That's a great change. Thank you

gchtr commented 2 years ago

Great! Thanks for the feedback 👍😊

mindkomm / timmy

Do not upscale/overize in filter_image_downsize #36