Named Transform Image Servlet

[davidjgonzalez]

Kinda of like the AEM Adaptive Image Servlet, except on bovine growth hormones.

https://github.com/davidjgonzalez/acs-aem-commons/tree/feature/named-transform-image-servlet

Inspired by previous discussions in Issue #24, with @ykisen and @justinedelson around non-destructive renditioning of Images. Note, this approach does NOT support authorability of renditions.

Allows developers to create transform rule sets (resize/crop/rotate - in whichever order) and apply the named transforms against (almost?) any image in AEM. Works against:

• cq:Pages (jcr:content/image)
• Image component resources
• DAM Assets (/content/dam/foo.png)
• DAM Asset Renditions (/content/dam/foo.png/jcr:content/renditions/whatever.jpg)
• Binary Images (/etc/designs/mysite/images/foo.png)

Requesting URIs require a "transform" extension, the first suffix segment is the named transform and the last suffix segment is "image." or "img." (to promote cacheability).

Current all transforms are maintained in a single OSGi Config; debating trying to break it out to allow "sets" of named transforms via configFactory configs.

[justinedelson]

makes sense. I would suggest making each configuration a separate factory component. These components should implement a simple interface like

public interface ImageTransformer {
     Layer transform(Layer);
}

We should provide (at least) one implementation of ImageTransformer which handles the operations you have here. But this shouldn't preclude downstream users from implemeting their own ImageTransformers to add other transformations. For example, someone might want a B&W transformer.

Perhaps YAGNI, but it'd be furthermore slick if you could have a simple pipeline of transformers just by registering them with the same name, ordered by service.ranking.

Two other comments:

• Should the resource types really be configurable? I imagine that adding a new resource type doesn't actually work. Or is the idea that this is configurable to allow the administrator to remove resource types?
• I keep forgetting to mention this - what's up with prefixing the property names with prop? This seems unnecessary to me (and is definitely not idiomatic)

[davidjgonzalez]

I had left them as configurable because anything that subRTs the parbase or copies gown the .img servlet is targetable. removing is less of a concern since if these resources are accessible who cares if they get transformed (famous last words). I'd almost like making that list a separate private property and have another list for any custom RTs)

I just like the way 'prop.' looks. It's very clear (to me) what it is and represents at a glance. I can stop using it if its bothersome.

No idea what YAGNI means, but I get worried when discrete configs combine combine for overall effect; it's easy to lose track. I'd rather have a 1-to-1 between a namedTransform and it's config.something like:

• transform-name: feature
• transforms: [ basic:/width/200/rotate/35 bw:/greys ]

WDYT (or did I just restate what you suggested - seems like we need 1 servlet, N NamedTransfomers and M ImageTransformers)?

[justinedelson]

YAGNI = You Ain't Gonna Need It. See http://c2.com/cgi/wiki?YouArentGonnaNeedIt

I don't think having a separate NamedTransformer service is necessary and I think the config will be a lot cleaner with discreet properties.

• transformer-name = feature
• width = {Long}300
• rotate = {Long}35 etc.

The need for multiple transformer is probably only in edge cases. And if those come up, you could always use composition, i.e.

@Component
@Service
@Property(name="transform-name", value="feature-bw")
public class SpecialTransformer implements ImageTransformer {

     @Reference(filter="(transformer-name=feature)")
     private ImageTransformer featureTransformer;

     public Layer transform(Layer l) {
          l = featureTransformer.transform(l);
          return makeBlackAndWhite(l);
     }

}

[davidjgonzalez]

hmm... code-based composition (for common uses like resizing and turning to b&w) seem too involved. TBH, i have a hard time coming up w use cases for not wanting the size or crop transform; The 80% use case is to provide normalized image renditions for the web, and in web layout, dimensions (raw or aspect) are king.

Also - its important to allow the order of the rules to be specified (in the implemented transform logic, order is critical; resize before crop vs. crop before resize; This order effects the result).

Im still leaning toward a Transform config of:

• name = feature
• transforms = [basic:<params>, bw:<params> ]

Would you feel better if <params> were in a more comman format, aka JSONy?

basic={ "width: "300", "rotate": "35", "crop": "0,0,100,200" }

Edit: hmm - on 2nd throught, escaping the JSON in the XMl might be annoying. But something like that.

[justinedelson]

In this example, where does "bw" come from? Is this an implementation of the ImageTransformer service interface?

I guess I'm just having trouble understanding what is extensible and what isn't.

Also, I was using B&W conversion as something which wasn't common. If you think it is, then I need a different example of an image transformation which wouldn't be provided OOTB, but could be done through an extension :)

[davidjgonzalez]

I snuck in the extra layer :) ... so in that example:

basic = BasicImageTransformerImpl extends ImageTransformer (handles resize/crop/rotate) bw = BlackAndWhiteImageTransformerImpl extends ImageTransformer (handles greyscaling of images)

When a request for .transform/feature/image.png is received, the servlet looks for a NamedTransform Component with getName() == "feature", and calls namedTransform.transform(image) which applies all the ImageTransforms listed in its transforms, in order.

• 1 Servlet entry point
• 1 NamedTransform (Component Factory) w N Component Config Instances (ex. com.adobe.acs.commons.images.impl.NamedTransformImpl-feature)
• N ImageTransformer impls (this is where new image transforms would be developed; BasicImageTransformerImpl, BlackAndWhiteImageTransformerImpl, FooImageTransformImpl ... ).

[justinedelson]

In that case, does it even make sense for NamedTransformerImpl to be an OSGi component? Couldn't this just be a node structure, i.e.

/etc/acs-commons/transforms
                           /feature
                                   /basic
                                          @width=300
                                          @cropHeight=100
                                   /bw

The order of child nodes under the transform node is respected.

Also, slightly separate, but having a method called getName() isn't necessary. You should really use service properties for that.

[davidjgonzalez]

Hm - i dont know.

If authors should be allowed to configure them I could see the benefit of nodes; Personally, I prefer dealing w "simple" OSGi configs rather than maintaining content trees.

Maybe v1 starts w OSGi props, and v2 adds node-based authorable support?

You'll have to show me what you mean by the service.properties; you dont mean reference filters, right? (since these would be dynamic).

[justinedelson]

The key benefit is that it is a more expressive structure. See my rant here: http://markmail.org/message/yxxloiiymqklfhu6

I guess these are relatively simple name/value pairs, so maybe transformer-name:name=value&name2=value2 would be ok. That path-like syntax looks confusing to me (but I'll grant that these are minor details as this stuff isn't meant to be edited frequrently).

Regarding the service property, what I'm suggesting is that you do: @Component @Service @Property(name="transformer-name", value="basic", propertyPrivate=true) public void BasicTransformer implements ImageTransformer {

    public Layer transform(Layer l, ValueMap properties) {
    ...
    }

}

Rather than

@Component
@Service
public void BasicTransformer implements ImageTransformer {

    public String getName() {
       return "basic";
    }

    public Layer transform(Layer l, ValueMap properties) {
    ...
    }

}

The advantage of the former is that the property is available in map passed to the bind method without calling into the service object. It is also fewer lines of code. And it is the purpose of service properties. From the OSGi R4.2 Specification: "The service properties are intended to provide information about the service object." which is exactly what is going on here - the BasicTransformer doesn't actually care what its name is; that information is pure metadata to be consumed by other components (like how a servlet usually doesn't care what resource types it is registered under)

[davidjgonzalez]

Ah - yes; I was speaking to NamedTransformerImpl getting a list of ImageTransformers ..

So a getName() like so -- unless there is a cleaner way? We can discuss in the PR post refactor anyhow.

    @Component
    @Service
    @Property(name="transformer-name", value="basic", propertyPrivate=true)
    public void BasicTransformer implements ImageTransformer {

        public String getName() { 
          return this.name; 
        }

        public Layer transform(Layer l, ValueMap properties) {
        ...
        }

        @Activate
         protected void activate(Map<String, String> config) { 
              this.name = PropertiesUtil.getString(config.get("transform-name"), ""); 
         }
    }

---

Agree the "path based" scheme (/width/100/..) could be improved - that was a holdover from when the transform params were passed via the suffix (too powerful and too brittle).

I guess i don't really care where the config is stored; but i do think osgi configs have a lower barrier to entry for dev consumers.

You pick :)

[justinedelson]

You don't need the getName() method at all. We don't actually have a good example of a bind method which reads properties in ACS AEM Commons (all the more reason to do it here). I'll send you a private link.

Let's go with a query-string like syntax.

[davidjgonzalez]

k

[davidjgonzalez]

super ..or super creepy?

Last night I refactored the code per this discussion - and it works pretty well!

Still needs some code cleanup and tests but i think its more or less there.

@justinedelson I wonder if we could refactor your 2 DAM image transformers into these..? Or have a WF that leverages the NamedImageTransformer to create renditions. YAGNI? (...am i doing it right?)

[justinedelson]

That's a good question. It should be easy enough to build a bridge from the workflow API into the ImageTransformer API. This maybe could be genercized into an ImageTransformerRenditionModifyingProcess (name?) and then everything is done through configuration. Although we'd need to keep the existing processes for backwards compatibility reasons.

[justinedelson]

how's this going?

[davidjgonzalez]

Should be about done; Ill check tonight what my test coverage is like ..

I wanted to build the web UI for it, but we can v2 that i think.

[justinedelson]

ok. one question - what image formats are supported for output?

[davidjgonzalez]

@justinedelson currently it reads the mimetype from the source image in the JCR and uses that in layer.write(..) which AFIAK is used to by ImageIO to write out the image file.

It might be more apt to look at the last suffix element's extension, since that should logically match the actual image format returned. Else we could make a ImageTypeTransformer that allows a mimetype to be specified, but then you could end up serving a JPEG w a .png extension which is weird.

WDYT?

[justinedelson]

OK - so the input and output formats are the same? I think that's OK for now, but something to think about as a potential enhancement. I 100% agree that the extension and the returned content should always match.

[davidjgonzalez]

@justinedelson correct. the image type of the binary in CRX will match the transformed output.

There is an onus on the "caller" to ensure these match up, which TBH might not be possible. (Ex Page Image component could take JPG or PNG, but the caller may always (progammatically) call using <img src="<%= currentPage.getPath() %>.tranform/super/image.png"/>

[davidjgonzalez]

Just for posterity; code was added to return the image in the format as defined by ...transform/transform-name/image.whatever-this-is regardless of what the underlying binary image format it.

[davidjgonzalez]

docs added via f3dc75a25d6183881459afaeb112c24f848658ad