adobe / aem-core-wcm-components

Standardized components to build websites with AEM.
https://docs.adobe.com/content/help/en/experience-manager-core-components/using/introduction.html
Apache License 2.0
723 stars 737 forks source link

[RFC] Image component with renditions support #496

Open lewandowskidawid opened 5 years ago

lewandowskidawid commented 5 years ago

Hi,

I have few questions about rendition in Image component. I hope you could help me, because I cannot find any historical tickets, which answer on my questions.

I am wondering was the main argument to implement own AdaptiveImageServlet instead of use e.g. picturetag in component markup. I see the value of AdaptiveImageServlet (optimal rendition is being loaded) but on the other hand the same solution has one main drawback: image link doesn’t point directly to DAM asset but to Image component placed on a page e.g. /content/we-retail/language-masters/en/men/_jcr_content/root/responsivegrid/teaser_980945351.coreimg.jpeg/1473680831529.jpeg.

The drawback could have impact on crucial areas like:

  1. Dispatcher cache size. When single image is referenced in many placed then instead of single copy on dispatcher (or CDN) level, we get many duplicated assets (when we ignorejpegQuality param then they are copies). What is the recommendation for big platforms where many sites are hosted on AEM or for sites, which are very rich in Image components? What is the recommendation in terms of dispatcher cache invalidation?
  2. Security. What is the recommendation in terms of security? Current implementation could be easily used for DDOS attack (pattern observed in links could be used for generating thousands of different requests). How to block invalid requests on Apache level when configuration could be changed in any time?
  3. Platform performance. Each requested image is returned by AdaptiveImageServlet. The servlet could execute custom calculations for each image. Do you have any information how it affects overall AEM performance and page readiness time?

Thanks

vladbailescu commented 5 years ago

We chose to not use picture as it's not supported in IE.

We chose to implement a custom servlet in order to offer more fine-grained control for image sizes. A future version of the Image component where authors/devs can choose to use either the adaptive servlet or existing renditions is something we would be interested in. We would very much appreciate a contribution in this regard.

The image link allows identifying the exact instance of that image and applying all transformations dictated by the content policy and authored properties.

As for the noted drawbacks:

  1. Agree, but this can probably be mitigated by using content fragments for images that are reused very often.

2 & 3. Since content policies are regarded as development/superuser configurations, changes will need to be properly synced between dispatcher and AEM.

lewandowskidawid commented 5 years ago

Yes, OOTB picture tag is not supported in IE browser, but picturefill library fixes that. I assume that you didn't decide on that approach because you don't want to have any external dependencies - am I right?

What are your expectations in terms of existing renditions usage?

  1. Should it use the same (or similar) custom frontend logic, which is already implemented?
  2. Should it improve areas mentioned in the ticket? 2.1. Links point to assets directly instead of component resource 2.2. When invalid rendition is being requested then 404 response is generated by AEM
  3. What should be relation between the current approach and the new one? Who will be able to decide which solution will be used (author - Image component level, super author - policy level or developer - implementation level or OSGi configuration)
vladbailescu commented 5 years ago

IMO a (good) polyfill would be acceptable. As for your questions:

  1. Since it's a completely new version, we can use the picture approach.

2.1. That would mean no in-page customization/editing for authors, unless we add some client-side support (which could be tricky). We can probably keep image maps but the rest will go.

2.2. Yes.

  1. I see this as a developer-lead decision. They can choose to use v2 or v3 (which only allows predefined renditions) based on their project needs. We can try to support both versions for a while, until we figure out what most people prefer.
lewandowskidawid commented 5 years ago

In addition, what do you think about creation brand new Image component (we could call it Simple/Basic Image component) instead of creating new version of existing one. I am considering this approach because when we are going to use existing renditions so, we have avoid any image processing on server side. It means that following features should be dropped: Cropping, Rotation, Image quality management. Also Launch Map management looks like option which is used very rarely, so maybe it could not be migrated to the new component.

Now, when we would like to render image supporting renditions we could do it by including image component inside different component. It has some impact on page performance because we have more components to render. On the other hand current image component applies rendition base on image size (when image to render is small then proper rendition will be used - small one). When we start using picture then we will lose that feature and the same image rendition will be provided for all components even for very small images (e.g. image in List component) because everything depends on browser width. It could be fixed by attaching rendition configuration to each component, which could render an image. Thanks for that we decrease number of components to render (because we will not include Image component). Code responsible for picture tag generation need be extracted to separate place to guarantee that other components could re-use it (e.g. image in List component, background in Layout Container component).

What is your opinion about that approach and consequences of introducing picture tag?

vladbailescu commented 5 years ago

@lewandowskidawid This is shaping up into an RTC, I will mark it as so for others to comment as well.

I'm personally ok with having a simpler/basic image component with no extra features and then layering responsive/adaptive, edit and maps functionalities on top.

As for configuring the image component when included by another component, you might want to look into the imageDelegate pattern that we're using to let the Teaser know who's going to render the image.

JasonUSC commented 5 years ago

I'm not crazy about having 2 different image components, I'd rather have one component that can be configured with policies.

We very much like giving authors the ability to custom crop an image for a page, so would hate to lose that as a feature. But there should also be compatibility with Smart Crop, not sure how to reconcile those 2 concepts.

It would also be helpful to take into consideration Dynamic Media customers so that we can use the image core component and not have to use the foundations-based dynamic media viewer.

jaydubb12 commented 5 years ago

Yes, as an enterprise level / principle architect who has invested in Dynamic Media it is critical to be able to deliver images via DM when using the image component.

I would recommend maintaining the capabilities to crop / rotate and leverage conditional logic for V1 to deliver the image via DM, UNLESS one of those features are leveraged, if / theb deliver the modified image via jcr:Content via the Adaptive image servlet as an exception vs the rule..and leverage CDN to cache the jcr:content image...which if using Akamai can be delivered using their Adaptive Image Compression feature..but that requires the image to be a jpg.

Different topic for a different day...is the text+Image component which is exposed via ACS commons...but that github repo is 3-5 years out dated and does not seem to leverage core components...unless I am mistaken?

On Tue, Mar 12, 2019 at 8:23 AM Jason Chandler notifications@github.com wrote:

I'm not crazy about having 2 different image components, I'd rather have one component that can be configured with policies.

We very much like giving authors the ability to custom crop an image for a page, so would hate to lose that as a feature. But there should also be compatibility with Smart Crop, not sure how to reconcile those 2 concepts.

It would also be helpful to take into consideration Dynamic Media customers so that we can use the image core component and not have to use the foundations-based dynamic media viewer.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/adobe/aem-core-wcm-components/issues/496#issuecomment-471978298, or mute the thread https://github.com/notifications/unsubscribe-auth/AUVnSPdfhhRrhTPIlJAoAkonh3B7BiXOks5vV5xHgaJpZM4becVY .

--

lewandowskidawid commented 5 years ago

I have suggested separated component because I would like to avoid situation when author or even super author will have many options to choose for single component. Especially when these options could change component technicalities (e.g. Adaptive Images vs Picture tag). Developers should take this kind of decisions during project development phase.

Similar situation could be with image cropping - is it authors responsibility to crop image properly? In most cases I would say "No" - they are responsible for content editorial and they would like to do it efficiently, so we should give proper tools for that - not too simple but not too complex also.

In case of support Dynamic Media, I think that is great feature with outstanding possibilities in the same time DM is not used as often as normal DAM, so it could be a next argument for components separation.

Could we sum up our discussion and agree next actions, if any? Moreover, we could stop discussion about components separation because it is not a key topic for now. We should do it as next step when we agree what we would like to achieve – how we would like to approach for already generated renditions.

vladbailescu commented 5 years ago

I think I can identify several changes that are proposed:

1. Allow using DAM renditions (with CDN configuration) instead of the adaptive image servlet

This could be added as a configuration option and defined via a content policy. It could be supported/added for the current version too (Image v2). We can have different defaults (v2 stays by default with adaptive images, v3 uses by default DAM renditions)

Activating DAM renditions would have the following consequences:

2. Change markup to use picture tag

This would replace the custom (JS-based) adaptive capabilities. This means a new version of the image component (v3) would need to be created. Projects have proxies to both v2 and v3 (and perhaps call them Image and DAM Image)

3. Supporting Dynamic Media

We already have Dynamic Media components in the product and our team (Core Components) was not planning to add this type of support at this moment.


I listed those in the priority our team perceives now. Of course, we would be supportive of any community efforts and open to more feedback. As always with any open projects, contributions can push things further and faster.

stefanseifert commented 5 years ago

comment on 1.: i do not fully understand why making the use of renditions has the consequence that in-page uloads are no longer possible or editing features have to be disabled. the image component should just pick the best solution: if and image was uploaded or edit features are used, the current behavior takes place. but if not, existing renditions are used if they match the required width/height ratio instead of creating a new one from scratch. (thats what we are doing in http://wcm.io/handler/media/ - additionally in wcm.io Media Handler the servlet for generating renditions on-the-fly is attached to the asset, and not to the component - so if many components are using the same image with the same width/height it's generated and cached only once and not multiple times)

comment on 2.: i think using HTML5-standard markup with picture/source tags or img + srcset/sizes attributes would be a big step forward. every frontend developer knows them well, and the current "adaptive" implementation of the core component is very proprietary and less flexible (e.g. it is not possible to use different ratios for different breakpoints, which is often required).

JasonUSC commented 5 years ago

@vladbailescu Regarding Dynamic Media, it was my understanding that Adobe was no longer improving components based on the legacy foundations technology that the Dynamic Media components were built on. Likewise, it was my understanding that new features would only be added to the Core Components. We really like the new "Smart Crop" feature but it seems that it's only available with the legacy foundations players for Dynamic Media and not available for Core Components. Is there some way to accommodate Smart Crop with the image Core Component?

vladbailescu commented 5 years ago

@JasonUSC we don't have plans at the moment to support Dynamic Media features. @gabrielwalt might know a bit more.

lewandowskidawid commented 5 years ago

after familiarisation with http://wcm.io/handler/media/ library I might say that it looks like solution for all drawbacks pointed out. @vladbailescu @stefanseifert what do you think about setting up official relation between Core Components and Media Handler libraries (contribution, official support etc).
It would be great to have OOTB support without need of Core Components library modification where all concepts are present and developers don't have to reinvent the wheel.

andrewmkhoury commented 5 years ago

Seems the licensing is compatible (Apache) http://wcm.io/licenses.html

stefanseifert commented 4 years ago

what do you think about setting up official relation between Core Components and Media Handler libraries (contribution, official support etc).

the wcm.io project provides now a "WCM Core Component" layout atop of the AEM Sites Core Components to add wcm.io Handler Support.

Further infos:

mmange commented 4 years ago

The caching strategy and cache invalidation issues w/ the image component in current wcm core implementation is a huge sticking point. If an image changes in the DAM, previously cache flushing was easier since the image was cached relative it's path in DAM (and even Edge caches could be purged similarly).

In the current image implementation the way image paths are rendered cause duplication of cache as well as a cache clearing nightmare. It would great if finding a solution to this issue could be prioritized.