Skip to content

Introducing the maximum size preview #13582

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.

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

Closed
wants to merge 0 commits into from
Closed

Introducing the maximum size preview #13582

wants to merge 0 commits into from

Conversation

oparoz
Copy link
Contributor

@oparoz oparoz commented Jan 22, 2015

Fixes

The problem

You've just uploaded a 100MB TIFF file that you need to share with a restricted audience. Here is what currently happens once the file has been uploaded.

  • A 36x36 thumbnail is generated in 1 minute
  • Go to the Gallery, an album thumbnail might be generated in 1 minute
  • Go inside the album, another thumbnail will be generated in 1 minute
  • Click on the picture to get a full screen preview. 1 minute
  • Go back to the Files app and share the file
  • Client 1 visits the link. 1 minute to get the preview
  • Client 2 with a different screen resolution visits the link. 1 minute again

You're starting to get it. Every time a slightly different preview size is asked, a full conversion of that 100MB TIFF to PNG is performed. This is highly inefficient.

Proposed solution

  • Enforce maximum preview dimensions in the config. They're currently set to null.
  • The first time we're asked to generate a preview we'll generate one of the maximum dimension indicated in the configuration and all future resizing requests will be done on that preview in order to not waste time converting the same large file over and over

Default size is 2048x2048
The maximum size preview is keeping the aspect ratio of the original and is not upscaled.
It's is immediately stored in the cache with a special max tag and size identifiers based on its real size.

Every time a preview of a different size is asked, we store it in the cache. This also speeds up operation at the cost of disk space.

This make a huge difference in libraries holding several large files, for people using apps such as the Gallery.

@ghost
Copy link

ghost commented Jan 22, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7635/

Build result: FAILURE

[...truncated 13 lines...] > git config --local --remove-section credential # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git4573009880408482453.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/13582/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/13582/merge^{commit} # timeout=10Checking out Revision 493307864349c7a609668e52a6f0772745ef6816 (refs/remotes/origin/pr/13582/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 493307864349c7a609668e52a6f0772745ef6816First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 890d31dc937ff4ac93054217461a0cd5d4e97542 to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/7635/ and message: Merged build finished.
💣 Test FAILed. 💣

@karlitschek
Copy link
Contributor

@georgehrke

@DeepDiver1975 DeepDiver1975 added this to the 8.1-next milestone Jan 22, 2015
@DeepDiver1975
Copy link
Member

@oparoz thanks a lot! really awesome! Let schedule this for 8.1

@oparoz oparoz self-assigned this Jan 22, 2015
@ghost
Copy link

ghost commented Jan 22, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7653/
🚀 Test PASSed. 🚀

@georgehrke
Copy link
Contributor

👍 for the proposed solution! Thanks a lot.

Will review the code asap

@DeepDiver1975
Copy link
Member

Will review the code asap

@georgehrke after OC8 has been released please - THX

}

// The providers have been kind enough to give us a preview
if (!is_null($preview)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Use if (preview) instead as provider's response can be boolean

@ghost
Copy link

ghost commented Jan 23, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7884/
🚀 Test PASSed. 🚀

@scrutinizer-notifier
Copy link

The inspection completed: 6 new issues, 5 updated code elements

@ghost
Copy link

ghost commented Jan 26, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/7975/
🚀 Test PASSed. 🚀

@oparoz oparoz closed this Jan 26, 2015
@MorrisJobke MorrisJobke removed this from the 8.1-next milestone Jan 26, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants