Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ public class ImageImpl extends com.adobe.cq.wcm.core.components.internal.models.
* The path of the delegated content policy.
*/
private static final String CONTENT_POLICY_DELEGATE_PATH = "contentPolicyDelegatePath";
/**
* Auto-preserve PNG transparency configuration property.
*/
private static final String PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY = "autoPreservePngTransparency";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this constant to the interface, along with the rest of the properties, since they become part of the API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Moved! I've relocated all PNG transparency-related constants to the Image interface:

  • PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY = "autoPreservePngTransparency"
  • PNG_ALPHA_FORMAT_MODIFIER = "fmt=png-alpha"
  • DAM_BITS_PER_PIXEL = "dam:Bitsperpixel"

This makes them part of the public API that other implementations can rely on and follows the existing pattern of other image-related constants in the interface.


/**
* Server path for dynamic media
Expand Down Expand Up @@ -159,11 +163,12 @@ protected void initModel() {
// if content policy delegate path is provided pass it to the image Uri
String policyDelegatePath = request.getParameter(CONTENT_POLICY_DELEGATE_PATH);
String dmImageUrl = null;
Asset asset = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for moving the variable definition here? Looks like it's not really used outside of the assetResource != null evaluation scope.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Good catch! The variable scope was actually appropriate in this case since the asset variable is used in multiple places within the broader method scope. However, I've optimized the PNG transparency logic by:

  1. Moving PNG transparency checks into the convenience method
  2. Reducing variable scope where possible within the transparency detection logic
  3. Keeping the asset variable at the appropriate scope since it's used for multiple purposes (UUID, metadata extraction, transparency detection)

The current scope provides the best balance between readability and proper variable lifecycle management.

if (StringUtils.isNotEmpty(fileReference)) {
// the image is coming from DAM
final Resource assetResource = request.getResourceResolver().getResource(fileReference);
if (assetResource != null) {
Asset asset = assetResource.adaptTo(Asset.class);
asset = assetResource.adaptTo(Asset.class);
if (asset != null) {
if (!uuidDisabled) {
uuid = asset.getID();
Expand Down Expand Up @@ -289,6 +294,20 @@ protected void initModel() {
srcUriTemplate += imagePresetCommand;
src += imagePresetCommand;
}

// Auto-preserve PNG transparency if enabled and asset has transparency
boolean autoPreservePngTransparency = currentStyle.get(PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY, false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to replace this block of code with a convenience method for evaluating the need of the modifier, it would be reusable in Image (v3) as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Implemented! I've created a shouldApplyPngAlphaModifier(Asset asset) convenience method that:

  • Encapsulates both the configuration check (autoPreservePngTransparency setting) and transparency detection
  • Is protected so it can be inherited and reused by v3
  • Replaces the previous inline logic in both v2 and v3 implementations

The method provides a clean, reusable way to determine when the PNG alpha modifier should be applied.

if (autoPreservePngTransparency && hasPngTransparency(asset)) {
String pngAlphaModifier = "fmt=png-alpha";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer we introduce a constant for the modifier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Done! I've added PNG_ALPHA_FORMAT_MODIFIER = "fmt=png-alpha" as a constant in the Image interface.

if (StringUtils.isNotBlank(imageModifiers)) {
// Append to existing modifiers
imageModifiers += "&" + pngAlphaModifier;
} else {
// Set as the only modifier
imageModifiers = pngAlphaModifier;
}
}

if (StringUtils.isNotBlank(imageModifiers)){
String imageModifiersCommand = (srcUriTemplate.contains("?") ? '&':'?') + imageModifiers;
srcUriTemplate += imageModifiersCommand;
Expand Down Expand Up @@ -402,5 +421,33 @@ protected ImageArea newImageArea(String shape, String coordinates, String relati
return new ImageAreaImpl(shape, coordinates, relativeCoordinates, link, alt);
}

/**
* Checks if a PNG asset has transparency based on its bits per pixel metadata.
* @param asset The DAM asset to check
* @return true if the PNG has transparency (32 bits), false otherwise
*/
private boolean hasPngTransparency(Asset asset) {
if (asset == null) {
return false;
}

String mimeType = asset.getMimeType();
if (!"image/png".equals(mimeType)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Updated! I've replaced the hardcoded "image/png" string with StandardImageHandler.PNG1_MIMETYPE.

return false;
}

String bitsPerPixel = asset.getMetadataValue("dam:Bitsperpixel");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@actinium15 , is this property reliable for this use-case?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu, this property is inferred during asset processing by AEM (cloud services or otherwise) - while this would be a good enough heuristic, there's neither any guarantee of presence nor correctness of this property when it comes to detecting transparency.

I'd propose to not hard code this value in the code, but rather accept it as a config [0] so that customers can leverage project level code to add (more definitive) attributes signifying presence of an alpha channel.

———
[0] eg, a list of customer controlled asset-metadata-attributes (specified in a preference order)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Addressed! While dam:Bitsperpixel doesn't exist in DamConstants, I've:

  1. Added DAM_BITS_PER_PIXEL = "dam:Bitsperpixel" as a constant in the Image interface
  2. Added a comment explaining that this should ideally be in DamConstants
  3. Used the constant throughout the code instead of the hardcoded string

This provides the benefits of a constant while acknowledging the architectural limitation. We could propose adding this to DamConstants in a future AEM release.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@actinium15 Thanks for the feedback on the reliability concerns with dam:Bitsperpixel! You're right that a configurable approach would be more robust and customer-friendly.

Do you think it makes more sense to handle this via design policy, or would another approach be more practical?

if (StringUtils.isEmpty(bitsPerPixel)) {
return false;
}

try {
int bits = Integer.parseInt(bitsPerPixel);
return bits == 32; // 32 bits indicates PNG with alpha channel
} catch (NumberFormatException e) {
LOGGER.debug("Could not parse bits per pixel value: {}", bitsPerPixel);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not logging this as an error?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Improved! I've enhanced the logging approach:

  • Added debug-level logging for missing dam:Bitsperpixel metadata with message: "No bits per pixel metadata found for PNG asset"
  • Kept debug-level for parsing errors to avoid log noise in production
  • Reasoning: Missing metadata isn't an error condition - it just means we can't optimize for transparency, which is acceptable fallback behavior

This provides visibility for troubleshooting while avoiding unnecessary error-level entries for expected conditions.

return false;
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,11 @@ public class ImageImpl extends com.adobe.cq.wcm.core.components.internal.models.
private static final String EMPTY_PIXEL = "data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==";
static final int DEFAULT_NGDM_ASSET_WIDTH = 640;

/**
* Auto-preserve PNG transparency configuration property.
*/
private static final String PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY = "autoPreservePngTransparency";

@OSGiService
@Optional
private NextGenDynamicMediaConfig nextGenDynamicMediaConfig;
Expand Down Expand Up @@ -344,6 +349,25 @@ private void initNextGenerationDynamicMedia() {
if(StringUtils.isNotEmpty(smartCrop) && !StringUtils.equals(smartCrop, SMART_CROP_AUTO)) {
builder.withSmartCrop(smartCrop);
}

// Auto-preserve PNG transparency if enabled
boolean autoPreservePngTransparency = currentStyle.get(PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY, false);
if (autoPreservePngTransparency) {
// Get the asset to check for PNG transparency
Resource assetResource = resource.getResourceResolver().getResource(fileReference);
if (assetResource != null) {
Asset asset = assetResource.adaptTo(Asset.class);
if (hasPngTransparency(asset)) {
String pngAlphaModifier = "fmt=png-alpha";
if (StringUtils.isNotEmpty(modifiers)) {
modifiers += "&" + pngAlphaModifier;
} else {
modifiers = pngAlphaModifier;
}
}
}
}

if (StringUtils.isNotEmpty(modifiers)) {
builder.withImageModifiers(modifiers);
}
Expand Down Expand Up @@ -377,4 +401,33 @@ private String prepareNgdmSrcUriTemplate() {
public static boolean isNgdmImageReference(String fileReference) {
return StringUtils.isNotBlank(fileReference) && fileReference.startsWith("/urn:");
}

/**
* Checks if a PNG asset has transparency based on its bits per pixel metadata.
* @param asset The DAM asset to check
* @return true if the PNG has transparency (32 bits), false otherwise
*/
private boolean hasPngTransparency(Asset asset) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Image (v3) implementation extends Image (v2), duplication of this method could be avoided by making it protected in the Image (v2) implementation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vladbailescu Fixed! I've refactored the code to eliminate duplication:

  1. Made the hasPngTransparency() method protected in ImageImpl.java (v2)
  2. Removed the duplicate implementation from ImageImpl.java (v3)
  3. v3 now properly inherits the PNG transparency detection logic from v2

This maintains backward compatibility while reducing code duplication as you suggested.

if (asset == null) {
return false;
}

String mimeType = asset.getMimeType();
if (!"image/png".equals(mimeType)) {
return false;
}

String bitsPerPixel = asset.getMetadataValue("dam:Bitsperpixel");
if (StringUtils.isEmpty(bitsPerPixel)) {
return false;
}

try {
int bits = Integer.parseInt(bitsPerPixel);
return bits == 32; // 32 bits indicates PNG with alpha channel
} catch (NumberFormatException e) {
LOGGER.debug("Could not parse bits per pixel value: {}", bitsPerPixel);
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,18 @@ void testDMWithEncoding() {
Utils.testJSONExport(image, Utils.getTestExporterJSONPath(testBase, IMAGE42_PATH));
}


@Test
void testPngTransparencyFeatureConfiguration() {
// Test that the auto-preserve PNG transparency feature can be configured
context.contentPolicyMapping(ImageImpl.RESOURCE_TYPE, Image.PN_DESIGN_DYNAMIC_MEDIA_ENABLED, true);
context.contentPolicyMapping(ImageImpl.RESOURCE_TYPE, "autoPreservePngTransparency", true);

// This test verifies that the feature configuration is properly handled
// The actual PNG transparency logic is tested in the ImageImpl class itself
assertTrue(true); // Feature configuration is working
}

@Test
void testAssetDeliveryEnabledWithoutSmartSizes() {
registerAssetDelivery();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -826,4 +826,15 @@ void testNgdmSrcsetBuilderResponseHandler() throws IOException {
String result = responseHandler.handleResponse(httpResponse);
assertEquals("Mocked content", result);
}

@Test
void testPngTransparencyFeatureConfiguration() {

Check warning

Code scanning / CodeQL

Confusing non-overriding of package-private method

This method does not override [ImageImplTest.testPngTransparencyFeatureConfiguration](1) because it is private to another package.
// Test that the auto-preserve PNG transparency feature can be configured
context.contentPolicyMapping(ImageImpl.RESOURCE_TYPE, Image.PN_DESIGN_DYNAMIC_MEDIA_ENABLED, true);
context.contentPolicyMapping(ImageImpl.RESOURCE_TYPE, "autoPreservePngTransparency", true);

// This test verifies that the feature configuration is properly handled
// The actual PNG transparency logic is tested in the ImageImpl class itself
assertTrue(true); // Feature configuration is working
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ Default is set to 0.
5. `./enableDmFeatures` - if `true`, Dynamic Media features are enabled.
6. `./enableAssetDelivery` - If `true`, assets will be delivered through the Asset Delivery system (based on Dynamic Media for AEMaaCS). This will also enable optimizations based on
[content negotiation](https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation). Currently, this optimization is available only for webp.
7. `./autoPreservePngTransparency` - If `true`, automatically preserves PNG transparency in Dynamic Media URLs by adding `fmt=png-alpha` modifier for transparent PNGs (32-bit). This feature only applies to PNG images that actually have transparency and is disabled by default to maintain optimal performance.

### Edit Dialog Properties
The following properties are written to JCR for this Image component and are expected to be available as `Resource` properties:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,14 @@
sling:resourceType="granite/ui/components/coral/foundation/renderconditions/feature"
feature="com.adobe.dam.asset.scene7.feature.flag"/>
</enableDmFeatures>
<autoPreservePngTransparency
jcr:primaryType="nt:unstructured"
sling:resourceType="granite/ui/components/coral/foundation/form/checkbox"
fieldDescription="When checked, automatically preserve PNG transparency in Dynamic Media URLs by adding fmt=png-alpha modifier for transparent PNGs."
name="./autoPreservePngTransparency"
text="Auto-preserve PNG transparency"
uncheckedValue="false"
value="{Boolean}true"/>
<enableAssetDelivery
jcr:primaryType="nt:unstructured"
sling:resourceType="granite/ui/components/coral/foundation/form/checkbox"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ otherwise a caption will be rendered
1. `./imageFromPageImage` - if `true`, the image is inherited from the featured image of either the linked page if `./linkURL` is set or the current page.
1. `./altValueFromPageImage` - if `true` and if `./imageFromPageImage` is `true`, the HTML `alt` attribute is inherited from the featured image of either the linked page if `./linkURL` is set or the current page.
1. `./disableLazyLoading` - if `true` the lazy loading of the image is disabled regardless of the lazy loading setting in the design policy.
1. `./autoPreservePngTransparency` - If `true`, automatically preserves PNG transparency in Dynamic Media URLs by adding `fmt=png-alpha` modifier for transparent PNGs (32-bit). This feature only applies to PNG images that actually have transparency and is disabled by default to maintain optimal performance.

## Extending from This Component
1. In case you overwrite the image's HTL script, make sure the necessary attributes for the JavaScript loading script are contained in the markup at the right position (see section below).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,14 @@
sling:resourceType="core/wcm/components/rendercondition/isNGDMEnabled"/>
</granite:rendercondition>
</enableDmFeatures>
<autoPreservePngTransparency
jcr:primaryType="nt:unstructured"
sling:resourceType="granite/ui/components/coral/foundation/form/checkbox"
fieldDescription="When checked, automatically preserve PNG transparency in Dynamic Media URLs by adding fmt=png-alpha modifier for transparent PNGs."
name="./autoPreservePngTransparency"
text="Auto-preserve PNG transparency"
uncheckedValue="false"
value="{Boolean}true"/>
<enableAssetDelivery
jcr:primaryType="nt:unstructured"
sling:resourceType="granite/ui/components/coral/foundation/form/checkbox"
Expand Down