Skip to content

Conversation

@PVince81
Copy link
Member

@PVince81 PVince81 commented Nov 16, 2020

This is a preview provider for "image/*" mime types that streams the file data to an Imaginary service, which itself will generate the preview and stream it back.

To test:

Using juliusknorr/nextcloud-docker-dev#59

  1. And enable the preview provider in config.php:
  'enabledPreviewProviders' => [
        'OC\Preview\MP3',
        'OC\Preview\TXT',
        'OC\Preview\MarkDown',
        'OC\Preview\OpenDocument',
        'OC\Preview\Krita',
        'OC\Preview\Imaginary',
  ],
  1. Upload some jpeg pictures and see the previews rendered

Some issues/topics:

Preview manager issues

  • why is the maxX/maxY always 4096 even though the UI is requesting less than 400x400 ? (did I find a bug?) Expected behavior, we crate a big preview first and then with GD resize it :/ Still something to improve and make it possible to also use imaginary to do the later resizing. Also worth investigating if having 4096x4096 has big previews make sense as most/many images have a smaller dimenssion.
  • BUG? even if setting only "Preview\Imaginary" in "enabledPreviewProviders", it still doesn't seem to be registered properly due to the other defaults. I was actually expecting "enabledPreviewProviders" to override the defaults. Expected behavior the previews needs to be registered (in core with registerCorePreviews) and then enabled.

Imaginary preview topics

  • $response->getBody() is a string, but should be a stream => solved, post response was missing an arg

  • optional perf improvement: if we know a file to be on local non-encrypted storage, could send the file path directly to the docker (when running on same instance)

  • gif not supported, it returns an error. might need more option

  • seems we do need multipart here, PUT not supported => fixed by sending POST with the actual mime type, not multipart

  • need to do some perf benchmarks and compare with the "native" way: Send images to Imaginary docker to generate previews #24166 (comment)

  • do benchmarks with photos app (assuming it uses the PreviewManager)

@PVince81 PVince81 force-pushed the imaginary-prototype branch from eab296b to 516d824 Compare November 16, 2020 21:52
@PVince81 PVince81 self-assigned this Nov 16, 2020
@PVince81
Copy link
Member Author

PVince81 commented Nov 16, 2020

Quick benchmark in the files app:

  1. create a new folder
  2. upload 4 selected JPG photos of roughly the same size (use the same set for both test runs)
  3. wait for the 4 previews to load
  4. check the timing tab of the 4 requests

Results:

  • with Imaginary on this PR (docker running locally): ~4.7 second
  • native way: ~7.5 seconds

@faily-bot
Copy link

faily-bot bot commented Nov 16, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 35607: failure

checkers

Show full log
The autoloaders are not up to date
Please run: bash build/autoloaderchecker.sh
And commit the result

@rullzer
Copy link
Member

rullzer commented Nov 17, 2020

@PVince81 why not do this as an app?

@rullzer
Copy link
Member

rullzer commented Nov 17, 2020

why is the maxX/maxY always 4096 even though the UI is requesting less than 400x400 ? (did I find a bug?)

No that was the part I also ran into when I wrote an imaginary app to play preview provider.
The preview provider is only used to generate the initial preview (so the highest resolution one)
Then we use GD to create the downsized images.

@PVince81
Copy link
Member Author

@PVince81 why not do this as an app?

because only a prototype for benchmarking, it's easier to push a PR that way.
in any case this is to be discussed first.
if worth it, we can make an app, yes.

I suspect we might need to tweak the preview manager to make it possible to override the default preview providers, which somehow didn't work when I tested (hence the early "return" in this PR)

@PVince81
Copy link
Member Author

PVince81 commented Nov 17, 2020

  • should we use Imaginary also for the small resizes from the cached image instead of GD ? (make the preview manager support such operations by third parties ?)

@PVince81
Copy link
Member Author

work to be continued in this app: rullzer/imaginary#1

I'll leave the PR open until we have transferred the tasks and code

@CarlSchwan
Copy link
Member

CarlSchwan commented Feb 11, 2022

Benchmarks with s3 and using microtime in the getThumbnail method:

Small images (256x256):

  • imaginary: ~0.7s
  • current Image provider: ~0.4s

Bigger image: HD

  • imaginary: ~1.5s
  • current image providers: ~2.5s

So it seems that the overhead from the network is really only worth it for larger images there the multithreading of imaginary really helps. This should be most pictures as taking small 256x356 images is not really a thing anymore

@CarlSchwan CarlSchwan changed the title WIP Send images to imaginary docker to generate previews Send images to imaginary docker to generate previews Feb 11, 2022
@CarlSchwan CarlSchwan marked this pull request as ready for review February 11, 2022 11:45
CarlSchwan added a commit to CarlSchwan/imaginary-1 that referenced this pull request Feb 11, 2022
Use case:

When using the fit image transformation, it is helpful to know the size
of the resulting image without having to either read the image locally
or do another request to the info endpoint.

Used in: nextcloud/server#24166

Signed-off-by: Carl Schwan <[email protected]>
@CarlSchwan
Copy link
Member

Found an issue, we don't get the width and height of the generated preview, so we currently assume it's 4096x4096 and this is causing an issue for small images (< 1000x1000) as we then generate an additional preview for nothing.

I made a PR upstream h2non/imaginary#382 to solve this

@CarlSchwan CarlSchwan force-pushed the imaginary-prototype branch 2 times, most recently from 36640cb to 4c26522 Compare February 14, 2022 17:14
@CarlSchwan CarlSchwan requested review from a team and juliusknorr and removed request for a team February 14, 2022 17:14
@CarlSchwan CarlSchwan requested a review from artonge March 14, 2022 15:19
@CarlSchwan
Copy link
Member

doc: nextcloud/documentation#8121

@PVince81 PVince81 added this to the Nextcloud 24 milestone Mar 15, 2022
@CarlSchwan CarlSchwan requested review from Pytal and icewind1991 March 16, 2022 08:55
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Tested and works, code looks good as well 👍

@PVince81
Copy link
Member Author

👍 from me for the changes past my original commits

Copy link
Member

@CarlSchwan CarlSchwan left a comment

Choose a reason for hiding this comment

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

+1 for the commits not from me 😆

Signed-off-by: Carl Schwan <[email protected]>
Co-Authored-by: Vincent Petry <[email protected]>
@juliusknorr juliusknorr force-pushed the imaginary-prototype branch from 092ead2 to 9b6a1cc Compare March 17, 2022 07:24
@juliusknorr
Copy link
Member

Fixed the missing phpdoc on IStreamImage, CI should be happy now

@CarlSchwan CarlSchwan merged commit 34988cf into master Mar 18, 2022
@CarlSchwan CarlSchwan deleted the imaginary-prototype branch March 18, 2022 11:32
@szaimen
Copy link
Contributor

szaimen commented Mar 18, 2022

backports?

@PVince81
Copy link
Member Author

backports?

no backport, this is a new feature and also brings in a new public interface

h2non added a commit to h2non/imaginary that referenced this pull request Mar 30, 2022
* Return with and heigh of the generated images

Use case:

When using the fit image transformation, it is helpful to know the size
of the resulting image without having to either read the image locally
or do another request to the info endpoint.

Used in: nextcloud/server#24166

Signed-off-by: Carl Schwan <[email protected]>

* Make mimetype support always return true

* Add command line option to enable this feature

Signed-off-by: Carl Schwan <[email protected]>

* refactor: remove deprecated X- prefix in response headers

Co-authored-by: Tom <[email protected]>
wslaghekke pushed a commit to recognizegroup/imaginary that referenced this pull request Feb 1, 2023
* Fixed Timeout in readme, 30 ~> 60 (h2non#340)

* Fix invalid parameters "-path-prefix" (h2non#344)

* fix: small errors in docs (h2non#346)

* fix: use proper formatter for usage template (h2non#347)

Co-authored-by: Mads Moeller <[email protected]>

* Add Cloud Run Button (h2non#362)

* Delete app.json

* Update README.md

* Update README.md

* updated docker builder OS to go version 1.17 (h2non#371)

* fix(readme): remove gocard obsolete badge

* feat(readme): update placeholder description

* fix(readme): update fly deploy tutorial

* fix(docs): allowed-origins examples h2non#333

* memory leak issue fixed with jemalloc (h2non#381)

* exposed palette from GET endpoints (h2non#380)

* Updated Dockerfile (h2non#384)

1. Changed base image to bullseye
2. The updated base image contains an updated version of libjemalloc too, so building from source is no longer necessary
3. Updated libvips version too

* Added dev container (h2non#385)

* Added dev container

* Removed irrelevant lines

* allow speed from get (h2non#383)

* allow speed from get

* updating the version to use effor param in libvips

* Return with and heigh of the generated images (h2non#382)

* Return with and heigh of the generated images

Use case:

When using the fit image transformation, it is helpful to know the size
of the resulting image without having to either read the image locally
or do another request to the info endpoint.

Used in: nextcloud/server#24166

Signed-off-by: Carl Schwan <[email protected]>

* Make mimetype support always return true

* Add command line option to enable this feature

Signed-off-by: Carl Schwan <[email protected]>

* refactor: remove deprecated X- prefix in response headers

Co-authored-by: Tom <[email protected]>

* Decompression exploit check (h2non#404)

* Bump bimg version to 1.1.7

* Add decompression bomb exploit check

* Update README with new flag

* Fix tests

* Fix typos (h2non#405)

Found via `codespell -S .git`.

---------

Co-authored-by: Julian <[email protected]>
Co-authored-by: liuxu <[email protected]>
Co-authored-by: 0xflotus <[email protected]>
Co-authored-by: Mads Moeller <[email protected]>
Co-authored-by: Mads Moeller <[email protected]>
Co-authored-by: James Ward <[email protected]>
Co-authored-by: Angelo Girardi <[email protected]>
Co-authored-by: Tom <[email protected]>
Co-authored-by: Vaibhav Sharma <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Co-authored-by: Vaibhav Sharma <[email protected]>
Co-authored-by: Carl Schwan <[email protected]>
Co-authored-by: SeaaaaaSharp <[email protected]>
Co-authored-by: Kian-Meng Ang <[email protected]>
suntong pushed a commit to suntong/imaginary that referenced this pull request Nov 28, 2023
* Return with and heigh of the generated images

Use case:

When using the fit image transformation, it is helpful to know the size
of the resulting image without having to either read the image locally
or do another request to the info endpoint.

Used in: nextcloud/server#24166

Signed-off-by: Carl Schwan <[email protected]>

* Make mimetype support always return true

* Add command line option to enable this feature

Signed-off-by: Carl Schwan <[email protected]>

* refactor: remove deprecated X- prefix in response headers

Co-authored-by: Tom <[email protected]>
gkmw pushed a commit to gkmw/imaginary that referenced this pull request Jun 19, 2024
* Return with and heigh of the generated images

Use case:

When using the fit image transformation, it is helpful to know the size
of the resulting image without having to either read the image locally
or do another request to the info endpoint.

Used in: nextcloud/server#24166

Signed-off-by: Carl Schwan <[email protected]>

* Make mimetype support always return true

* Add command line option to enable this feature

Signed-off-by: Carl Schwan <[email protected]>

* refactor: remove deprecated X- prefix in response headers

Co-authored-by: Tom <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants