-
Notifications
You must be signed in to change notification settings - Fork 2
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
ENH Add ability to render any image using a default picture template and automatically render retina and webp version #1
base: master
Are you sure you want to change the base?
Conversation
Let's say you have this in your template:
This will be rendered with something like <picture>
<source
srcset="/assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0_ExtRewriteWyJqcGVnIiwid2VicCJd.webp 1x, /assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0_ExtRewriteWyJqcGVnIiwid2VicCJd.webp 2x, /assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0_ExtRewriteWyJqcGVnIiwid2VicCJd.webp 3x"
type="image/webp">
<img
alt="clippy"
src="/assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0.jpeg"
srcset="/assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0.jpeg, /assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0.jpeg 2x, /assets/clippy__ScaleWidthWzMwMF0_CropHeightWzE1MF0.jpeg 3x"
loading="lazy"
width="300"
height="150"
class="img-responsive">
</picture> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Breaking things
This approach makes some breaking changes to the module. Maybe @kinglozzer is happy shipping a brand new major with some breaking changes?
The biggest one is obviously that all images will suddenly be using `
If we didn't want to merge all my changes directly in this module, we could probably ship the default image rendering and my retina changes in a separate module.
We would still need to loosen some assumptions in this module and tweak some method signatures.
How far I got
Images that would be degraded will not be rendered.
Some of the srcset
are showing pointless entries. e.g. If you use a WEBP, then you'll get an entry <source>
tag with a srcset
identical to the <img>
tag.
That's not a big downside. You'll just ship a bit of pointless HTML.
Right now, the fallback image will always render the original image. If someone decided to enable AVIF support, that might not be suitable. That can be customised if need be.
CI
I didn't write any unit tests. I would be keen to write some ... at least for the part I just added. But I would want to have some confirmation about whether we merge this in the current module or in a separate module first.
@@ -1,6 +1,35 @@ | |||
--- | |||
Name: silverstripe-picture | |||
--- | |||
SilverStripe\Assets\Image: | |||
SilverStripe\Assets\Storage\DBFile: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By adding this to DBFile instead of Image it will allow us to chain it to manipulation.
styles: | ||
DefaultRender: | ||
default: | ||
- {method: 'noop'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added the ability to do a NOOP manipulation that just returns you the image as is.
* @param $targetWidth The width of the original image | ||
* @param $targetHeight The height of the original image | ||
*/ | ||
private function replayManipulations( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will received an Image or DBFile and recursively redo all variants calls on it doubling the size.
So far this is working pretty well with what I have tested. When a manipulation is performed, the getFailover
method will allow you to retrieve the parent image on which the manipulation was performs.
I think it's possible you could end up with an image in a different aspect ratio if you were using some of the more esoteric manipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's figuring out what operation to replay by parsing the variant string for the File. I'm not too happy with this. There's probably a better way to do this, but it could require some none trivial change to core.
@@ -17,14 +18,14 @@ class Img extends ViewableData | |||
/** | |||
* The default image for this <picture> element, to be rendered as a nested <img /> tag | |||
*/ | |||
protected Image $defaultImage; | |||
protected Image|DBFile $defaultImage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everywhere we strongly type for Image
, I updated to use Image|DBFile
.
Maybe this should be AssetContainer instead? But that would also allow Upload.
Maybe we need to add explicit check that we have an image everywhere as well to make sure people don't call these methods with plain File.
throw new InvalidArgumentException("Invalid source config for style “{$this->style}”"); | ||
} | ||
|
||
if (is_numeric($media)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base implementation was using the source's media attribute as the config key.
This doesn't work for our use case because all our sources won't be using the media attribute. I had to tweak this bit here so you could use a plain array instead.
I contemplated adding some logic here so people could use Injector to load their own custom Source object here. If someone wanted to create some crazy complex scenario, that might be something worth exploring.
@@ -38,8 +39,6 @@ public function forTemplate(): string | |||
'media' => $this->media, | |||
'srcset' => $this->getImageCandidatesString(), | |||
'type' => $lastImage->getMimeType(), | |||
'width' => $firstImage->getWidth(), | |||
'height' => $firstImage->getHeight(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove this because the Retina image would try to put it's own dimension in there instead of using it's native size. Out of the box, the dimensions of the fallback <img>
will be used.
This may break other use case, I'm not sure.
]; | ||
// If we have a valid image, add it to the candidates | ||
// If at any point we failed to generate an image, we'll just skip this candidate | ||
if (!empty($image)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's some scenarios where we won't be returning an image and they shouldn't be added to the source set.
"silverstripe/framework": "^5" | ||
"php": "^8.1", | ||
"silverstripe/assets": "^2.3", | ||
"silverstripe/framework": "^5.3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the APIs needed for this PR won't be available until 5.3.
…and automatically render retina and webp version
3acc78e
to
355c005
Compare
I haven't looked too deep into the code changes, though I'm not quite sure what this PR does? Seems like there are a few things that you are trying to achieve:
I think this module can already do both 1 and 2? I'm not expert, though as I understand it creating a "retina image" is basically just a matter of uploading a high res image to start with? From there you can create smaller variants, though you cannot go in the other direction and upsize image because the upscaled images will look pixelated Looking at the example config this module can already handle creating the variants? I think converting to webp images can also already be done via the config this module provided that CMS 5.3 is installed via For 3 - It seems that you want to change this module to do both of these things by default, which IMO is simply not a good thing in the context of this module. If I was using this module then I wouldn't want these defaults specified, I'd want to configure everything myself so that I don't end up with a bunch of unwanted image variants |
The current module has a few important limitations which this PR is trying to remove. Can only work with
|
I wasn't able to fully work this out from looking at the code changes, though it looks like using the new 'Retina' method with x2 will upscale the original image? That's not something people should be doing? Creating an upscaled image variant will just result in a pixellated image variant. The upscaled variant will probably have a slightly larger fize size. As mentioned earlier you need to start with the largest image size and then downscale. If you really need an image sized bigger that the original you may as well just use the original and let the browser upscale to whatever width/height attributes are specified so you don't waste disk space. If I've got this wrong then let me know what actually happens
It seems like these code changes make the use of picture tags with many variants a default? If that's something that happens it should definitely be an opt-in, rather than a default. Existing users of this module will be using picture tags for specific things. Suddenly having every image on the site turn into a picture tag with variants when they run |
Retina imageIf you look at the the
From there, it will parse the variant to figure what manipulation was applied to the image. It will "replay" that manipulation, but multiply the arguments by the provided factor. On every recursion, I've tested this locally and the retina pictures appear sharp ... and they are omitted when the original image is too small. Having a default picture tagThat's a conversation we need to have with @kinglozzer. I agree this would be a breaking change. Maybe he thinks this is valuable enough to warrant a major release. Maybe we split the bits that would be breaking into a separate module. Maybe we just remove the default Retina Picture setup. We would just tell people to install this module and give them the matching YML snippet to include in their project. |
OK, so to clarify the retina image, ignoring the webp conversion bit, it's not actually upscaling it, so what it's doing is just creating some smaller variants, which this module can already do by just stacking some manipulations? It seems like the main intention of this PR is really a way to convert all the images to retina images with fallbacks with minimal input from the developer - is that a correct assumption? |
WIP
This is design to define a default Picture format and will update the core rendering logic so all images come with WEBP and retina versions.
It's also smart enough that if you resize a picture, it will try to create the matching retina version of your resize picture.
This will work for both images referenced in a template and images added to a WYSIWYG.
Parent issue
Depends on