-
Notifications
You must be signed in to change notification settings - Fork 765
Issue Fix for #2958 Transparent backgrounds on PNG images lost in dynamic media Urls #2978
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
base: main
Are you sure you want to change the base?
Conversation
…namic media URLs
vladbailescu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@khanmaroof , thank you for your contribution, this a really nice improvement!
I'm a bit concerned about usage of a non-public property from DAM (@actinium15 might be able to confirm this is the way forward).
I also have a few small comments around avoiding duplicating code.
| * @param asset The DAM asset to check | ||
| * @return true if the PNG has transparency (32 bits), false otherwise | ||
| */ | ||
| private boolean hasPngTransparency(Asset asset) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Made the
hasPngTransparency()methodprotectedinImageImpl.java(v2) - Removed the duplicate implementation from
ImageImpl.java(v3) - v3 now properly inherits the PNG transparency detection logic from v2
This maintains backward compatibility while reducing code duplication as you suggested.
| } | ||
|
|
||
| // Auto-preserve PNG transparency if enabled and asset has transparency | ||
| boolean autoPreservePngTransparency = currentStyle.get(PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY, false); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 (
autoPreservePngTransparencysetting) and transparency detection - Is
protectedso 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.
| // Auto-preserve PNG transparency if enabled and asset has transparency | ||
| boolean autoPreservePngTransparency = currentStyle.get(PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY, false); | ||
| if (autoPreservePngTransparency && hasPngTransparency(asset)) { | ||
| String pngAlphaModifier = "fmt=png-alpha"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| String mimeType = asset.getMimeType(); | ||
| if (!"image/png".equals(mimeType)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we use a constant for the image/png, like https://developer.adobe.com/experience-manager/reference-materials/6-5/javadoc/com/day/cq/dam/commons/handler/StandardImageHandler.html#PNG1_MIMETYPE
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally dam:Bitsperpixel would be a constant in https://developer.adobe.com/experience-manager/reference-materials/6-5/javadoc/com/day/cq/dam/api/DamConstants.html 🤷♂️
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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:
- Added
DAM_BITS_PER_PIXEL = "dam:Bitsperpixel"as a constant in theImageinterface - Added a comment explaining that this should ideally be in
DamConstants - 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.
There was a problem hiding this comment.
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?
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:Bitsperpixelmetadata 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.
| /** | ||
| * Auto-preserve PNG transparency configuration property. | ||
| */ | ||
| private static final String PN_DESIGN_AUTO_PRESERVE_PNG_TRANSPARENCY = "autoPreservePngTransparency"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- Moving PNG transparency checks into the convenience method
- Reducing variable scope where possible within the transparency detection logic
- Keeping the
assetvariable 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.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
- Add constants to Image interface (PNG_ALPHA_FORMAT_MODIFIER, DAM_BITS_PER_PIXEL) - Use StandardImageHandler.PNG1_MIMETYPE instead of hardcoded string - Eliminate code duplication between v2/v3 by making methods protected - Create shouldApplyPngAlphaModifier() convenience method - Fix test method visibility with proper @OverRide - Add debug logging for missing metadata Addresses code quality concerns while maintaining backward compatibility.
|



Fixes #2958Conditional PNG Transparency Detection
Rather than applying
fmt=png-alphato all PNGs (which would cause significant performance issues), we can automatically detect which PNGs actually have transparency using existing DAM metadata.Key Insight:
dam:Bitsperpixelmetadata reliably indicates PNG transparency:32bits = PNG with alpha channel (transparent) → Addfmt=png-alpha24bits = RGB PNG (opaque) → No format modifier needed8bits = Indexed PNG (opaque) → No format modifier neededImplementation: Modify the Dynamic Media URL building logic in
ImageImpl.javato automatically prependfmt=png-alphaonly whendam:Bitsperpixel = 32for PNG assets.Benefits:
autoPreservePngTransparencysettingThis approach solves the transparency issue while completely avoiding the performance penalty since
fmt=png-alphais only applied when transparency actually exists.