Basically that my fault 
I did the change how thumbnail store the configuration, but not for the other part of the image handling. I introduce the ThumbnailConfiguration during the same change, to cleanup method signature in our code base.
From my POV this RFC is not clear.
to always store the configuration of the ajustement as JSON (how it’s not done for the Thumbnail)
to introduce ThumbnailPreset… until we are sure about the usefulness of this
Question:
Currently we have ThumbnailGenerator that can be extended, why not using this in your case ? If not possible, what’s missing to make it possible ?
1 Like
I think to clear things up I start from the bottom and move upwards.
My primary problem with implementing a custom ThumbnailGenerator is the interface declaration: public function refresh(Thumbnail $thumbnail). The information on how to refresh the thumbnail is stored in the ThumbnailConfiguration class which provides a static set of properties. For adding a watermark to the thumbnail, I need additional properties (like a reference to the watermark image). So I’d have to extend the ThumbnailConfiguration as well and somehow make Media construct all Thumbnails using this extended configuration model. How would I do that (without AOP ;))? All of this feels overly cumbersome.
My secondary problem is, that the ThumbnailConfiguration is extremely similar to the ResizeImageAdjustment, which in some occasions it is even directly translated to. That leads to the idea to transform the ThumbnailConfiguration from its current, static state into a collection of ImageAdjustments, which is much more easily extendable / configurable. The term ThumbnailPreset therefore was rather there to replace the term ThumbnailConfiguration than to implement something completely new.
I see some issues regarding document thumbnails here, since the thumbnails are not generated from an image via Adjustments, at least not directly. Maybe that would be a nice place for the Generators who first create a preview image and then apply the Adjustments? Just an idea…
I understant your point, but I have the feeling that introducing a concept like ThumbnailPreset overlad with the current ThumbnailGenerator. Or at least ThumbnailPreset is a workaround for current limitation of the Generators. I prefer some improvement regarding the current Thumbnail generator that yet an other way to extend the system.
ThumbnailConfiguration is a DTO, so it just transfer value
Adjustments, make Adjustments / modify a given image
The ThumbnailConfiguration look like ResizeImageAdjustment because it’s the most used Adjustment currently. Before the rewritte of the Resource Manager (@robert, @christianm) the Adjustments was not extendable / customizable. But I agree that the current API must be improved in this area, the question now, it how to do this correctly without create more mess / inconsistencies.
Maybe @aertmann have some ideas in this area
1 Like
I don’t know your complete requirement, but use can use settings to provide more global configuration (like the watermark image) and apply the watermark based on some constraints on the image (width / height).
I do agree with the problems you mention and it would be good to improve it. Especially the static thumbnail configuration, which I found annoying/in the way as well. I don’t really have a good idea or good enough overview to provide a better solution though. Also I agree to all the good points @dfeyer is making.
My question is, taking the use case you have with watermarks, where would you want to add those? An argument in a view helper? A thumbnail preset? A general image post processing? Or maybe another way?
Because making thumbnail configuration extendable points towards creating your own thumbnail generator for images, but does the thumbnail need to know specific watermark information or could the generator just do that?
I think I should elaborate a bit (sorry, wall of text incoming)
I’d like to use the thumbnail presets defined in the configuration to determine which image to use as a watermark and how. Think about having a large image for your gallery as well as the actual thumbnail. Both need watermarks to be applied, but in a different way. Also, in said project there are literally thousands of images in large galleries, which makes using asynchronous thumbnail generation via CLI very desirable.
A first step would be to define the configuration (example only):
TYPO3:
Media:
thumbnailPresets:
'Vendor.ProjectTld:GalleryAsset':
maximumWidth: 960
maximumHeight: 540
allowUpScaling: TRUE
allowCropping: TRUE
watermark: 'resource://Vendor.ProjectTld/Public/Images/Watermarks/GalleryAssetWatermark.png'
watermarkHorizontalPosition: 200
watermarkHorizontalAlignment: 'left'
watermarkMaximumWith: 50
'Vendor.ProjectTld:MediaLibraryThumbnail':
maximumWidth: 203
maximumHeight: 136
allowUpScaling: TRUE
allowCropping: TRUE
watermark: 'resource://Vendor.ProjectTld/Public/Images/Watermarks/MediaLibraryThumbnailWatermark.png'
watermarkHorizontalPosition: 100
watermarkHorizontalAlignment: 'left'
watermarkMaximumWidth: 50
(This already looks like it could be improved by another level of hierarchy as the watermark prefix becomes necessary)
As for the implementation part, I’d like to use ImageAdjustments for applying the watermarks, for several reasons:
- Well, applying a watermark to an image simply is an image adjustment. Also, image adjustments are a very nice extension point
- The idea of using watermark adjustments for creating image variants, e.g. in the inspector - is not that far-fetched, is it?
- I don’t like the idea of writing a custom
ImageThumbnailGenerator, injecting custom settings and implementing complex business logic there. It feels like programming around the framework instead of with it.
The latter point needs some further explanation. Under the impression I get from looking at the interface structure and the implementation of the ThumbnailGenerators, it seems to me like they are designed to handle different document types (Image vs. Font vs. Text document etc.). Distinguishing between an ImageWithWatermarkThumbnailGenerator and an ImageWithoutWatermarkThumbnailGenerator simply seems out of scope. Please correct me if I got it wrong there, though.
The regular ImageThumbnailGenerator could easily cope with all my requirements - if I could just pass a preconfigured set of ImageAdjustments to it. The generator then can relay the rendering completely to those adjustments and just return the result.
The respective configuration could look like this:
'Vendor.ProjectTld:GalleryAsset':
adjustments:
resize:
type: 'Neos.Media:ResizeImage' # We could use an ImageAdjustment resolver similar to validators here
position: 'start' # Positional array sorter 4tw
options:
maximumWidth: 960
maximumHeight: 540
allowUpScaling: TRUE
allowCropping: TRUE
watermark:
type: 'Neos.Media:Watermark'
position: 'after resize'
options:
watermark: 'resource://Vendor.ProjectTld/Public/Images/Watermarks/GalleryAssetWatermark.png'
horizontalPosition: 200
horizontalAlignment: 'left'
maximumWith: 50
We still have to resolve the issue of ThumbnailConfiguration being static because otherwise none of this would ever reach the generator.
First, I see that the presets must be able to provide certain properties to generators that cannot rely on ImageAdjustments (e.g. the DocumentThumbnailGenerator), so there definitely are properties other than the adjustment array. But that should not be a problem.
Second, if we want to avoid the implementation of a ThumbnailPreset model, the ThumbnailConfiguration must become more flexible or learn how to manage ImageAdjustments to pass them on to the generator.
At this point, before I go deeper into how I’d expect an improved ImageThumbnailGenerator to work, I have another question. The ThumbnailConfiguration is modeled as a DTO, which means it must be restricted to primitive properties and should not contain too much business logic. Is there any particular reason why the DTO pattern was chosen? Is there any external context to be fed with the TC’s data? I ask this because refactoring this might become unnecessarily restricted by keeping the DTO pattern.
Thanks for elaborating, that’s makes it more clear.
I don’t see anything wrong in creating a WatermarkThumbnailGenerator to solve your specific watermarking needs.
That aside, the proposal you’re making would certainly be powerful. But it starts becoming much more than a thumbnail generator with thumbnail presets, as in more like a asset post processor and asset manipulation configurations/presets. So not sure if the current makes sense to build upon the existing solution instead of thinking of what the ideal solution would be and implement that in a way that the current stuff can be migrated to.
Regarding the DTOs, I’m not sure, but I think it was due to having a way to determine if a thumbnail already existed by being able to hash the configuration. Maybe @christianm knows.