Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ImageMedium#derivatives now works with image filters #1107

Merged
merged 10 commits into from
Oct 23, 2016

Conversation

fredrikekelund
Copy link
Contributor

This is a follow up to #1035, I had to close that PR since I forgot to point it to a feature branch.

Original description:

My original motivation for writing this PR was the problem described in #1016, where using the derivatives method on an image in a Twig template wouldn't work well in combination with the other method of producing responsive images in Grav - appending a pixel density indicator to the image's filename (eg. "@2x").

The underlying problem here also meant that image filters would not work in combination with image derivatives. They would work with image alternatives, which is what was generated from appending "@2x" to the filename of the image, but it wouldn't work with derivatives.

This commit fixes both the first and the second problem by making it so that the ImageMedium#derivatives method doesn't add image variants to a separate array, but rather to the same array as it would have if the filename method was used.

I've made some line comments to highlight a few things in the code as well!

Previously, using ImageMedium->derivatives would not work well in
combination with image filters or the other method of generating
srcset variants of images (by appending eg. "@2x" to their
filenames). This commit hopefully fixes that.
The biggest alternative will now become the base medium, and
alternatives will be filled out as necessary in a descending
manner.
Otherwise we get some funky results, with the possibility of two
different images being rendered between the full-width srcset
version and the original src version.
When using naming conventions to generate image alternatives, this
patch would previously generate a “@1x” alternative if one didn’t
exist. That’s no longer the case
When an image only has an alternative medium - ie. the only
version of the image ends in eg. "@3x", then we construct the
missing alternatives automatically. Previously, we would only do
this down till "@2x", meaning that the image that would have been
the base medium, had the image been manually resized, wasn't
created. This has now been fixed.
@fredrikekelund
Copy link
Contributor Author

@rhukster the issue you described in #1035 (comment) seems to have related to 07c9591. I fixed that by always appending an @1x alternative when an image lacks a base medium.

@rhukster
Copy link
Member

rhukster commented Oct 15, 2016

Ok, definitely working better now. However, I don't agree with one change you have made.

Before when using regular retina images, the default src= tag was set to use the smallest @1x variant. This serves as only a placeholder for the image, and shows when the browser doesn't support srcset.

In your PR branch, this now uses the largest image, eg @3x in my test case:

<img alt="Retina Image" src="/grav-retina/images/4/0/f/7/c/40f7c0f9186571844c2de98d74b27224d49c8238-sierra3x.jpeg" srcset="
/grav-retina/images/a/0/f/5/b/a0f5b2aacd42fd45534b87b74810d9baca2a3cea-sierra1x.jpeg 1706w, 
/grav-retina/images/2/9/2/3/8/29238d9b9ddd3a244804347a095345a0f7825a4e-sierra2x.jpeg 3413w, 
/grav-retina/images/4/0/f/7/c/40f7c0f9186571844c2de98d74b27224d49c8238-sierra3x.jpeg 5120w" sizes="100vw">

All the examples I have ever seen on src/srcset usages shows the src containing the smallest image available. That was our previous behavior, I think it should be the same going forward.

Of course, the same issues applies to derivatives, the largest image is being used in src there too.

@rhukster
Copy link
Member

Oh one other thing I noticed. Your PR does fix filters in derivatives which is great, but you MUST put the filter after the derivative. It would be nice if derivatives are always handled first (like automatic @2x/@1x generated images) then removed from the list of actions. This way you could put filters first if you wished, and it would still work.

@rhukster
Copy link
Member

rhukster commented Oct 15, 2016

One more potentially useful feature, perhaps you can use the setImagePrettyName() method to append the px size to the image name stored in cache.. currently they are all 2x for example, but it would make it easier to see which image is which (even though the size is after, it's still nice to see some difference in the filename).

@fredrikekelund
Copy link
Contributor Author

The reason that the biggest image is now the base medium (if one wasnt present to begin with) is to cover scenarios where the content provides just an alternative medium (eg an @2x), but the theme still calls the derivatives method. Misunderstanding a bit how the two different responsive image systems work in Grav, I ran into some confusion around this in my own project. But I guess a more logical way might be to maintain the smallest image as a base medium, and then if the theme calls derivatives, we could base the new image alternatives on the biggest existing image alternative. Might be a nice touch to only generate alternative sizes that don't already exist as well. What do you think?

I totally agree that it would be much more intuitive to be able to call derivatives and filters in any order, but that requires a bigger overhaul, where the most efficient solution might be to simply store all the methods called on the ImageMedium until url() is called. I'd be happy to look into that, but maybe it should be a separate PR.

Seems like the setImagePrettyName shouldn't be too big of a deal to implement either, I'll have a look at it.

@rhukster
Copy link
Member

I feel like derivatives is a much more advanced and less used feature than the simple @2x method. That is used by 'most' people looking for quick retina support.

So with that said, a change to add filters and other improvements support to the much less used derivatives functionality, should not change how @2x style retina works. It may not seem like a big change to you, but for older browsers without srcset support, a site could potentially be sending HUGE images to these clients. Another example is with site testing tools that don't check srcset, now are going to spit out errors about huge, un-optimized images..

I really don't see a benefit for this anyway. The whole point of the larger image is for retina support. If you want retina support, then your device is pretty new, and will very likely support srcset. If you browser supports srcset, then you will get the the appropriate image anyway. Why put the biggest image in src? makes absolutely no sense to me.

@fredrikekelund
Copy link
Contributor Author

Maybe you misunderstood my previous comment. What I proposed was to revert the behavior to use a scaled down image as the base medium when there's no base medium available on disk.

The reason for the current solution in here was to cover for situations where theme developers call derivatives on images that already have image alternatives. That wouldn't work previously, as the smallest image couldn't be scaled up, but the existing alternatives would still be removed.

So maybe a better solution (like I suggested above) would be to change the logic of the derivatives method, so that if an image already has registered alternatives, we generate new alternatives from the biggest existing one. And maybe, while we're at it, it would be convenient to change the derivatives method to only generate new alternatives that dont already exist, based on the images read from disk.

So basically - always make the smallest image the base medium, ie. the one rendered in src (the way things work currently), but also provide support for situations where image alternatives exist on disk, but derivatives is still called in the Twig template.

@rhukster
Copy link
Member

Ah ok, i misread (was early!), that sounds like a good solution. :)

@fredrikekelund
Copy link
Contributor Author

Cool :) Will try to churn out those changes as soon as possible

When an image lacks a base medium on disk (eg. the only existing
image is an @2x version), then we make a scaled down version the
base medium, which ensures that the smaller version is served up
in the src attribute in the HTML.

Also, don't reset the image alternatives when calling
ImageMedium#derivatives, instead only generate the image
alternatives that are missing.
@fredrikekelund
Copy link
Contributor Author

@rhukster I pushed some more changes that should hopefully deal with the things we discussed yesterday.

Something to be aware of is that the ImageMedium#alternatives array now uses image widths for keys, instead of image ratios. I changed this to make it easier to look up existing image alternatives' widths when generating new ones with ImageMedium#derivatives. There aren't any public API's for getting at the image alternatives directly, so I don't think this should break anything for users.

ImageMedium#setImagePrettyName is now also called ImageMedium#derivatives, but in the interest of consistency, I'm basing prettyname on the image ratio rather than width. This makes prettynames a bit funky when the ratio is a floating point number rather than an integer, though. Gregwar/Image removes any dots from the filename before saving it to disk, so example@1.5x.jpg becomes example15x.jpg. This is maybe not that big of a deal, but to the end user, prettynames like example@1500w (which would result in example1500w) might make more sense. I think that it might be a good idea to support the example@1500w.jpg naming convention the same way that example@2x.jpg is supported currently, so if that's something we're interested in, then maybe the width based prettynames is the best alternative for derivatives

@rhukster
Copy link
Member

derivatives test grav 2016-10-16 11-44-22

I see what you mean, without the period this makes it very tough to work out. I guess the smallest should be 1x and move on up to the largest size.. Alternatively, just use the 1500w idea (the @ gets removed too).

@fredrikekelund
Copy link
Contributor Author

Good solution! I implemented and pushed it. Anything else we need to address before this is ready to merge?

@rhukster
Copy link
Member

It's close but it's not quite right...

<img src="/grav-retina/images/6/f/2/5/b/6f25b266e0f68034c15fdb9859c64351ae30481c-yosemite1x.jpeg" srcset="
/grav-retina/images/f/d/2/6/1/fd261094f0bd2ef4fb87e56f3503ceacd4b00d10-yosemite2x.jpeg 2500w, 
/grav-retina/images/1/d/b/a/6/1dba6751d69378e1c0120c83531ea17b0aa5ec64-yosemite2x.jpeg 500w, 
/grav-retina/images/5/0/2/d/8/502d80ae9ce4f3c3d73250c1deb16eb2eb2aadcf-yosemite3x.jpeg 1000w, 
/grav-retina/images/c/b/c/9/d/cbc9dd0f6c991cf95433d62be15fbcf4bd7177ed-yosemite4x.jpeg 1500w, 
/grav-retina/images/b/b/0/a/3/bb0a346014d0b1c1f0ae1d299450518cdbeb8564-yosemite6x.jpeg 2000w, 
/grav-retina/images/6/f/2/5/b/6f25b266e0f68034c15fdb9859c64351ae30481c-yosemite1x.jpeg 1250w" sizes="100vw">

The x values don't quite line up with the widths? Does it work for you?

@fredrikekelund
Copy link
Contributor Author

@rhukster were all of those alternatives generated using derivatives, or did some of them exist on disk? I ran into an issue with name collisions if I used both methods, but we might have to live with that, as we don't want to force images to be processed by Gregwar/Image just to be able to set a prettyname. What do you think?

@rhukster
Copy link
Member

All generate from a single 2500px wide image called yosemite.jpg.

@fredrikekelund
Copy link
Contributor Author

fredrikekelund commented Oct 18, 2016

@rhukster I tested again, and I think it actually does work as expected for me. I'm using this Twig markup

{{ page.media['world.jpg'].derivatives(500, 3500, 500).html() }}

The source image is world.jpg, 2500px in width (I'm passing 3500 to derivatives to test that the max width caps properly). This is the resulting HTML for me (line breaks added for clarity):

<img
    src="/user/pages/02.test/world.jpg"
    srcset="/images/f/4/6/9/8/f4698c102cf59cf97b5227eacc60639cf9e6aa8a-world2x.jpeg 500w,
        /images/d/5/1/3/8/d51387c3e66cffe68c8b7c6f41e272372d65de82-world3x.jpeg 1000w,
        /images/8/b/2/4/9/8b2494ee697a9c66995d6a7f52b7374f3c913ef3-world4x.jpeg 1500w,
        /images/f/c/5/7/4/fc5749067f023c986b9cfc05a741b748a6285bcf-world6x.jpeg 2000w,
        /user/pages/02.test/world.jpg 2500w"
    sizes="100vw" />

It's strange that the primary source in your example directs to a cached image and contains 1x in the filename. Are we looking at a potential bug there, or is it possible you were looking at a cached page maybe?

@rhukster
Copy link
Member

Actually I did have it as yosemite@2x.jpg.. I changed to yosmite.jpg which is 2500px wide

![](yosemite.jpg?derivatives=500,2500,500&grayscale)

and get:

<img src="/grav-retina/images/7/6/6/6/e/7666e0abc53f5e8d95be9607246e1fcd63f473e8-yosemite.jpeg" srcset="
/grav-retina/images/2/f/f/2/d/2ff2d1f503b207eea8d967cf321cf22c7420fddb-yosemite2x.jpeg 500w, 
/grav-retina/images/e/5/6/f/e/e56fe33d6fd964aba5a272d37ea42b316eefc747-yosemite3x.jpeg 1000w, 
/grav-retina/images/b/8/6/d/a/b86dae05240eb0f9eacfb110866bfe7c26fbea28-yosemite4x.jpeg 1500w, 
/grav-retina/images/c/0/3/0/e/c030eea0a8d7b2a3996c7c66f851fc6ba241abaa-yosemite6x.jpeg 2000w, 
/grav-retina/images/7/6/6/6/e/7666e0abc53f5e8d95be9607246e1fcd63f473e8-yosemite.jpeg 2500w" sizes="100vw">

it's still not right though, first it probably makes more sense to just use the width in the derivatives rather than the x values, or even just a numeric value (1,2,3,4 etc) because the (1x, 2x, 3x, etc) are not correct. 2x is double, 3x is triple, and 4x is quadrupled.. however, derivatives are not doubled each time, they are just X pixels larger each time.

  1. the src attribute is using the 2500px wide -yosemite.jpeg file.
  2. the 500px wide image is labeled -yosemite2x.jpeg and it should be 1x or 1 or 500w, but not 2x.
  3. the 2000px wide image is labeled -yosemite6x.jpg, but really should be the 5th one, so5or2000w`
  4. last one that is 2500px is the original so that's fine having nothing appended.

@fredrikekelund
Copy link
Contributor Author

fredrikekelund commented Oct 20, 2016

If we're skipping the 2x syntax altogether (and I agree that it's a pretty logical choice), then we might as well go with the widths to at least get some meaning out of the numbers. I'll go ahead and make that change!

I realize that I probably was a bit unclear in my previous comment, but I didn't intend for the smallest alternative to be returned ImageMedium#url when using just the derivatives method of generating image alternatives. It makes for consistent behavior in one way, but the $reset argument to ImageMedium#url and ImageMedium#srcset makes things a bit trickier.

If we reset each alternative when calling ImageMedium#srcset, we lose the smallest alternative (which we want to be able to use later). And if we counter the reset parameter by maintaining the smallest alternative, we need to track a state where we've called derivatives first, then srcset, now waiting to call url and return a faux base image, which we'll probably remove when ImageMedium#url is called, which in turn would make the original image on disk the base image again, I suppose. Kinda confusing.

Maybe I'm missing a much simpler solution to this, but I'm not sure it'll be intuitive or elegant to write special logic that returns the smallest derivative when calling ImageMedium#url.

Btw, what's the purpose of resetting the image and its alternatives when calling ImageMedium#url and ImageMedium#srcset?

Instead of example2x.jpeg, we now have eg. example1280w.jpeg
@rhukster rhukster merged commit db4c9c1 into getgrav:develop Oct 23, 2016
@rhukster
Copy link
Member

It's been a long road but I think we have reached a good end result! Thanks for your hard work.

@fredrikekelund
Copy link
Contributor Author

Cool, thanks! :) Still a bit interested in the $reset argument, though - what's the reasoning behind having that there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants