Adding SourceWidth and SourceHeight to ImageUrlGenerationOptions

[Jeavon]

In order to be able to convert the coordinates provided by a pre-defined crop for various image processing services the source image width and source image height are required. Currently, these values are not available to a ImageUrlGenerator so in this draft PR I'm adding them to ImageUrlGenerationOptions.

For example, this then makes it possible to convert the crop coordinates to use Cloudflare's trim command I need to do the following:

  if (options.Crop is not null && options.SourceWidth is not null && options.SourceHeight is not null)
  {
      var top = Math.Round(options.Crop.Top * (decimal)options.SourceHeight);
      var left = Math.Round(options.Crop.Left * (decimal)options.SourceWidth);
      var bottom = Math.Round(options.Crop.Bottom * (decimal)options.SourceHeight);
      var right = Math.Round(options.Crop.Right * (decimal)options.SourceWidth);

      cfCommands.Add("trim",string.Join(';',top,left,bottom,right));
  }

I've also looked at Cloudinary and that would also require the source width and height in order to generate a valid command.

Adding these values to ImageUrlGenerationOptions is one approach to solving this problem, there are others... Open to suggestions....

[github-actions]

Hi there @Jeavon, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[ronaldbarendse]

Having to manually provide these dimensions when calling GetCropUrl() would mean you have to update all those method calls (including any core or package usage, so not really a feasible solution). And adding parameters to an existing method is a binary breaking change (even if they have default values/are optional).

Since the crop URL is generated on the server, the custom IImageUrlGenerator implementation could fetch the source image dimensions by either:

• Opening the file using IWebHostEnvironment.WebRootFileProvider.GetFileInfo(...).CreateReadStream() and getting the dimensions using IImageDimensionExtractor;
• Get the media item using IMediaService.GetMediaByPath() and getting the dimensions from the media properties (Constants.Conventions.Media.Width and Constants.Conventions.Media.Height), although that would only work for images stored as media.

In either case, you still need to have a dependency on ImageSharp to be able to get/populate the image dimensions on the server side though. Maybe an even better solution is to implement this logic directly in Cloudflare (maybe using a custom Worker) or Cloudinary (which already seems to support percentage based crops). Would be awesome to see some cloud-based image processing alternatives for Umbraco, since ImageSharp is now fully decoupled from the CMS and can be completely removed if you want 😄

[Jeavon]

@ronaldbarendse I really don't want to have to update all of the GetCropUrl methods either, if you check on the changes I've made in the draft PR, I only changed the string methods as anything with a IPublishedThing can get the source dimensions from the cache. I totally know this would be a breaking change.

Given that GetCropUrl can be called a lot in a view, looking up the dimensions from the image file or using the MediaService is going to be really inefficient for a UrlGenerator don't you think (of course it could be a bespoke cached)? It's what lead me to think that this needs to change deeper and therefore adding source dimensions to ImageUrlGenerationOptions might be the best solution and would be useful to any generator...

I have a UrlGenerator for both Cloudflare and Cloudinary and unfortunately, the percentage-based cropping on Cloudinary is only accurate to 2 decimals so it doesn't work anywhere near close enough for the Umbraco cropper.
If you are interested compare these demos:

• Imagesharp - https://slimsydemo.crumpled-dog.com/media/dsypff0x/laura_weatherhead.jpg?cc=0.3666652719665271,0.08712134681411768,0.3543947001394701,0.5177794635343203
• Percentage - https://res.cloudinary.com/dnbkvanna/image/fetch/c_crop,h_0.39509918965,w_0.2789400278940028,x_0.3666652719665271,y_0.08712134681411768/https://slimsydemo.crumpled-dog.com/media/dsypff0x/laura_weatherhead.jpg
• Converted Percentage to pixels using source dimensions - https://res.cloudinary.com/dnbkvanna/image/fetch/c_crop,h_571,w_571,y_126,x_751/https://slimsydemo.crumpled-dog.com/media/dsypff0x/laura_weatherhead.jpg

[nielslyngsoe]

Hi @Jeavon and @ronaldbarendse

This PR seems interesting, but has gone stale?

It seems relevant, so could you let me in on what state we are at?
Is it a performance concern holdning this in Draft mode and are there other things that needs clarifications?

[ronaldbarendse]

I think the main concerns are:

• Maintaining backwards compatibility, as the source width/height would need to be provided on every call to GetCropUrl() (adding parameters or required properties will be a breaking change), even if you're not using an IImageUrlGenerator implementation that uses these properties. If the values are made optional, not all implementations would be able to create a correct crop URL, which is also not ideal.
• The performance impact, as we don't want to read/parse the original image sizes on each call.
• Compatibility with a matrix of file locations (site assets, uploaded media, external files, etc.) and image processing providers (ImageSharp, Cloudflare, Cloudinary, etc.).

So it all comes down to whether we want to improve image processing support for these external providers in our own abstraction. On the front-end, you can already use your own implementation, so it's more to ensure the backoffice can get cropped images as well.

One workaround that could work on the front-end would be to create new class that inherits from ImageUrlGenerationOptions, add the 2 properties and in the custom IImageUrlGenerator check whether the instance is of that type to retrieve the values again.

[nielslyngsoe]

Hi @Jeavon, do you have any thoughts on Ronalds comment here. Do you agree with his perspectives? Or do you have other perspectives that should be weighted as well?

[Jeavon]

I still think that the SourceWidth and SourceHeight should be available when implementing a provider one way or another in order to support implementations using any third party providers. These 2 values are available from published cache as they are extracted at upload so it's not necessary to read the file again to find them for this.

I have worked around this issue on the front end for my Cloudflare provider by adding these 2 values from published cache onto the querystring so that my IImageUrlGenerator can pick them up (and remove them from the querystring) and use them for it's calculations. It works....

@ronaldbarendse workaround sounds viable to me but I do think that if you implement a provider it should work in the front end and back end really.