silverstripe / .github

0 stars 2 forks source link

Create an advanced image rendering solution #230

Closed maxime-rainville closed 4 months ago

maxime-rainville commented 6 months ago

Overview

Silverstripe CMS doesn't have a native solution to managing more advanced image use cases such as displaying retina images or rendering images in multiple formats with a fallback.

Recent work has made it easier to convert images between formats (e.g. JPEG to WEBP). We also did some exploratory work following our image lazy loading work a few years ago.

There is an opportunity to explore what modern image rendering could look like in the CMS by providing an MVP solution.

User story

As a developer, I want a transparent way to adopt modern image practices so my end users can benefit from the latest browser standards without being left behind if they use older browsers.

Acceptance criteria

Possible follow up work

Original cards

Pull requests

maxime-rainville commented 6 months ago

Proposed approach

Configuration

The default PictureSourceList would be define in YML.

You'll be able to define your ImageSourceGenerator to handle specific use cases there and assign them to your PictureSourceList.

Potential problem with generating the retina image variant

Let's say you have an 100x100 image and you do a transformation like this:

$Image.ResizedImage(50,50).ResizedImage(100,100)

In this scenario, the image will be downscale to 50x50. Then the 50x50 will be upscale back to 100x100. Obviously this will lead to a degradation ... which would defeat the entire purpose of this exercise.

My suggestion for tackling would like something like this.

// This will return the orginal unmanipulated image
$original = $image->getFailover();
$variant = $image->getVariant();
$manipulations = $this->decodeVariants($variant);
foreach ($manipulations as $manipulation) {
   // This method would rerun a manipulation, but doubling the dimensions
   $original = $this->rerunManipulation($original, $manipulation);
}

$original = $orginal->ResizedImage($image->getWidth(), $image->getHeight())

Some of the image manipulations will change the aspect ratio of the picture, so you can't blindly take the dimension of $image, double them and do a straight resize on the original.

The "rerun the manipulation" approach should work in most cases ... although there will be scenario where you might get slightly different results e.g. FitMax or ScaleMax.

emteknetnz commented 6 months ago

Proposed classes/interfaces seem fine, though that's probably more implementation detail than is required here. That said it looks like you've made an assumption that only <picture> tags will be used and not <img srcset="..."> - though possibly that's a good thing forcing people to use <picture>?

The default PictureSourceList would be define in YML.

What would the PictureSourceList look like? Would this be a list of variants to create and % size they should be in relation to the original?

$Image.ResizedImage(50,50).ResizedImage(100,100)

I'm making the assumption here that the yml config would contain percentages smaller than the original image size. In that scneario you shouldn't get into scenario where you are resizing down then up. Presumbably that's a bad assumption by me? How would you get into this scenario?

GuySartorelli commented 6 months ago

I don't see the benefit of having so many new classes/interfaces... what does the ImageSource class do that an associative array or ArrayData doesn't do? What does PictureSourceList do that an array or ArrayList doesn't do? Why doesn't the extension just build the list/array of <source> tags based on yaml configuration (either using ImageSourceGenerator or frankly just directly with logic in the extension itself)?

This feels like a lot of abstraction that I feel isn't really necessary.

maxime-rainville commented 6 months ago

it looks like you've made an assumption that only tags will be used and not - though possibly that's a good thing forcing people to use ?

Yes, I made the assumption that we would be using <picture> and <source/>.

If we only cared about retina images, we could get away with only using srcset attribute. But being able to use multiple media format requires the <picture> tag.

It probably wouldn't be horribly complicated to allow people to use an <img> with a srcset attribute if the use case they care about can be met that way. But I would want to see if there's actual demand for it.

What would the PictureSourceList look like? Would this be a list of variants to create and % size they should be in relation to the original?

PictureSourceList would mostly be a place to register specific ImageSourceGenerator.

PictureSourceList will probably have something like generateSource method. That method will accept a DBFile object and will return the markup with all the <source> tag.

The registration could look something like that:

SilverStripe\Core\Injector\Injector:

  SilverStripe\ImageSrcSet\ImageSourceGenerator.WebpRetina:
    class: SilverStripe\ImageSrcSet\ImageSourceGenerator
    constructor:
      retina: 2x
      format: webp

  SilverStripe\ImageSrcSet\ImageSourceGenerator.OriginalRetina:
    class: SilverStripe\ImageSrcSet\ImageSourceGenerator
    constructor:
      retina: 2x

  SilverStripe\ImageSrcSet\ImageSourceGenerator.Webp:
    class: SilverStripe\ImageSrcSet\ImageSourceGenerator
    constructor:
      format: webp

  SilverStripe\ImageSrcSet\PictureSourceList:
    class: SilverStripe\ImageSrcSet\PictureSourceList
    constructor:
      generators:
        - '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.WebpRetina'
        - '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.OriginalRetina'
        - '$%SilverStripe\ImageSrcSet\ImageSourceGenerator.Webp'

The example above assumes that a generic ImageSourceGenerator will be able to do all the work with a few parameters. I may end up having more than one ImageSourceGenerator implementation to achieve specific use cases ... haven't decided yet.

$Image.ResizedImage(50,50).ResizedImage(100,100) I'm making the assumption here that the yml config would contain percentages smaller than the original image size.

That example was there to illustrate a potential problem if we just blindly resize whatever image we get from the WYSIWIG.

My hope is that developers using this module will only need to provide the generic image they want to use as the fallback. e.g. I want them to be able to write $MyImage.ResizedImage(50,50) in their template and get an image displayed on the page that is 50px by 50px. Whether the physical image is 100px by 100px should be transparent to them.

The actual image that will be provided to PictureSourceList will not be the original image ... it will be the ResizedImage(50,50).

In that scneario you shouldn't get into scenario where you are resizing down then up. Presumbably that's a bad assumption by me? How would you get into this scenario?

ImageSourceGenerator will have the possibility to return a falsy value. This will be used to communicate that, given the provided image, their would be no point outputting a source.

what does the ImageSource class do that an associative array or ArrayData doesn't do?

It would allow us to be explicit about the attribute that are valid on ImageSource and save us the trouble of constantly having to do empty check on attributes.

You could also add some convenience method on it or a forTemplate to make it easy to render the the <source> tag.

I think the small added complexity of having a dedicated class for this will be outweighed by reduce complexity elsewhere.

My gut feeling is that having a dedicated class for this would be more elegant. But it probably doesn't matter all that much in the grander scheme of things.

Why doesn't the extension just build the list/array of tags based on yaml configuration (either using ImageSourceGenerator or frankly just directly with logic in the extension itself)?

I'm writing this with the expectation that it will be merged back into core at some stage. I'm also trying to leave open the possibility that people could want to use this for other use cases

Given the context I'm writing this in, I'm admittedly trying to make the implementation a bit more sophisticated than it needs to be. If we feel the thing is over engineered down the track, we can look at pulling it back a bit to simplify the codebase.

kinglozzer commented 6 months ago

Just dropping in to mention that we had a need for this a while back, and I ended up building a module that sounds like it has quite a few similarities to the proposed class structure above: https://github.com/kinglozzer/silverstripe-picture.

I realise it achieves quite different goals to this ticket (it’s built so image sizes are stored in YAML, just calling $Image.ResizedImage(50, 50) wouldn’t do any picture magic), but I think the general approach for how it builds <source>/srcset could probably be built out to work for both use-cases.

If you want to take anything from the module then feel free, otherwise ignore me and sorry for the noise 🙂

maxime-rainville commented 6 months ago

@kinglozzer Thanks! It's always nice to have some prior art to compare. I'll have a look.

They are a few things we added in 5.2 and are planning to add in 5.3 that open new options.

We might be able to adapt your approach or part of it into our own solution.

maxime-rainville commented 6 months ago

Features

Architecture

I think the approach @kinglozzer is taking is broadly similar to how I was planning to do it.

Some of the differences

Generally speaking, I like @kinglozzer naming convention better, so I'll steal that as well.

GuySartorelli commented 6 months ago

I think a question worth asking at this stage is: Is it worth doing this as a new separate module, or could this functionality be suitably implemented as a PR to Loz's existing module? I don't have an opinion on that, just seems worth asking.

michalkleiner commented 6 months ago

Very good point @GuySartorelli. I do have an opinion on that :D

Adopting Loz's module and bringing it under silverstripe org, or just contributing to it perhaps as co-maintainers, could also alleviate the already voiced concerns of ss just taking ideas and doing their own module for nearly the same functionality vs contributing back to existing community modules (the most recent example being the linkfield module).

With the linkfield module I questioned why it was done in the first place, with this image module I see there's some potential to fill a gap in functionality around images in default CMS installs, so I would have less concern in this case, though knowing now there's a module that does nearly all of what we need, some of it better, I'd say we should make a really good argument on why not to contribute there vs doing nearly the same twice.

maxime-rainville commented 6 months ago

For context, this idea was prioritise a bit out of left field. I'm happy to go over the exact reason at our next core committer catch up.

To the extent that @kinglozzer would be happy to consider a PR to implement some of the extra feature in this issues' ACs, I'm happy to target a PR there.

kinglozzer commented 6 months ago

I think it’s worth clarifying what the ultimate goal of this work is - I’ve probably added a lot of scope creep by linking to my module 😅

  1. Automatically output high-res/webp/avif when users call things like $ResizedImage(123, 123) as part of core code
  2. Do the above, but opt-in with an optional module: e.g. enabling an extension, or calling .AutoFormat or something
  3. Offer a full suite of image output options, something like the YAML in my module
  4. Provide a base API for modules to hook into
  5. A combination of the some, or all, of the above

I think the initial AC was more like 1 or 2, which I think should live in the Silverstripe org/namespace. Basic image rendering is very core to a CMS, and most - if not all - sites will want auto-webp/avif etc, so I think it’d look a bit odd to recommend installing a third-party module (even if it’s officially supported) for that.

If we want to offer something like 3, it probably makes sense as a module (i.e. separate from silverstripe-assets). Regardless of whether mine is used as a starting point/inspiration, I imagine it will end up looking quite different anyway.

In terms of “ownership” of that theoretical module, my personal preference would be for it to live in the Silverstripe org/namespace - as Silverstripe would be doing a lot of build work, then taking on the maintenance burden, it’d feel weird for it to be all sitting under my nickname. I could either transfer the repo (we’ve done that before), or something new could just be built from scratch taking whatever ideas are useful (which might be the easier approach for prototyping ideas).

GuySartorelli commented 6 months ago

as Silverstripe would be doing a lot of build work, then taking on the maintenance burden

This issue is to implement optional functionality that won't sit inside the commercial support commitments. There's no expectation that Silverstripe will be doing any maintenance of it post-implementation at this stage, I think.

maxime-rainville commented 6 months ago

The idea of creating a new module was to have a proof-of-concept that people could choose to install. This is mostly meant to be a tool to allow us to iterate over quickly and try things.

But the long term intention is to merge this kind of functionality back into a core module, probably around CMS 6.

I'm making a few minimal change to CMS 5.3 to make it easier for that module to exists: https://github.com/silverstripe/silverstripe-assets/pull/596

I'm somewhat indifferent to whether the proof-of-concept part is in kinglozzer/silverstripe-picture or under a silverstripe repo. If we do merge that functionality back into core, then I would expect the POC repo to be short live.

If no one object, I'll start working of kinglozzer/silverstripe-picture for now ... if nothing else, it will save me the trouble of creating a bunch of boilerplate and agonising over class names. Once I'm a bit further along, we can judge whether it makes more sense for it to be merge back into kinglozzer/silverstripe-picture or if it should become its own thing.

michalkleiner commented 6 months ago

as Silverstripe would be doing a lot of build work, then taking on the maintenance burden

This issue is to implement optional functionality that won't sit inside the commercial support commitments. There's no expectation that Silverstripe will be doing any maintenance of it post-implementation at this stage, I think.

Why does the CMS squad even take this on then?

maxime-rainville commented 6 months ago

Some of the recent work like image lazy loading and allowing variants with different extension and image converters have unlock some cool new possibilities.

But we're not quite sure how best to seize those opportunities. So we want to create a "proof-of-concept" module that people can try out on existing or new projects.

This will allow to validate concerns like:

That's where we can to have a proof-of-concept module to validate how these concern will behave in the real world.

Once that gets sorted, we'll probably take the lessons we've learn and will probably merge this functionality back into core with some migration path for people who were using the proof-of-concept module.

We had a SPIKE internally about this 2-3 years ago. For some reason I can't recall, we kept this in-house instead of discussing it publicly.

That's the discussion we had back then. SPIKE Retina image support & picture tag · Issue #449 · silverstripeltd_product-issues.pdf

maxime-rainville commented 6 months ago

Got a draft PR. It meets most of the ACs but still need to be cleaned up a bit.

https://github.com/kinglozzer/silverstripe-picture/pull/1

emteknetnz commented 4 months ago

Linked PRs have been merged, except for the one to silverstripe-picture though that PR should be dealt with seperately