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
744 stars 750 forks source link

Image: Make the quality selector optionally always take effect #472

Closed kwin closed 5 years ago

kwin commented 5 years ago

Current Behavior The quality selector being introduced with #254 only takes effect in case

  1. the request width is not larger than the original (https://github.com/adobe/aem-core-wcm-components/blob/8a47281f2831f8a841756e280b4e5cb22f3c8949/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L407) and
  2. no rendition exists with exactly the requested width (https://github.com/adobe/aem-core-wcm-components/blob/8a47281f2831f8a841756e280b4e5cb22f3c8949/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java#L395)

(both only in case there is no additional transformation being added on top)

Expected behavior/code In some cases the quality is reduced to a level, where the result would be even smaller than the already less wide original and also smaller than precompiled renditions. So at least optionally it should be possible to alway recalculate the image (especially when you do aggressive caching in the web server/CDN)

Environment

Possible Solution Add an additional flag which forces the AdaptiveImageServlet to always rerender the image!

kwin commented 5 years ago

Just for the record: The default quality being used to generate the WebRendition (1280 pixels wide) in AEM 6.4.1 is 90 (it can be overwritten though in the workflow model at /editor.html/conf/global/settings/workflow/models/dam/update_asset.html)

jckautzmann commented 5 years ago

Thx @kwin for your feedback. We will discuss this feature within our team. IMHO the quality transformation should always be applied to the original asset (when streamed as is or resized), but not when streaming a rendition, which is smaller than the original. This would mean modifying lines 409 and 417 to also pass the quality parameter: https://github.com/adobe/aem-core-wcm-components/blob/development/bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/servlets/AdaptiveImageServlet.java

kwin commented 5 years ago

I am myself not really convinced that this would be worth the effort. You would then need to rerender the original/cached rendition just to consider the quality parameter. That consumes considerable resources (CPU and memory) and might not even be worth the effort, because the original and the web rendition should have a proper quality in the first place. But maybe as an option for use cases where you really strive for minimum sizes and don't care about rendering performance it might be worth implementing this. In any case you should compare the rerendered one with the underlying image and only deliver the rerendered one in case it is considerably (TBD) smaller!

jckautzmann commented 5 years ago

(CQ-4264377)

gabrielwalt commented 5 years ago

In the design dialog of the image component, I’d rather have a checkbox saying “Use existing renditions for DAM assets”:

This would clarify how the renditions are generated and avoid mixed behaviors that bring complexity with little value.

kwin commented 5 years ago

@gabrielwalt From which DAM rendition would the servlet generate the new image in case that checkbox would be false? In case the checkbox is true which rendition would be taken for which requested width?

gabrielwalt commented 5 years ago

@kwin, the original rendition would always be taken by the image servlet to render any allowed sizes that were configured in the design dialog.

kwin commented 5 years ago

Always taking the original as base might not be very wise. I know environments were the original renditions are huge (multiple hundreds of megabytes), so I think instead picking a (already existing) DAM rendition with a suitable size (TBD) as basis makes way more sense.

bpauli commented 5 years ago

We take this requirement into consideration when we create a new version of the image component. The design of the new image component should be discussed in #496. Therefore I will close this issue and the related PR without merging.