flokosiol / kirby-focus

Better image cropping for Kirby CMS
184 stars 14 forks source link

WebP and AVIF generated images put into wrong folder in v3.0.8 #65

Closed renestalder closed 2 years ago

renestalder commented 2 years ago

It seems like in 3.0.8, the assets for avif and webp somehow land in the wrong folder, which in my cases causes the frontend to fallback to some non-cropped and not properly sized version of the image. I had to downgrade for the moment, as it broke the website.

What I see so far, in my root folder (I have a public folder setup as described on the Kirby website), the cropped version of the avif and webp images added (this is the wrong location. They also have double extension naming, so there is definitvely something wrong.

When I check the correct location for the images (in this case, media/pages/team, I find some avif and webp version, but they are not cropped.

Screenshot from 2022-01-17 17-38-30

srcset config:

  'default' => [
    '280w' => ['width' => 256, 'height' => 256, 'quality' => 100, 'crop' => true],
    '335w' => ['width' => 335, 'height' => 335, 'quality' => 100, 'crop' => true],
    '560w' => ['width' => 560, 'height' => 560, 'quality' => 100, 'crop' => true],
    '670w' => ['width' => 670, 'height' => 670, 'quality' => 100, 'crop' => true],
    '728w' => ['width' => 728, 'height' => 728, 'quality' => 100, 'crop' => true],
    '1024w' => ['width' => 1024, 'height' => 1024, 'quality' => 100, 'crop' => true],
    '1456w' => ['width' => 1456, 'height' => 1456, 'quality' => 100, 'crop' => true],
    '2048w' => ['width' => 2048, 'height' => 2048, 'quality' => 100, 'crop' => true]
  ]

Edit: The srcset config example doesn't above is not showing the chosen image format, as in my case, I have a function generating this and adding the 'format' options for avif and webp.

renestalder commented 2 years ago

@flokosiol Are you able to reproduce it? I am currently having to pin the plugin in all websites to the 3.0.7 release as of my CI/CD automation automatically update patch versions, and therefore breaking websites with 3.0.8.

I tried to look at your code changes to get a sense of what could go wrong, but I don't exactly see how your changes lead to that result.

HashandSalt commented 2 years ago

Seeing this too :(

flokosiol commented 2 years ago

Thanks for the reminder. I set up a test installation (which I hopefully will extend furthermore). Could you please check out if this works for you?

https://github.com/flokosiol/kirby-focuskit

For me everything worked fine with this installation, but it's based on the plainkit (which has no public folder setup) and I couldn't get avif files to work at all. At least the generated webp files have been saved to the correct media subfolder. So maybe we can use this as a starting point?!

screenshot-focuskit

HashandSalt commented 2 years ago

I am not getting the srcset images at all with that.

image

In my test i used the Starter kit and switched the album.php template to use the plugin.

HashandSalt commented 2 years ago

Like this:

<?php
/*
  Templates render the content of your pages.

  They contain the markup together with some control structures
  like loops or if-statements. The `$page` variable always
  refers to the currently active page.

  To fetch the content from each field we call the field name as a
  method on the `$page` object, e.g. `$page->title()`.

  This example template makes use of the `$gallery` variable defined
  in the `album.php` controller (/site/controllers/album.php)

  Snippets like the header and footer contain markup used in
  multiple templates. They also help to keep templates clean.

  More about templates: https://getkirby.com/docs/guide/templates/basics
*/
?>
<?php snippet('header') ?>
<article>
  <?php snippet('intro') ?>
  <div class="grid">

    <div class="column" style="--columns: 4">
      <div class="text">
        <?= $page->text() ?>
      </div>
    </div>

    <div class="column" style="--columns: 8">
      <ul class="album-gallery">
        <?php foreach ($gallery as $image): ?>
        <li>
          <a href="<?= $image->url() ?>" data-lightbox>
            <figure class="img" style="--w:<?= $image->width() ?>;--h:<?= $image->height() ?>">

                <picture class="item-class">
                <source srcset="<?= $image->focusCrop(800, null, ['format' => 'avif'])->url()?>" type="image/avif">
                <source srcset="<?= $image->focusCrop(800, null, ['format' => 'webp'])->url()?>" type="image/webp">
                <img src="<?= $image->thumb(['width' => 800, 'height' => null, 'format' => $image->extension()])->url() ?>" alt="<?= $image->alt() ?>">
                </picture>

            </figure>
          </a>

        </li>
        <?php endforeach ?>
      </ul>
    </div>

</article>
<?php snippet('footer') ?>
HashandSalt commented 2 years ago

With that i get i get the thumbs in the webroot with names like

last-tree-standing-800x800-crop-50-50.avif.avif

and only Avifs... no webps... i think it uses the first thing in the source list and ignores the rest.

flokosiol commented 2 years ago

Thanks for the quick reply.

Maybe it's an GD vs. ImageMagick thing. I used GD version 2.3.3 for my tests and just learned that it doesn't support AVIF at all until PHP 8.1.

I will try to get IM up and running and test it again. What do you use?

lukasbestle commented 2 years ago

So does this only happen with ImageMagick or also with GDLib? If only with ImageMagick, I suspect that something could be off with the command line arguments.

renestalder commented 2 years ago

I mean, I never tried with GD. To my knowledge, or that's what I read when Kirby added support for it is, that only ImageMagick supports AVIF. That said, I always used ImageMagick then. But for sure, the issue doesn't apply when "Kirby Focus" isn't used.

HashandSalt commented 2 years ago

Just to confirm, im using IM as the image driver.

HashandSalt commented 2 years ago

Oh and i discovered that Kirby generates on access and on browser support. If you knock out the avif line so you just have source tags for webp and jpg, you will get webps. Adjust the browsers window size and you get the other sizes too.

lukasbestle commented 2 years ago

I feel like the issue could be coming from these lines:

https://github.com/flokosiol/kirby-focus/blob/96177849a70fdca2ec7b36028f8d9bcaea3393f6/src/Focus/ImageMagick.php#L220-L222

It would simultaneously explain the images being stored in the root and also the double extension.

@flokosiol I'd also recommend using escapeshellarg() for the dynamic arguments for added security and reliability. We've updated the core driver recently.

flokosiol commented 2 years ago

That's what I thought, too. The image driver need to be in sync with Kirby core – with a few additional focus specific lines of code. I'm pretty sure this can be fixed with a re-sync of the IM driver.

I'll have a look at this. Thanks for your investigation and your patience.

flokosiol commented 2 years ago

I updated the IM driver for the kirby-focuskit but WITHOUT ANY SINGLE TESTING, because I need to setup ImageMagick on my machine first. But if I didn't introduce any syntax errors, this might already be the solution.

You can either check this untested thing or wait until I set up IM locally and test it myself :)

Does anyone know if it's possible to watch files on Github? Would be awesome, if I could kind of subscribe to get a notification from Github as soon as the two files change in Kirby core.

renestalder commented 2 years ago

I mean, you could watch composer dependencies (pointing to the Kirby dev branch directly) with Dependabot or Renovate and automatically test those in GitHub Actions or with Docker images. At least, that's how my setup is with websites: Checking for new dependencies with Renovate every night, then automatically update those dependencies on a branch and make docker builds to test them. Probably kind of a stretch for what you want.

HashandSalt commented 2 years ago

@flokosiol ok so some progress with your tweaked driver - GD is working fine, with IM the images are now ending up in the media folder BUT no cropping is happening.

image

lukasbestle commented 2 years ago

@flokosiol Since you don't extend our core drivers but the general Darkroom class, your drivers don't need to be perfectly in sync with ours. If there were any breaking changes for plugin devs, we would communicate that in the release notes.

flokosiol commented 2 years ago

@HashandSalt Thanks for testing. Obviously, changing code blindly is not the best option. I will install IM on my machine and test it propperly 😉

@lukasbestle Thanks for the info!

flokosiol commented 2 years ago

Hey folks. I think I fixed it for the https://github.com/flokosiol/kirby-focuskit/ Would be awesome if someone could confirm this. Afterwards I would update the plugin and publish it as 3.1.0 here.

Thanks for your patience!

HashandSalt commented 2 years ago

@flokosiol Awsome :) Just tested it and everything seems good now on IM

image

flokosiol commented 2 years ago

Thanks a lot for testing! Unfortunately the grayscale version does not work yet. I totally missed that, too.

Nevertheless the main work is done and I can release 3.1.0 for K3.6+ soon. 👍

flokosiol commented 2 years ago

I just released 3.1.0 and hope I configured the version numbers and dependencies correctly.

renestalder commented 2 years ago

Can those people that tested this confirm, that the AVIF file is really an AVIF file and not just a renamed JPG? I just checked the new version, set the focus point on an image, used by overcomplicated Twig macros to render it and checked with Google Lighthouse. The performance stats gave me a warning that I should use next gen image formats. The AVIF file triggers that warning. When I download the AVIF file, it appears as a JPG on my local file system.

I suspect there is something wrong in the AVIF conversion, which I never had before without the focus plugin. I might have to dig a bit deeper to confirm this is not because of my setup or config, but it would be good if anyone having the new version running with AVIF could check too.

I gladly create a new issue for that as soon as I can confirm that it's indeed an issue caused by the plugin.

Still, possible this is an issue on my end or in Kirby itself.

flokosiol commented 2 years ago

I added an AVIF test case to the FocusKit test setup and updated to the latest versions of Kirby and Focus. https://github.com/flokosiol/kirby-focuskit

For me everything seems to be fine with the generated AVIF files. Strange. @renestalder

renestalder commented 2 years ago

@flokosiol Thanks a lot for taking the time for checking. After your check. I pushed the code where this happens to a remote server and there it worked flawlessly, so I suppose there is something broken with my local docker images.

flokosiol commented 2 years ago

Alright. Thanks a lot for the info!