Merge branch 'develop'

wcm-io · Nov 9, 2022 · 3e7407f · 3e7407f
2 parents ec234c6 + a4bfc4f
commit 3e7407f
Show file tree

Hide file tree

Showing 11 changed files with 141 additions and 7 deletions.
diff --git a/changes.xml b/changes.xml
@@ -23,6 +23,15 @@
     xsi:schemaLocation="http://maven.apache.org/changes/1.0.0 http://maven.apache.org/plugins/maven-changes-plugin/xsd/changes-1.0.0.xsd">
   <body>
 
+    <release version="1.14.18" date="2022-11-09">
+      <action type="update" dev="sseifert">
+        Dynamic Media Support: Make Smart Crop rendition validation configurable (default: enabled).
+      </action>
+      <action type="fix" dev="sseifert">
+        Dynamic Media Support: Apply fail-safe approach when normalized width/height provided by Dynamic Media for smart cropping are not matching the defined aspect ratio.
+      </action>
+    </release>
+
     <release version="1.14.16" date="2022-10-22">
       <action type="update" dev="sseifert">
         DAM Media Source: Use cropping dimension based on original rendition internally. Re-calculate webenabled rendition-based cropping coordinates when loading them from repository before starting the media processing, and not only when doing the actual cropping.

diff --git a/pom.xml b/pom.xml
@@ -31,7 +31,7 @@
 
   <groupId>io.wcm</groupId>
   <artifactId>io.wcm.handler.media</artifactId>
-  <version>1.14.16</version>
+  <version>1.14.18</version>
   <packaging>jar</packaging>
 
   <name>Media Handler</name>
@@ -49,7 +49,7 @@
     <site.url.module.prefix>handler/media</site.url.module.prefix>
 
     <!-- Enable reproducible builds -->
-    <project.build.outputTimestamp>2022-10-22T11:03:35Z</project.build.outputTimestamp>
+    <project.build.outputTimestamp>2022-11-09T09:42:26Z</project.build.outputTimestamp>
   </properties>
 
   <dependencies>

diff --git a/src/main/java/io/wcm/handler/mediasource/dam/impl/DamAutoCropping.java b/src/main/java/io/wcm/handler/mediasource/dam/impl/DamAutoCropping.java
@@ -95,7 +95,8 @@ public List<CropDimension> calculateAutoCropDimensions() {
    * @return Cropping dimension or null if not found
    */
   private @Nullable CropDimension getDynamicMediaCropDimension(double requestedRatio) {
-    if (damContext.isDynamicMediaEnabled() && damContext.isDynamicMediaAsset()) {
+    if (damContext.isDynamicMediaEnabled() && damContext.isDynamicMediaAsset()
+        && damContext.isDynamicMediaValidateSmartCropRenditionSizes()) {
       NamedDimension smartCropDef = SmartCrop.getDimensionForRatio(damContext.getImageProfile(), requestedRatio);
       if (smartCropDef != null) {
         return SmartCrop.getCropDimensionForAsset(damContext.getAsset(), damContext.getResourceResolver(), smartCropDef);

diff --git a/src/main/java/io/wcm/handler/mediasource/dam/impl/DamContext.java b/src/main/java/io/wcm/handler/mediasource/dam/impl/DamContext.java
@@ -140,6 +140,14 @@ public boolean isDynamicMediaAsset() {
     return dynamicMediaServerUrl;
   }
 
+  /**
+   * @return Whether to validate that the renditions defined via smart cropping fulfill the requested image width/height
+   *         to avoid upscaling or white borders.
+   */
+  public boolean isDynamicMediaValidateSmartCropRenditionSizes() {
+    return dynamicMediaSupportService.isValidateSmartCropRenditionSizes();
+  }
+
   /**
    * @return Dynamic media reply image size limit
    */

diff --git a/src/main/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaPath.java b/src/main/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaPath.java
@@ -118,7 +118,8 @@ private DynamicMediaPath() {
       // check for matching image profile and use predefined cropping preset if match found
       NamedDimension smartCropDef = SmartCrop.getDimensionForWidthHeight(damContext.getImageProfile(), width, height);
       if (smartCropDef != null) {
-        if (!SmartCrop.isMatchingSize(damContext.getAsset(), damContext.getResourceResolver(), smartCropDef, width, height)) {
+        if (damContext.isDynamicMediaValidateSmartCropRenditionSizes()
+            && !SmartCrop.isMatchingSize(damContext.getAsset(), damContext.getResourceResolver(), smartCropDef, width, height)) {
           // smart crop should be applied, but selected area is too small, treat as invalid
           logResult(damContext, "<too small for " + width + "x" + height + ">");
           return null;

diff --git a/...ain/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportService.java b/...ain/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportService.java
@@ -51,6 +51,12 @@ public interface DynamicMediaSupportService {
    */
   boolean isAemFallbackDisabled();
 
+  /**
+   * @return Whether to validate that the renditions defined via smart cropping fulfill the requested image width/height
+   *         to avoid upscaling or white borders.
+   */
+  boolean isValidateSmartCropRenditionSizes();
+
   /**
    * @return Reply image size limit as configured in dynamic media.
    */

diff --git a/...java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportServiceImpl.java b/...java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportServiceImpl.java
@@ -90,6 +90,11 @@ public class DynamicMediaSupportServiceImpl implements DynamicMediaSupportServic
             + "if Dynamic Media is enabled, but the asset has not the appropriate Dynamic Media metadata.")
     boolean disableAemFallback() default false;
 
+    @AttributeDefinition(
+        name = "Validte Smart Crop Rendition Sizes",
+        description = "Validates that the renditions defined via smart cropping fulfill the requested image width/height to avoid upscaling or white borders.")
+    boolean validateSmartCropRenditionSizes() default true;
+
     @AttributeDefinition(
         name = "Image width limit",
         description = "The configured width value for 'Reply Image Size Limit'.")
@@ -111,6 +116,7 @@ public class DynamicMediaSupportServiceImpl implements DynamicMediaSupportServic
   private DynamicMediaCapabilityDetection dmCapabilityDetection;
   private boolean authorPreviewMode;
   private boolean disableAemFallback;
+  private boolean validateSmartCropRenditionSizes;
   private Dimension imageSizeLimit;
 
   private static final String SERVICEUSER_SUBSERVICE = "dynamic-media-support";
@@ -124,6 +130,7 @@ private void activate(Config config) {
     this.dmCapabilityDetection = config.dmCapabilityDetection();
     this.authorPreviewMode = config.authorPreviewMode();
     this.disableAemFallback = config.disableAemFallback();
+    this.validateSmartCropRenditionSizes = config.validateSmartCropRenditionSizes();
     this.imageSizeLimit = new Dimension(config.imageSizeLimitWidth(), config.imageSizeLimitHeight());
 
     if (this.enabled) {
@@ -157,6 +164,11 @@ public boolean isAemFallbackDisabled() {
     return disableAemFallback;
   }
 
+  @Override
+  public boolean isValidateSmartCropRenditionSizes() {
+    return validateSmartCropRenditionSizes;
+  }
+
   @Override
   public @NotNull Dimension getImageSizeLimit() {
     return this.imageSizeLimit;

diff --git a/src/main/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/SmartCrop.java b/src/main/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/SmartCrop.java
@@ -160,6 +160,21 @@ public static boolean canApply(@Nullable CropDimension cropDimension, @Nullable
     long top = Math.round(originalHeight * topPercentage);
     long width = Math.round(originalWidth * widthPercentage);
     long height = Math.round(originalHeight * heightPercentage);
+
+    // it may happen that DM provides inconsistent normalizedWidth/normalizedHeight values which results
+    // in renditions not matching the ratio of the cropping definition. In that case use only the one from the
+    // the two which results in the smaller rendition and calculate the missing value from the other
+    double expectedRatio = Ratio.get(smartCropDef.getWidth(), smartCropDef.getHeight());
+    double actualRatio = Ratio.get(width, height);
+    if (!Ratio.matches(expectedRatio, actualRatio)) {
+      if (actualRatio > expectedRatio) {
+        width = Math.round(height * expectedRatio);
+      }
+      else {
+        height = Math.round(width / expectedRatio);
+      }
+    }
+
     return new CropDimension(left, top, width, height, true);
   }
 
@@ -177,15 +192,15 @@ public static boolean isMatchingSize(@NotNull Asset asset, @NotNull ResourceReso
       @NotNull NamedDimension smartCropDef, long width, long height) {
     CropDimension cropDimension = getCropDimensionForAsset(asset, resourceResolver, smartCropDef);
     if (cropDimension == null) {
-      // no smart cropping rendition is not found in repository or it contains invalid values,
+      // smart cropping rendition is not found in repository or it contains invalid values,
       // we assume the size should be fine and skip further checking
       return true;
     }
 
     // check if smart cropping area is large enough
     long croppedWidth = cropDimension.getWidth();
     long croppedHeight = cropDimension.getHeight();
-    boolean isMatchingSize = (cropDimension.getWidth() >= width || croppedHeight >= height);
+    boolean isMatchingSize = (cropDimension.getWidth() >= width && croppedHeight >= height);
     if (!isMatchingSize) {
       log.debug("Smart cropping area '{}' for asset {} is too small ({} x {}) for requested size {} x {}.",
           smartCropDef.getName(), asset.getPath(), croppedWidth, croppedHeight, width, height);

diff --git a/...test/java/io/wcm/handler/media/impl/MediaHandlerImplEnd2EndDynamicMediaSmartCropTest.java b/...test/java/io/wcm/handler/media/impl/MediaHandlerImplEnd2EndDynamicMediaSmartCropTest.java
@@ -36,6 +36,7 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.osgi.framework.Constants;
 
 import com.day.cq.dam.api.Asset;
 import com.day.cq.dam.api.DamConstants;
@@ -49,6 +50,7 @@
 import io.wcm.handler.media.Rendition;
 import io.wcm.handler.media.testcontext.AppAemContext;
 import io.wcm.handler.media.testcontext.DummyMediaFormats;
+import io.wcm.handler.mediasource.dam.impl.dynamicmedia.DynamicMediaSupportServiceImpl;
 import io.wcm.sling.commons.adapter.AdaptTo;
 import io.wcm.testing.mock.aem.junit5.AemContext;
 import io.wcm.testing.mock.aem.junit5.AemContextExtension;
@@ -100,6 +102,23 @@ void testValidSmartCroppedRenditionAndWidths() {
     assertEquals("https://dummy.scene7.com/is/image/DummyFolder/test%3A4-3?wid=40&hei=30&fit=stretch", renditions.get(1).getUrl());
   }
 
+  @Test
+  void testValidSmartCroppedRenditionAndWidths_DisableValidateSmartCropRenditionSizes() {
+    context.registerInjectActivateService(DynamicMediaSupportServiceImpl.class,
+        "validateSmartCropRenditionSizes", false,
+        Constants.SERVICE_RANKING, 100);
+    mediaHandler = AdaptTo.notNull(context.request(), MediaHandler.class);
+
+    Media media = getMediaWithWidths(100, 80, 40);
+    assertTrue(media.isValid());
+
+    List<Rendition> renditions = ImmutableList.copyOf(media.getRenditions());
+    assertEquals(3, renditions.size());
+    assertEquals("https://dummy.scene7.com/is/image/DummyFolder/test%3A4-3?wid=100&hei=75&fit=stretch", renditions.get(0).getUrl());
+    assertEquals("https://dummy.scene7.com/is/image/DummyFolder/test%3A4-3?wid=80&hei=60&fit=stretch", renditions.get(1).getUrl());
+    assertEquals("https://dummy.scene7.com/is/image/DummyFolder/test%3A4-3?wid=40&hei=30&fit=stretch", renditions.get(2).getUrl());
+  }
+
   @Test
   void testInvalidSmartCroppedRendition() {
     Media media = getMediaWithWidths(100);

diff --git a/.../io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportServiceImplTest.java b/.../io/wcm/handler/mediasource/dam/impl/dynamicmedia/DynamicMediaSupportServiceImplTest.java
@@ -67,6 +67,7 @@ void setUp() {
   void testDefaultConfig() {
     DynamicMediaSupportService underTest = context.registerInjectActivateService(new DynamicMediaSupportServiceImpl());
     assertTrue(underTest.isDynamicMediaEnabled());
+    assertTrue(underTest.isValidateSmartCropRenditionSizes());
     assertEquals(new Dimension(2000, 2000), underTest.getImageSizeLimit());
     assertEquals("https://dummy.scene7.com", underTest.getDynamicMediaServerUrl(asset, null, context.request()));
     assertTrue(underTest.isDynamicMediaCapabilityEnabled(true));
@@ -100,9 +101,11 @@ void testConfigDMCapability_ON() {
   void testAuthorPreviewMode() {
     DynamicMediaSupportService underTest = context.registerInjectActivateService(new DynamicMediaSupportServiceImpl(),
         "authorPreviewMode", true,
+        "validateSmartCropRenditionSizes", false,
         "imageSizeLimitWidth", 4000,
         "imageSizeLimitHeight", 3000);
     assertTrue(underTest.isDynamicMediaEnabled());
+    assertFalse(underTest.isValidateSmartCropRenditionSizes());
     assertEquals(new Dimension(4000, 3000), underTest.getImageSizeLimit());
     assertEquals("https://author.dummysite.org", underTest.getDynamicMediaServerUrl(asset, null, context.request()));
   }
@@ -216,7 +219,7 @@ void testGetImageProfileForAsset_MultipleFolders_IncompleteFolders() {
 
   @ParameterizedTest
   @MethodSource("forcePublicshUrlModes")
-  void testAuthorPreviewMode_SiteConfig_FourcePublish(UrlMode urlMode) {
+  void testAuthorPreviewMode_SiteConfig_ForcePublish(UrlMode urlMode) {
     MockCAConfig.contextPathStrategyAbsoluteParent(context, 1);
     MockContextAwareConfig.writeConfiguration(context, "/content/dam", SiteConfig.class,
         "siteUrlAuthor", "https://author");

diff --git a/src/test/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/SmartCropTest.java b/src/test/java/io/wcm/handler/mediasource/dam/impl/dynamicmedia/SmartCropTest.java
@@ -117,6 +117,30 @@ void testGetCropDimensionForAsset() {
     assertEquals(50, cropDimension.getHeight());
   }
 
+  @Test
+  void testGetCropDimensionForAsset_WidthDeviation() {
+    prepareSmartCropRendition(0.1, 0.2, 0.75, 0.5); // results in 120x50 cropping area, treated as 80x50
+    CropDimension cropDimension = getCropDimensionForAsset(asset, context.resourceResolver(), dimension16_10);
+
+    assertNotNull(cropDimension);
+    assertEquals(16, cropDimension.getLeft());
+    assertEquals(20, cropDimension.getTop());
+    assertEquals(80, cropDimension.getWidth());
+    assertEquals(50, cropDimension.getHeight());
+  }
+
+  @Test
+  void testGetCropDimensionForAsset_HeightDeviation() {
+    prepareSmartCropRendition(0.1, 0.2, 0.5, 0.75); // results in 80x75 cropping area, treated as 80x50
+    CropDimension cropDimension = getCropDimensionForAsset(asset, context.resourceResolver(), dimension16_10);
+
+    assertNotNull(cropDimension);
+    assertEquals(16, cropDimension.getLeft());
+    assertEquals(20, cropDimension.getTop());
+    assertEquals(80, cropDimension.getWidth());
+    assertEquals(50, cropDimension.getHeight());
+  }
+
   @Test
   void testIsMatchingSize_NoRenditionResource() {
     // assume everything is ok if no "16-10" rendition exists (we have no other chance)
@@ -141,6 +165,42 @@ void testIsMatchingSize_TooSmall() {
     assertFalse(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 120, 75));
   }
 
+  @Test
+  void testIsMatchingSize_MatchesExact_WidthDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 120x50 cropping area, treated as 80x50
+    assertTrue(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 80, 50));
+  }
+
+  @Test
+  void testIsMatchingSize_MatchesSmaller_WidthDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 120x50 cropping area, treated as 80x50
+    assertTrue(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 40, 25));
+  }
+
+  @Test
+  void testIsMatchingSize_TooSmall_WidthDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 120x50 cropping area, treated as 80x50
+    assertFalse(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 120, 75));
+  }
+
+  @Test
+  void testIsMatchingSize_MatchesExact_HeightDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 80x75 cropping area, treated as 80x50
+    assertTrue(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 80, 50));
+  }
+
+  @Test
+  void testIsMatchingSize_MatchesSmaller_HeightDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 80x75 cropping area, treated as 80x50
+    assertTrue(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 40, 25));
+  }
+
+  @Test
+  void testIsMatchingSize_TooSmall_HeightDeviation() {
+    prepareSmartCropRendition(0, 0, 0.75, 0.5); // results in 80x75 cropping area, treated as 80x50
+    assertFalse(isMatchingSize(asset, context.resourceResolver(), dimension16_10, 120, 75));
+  }
+
   @Test
   void testIsMatchingSize_InvalidNormalizedWidthHeight() {
     prepareSmartCropRendition(0, 0, 0, 0);