[butonic]

This PR tries to fix #1128 in a backwards compatible way.

Thumbnail generation used to live entirely in the thumbnails service behind a gRPC API: webdav called GetThumbnail, the service fetched the source from storage, preprocessed and generated the image, stored it on its own filesystem, and returned a JWT-signed URL that webdav had to follow with a second authenticated HTTP call just to get the bytes back. This branch inverts that: the thumbnails service becomes a stateless imagor-compatible resizer (one POST endpoint — image bytes in, resized bytes out; no auth, storage, or gRPC), and webdav owns the whole workflow via a single ThumbnailWorkflow type: stat via gateway → cache check → download source → preprocess → POST to generator → cache → respond stat via gateway → snap the requested size → cache check → download source → preprocess → POST to generator → cache → respond.

Webdav gains what it needs to own that pipeline: the preprocessors (text→image, audio cover art) moved over from the thumbnails service, a new checksum-keyed thumbnail cache with memory/file/S3/noop backends, and a pkg/generator package that builds resizer URLs and posts multipart images. Config shrinks to one generator URL plus timeout and max input file size — the URL can point at the built-in thumbnails service or any imagor instance.

The old architecture is deleted from the thumbnails service: proto files, gRPC handler, JWT transfer tokens, filesystem storage, source fetchers, the /data endpoint, and duplicated utilities (~4,000 lines), plus leftover dead config, no-op metrics wrappers, and opencloud init's now-meaningless transfer-secret generation. Net diff: 102 files, +3,337/−4,336 118 files, +6,863/−4,861; thumbnail requests no longer need the second round-trip, and the resizer is trivially replaceable.

Update: sizing and caching

THUMBNAILS_RESOLUTIONS is a ladder of edge lengths, not a list of boxes: each axis is rounded up on its own, so a square request can no longer land in a landscape box, which is what cropped and shrank portrait previews. Legacy WidthxHeight entries still count with both sides, and the cache is keyed on the snapped box, so x=447 and x=448 share an entry.

Clients no longer need to know the list: x and y are independently optional, an omitted axis is unconstrained and capped at the largest length, while an explicit 0 stays the 400 it has always been. A cropping processor plus an unconstrained axis is a 400, no dimensions at all stays the legacy 32x32. A public link that needs a password answers 404 again instead of 500.

Update: what a thumbnail is made of

Three settings, read as a chain. For a .heic on a deployment running a stock imagor:

image/heic                                                  the source type, from the file
OC_THUMBNAILS_SOURCE_TYPES           image/heic              a preview is announced and attempted
WEBDAV_THUMBNAILS_TYPE_MAPPING       image/heic:image/jpeg   the type it is returned in
WEBDAV_THUMBNAILS_GENERATOR_FORMATS  image/jpeg:jpg          what the generator calls that type

POST /unsafe/fit-in/256x256/filters:format(jpg)/

Each one merges onto the built-in defaults, so naming a type does not take the answer away from the others. A target the generator has no format for is refused at startup, a source it cannot read answers 404. image/heic and image/avif, both unsupported today, were verified end to end this way, no code change.

Update: parity suite

services/thumbnails/pkg/parity posts the same image to the built-in generator and to two real imagor images and compares status, dimensions, format, frames and alpha, with imagor from testcontainers locally and service containers in CI. A generator that answers differently is held to that answer by a recorded override carrying the reason, which fails once the divergence disappears.

It runs twice, once per resize implementation, because make test compiles the pure Go path while the images ship vips, so the shipped resize code had no test at all. That exposed an older problem, not fixed here: CI's libvips is not the one the images ship, so enable_vips tests measure capabilities we do not deliver (#3746).

Follow-ups

• oc:has-preview is answered from the same configured list, which needed a change in reva: fix(ocdav): answer has-preview from the ocdav configuration reva#885. ocdav takes the types from its own configuration instead of importing an OpenCloud package, and the frontend hands in what webdav attempts. The vendored copy carries it until that PR lands. It also removes the import that makes a module cycle, reva's go.mod requires opencloud v1.1.0, so a standalone reva announced previews from a frozen OpenCloud list. The requirement itself stays, pkg/app/registry/micro and pkg/micro/ocdav still use opencloud/pkg/registry.
• image/jpg cannot occur, reva maps jpg to image/jpeg. Remove it from the supported list and the encode step if nothing outside the repo depends on it.
• Turning previews off for a single type has no lever; the gate is the source type list, read by webdav, graph and reva.
• Extracted previews (an audio cover, a rendered text page) are not cached themselves, only the thumbnail made from them, so a second size extracts again. @butonic wanted them cached, @dschmidt at index time (feat(search): honest has-preview prop and thumbnails relationship #3210).
• Pages are still unbounded. THUMBNAILS_MAX_ANIMATION_FRAMES now caps a gif from its block structure before a frame is decoded, but THUMBNAILS_MAX_INPUT_WIDTH and _HEIGHT describe one page, so a multi-page heic or tiff is bounded only by both backends loading a single page today.
• S3Cache has two gaps the unit tests cannot see, because the mock is not minio. Secure: true is hard-wired in BuildS3CacheConfig, and minio.New prepends the scheme itself, so the http://host:port form the config text asks for is refused as an invalid endpoint. Deriving the scheme from the endpoint and handing minio a bare host fixes both, and a real MinIO behind testcontainers (the imagortest pattern) is what should pin it.
• ThumbnailCache.Get takes no context, so the s3 backend reads on context.Background() and ignores the request deadline.
• One http.Client carrying WEBDAV_THUMBNAILS_GENERATOR_TIMEOUT is shared by the generator POST and the source download, so the generator timeout silently doubles as the download timeout for a source of up to WEBDAV_THUMBNAILS_MAX_INPUT_IMAGE_FILE_SIZE.
• FontLoader is rebuilt for every text preview, because preprocessor.ForType runs per request.
• A thumbnail response carries no ETag and no Cache-Control, although the cache key (checksum, box, operation, format) is a content-addressed ETag already.
• The push endpoint's HTTP behaviour is asserted twice, in push_test.go and in the parity suite. A parity case runs the built-in generator in-process, so it needs no container and compares imagor on top; the service specs should keep only what a non-default config decides (the concurrency limiter, the input limits) and the helper unit tests.
• Without processor and without a, the default is now fill with upscaling, where main capped the result at the source size; previewsAutoAdustedSizing.feature carries the new numbers. Web always sends a processor, so this is visible only to a client that sends a bare x/y.
• THUMBNAILS_DATA_ENDPOINT and THUMBNAILS_TRANSFER_TOKEN are gone without a deprecation path: they configured the pull pipeline this PR removes, so there is nothing to point them at. THUMBNAILS_FILESYSTEMSTORAGE_ROOT is still read as a fallback for WEBDAV_THUMBNAILS_CACHE_DIR and marked deprecated.

Related:
#630
#3364
#3332
opencloud-eu/reva#781
opencloud-eu/reva#773
owncloud/core#31268 - discusses a and mode parameters
Thumbnails lessons from oc10 #1378

Discussion:
https://github.com/orgs/opencloud-eu/discussions/2368
https://github.com/orgs/opencloud-eu/discussions/1090

@dschmidt This PR is not ready, but I want to bring your attention to this approach, which is why I am pushing this code now.

Taken over from @butonic with his and the team's agreement, and rebased onto current main. The sizing and caching changes described above are mine. ~ @dschmidt

Follow-up after this lands

OC_DISABLE_PREVIEWS switches off every preview path in webdav, but nothing stops the announcement: the graph thumbnails relationship and ocdav's oc:has-preview keep saying a preview exists, so clients go on asking and get 404. That is the state on main as well, both only read the mime list, so it is not a regression here, and it is the smaller half of answering has-preview honestly.

[codacy-production]

Not up to standards ⛔

🔴 Issues 12 critical · 37 minor

Alerts:
⚠ 49 issues (≤ 0 issues of at least minor severity)

Results:
49 new issues

Category	Results
Security	12 critical
CodeStyle	37 minor

View in Codacy

🟢 Metrics 229 complexity · 312 duplication

Metric	Results
Complexity	229
Duplication	312

View in Codacy

🟢 Coverage 65.15% diff coverage

Metric	Results
Coverage variation	Report missing for 5a7afe01
Diff coverage	✅ 65.15% diff coverage

View coverage diff in Codacy

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (5a7afe0)	Report Missing	Report Missing	Report Missing
Head commit (9eb1bcd)	85287	20874	24.48%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#3397)	1145	746	65.15%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

¹ Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[dschmidt]

How do audio artwork, geogebra thumbnail and tiff preview extraction fit into this approach?

Have you seen that I reworked the tiff extraction pr to use tika?

I already upstreamed audio artwork extraction and tiff extraction to tika. Geogebra is still pending, but I expect it to be merged very soon as well.

So I would like to see a preprocessing step for extraction of embedded pictures before they are handed over to imagor for resizing (with tika it would be push based and out of process too).

I was a bit surprised to see that you are moving so much into the webdav service, my expectation would have been that you just replace the Decoder in the thumbnails service - can you elaborate a little why you chose this way more invasive approach?

[butonic]

How do audio artwork, geogebra thumbnail and tiff preview extraction fit into this approach?
The preprocessing pipeline moved to the webdav service. I would add the extraction there as well.

Have you seen that I reworked the tiff extraction pr to use tika?
Yes, which is why I pinged you in this PR, because I think using a sandboxable thumbnailer this architecture allows is crucial for security.

I already upstreamed audio artwork extraction and tiff extraction to tika. Geogebra is still pending, but I expect it to be merged very soon as well.

So I would like to see a preprocessing step for extraction of embedded pictures before they are handed over to imagor for resizing (with tika it would be push based and out of process too).
I tried to move the preprocesor package from thumbnailer to webdav as is. More extractors should be added here, or did you have to refactor a lot for your extractors? In any case that should work for this approach as well.

I was a bit surprised to see that you are moving so much into the webdav service, my expectation would have been that you just replace the Decoder in the thumbnails service - can you elaborate a little why you chose this way more invasive approach?
Sure. On main, webdav doesn't own the pipeline at all: each thumbnail request is a grpc GetThumbnail call where the thumbnails service does stat, cache check, source fetch, preprocess (the decoder), generate, and disk storage for caching, then returns a JWT-signed download URL that webdav has to make a second HTTP call to just retrieve the bytes. So swapping only the decoder would leave that grpc hop, the JWT endpoint, the storage layer, and the double-fetch intact, fixing how a file becomes an image but not where the pipeline runs or the extra indirection.

We can move the workflow into webdav, because webdav already has everything it needs locally (the CS3 gateway client for stat + download, the auth context, the response writer), which lets the generator shrink to a stateless resizer (POST bytes in, resized bytes out) that's swappable with an external imagor via a single URL. The thumbnail generation process is owned, end-to-end, by webdav instead of logic split across two services and two round-trips.

And we can use an external container to support HEIC thumbnails, just by changing the URL. And I really want to be able to reuse the thumbnailer like tika, because it now becomes stateless.

[dschmidt]

How do audio artwork, geogebra thumbnail and tiff preview extraction fit into this approach?
The preprocessing pipeline moved to the webdav service. I would add the extraction there as well.

Okay, alright. Works for me.

Have you seen that I reworked the tiff extraction pr to use tika?
Yes, which is why I pinged you in this PR, because I think using a sandboxable thumbnailer this architecture allows is crucial for security.

👍🏻

I already upstreamed audio artwork extraction and tiff extraction to tika. Geogebra is still pending, but I expect it to be merged very soon as well.
So I would like to see a preprocessing step for extraction of embedded pictures before they are handed over to imagor for resizing (with tika it would be push based and out of process too).
I tried to move the preprocesor package from thumbnailer to webdav as is. More extractors should be added here, or did you have to refactor a lot for your extractors? In any case that should work for this approach as well.

I'm fine with that, I just wanted you to be aware. No problem at all to port it. It was just important to me that extractors still have a place in the architecture and we don't need a magical one service that can handle all file types.

I was a bit surprised to see that you are moving so much into the webdav service, my expectation would have been that you just replace the Decoder in the thumbnails service - can you elaborate a little why you chose this way more invasive approach?
Sure. On main, webdav doesn't own the pipeline at all: each thumbnail request is a grpc GetThumbnail call where the thumbnails service does stat, cache check, source fetch, preprocess (the decoder), generate, and disk storage for caching, then returns a JWT-signed download URL that webdav has to make a second HTTP call to just retrieve the bytes. So swapping only the decoder would leave that grpc hop, the JWT endpoint, the storage layer, and the double-fetch intact, fixing how a file becomes an image but not where the pipeline runs or the extra indirection.

Fair enough, thanks!

We can move the workflow into webdav, because webdav already has everything it needs locally (the CS3 gateway client for stat + download, the auth context, the response writer), which lets the generator shrink to a stateless resizer (POST bytes in, resized bytes out) that's swappable with an external imagor via a single URL. The thumbnail generation process is owned, end-to-end, by webdav instead of logic split across two services and two round-trips.

And we can use an external container to support HEIC thumbnails, just by changing the URL. And I really want to be able to reuse the thumbnailer like tika, because it now becomes stateless.

That part was clear :)

One more note: It might be the time to at least quickly think about the "honest hasPreview" implementation.
I need the availability and dimensions of embedded previews at index time (#3210).
That leads me to the following questions:

1. Should we use the extraction pipeline as library code or do we need an (internal) api to just extract the original size embedded preview?
2. Should extracted previews be cached? For search service and thumbnails, but also simply so that multiple thumbnail sizes don't need the extraction through tika multiple times

Thoughts? Do you see any problems with your drafted architecture? (I don't see any, just to be sure)

[butonic]

One more note: It might be the time to at least quickly think about the "honest hasPreview" implementation. I need the availability and dimensions of embedded previews at index time (#3210).

👀 🤔 I'll comment there...

That leads me to the following questions:

1. Should we use the extraction pipeline as library code or do we need an (internal) api to just extract the original size embedded preview?

I would prefer library code. I was thinking of moving it to /pkg, in case other services also call the grpc api.

2. Should extracted previews be cached? For search service and thumbnails, but also simply so that multiple thumbnail sizes don't need the extraction through tika multiple times

Yes, they should be cached.

Thoughts? Do you see any problems with your drafted architecture? (I don't see any, just to be sure)
Nope.

Althought, I plan to add a configurable blobstore for the cache so we can store thumbnails in nats or s3 ... with a checksum based key. But that would be the topic of a future PR.

[dschmidt]

FYI, all merged:

• TIKA-4831: Add content-based detection and a parser for GeoGebra files (ggb, ggs, ggt) apache/tika#3044
• TIKA-4801: Extract cover art from MP3, MP4, Vorbis and FLAC audio apache/tika#2983

So we can replace all existing extractors and get this on top:

• [TIKA-4824] extract embedded JPEG previews from TIFF-based raw images (NEF, ARW, PEF, DNG, CR2) apache/tika#3037

[butonic]

Some more thoughts on why POST and not GET:

With POST the image generator does not need to be granted access to all instances it should GET images from. In a multi instance deployment or at hosters all instances can just POST to the imagor service. New instances do not need to be registered in the firewall or network configuration to let imagor make requests to them.

The imagor service itself can just be horizontally scaled.

Finally, if malicious bytes manage to take over the container/pod it can still not make any requests to the outside world.

So POST really is driven by security and scalability. This also explains why we cannot let clients make signed GET requests directly to the imagor service. It would have to be granted access to all instances. IMO that is too much of an attack surface, even if the requests are signed by the instance itself. At the network level you dont want to inspect every request to ensure it really is a GET / download request for a file ... but if you don't the request might also POST to another endpoint to probe the infrastructure ... nah. Lock it down: no egress whatsoever.

As a sidenote: the image cache uses hash based keys, so all instances could share the same objectstorage as well. And the objectstorage can then implement the cache invalidation.

[dschmidt]

Found a compatibility bug while pointing the generator URL at a real imagor (imagorvideo): generator.BuildURL emits two filter segments (filters:no_upscale()/filters:format(x)/), but imagor only reads a single filters segment, so format() is silently ignored and e.g. PNG input comes back as PNG despite format(jpeg). Our own thumbnails router parses the two-segment form, which hides this.

Related: for the default fill operation (no operation segment) the router has no route with filters:no_upscale(), so every default-operation request 404s.

Fix is building a single segment and matching the routes:

--- a/services/webdav/pkg/generator/url.go
+++ b/services/webdav/pkg/generator/url.go
@@
 	box := fmt.Sprintf("%dx%d", width, height)
-	noUpscale := "filters:no_upscale()"
-	formatFilter := fmt.Sprintf("filters:format(%s)", outputExt)
+	filterSegment := fmt.Sprintf("filters:no_upscale():format(%s)", outputExt)
 
 	segment := operationSegment(operation)
 	if segment == "" {
-		return fmt.Sprintf("%s/unsafe/%s/%s/%s/", base, box, noUpscale, formatFilter)
+		return fmt.Sprintf("%s/unsafe/%s/%s/", base, box, filterSegment)
 	}
-	return fmt.Sprintf("%s/unsafe/%s/%s/%s/%s/", base, segment, box, noUpscale, formatFilter)
+	return fmt.Sprintf("%s/unsafe/%s/%s/%s/", base, segment, box, filterSegment)
--- a/services/thumbnails/pkg/service/http/v0/service.go
+++ b/services/thumbnails/pkg/service/http/v0/service.go
@@
-	m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale()/filters:format({format})/", handler)
-	m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale()/filters:format({format})", handler)
-	m.Post("/unsafe/{width}x{height}/filters:format({format})/", handler)
-	m.Post("/unsafe/{width}x{height}/filters:format({format})", handler)
+	m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale():format({format})/", handler)
+	m.Post("/unsafe/{operation}/{width}x{height}/filters:no_upscale():format({format})", handler)
+	m.Post("/unsafe/{width}x{height}/filters:no_upscale():format({format})/", handler)
+	m.Post("/unsafe/{width}x{height}/filters:no_upscale():format({format})", handler)

(plus the URL literals in generator_test.go / push_test.go). Verified against imagorvideo: with one segment format() is honored and the default operation stops 404ing.

[butonic]

The web ui fetches previews with

• ?scalingup=0&preview=1&a=1&processor=thumbnail&x=32&y=32 for the list view and
• ?scalingup=0&preview=1&a=1&processor=fit&x=320&y=320 for the tiles view
• ?scalingup=0&preview=1&a=1&processor=fit&x=1920&y=1920 for the gallery or even
• ?scalingup=0&preview=1&a=1&processor=fit&x=3840&y=3840 based an the available space

The tests fetch a 1200x1200 image to test text rendering ...

However, the available preview resolutions are configured on the server side and NONE of the resolutions is in the default list (save 32x32): "16x16", "32x32", "64x64", "128x128", "500x280", "280x500", "1000x560", "560x1000", "512x2048", "1080x1920", "1920x1080", "2160x3840", "3840x2160", "4320x7680", "7680x4320".

The only real difference is the processor.

• thumbnail (aka imagor fill = the default) resizes the image to the given width W and height H, auto-cropping the excess to fill the box. The crop is centered.
• fit (aka imagor fit-in) resizes the image to fit within the given dimensions without cropping. The result may be letterboxed

THUMBNAILS_RESOLUTIONS was used to limit the number of thumbnail resolutions that will be cached AND returned by the API. So the quare resolutions requested by the web ui with fit (fit-in), produce landscape or portrait previews, depending on the source image dimensions.

If the source image happens to be wider or taller than the configured resolution, the largest configured resolution will be generated and cached. This might be smaller than the requested width or height.

But since the web ui does not always know the size of the image all it can do is tell the server to fit the image into a rectangular box and the server will fit-in the image. For fit-in the response never is a square, which the web ui already expects.

I think we can delete all non square resolutions, because in effect only they are requested. thumbnail and fit are only used to make the server crop the image to the exact resolution (thumbnail), or fill the image to the box without cropping.

[dschmidt]

Not sure, happy to discuss (if needed at all) - but how do I request previews for ultrawide pictures?

I'm requesting non-quadratic previews here: opencloud-eu/web-extensions#529

[JammingBen]

Not sure if or how this affects the web client, but if it does in any capacity, please check in with one of us to verify the desired behavior. We've made quite some performance improvements over the last few weeks by optimizing requested and delivered resolutions, it would be a shame if we regressed on that.

[codacy-production]

Not up to standards ⛔

🔴 Issues 12 critical · 37 minor

Alerts:
⚠ 49 issues (≤ 0 issues of at least minor severity)

Results:
49 new issues

Category	Results
Security	12 critical
CodeStyle	37 minor

View in Codacy

🟢 Metrics 941 complexity

Metric	Results
Complexity	941

View in Codacy

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

[dschmidt]

I guess we could/should put it in a thumbnail block as well?