Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[full-ci] thumbnails: add libvips based thumbnail generator #10310

Merged
merged 11 commits into from
Oct 17, 2024
Merged

[full-ci] thumbnails: add libvips based thumbnail generator #10310

merged 11 commits into from
Oct 17, 2024

Conversation

rhafer
Copy link
Contributor

@rhafer rhafer commented Oct 15, 2024 •
edited
Loading

Needs: owncloud-ci/golang#149

To improve performance (and to be able to support a wider range of images formats in the future)
the thumbnails service is now able to utilize libvips (https://www.libvips.org/) for generating thumbnails.
Enabling the use of libvips is implemented as a build-time option which is currently disabled for the
"bare-metal" build of the ocis binary and enabled for the docker image builds.

See README change for more details.

@rhafer rhafer self-assigned this Oct 15, 2024
@update-docs Update Docs
Copy link

update-docs bot commented Oct 15, 2024

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@rhafer rhafer force-pushed the vips-thumbnailer branch 4 times, most recently from 65a94cc to 52ea02a Compare October 16, 2024 09:13
@rhafer rhafer force-pushed the vips-thumbnailer branch 2 times, most recently from 26ff408 to 87b2b13 Compare October 16, 2024 13:16
@mmattel mmattel mentioned this pull request Oct 16, 2024
Closed
butonic
butonic approved these changes Oct 16, 2024
View reviewed changes
Copy link
Member

@butonic butonic left a comment

Choose a reason for hiding this comment

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

works nicely!

@rhafer
Copy link
Contributor Author

rhafer commented Oct 16, 2024 •
edited
Loading

The acceptance test are currently still using the native go thumbnailer. We might want to change that to test the libvips variant instead in a future PR

@rhafer rhafer marked this pull request as ready for review October 16, 2024 16:02
@micbar
Copy link
Contributor

micbar commented Oct 16, 2024 •
edited
Loading

I just compiled ocis on Mac (Intel)

Steps

brew install vips pkg-config
export PKG_CONFIG_PATH="/usr/local/opt/libffi/lib/pkgconfig"

Add -tags enable_vips to the go tool arguments of the debugger.

Then it compiles sucessfully and the thumbnails are generated.

First impression: 🚀

micbar
micbar approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@micbar micbar left a comment

Choose a reason for hiding this comment

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

Awesome! Tested it locally. I see no regressions. 🚀

@ScharfViktor
Copy link
Contributor

I just compiled ocis on Mac (Intel)

checked on mac (m1) - works fine 👍

@rhafer
Copy link
Contributor Author

rhafer commented Oct 17, 2024

I just found an issue with rendering thumbnails for txt files. 🤦‍♂️ . Working on it ...

@ScharfViktor
Copy link
Contributor

I tried to run e2e test which checks thumbnail and preview.
for many preview requests we get 500 error

image

How to run: BASE_URL_OCIS=localhost:9200 HEADLESS=false RETRY=0 pnpm test:e2e:cucumber tests/e2e/cucumber/features/shares/share.feature:79

to enable e2e test in CI please rebase (e2e tests were disabled) #10325

@rhafer
refactor(thumbnails): merge processor into generator
53d39dd 
Processor is now part of the generator. This should make it possible
to implement thumbnail generators that do not depend on the 'imaging'
module.
@rhafer
refactor(thumbnails): separate out 'kovidgoyal/imaging' dependencies
57a6b21 
Move code using the 'kovidgoyal/imaging' package to separate files
to make it easier to create alternative implementations based on build
tags.
@rhafer
feat(thumbnails): optional libvips based thumbnail generation
4e8a23f 
Can be enabled by setting the 'enable_vips' tag on 'go build'
rhafer and others added 7 commits October 17, 2024 12:23
@rhafer
Add ENABLE_VIPS flag to Makefile
85aee62 
To build with libvips support use 'make -C ocis build ENABLE_VIPS=true'
@rhafer
dev: Enable VIPS in local docker builds
777fa9e 
As libvips is not available as a static library we can no longer
create a statically linked ocis binary for the docker images when
libvips is enabled.
We also need to make sure that the base images used for building
ocis needs to match the image where ocis is installed when creating
a shared binary.
@rhafer
Enable vips based thumbnailer for docker builds in the CI
97f67d3
@rhafer
Changelog entry and Readme updates related to libvips
b247cef
@butonic @rhafer
add buildFlags comment
9cdc978 
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@rhafer @mmattel
Apply suggestions from proof reading
d12a065 
Co-authored-by: Martin <github@diemattels.at>
@rhafer
Fix GeoGebra thumbnails when libvips support is enabled
04f6c17 
For GeoGebra files we where still using the imaging package to decode the
embedded png, but handed it of to vips for scaling, which can't work. Now
we use vips for decoding the image as well.
@rhafer rhafer changed the title thumbnails: add libvips based thumbnail generator [full-ci] thumbnails: add libvips based thumbnail generator Oct 17, 2024
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Oct 17, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
15.8% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@rhafer
Copy link
Contributor Author

rhafer commented Oct 17, 2024

I tried to run e2e test which checks thumbnail and preview.
....
to enable e2e test in CI please rebase (e2e tests were disabled) #10325

@ScharfViktor Ok. I rebased. e2e test seem to have succeeded now.

@rhafer rhafer merged commit e43a858 into owncloud:master Oct 17, 2024
3 checks passed
@rhafer rhafer deleted the vips-thumbnailer branch October 17, 2024 12:05
@micbar micbar mentioned this pull request Oct 21, 2024
Closed
18 tasks
This was referenced Oct 23, 2024
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmattel mmattel mmattel left review comments

@butonic butonic butonic approved these changes

@micbar micbar micbar approved these changes

@wkloucek wkloucek Awaiting requested review from wkloucek wkloucek is a code owner

@phil-davis phil-davis Awaiting requested review from phil-davis phil-davis is a code owner

@individual-it individual-it Awaiting requested review from individual-it individual-it is a code owner

Assignees

@rhafer rhafer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rhafer @micbar @ScharfViktor @butonic @mmattel