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

ENH Add ability to render any image using a default picture template and automatically render retina and webp version #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maxime-rainville
Copy link

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

@maxime-rainville
Copy link
Author

Let's say you have this in your template:

$Logo.ScaleWidth(300).CropHeight(150)

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>

Copy link
Author

@maxime-rainville maxime-rainville left a 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 ` tag for rendering their images. There's also some method signature changes here and there.

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:
Copy link
Author

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'}
Copy link
Author

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(
Copy link
Author

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.

Copy link
Author

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;
Copy link
Author

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)) {
Copy link
Author

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(),
Copy link
Author

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)) {
Copy link
Author

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"
Copy link
Author

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
@maxime-rainville maxime-rainville force-pushed the pulls/master/retina-andconvert-support branch from 3acc78e to 355c005 Compare May 14, 2024 10:16
@maxime-rainville maxime-rainville marked this pull request as ready for review May 14, 2024 10:16
@emteknetnz
Copy link

emteknetnz commented May 14, 2024

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:

  1. Automatically create retina images
  2. Automatically convert images to webp
  3. Doing both of these things by default

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 - {method: 'Convert', arguments: ['webp']}

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

@maxime-rainville
Copy link
Author

The current module has a few important limitations which this PR is trying to remove.

Can only work with Image

When you do something like $image->ResizedImage($x, $y), the end results will not be an instance of Image. It will be a DBFile. Because most of the methods in this module are strongly typed to expect an Image, you wouldn't be able to call one of those Picture pre-set on an image that was already manipulated.

Creating Retina images

Our image manipulation methods, for the most part, only deal in absolutes. e.g.: I want this image to be 300 pixel by 200 pixels.

There's no manipulation that allows you to say "I want this image to be 2 times bigger than its normal size".

If you knew upfront what dimensions your image was going to be, I guess you could hard code all the retina dimensions in your pre-set, but that would require a lot of manual work to create all those pre-sets.

Sources require a media

The current YML syntax expect a media key for each source. This would normally contain some media query syntax to control if the source image should be displayed or not. e.g. You could say something like "if you screen is in portrait mod, use this image" or "if you're printing the page, use this other image".

Retina image don't care about the media attribute. They use the srcset="/my-image.jpg, /my-retina.jpg 2x" format. So you would need to create some non-sense media key for each source tag you want to generate.

Not degrading images

Even after solving the first three problems, it's not as simple as "let's create an image that's twice the size".

Let's say you start with this $Image.ResizedImage(300, 200). If you call ResizeImage(600,400) on that image to create a retina variant, that variant will be create from the already resized 300x200 picture, not the original.

That's why the new Retina method on ImageExtension has this weird recursive logic. It's going back all the way to the original image and replaying all the previous manipulations doubling the sizes so the final image is nice and crisp.

Making usage transparent to the end-user/end-developer

Right now, developers have to make an explicit choice to use the pre-sets. There's no way to say "use this default pre-set everywhere". e.g. $Logo will just render a regular image with the current status quo.

So you would need to update every single instance where you render an image in your template. And you wouldn't be able to do anything about WYSIWYG images.

@emteknetnz
Copy link

emteknetnz commented May 15, 2024

I want this image to be 2 times bigger than its normal size

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

Right now, developers have to make an explicit choice to use the pre-sets

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 composer update would be very unexpected

@maxime-rainville
Copy link
Author

Retina image

If you look at the the replayManipulations method on ImageExtension, it receives:

  • the current Image/DBFile we are working on
  • the factor we should be multiplying by
  • the target dimensions for the image (the size the image will be rendered at)

replayManipulations is recursive. It will call itself until it reaches the original Image object.

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, replayManipulations validates that the current image is big enough to not cause upscaling. If possible up-scaling is detected, it returns false. The Source class has been updated to handle this instance and remove that version from the srcset.

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 tag

That'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.

@emteknetnz
Copy link

emteknetnz commented May 15, 2024

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?

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.

None yet

2 participants