ImageMedium#derivatives now works with image filters #1035

fredrikekelund · 2016-09-12T08:44:50Z

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!

@2x

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.

fredrikekelund · 2016-09-12T08:46:03Z

system/src/Grav/Common/Page/Medium/MediumFactory.php

@@ -119,8 +119,8 @@ public static function scaledFromMedium($medium, $from, $to)
        }

        $ratio = $to / $from;
-        $width = (int) ($medium->get('width') * $ratio);
-        $height = (int) ($medium->get('height') * $ratio);
+        $width = $medium->get('width') * $ratio;


This isn't directly related to the original intention with this PR, but I've previously found that downscaled images sometimes got a white stripe at the far right edge. Not rounding the pixel values sent to Gregwar/Image fixed that

rhukster · 2016-09-12T12:46:37Z

Wow this is the nicest documented PR I've ever received. Love it! Will review and test.

fredrikekelund · 2016-09-12T12:48:54Z

Thanks! :) The changes so far did fix the fact that general image operations weren't applied to derivatives, but I found that I stumbled a bit with the "@2x" filename plus the derivatives method, it's not working quite the way I wanted it to just yet. I'm working on it now, so will hopefully push another commit pretty soon!

The biggest alternative will now become the base medium, and alternatives will be filled out as necessary in a descending manner.

fredrikekelund · 2016-09-12T13:26:41Z

@rhukster alright, I've pushed another commit. Hopefully that should do it!

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.

rhukster · 2016-09-18T16:29:26Z

Sorry got busy this past week.. will check this out soon.

fredrikekelund · 2016-09-20T19:38:49Z

system/src/Grav/Common/Page/Medium/ImageMedium.php

+            }
+
+            $derivative = MediumFactory::fromFile($this->get('filepath'));
+            $derivative_width = $derivative->get('width') * $ratio;


I've been using this fork for my own project, and I've run across an error on this row a few times. MediumFactory::fromFile seems to have returned null, presumably because $this->get('filepath') in turn returned null. Both times this happened was after I had removed content, and the problem went away by restarting the php-fpm service on the server, so I suspect Grav has somehow cached the content associated with a page and then runs into an error.

Does this seem like a separate issue, or do we need to check to ensure the filepath isn't null or something here?

There is a check in the first line of that Medium::fromFile() method that checks the file exists, it returns null if not, so probably at least need to check for null before using $derivative.

Hmm, I see. How come a medium can be constructed in the first place if the corresponding file doesn't exist? I mean, if MediumFactory calls file_exists, how come the Media class that assembles page media doesn't?

Let me know if you want me to add a null check here or if it'd better to make changes further up the hierarchy, @rhukster!

Some more details on the issue I've been experiencing here. I'm debugging this right now, but for reference, or if anyone has a good idea of what might cause this:

I get an issue here after having synced new content to the server with rsync from my OS X El Capitan desktop to the Ubuntu 16.04 server. It looks like some of the files that rsync synchronizes are turned from having some uppercase letters in the filename to all lowercase. For some reason, Grav doesn't pick up on this, and still believes that the filenames are the same as they used to be - ie. they contain some uppercase letters. So Grav/PHP is partly right - the file does exist, just not with that exact filename.

Alright, getting through this was a real ordeal, but it looks like I figured out what the issue was.

At first I thought it was PHP that was returning errant results when listing the contents of a directory, but it turns out that it was Grav's page object caching mechanism that didn't pick up on updates to just the media files of a page object. I described this issue in #1080 as well.

Media::__construct (which initializes a page's media), is allowed to continue even if MediumFactory::fromFile returns null. This patch doesn't take that risk into account, and IMO, it shouldn't either. If the original ImageMedium was initiated, it should reliably point to an actual file.

Would be awesome if we could work through this final issue and get this PR merged, @rhukster!

If the cache is working as intended and it's a conscious convention to return null when the image can't be found, then we should probably check if the method call returned null. But in that case maybe we can have a discussion about how the caching mechanisms can be clarified in the docs (but that's better done in #1080).

…derivatives

fredrikekelund · 2016-10-01T09:49:16Z

@rhukster I've pushed another commit that accounts for the risk of the image derivatives being generated from a cached ImageMedium instance and the original file no longer existing. I've also verified that this works (it results in an "error" image in the browser). This should hopefully be good for a merge now!

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

fredrikekelund · 2016-10-05T12:42:11Z

One more fix! I've been testing out this patch a lot recently, and it's worked well for me. But I realized that with pre-generated responsive image alternatives, it would auto-generate an "@1x" alternative if such a file wasn't found. I've fixed that now so that the first image alternative it looks for is "@2x"

rhukster · 2016-10-14T22:10:11Z

So I was able to test this PR out and it there's something weird with the old 'alternative' style @2x, @3x syntax. For example, my test in Develop branch with a @3x image yields:

<img alt="Retina Image" src="/grav-retina/images/3/f/3/6/7/3f367fd0562b9ec5fc670ef8d9dce6e09bdc1d54-48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra3x.jpeg" srcset="
/grav-retina/images/3/f/3/6/7/3f367fd0562b9ec5fc670ef8d9dce6e09bdc1d54-48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra3x.jpeg 1706w, 
/grav-retina/images/2/a/a/c/b/2aacb1f0128f13fec18ecc7105f09c2ffd83bb85-sierra3x.jpeg 5120w, 
/grav-retina/images/4/a/2/8/8/4a2886595fb56fcfbd6a624ea6e471ccd3891e54-05784e5c11e5ea1dc0770b0b75b520bd48cc9375-sierra3x.jpeg 3413w" sizes="100vw">

Your PR branch for the same page yields:

<img alt="Retina Image" src="/grav-retina/images/4/0/f/7/c/40f7c0f9186571844c2de98d74b27224d49c8238-sierra3x.jpeg" srcset="
/grav-retina/images/4/a/5/9/2/4a592f4eb9d4bf02c219976b4931579bac6b4681-d0c20a48dda5aa67a90fd53fd4b91140b34dd568-sierra3x.jpeg 3413w, 
/grav-retina/images/4/0/f/7/c/40f7c0f9186571844c2de98d74b27224d49c8238-sierra3x.jpeg 5120w" sizes="100vw">

I added some line breaks for clarity. But before it uses the smallest image for the src, but serves up the appropriate of 3 generated images in the srcset.

In yours, it serves up the original 'largest' image in the src and only 2 generated images in the srcset.

rhukster · 2016-10-14T22:55:34Z

BTW you might want to merge in this commit: 31e358c

It adds the correct 'prettyname' to the generated alternative images.

<img alt="Retina Image" src="/grav-retina/images/d/2/5/2/9/d2529b606ad3a797704c18099c574995d72b7eda-48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra1x.jpeg" srcset="
/grav-retina/images/d/2/5/2/9/d2529b606ad3a797704c18099c574995d72b7eda-48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra1x.jpeg 1706w, 
/grav-retina/images/2/a/a/c/b/2aacb1f0128f13fec18ecc7105f09c2ffd83bb85-sierra3x.jpeg 5120w, 
/grav-retina/images/5/d/f/f/6/5dff6396db4b21d969ede476ad209de8c876ebdc-05784e5c11e5ea1dc0770b0b75b520bd48cc9375-sierra2x.jpeg 3413w" sizes="100vw">

This is the same example as above.

rhukster · 2016-10-14T23:55:30Z

Btw another commit (92401de) fixes the issue with the increasing hashing when you append operations..results look better now:

<img alt="Retina Image" src="/grav-retina/images/4/8/a/4/3/48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra1x.jpeg" srcset="
/grav-retina/images/4/8/a/4/3/48a4355cc289c3a5f2520c1cf804c9e0b4e50078-sierra1x.jpeg 1706w, 
/grav-retina/images/f/1/2/b/8/f12b8d3f8d049dc7ada651fd98fe22848e82361f-sierra3x.jpeg 5120w, 
/grav-retina/images/0/5/7/8/4/05784e5c11e5ea1dc0770b0b75b520bd48cc9375-sierra2x.jpeg 3413w" sizes="100vw">

fredrikekelund · 2016-10-15T09:41:55Z

I knew it was going to bite me sooner or later that I forgot to do this in a feature branch... I've pushed a fix for the problem you described as well as merged your changes from develop, but I did it in a separate branch, so I'll go ahead and open a new PR!

fredrikekelund reviewed Sep 12, 2016
View reviewed changes

Modified initialization of image alternatives

c2e6292

The biggest alternative will now become the base medium, and alternatives will be filled out as necessary in a descending manner.

Fully reset image when derivatives method is called

4d19620

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.

fredrikekelund force-pushed the develop branch from 616ccf6 to 4d19620 Compare September 12, 2016 13:48

fredrikekelund mentioned this pull request Sep 13, 2016

CropZoom and responsive image #1033

Closed

fredrikekelund commented Sep 20, 2016

View reviewed changes

Account for risk of original file not existing when generating image …

7efd55d

…derivatives

Fixed an issue where too many alternatives would be generated

07c9591

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

rhukster closed this Oct 14, 2016

rhukster reopened this Oct 14, 2016

fredrikekelund closed this Oct 15, 2016

fredrikekelund mentioned this pull request Oct 15, 2016

ImageMedium#derivatives now works with image filters #1107

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImageMedium#derivatives now works with image filters #1035

ImageMedium#derivatives now works with image filters #1035

fredrikekelund commented Sep 12, 2016

fredrikekelund Sep 12, 2016 •

edited

Loading

rhukster commented Sep 12, 2016

fredrikekelund commented Sep 12, 2016

fredrikekelund commented Sep 12, 2016

rhukster commented Sep 18, 2016

fredrikekelund Sep 20, 2016

rhukster Sep 20, 2016 •

edited

Loading

fredrikekelund Sep 20, 2016

fredrikekelund Sep 21, 2016

fredrikekelund Sep 28, 2016

fredrikekelund Sep 29, 2016

fredrikekelund Sep 29, 2016

fredrikekelund commented Oct 1, 2016

fredrikekelund commented Oct 5, 2016

rhukster commented Oct 14, 2016

rhukster commented Oct 14, 2016

rhukster commented Oct 14, 2016

fredrikekelund commented Oct 15, 2016

ImageMedium#derivatives now works with image filters #1035

ImageMedium#derivatives now works with image filters #1035

Conversation

fredrikekelund commented Sep 12, 2016

fredrikekelund Sep 12, 2016 • edited Loading

Choose a reason for hiding this comment

rhukster commented Sep 12, 2016

fredrikekelund commented Sep 12, 2016

fredrikekelund commented Sep 12, 2016

rhukster commented Sep 18, 2016

fredrikekelund Sep 20, 2016

Choose a reason for hiding this comment

rhukster Sep 20, 2016 • edited Loading

Choose a reason for hiding this comment

fredrikekelund Sep 20, 2016

Choose a reason for hiding this comment

fredrikekelund Sep 21, 2016

Choose a reason for hiding this comment

fredrikekelund Sep 28, 2016

Choose a reason for hiding this comment

fredrikekelund Sep 29, 2016

Choose a reason for hiding this comment

fredrikekelund Sep 29, 2016

Choose a reason for hiding this comment

fredrikekelund commented Oct 1, 2016

fredrikekelund commented Oct 5, 2016

rhukster commented Oct 14, 2016

rhukster commented Oct 14, 2016

rhukster commented Oct 14, 2016

fredrikekelund commented Oct 15, 2016

fredrikekelund Sep 12, 2016 •

edited

Loading

rhukster Sep 20, 2016 •

edited

Loading