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

Feature: Dynamically Media-process images with fixed size in HTML #6986

Closed
anoordende opened this issue Jun 17, 2016 · 14 comments · Fixed by #8806
Closed

Feature: Dynamically Media-process images with fixed size in HTML #6986

anoordende opened this issue Jun 17, 2016 · 14 comments · Fixed by #8806
Milestone

Comments

@anoordende
Copy link
Contributor

Images inserted in HTML through, e.g. any content with a Body can be resized through the editor. However, the full size image will still be served whereas it would be more efficient, especially on mobile clients, to serve a pre-sized image.

It is quite straightforward to dynamically cut these images to size using the MediaProcessing module in combination with IHtmlFilter. Of course, it needs to be a feature that can be turned on or off as desired.

The one requirement is that the height/width attributes are provided, which will be when resizing through the TinyMce editor. This also provides an excellent trick when you particularly don't want an image to be resized: omit the height or width.

As I've had this code live for a few years already, the PR is to follow shortly.

@Skrypt
Copy link
Contributor

Skrypt commented Jun 17, 2016

I agree that this need to be a feature that you can enable/disable mostly because now responsive design uses images that have width="100%" that stretch with the window size. If we keep the full sized image for keeping image quality on wider screen it makes less files to manage and sometimes as much efficient size wise.

@anoordende
Copy link
Contributor Author

anoordende commented Jun 17, 2016

You can still use 100% width and use this feature. Just set the height/width atts to the maximum size you'll need for any viewport, but upload just the full-res.

As mentioned, this is targeting IHTMLFilter implementors. If you are using or thinking about e.g. sliders then these are better implemented with fixed Image profiles.

A common usage scenario is:

  • user uploads > 3000px 5Mb original
  • inserts it into a body part
  • drags handles to resize it to 120px

This is where this feature kicks in and creates a dynamic 120px profile.


I agree that this need to be a feature that you can enable/disable mostly because now responsive design uses images that have width="100%" that stretch twith the window size. If we keep the full sized image for keeping image quality on wider screen it makes less files to manage and sometimes as much efficient size wise.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#6986 (comment)

@Skrypt
Copy link
Contributor

Skrypt commented Jun 17, 2016

Yeah, but if your image is 120px and you need to display it in 100% width on a QHD screen you will only see big pixels 😄

I understand your request and I think this is a good idea, though it needs to be a feature that works with any Wysiwyg editor out there. So, it must be post-processed if enabled. The only thing I'm worried about is the scope; per image, per editor, per body part, per ...

@anoordende
Copy link
Contributor Author

anoordende commented Jun 17, 2016

Simple: Don't resize to 120px in the editor and you'll get the original size.

It will work with any editor, TinyMce included, because it doesn't operate at that level, it uses IHTMLFilter.

Important to note it also does not apply to images already the result of a profile.

Anyway, all good input. I'll submit the code shortly and you can check it out.


Yeah, but if your image is 120px and you need to display it in 100% width on a QHD screen you will only see big pixels 😄

I understand your request and I think this is a good idea, though it needs to be a feature that works with any Wysiwyg editor out there. So, it must be post-processed if enabled. The only thing I'm worried about is the scope; per image, per editor, per body part, per ...


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#6986 (comment)

@anoordende
Copy link
Contributor Author

PR created: #6992

@sfmskywalker
Copy link
Member

This seems like a useful feature, but I'm not sure it should be part of the core distribution, so I would like to ask other people's opinion on this as well. /cc @sebastienros @bleroy @pszmyd @DanielStolt @Jetski5822

We also discussed enhancing the media library picker to allow providing additional information, for example selecting one or more profiles associated with view port sizes to render multiple sources.

@sfmskywalker
Copy link
Member

sfmskywalker commented Jun 30, 2016

Maybe add support for <picture> and <img srcset> element and atrribute.

@jonvee
Copy link

jonvee commented Jun 30, 2016

+1 for picture and img srcset

@anoordende
Copy link
Contributor Author

...add support for <picture> and <img srcset> element and atrribute.

Yes, agreed.

...enhancing the media library picker to allow providing additional information, for example selecting one or more profiles associated with view port sizes to render multiple sources.

To work with srcset? That's an excellent idea.

The one thing though, that I find a recurring issue with my users is that working with Orchard is difficult for an average editor, especially uploading and working with images. Further requirements on a user inputting additional info makes me a bit wary that we're adding further end-user complexity. If we're considering dynamically populating e.g. a srcset, perhaps the variations this creates should be pre-configured through admin settings? Regardless of this being the right solution or not, I think it would be quite straightforward to add this functionality here, if only as a PoC.

@DaRosenberg
Copy link
Member

@sfmskywalker I recall we had a discussion about using in IHtmlFilter to stop embedding absolute media URLs in HTML/markdown content, and instead embed an encoded reference to a media item by ID, and have the filter resolve this reference relative to wherever the media library storage happens to reside for the current tenant. I vaguely remembered you opened an issue for this, but I can't seem to locate it.

It occurs to me that this request could be combined with that one, to solve both problems at once. If we stop embedding pre-resolved media URLs in content and instead embed media item references, those could also contain size/profile information, and we could use all existing logic in the media library picker field to process.

@anoordende
Copy link
Contributor Author

anoordende commented Jul 11, 2016

Good point, I have always argued that the media URL should be embedded relative to /Media as a level of indirection. Though that has it's issues and your idea of using an encoded ID will work around those. In fact, where needed we use a custom implementation of the media module that does exactly that (but relative to /Media). MediaProcessing should also make the /_Profile/ URL's hashes host-agnostic: see #6984. But, as long as it eliminates the need for a)

UPDATE [prefix_Orchard_Framework_ContentItemVersionRecord]
SET [Data] = REPLACE([Data], 'src=&quot;/Media/TenantName/', 'src=&quot;https://my.blob.core.windows.net/media/TenantName/')
WHERE [Data] LIKE '%src=&quot;/Media/TenantName%'

b) allows for domain moves with wildcard 301's to images and image profiles,
c) allows for switching to- and between cdn's,
d) respects the scheme (http/https),
e) dynamically resizes images

That would complete the Use Case?

My only doubt is that this filter would have one leg in MediaLibrary and another in MediaProcessing. So perhaps the MediaLibrary module should expose an interface for eg. an IPreRenderProvider defining a method that accepts an HTML node, called from a single implementation of IHtmlFilter in MediaLibrary, so that we can separate the functionality, without having to parse the HTML multiple times.

@DaRosenberg
Copy link
Member

DaRosenberg commented Jul 12, 2016

a) Exactly - we are doing roughly the same thing, but in the recipe during the deployment process.
b) I think it might be best to leave redirects out of scope.
c) Yes, this would be inherently supported just like for media library picker fields. The media storage implementation can support such configuration, like the Azure Blob Storage implementation does.
d) I think perhaps this too should be up to the media storage implementation, e.g. perhaps you want media requested via HTTPS from blob storage or from your CDN even if your site runs on HTTP. The media storage implementation can always choose to emit a scheme-relative URL (starting with double slash) to have the media requested using the same scheme as the current Orchard request. This should be the default behavior of the built-in implementation.
e) Yep.

Fair point about separating the responsibilities. On the other hand, MediaLibrary already depends on MediaProcessing and knows about it, so perhaps it's OK that MediaLibrary has the knowledge of how to embed a processing profile in the media reference and resolve this to a URL that invokes MediaProcessing to execute it?

@sebastienros
Copy link
Member

Is the current PR the final answer or do we want to wait for more features like the ones discussed here?
As it's an optional feature I am not too worried by the perf impact, though there is some.

@anoordende
Copy link
Contributor Author

I would like to include picture and img srcset first, then that will be a rounded PR.

@sebastienros sebastienros added this to the dev milestone Aug 18, 2016
BenedekFarkas added a commit that referenced this issue Dec 7, 2024
* #6986 Created feature Media Processing Html Filter

* Fixing HtmlAgilityPack and upgrading FW and package version to match Orchard.Specs

* Adapting MediaProcessingHtmlFilter to IHtmlFilter breaking change

* Fixing that Orchard.MediaProcessingHtmlFilter should depend on Orchard.MediaProcessing

* Code styling

* Using regexes instead of HtmlAgilityPack, thanks GHCP

* Updating comments and code styling

* Code styling

* Reworking ProcessContent to use StringBuilder instead of replaces

* Fixing that GetAttributeRegex should find attributes with empty value

* Code styling

* Fixing that detecting the extension works regardless of casing

but it still works with Azure Blob Storage (which is case-sensitive) too

* Optimizing image tag regex

* Caching attribute regexes

* Caching attribute values of img tags

* Simplifying attribute value cache

---------

Co-authored-by: Arjan Noordende <arjan@zumey.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants