Thumbnail image size improvements

[purplespider]

Issue: #1228

This PR corrects thumbnail image sizes to ensure they are crisp and not blurry.

Thumbnails are typically displayed at a consistent size, yet, ThumbnailGenerator currently uses the FitMax resize method, which results in differently sized images. These are then currently resized and cropped in CSS (using background-size: cover) to make them display at a consistent size. However, due to FitMax this results in nearly all thumbnails being upscaled and thus displaying blurrier than they should. Very wide or tall images are most affected by this, especially in UploadField thumbnails.

This PR changes the default ThumbnailGenerator resize method from FitMax to Fill so that the resulting image is exactly the desired size, and no upscaling in CSS is necessary.

NOTE: After this change, it is necessary to run ./vendor/bin/sake dev/tasks/MigrateFileTask generate-cms-thumbnails to re-create the CMS thumbnails, otherwise NO thumbnails will display in the "Files" area. Open to suggestions for making this an optional change for existing sites.

The default thumbnail dimensions for UploadField are also doubled from 60 to 120 to ensure UploadField thumbnails remain crisp on HiDPI displays.

[emteknetnz]

After this change, it is necessary to run ./vendor/bin/sake dev/tasks/MigrateFileTask generate-cms-thumbnails

Yeah that's a bit of a deal breaker right now, as is seems like we'll break all the sites :-) We're not going to do that just to make thumbnails a little sharper

HiDPI

What type of device are you using so that you noticed the blurry thumbnails? Are you running the CMS on an ipad?

[purplespider]

Are you running the CMS on an ipad?

Nope, I primarily use an iMac & MacBook (all Mac desktops and laptops have HiDPI displays and have had for several years now. I believe they’re also common in PC laptops).

However, the issue with many UploadField thumbnails looking crappy is apparent on all displays, not just HiDPI ones. (I have a 2nd LoDPI display too).

Example

Take this image:

If you add it to an UploadField the thumbnail is barely legible on any display (try it yourself):

If you change the ThumbnailGenerator $method from FitMax to Fill it now displays fine for LoDPI displays (but still less than ideal on HiDPI displays):

Changing UploadField $thumbnail_width and $thumbnail_height from 60 to 120 makes it display perfectly on both:

Why does this happen?

The main issue here is that using FitMax for CMS thumbnails was clearly an error (or leftover from a legacy implementation), as this is what it currently ends up doing:

It applies FitMax(60,60) to the (above) image resulting in an image that has width 60 px and height 22 px:

But UploadField's thumbnails are displayed at 60 height and 60 width, so it has to stretch the 22 px high image to a height of 60 px:

CSS then crops off the edges (using background-size: cover) and this is why you end up with a pixelated thumbnail:

What should it be doing?

It should be using Fill(60,60) instead of FitMax(60,60), as this results in:

Which is the perfect size thumb for UploadField thumbnails on a LoDPI display (and if we change the width/height to 120, it's perfect for Lo and HiDPI displays).

Going Forward

So hopefully you'll agree that this does need to be fixed at some point, and that it is more serious than just making images a bit sharper on HiDPI displays. If Silverstripe v5 was on the horizon, then it could make sense to just include these breaking changes in it, but I don't believe there is a plan for a v5 anytime soon.

I agree we should avoid breaking existing sites on update, so I see a few options:

1. We make this an opt-in change for existing sites. I'd need help with the best way to do this. I should add that it "only" breaks thumbnails in the Files tab. UploadField thumbnails update automatically without needing to run the migration script.
2. We find a way to stop it from breaking old thumbnails, and only apply it to newly generated thumbnails (unless the migrate task is run).
3. We make UploadField request that its thumbnails are generated using Fill but keep others using FitMax, so that existing thumbnails in Files don't break.

Using Config

While I feel it's serious enough to need to be addressed permanently, I am aware that it is possible to make the changes required in config:

SilverStripe\AssetAdmin\Model\ThumbnailGenerator:
  method: Fill

SilverStripe\AssetAdmin\Forms\UploadField:
  thumbnail_width: 120
  thumbnail_height: 120

So maybe there is a way this config could automatically apply to new sites, and be opt-in for existing sites?

[emteknetnz]

Thanks for this detailed explanation with screenshots, this is super helpful

I've tried uploading the image on a low/regular dpi screen (24 inch 1920x1080)

Compare next to the one added above on the iMac HiDPI screen, as rendered on my screen

Compared next to the source image from the iMac HiDPI screen

Thumbnail in the files section renders fine

Just to reconfirm that changing the yml for ThumbnailGenerator to Fit will cause this to happen in the Files section

Though the UploadField thumbnail will auto-regenerate, though the old, unused thumbnail jpg will remain on the server wasting a bit of disk space

[emteknetnz]

@purplespider I've created a parent issue for this here and which also links another issue for retina image support

[maxime-rainville]

Yeah ... I agree with Steve that requiring everyone to run a migration task is a bit overkill given the severity of this issue.

I guess we could ship an updated config in the installer, so people starting new project get the better settings when starting a new project

[GuySartorelli]

I've rebased and retargetted to 1.13 - I'm looking to see if there's an easy way to get asset admin to re-create thumbnails if the thumbnails it wants are missing (the same way everywhere else including upload fields will).

The same problem with needing to update all thumbnails (currently by saving the files, or by running a migration task as discussed above) also plagues the scenario where developers choose to change this same configuration in their project, so definitely is a bug we should look to fix if feasible.

Edit: Looks like the graphql request for files was explicitly not generating missing thumbnails... which seems supremely silly so I've switch that. We only load a limited number of items per page and in most scenarios those will already have their thumbnails generated ahead of time (on initial image save).

I don't think there's any risk of a noticable performance degradation by making this change, and it lets us have these nice non-blurry thumbnails.

[GuySartorelli]

And updated unit tests to match the new file names.

[michalkleiner]

It's not just the UI that can run the gql requests, right? It would be limited to logged-in users, but could still be a DDoS/performance degradation vector if queried without the limits?
Saying that — I think there was a reason why it was explicitly turned off for these requests.

[michalkleiner]

I'd be probably ok with this in a minor and mention in a changelog.

[GuySartorelli]

It's not just the UI that can run the gql requests, right? It would be limited to logged-in users, but could still be a DDoS/performance degradation vector if queried without the limits?

Let me take a look into that - that's a good point.

[GuySartorelli]

I've added a new commit which aims to limit the scope of which files we're allowed to generate thumbnails for - each GraphQL query should now only be allowed to generate thumbnails for up to 100 files, which is 2x the current asset admin page limit.
That number is fairly arbitrary so I'm quite happy to set it to use the asset admin page limit - but for now I'm just doing this to check:
@michalkleiner Do you think this new change sufficiently mitigates the potential attack vector?

I've also retargetted and rebased for the 2 branch since this now adds API and therefore has to be released in a minor.

[michalkleiner]

Looks ok to me but I'd prefer if there was another pair of eyes over this.

[GuySartorelli]

Sweet. I've updated to use the AssetAdmin.page_length config and fixed tests - I'll get someone else in the team to give this a review.

[sabina-talipova]

Looks good, just small question.

[sabina-talipova]

Just tiny question, should we have $origGenerates declaration in if block? This question just in case, if in the future we will have additional logic in this method.

[sabina-talipova]

Upload field:

Image gallery (List view):

I'm not entirely sure that this is important or that anyone will notice it, but there is a slight difference between how the preview is displayed when viewing in list mode in the "Files" section. I personally like PR version a lot, but what about the user’s expectations.
And one more small clarification. I have no problems displaying previously generated thumbnails with new solution. Should we still make changes to the documentation regarding migration from one version to another? And also indicate that the default method for generating thumbnails has now changed from FitMax to Fill, as well as their minimum size.

[GuySartorelli]

I'm not entirely sure that this is important

Good spotting! That is important. I hadn't noticed it in my testing but yeah we don't want to give false representations of what images look like.

I'll take a gander at what we can do about that.

[GuySartorelli]

That's made me realise this change would affect anyone who's using ThumbnailGenerator to generate their own front-end thumbnails as well.

I think what we really need is a way to declare different thumbnails depending on the context - so we would use Fill or FillMax for the gallery view and upload field, but use FitMax for the list view and by default for any other usage (e.g. on the frontend).

This PR isn't doing that, and I think that would be a lot of work.
I'm going to close this PR for now.

@purplespider if you'd like to persue this further lets continue discussion in the issue, get a way forward we're all happy with, and then open a new PR from there.