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.

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

New occ commands to manage thumbnails #387

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@oparoz
Copy link
Contributor

commented Sep 27, 2015

Sunday Funday!

Here are some command some of you may find useful to manage your thumbnails:

Create thumbnails

Solution for owncloud/core#17536

Documentation

Create thumbnails

Specs

  • Collect user(s) or path info from the list of command arguments
  • Die if encryption is enabled
  • Compute path to analyse
  • Only work with folders starting with /<user>/files
  • Scan the given path using Cache\Scanner in order to work with reliable data
  • Update the DB cache
  • Generate previews only for media types supported by the Gallery and for which we have a preview provider
  • Collect stats

Stats

  • of folders

  • of files

  • of images

  • of previews generated

  • of failed previews (listed)

  • Time to complete request
  • Last scanned file
  • Size of processed data

TODO

  • Quit if encryption is enabled
  • Only scan a mounted volume if previews are enabled for that storage location
  • Restrict scan to /<user>/files
  • Add some stats
  • Add a switch to force the regeneration of thumbnails
  • Offer a safer scanning option, with transactional locking. I'm hoping this will solve the error 1205 problems and not create more problems when trying to generate thumbnails
  • Presents stats even if operation was cancelled
  • Add tests
  • Make operation cancelling optional
  • Use OCP\Files\Cache\IPropagator on 9
  • Check if this is fixed owncloud/core#17196

Possible extensions

For future PRs

  • Make coffee
  • Support mimetype as a filter

Delete thumbnails

Documentation

Delete thumbnails

Specs

  • Collect user(s) or path info from the list of command arguments
  • We do not care about encryption
  • Wipe the thumbnails folders if we're asked to remove thumbnails for one, a few or all users
  • Compute path to analyse in all other cases
  • Search for files of media type supported by Gallery and for which we have a preview provider and delete as we go
  • Collect stats

Stats

  • of images

  • of thumbnails deleted

  • of failed deletion (listed)

  • Time to complete request
  • Saved space

Possible extensions

For future PRs

  • There could be an option to delete thumbnails for all files. This would only be useful if the providers were changed in between the time the thumbnails were generated and the time at which the command is run

TODO

  • Find an easy way to update the cache without re-scanning
  • Fix bug in core which prevents the use of a shared folder as the path (sub-folders work) owncloud/core#19544 (8.1+)
  • Ask confirmation before launching the operation as we're deleting files from the system
  • Add tests

Notes

Of course, it would be much more useful to be able to run that from a cron, from the user's settings panel, etc. so don't hesitate to improve this solution ;)

@mmattel @setnes @EricReiche @jospoortvliet @DeepDiver1975 @icewind1991 @Xenopathic @deMattin @TheDD @patman15 @africanforest @MorrisJobke @PVince81 @libasys @mbazdell @blackm

@oparoz oparoz changed the title New command to generate missing thumbnails New occ command to generate missing thumbnails Sep 27, 2015

@oparoz oparoz added the enhancement label Sep 27, 2015

@oparoz oparoz self-assigned this Sep 27, 2015

@deMattin

This comment has been minimized.

Copy link

commented Sep 27, 2015

👍

@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

👍
One additional parameter would be great --mount=mount_name (with the ability in combination with --all, or general for all). Then you can add a mount with files external and (after scanning) you can run the thumbnail creation immediately for exactly and only this mount for (all) users...

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

@mmattel - If you know the path to the mount, you can already use --path. Let me know if it doesn't work. It would also be included in the --all command

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

I've just tested with a mounted ownCloud instance and it worked. I'll updated the OP.

@icewind1991

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

From what I can tell it only creates thumbnails for a single size which wont be enough for people with HiDPI screens, maybe add some sort of scale parameter to handle that?

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

From what I can tell it only creates thumbnails for a single size which wont be enough for people with HiDPI screens, maybe add some sort of scale parameter to handle that?

The main goal is to generate the max preview, but since I need to also give another size (there is no getMaxPreview() method), I picked the Gallery thumbnail dimensions with a pixel ratio of 1 since we can't guess what the ratio is going to be in the various clients connecting to the app.
I could make some modifications in Gallery to always round up the ratio and offer the option to generate photowall thumbnails at a ratio of 1,2,3 and 4.

If core ever gets standardised sizes for thumbnails (36,256,1024,2048,4096 per example), there could be a switch to generate all of those, but currently it wouldn't make sense. If we take into consideration the various pixel ratios, we would end up having to pre-generate 20 thumbnails per image.

@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@oparoz

Testing if creating thumbnails of a mount with --path works, here is the result:

Having a clean fresh install from git master and finalizing it.
Adding the PR to test it
Checking if ./occ has the newly added parameter -- ok
Enabling External Storage Support
Adding a SMB share named "SMB"

sudo -u www-data ./occ gallery:create-thumbnails --path=SMB
--> Unknown user SMB

This is what I expected, because having a mount only is not a path having a user in. And the mounts are not in the users home...

But this is what my suggestion is about. Usually lots of picture files are mounted with external storage support... And advanced thumbnail creation on an mount would be a big help.
sudo -u www-data ./occ gallery:create-thumbnails --mount=SMB

@icewind1991

This comment has been minimized.

Copy link
Member

commented Sep 28, 2015

./occ gallery:create-thumbnails --path=/myuser/files/SMB ?

@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@icewind1991 @oparoz
YES 😄 this works. Thanks for the tip!
Pls document this ability because it is not explicit clear from the example above.

Question: do I get it right, that --all and --path do exclude each other?
Having this would be perfect btw... (if the first path parameter (user) is not found and --all is not present than error else do for all)
Ok, I know, many things, but this is really a great command !!

@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@oparoz
For your info, when starting the command, I get this printed on the terminal before the generation starts. After 2-3 seconds, generation ist starting fine.

tdb(__NULL__): tdb_open_ex: called with name == NULL
tdb(__NULL__): tdb_open_ex: called with name == NULL
tdb(__NULL__): tdb_open_ex: called with name == NULL
tdb(__NULL__): tdb_open_ex: called with name == NULL
@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2015

@icewind1991
I think that the following is not related to this PR, but don´t you work on an issue like that?
Maybe this helps finding/fixing the problem...

After a short time running the command, I get on each thumb created a message like below logged, and I have thousands of pictures to be processed:

{"reqId":"G+T\/voqBTE73dsav+oi5","remoteAddr":"","app":"no app in context","mes
sage":"Trying to acquire a lock for 'files\/0d1cc4de07964c1a4c92887962ccef01' w
hile inside a transition","level":2,"time":"2015-09-28T17:13:27+00:00"}
@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2015

Question: do I get it right, that --all and --path do exclude each other?

Yes, the user will be determined from the path

tdb(NULL): tdb_open_ex: called with name == NULL

Samba error. Perhaps it's scanning a folder it doesn't have access to, like some cache? You can debug this with the files:scan command on which this is based.

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2015

@mmattel - I investigated a bit and pushed some changes.

  • Folders where being sent as files, so that's never good and I've included an extra check
  • The scanFile event fires before a file is actually ready to be processed and scanFilePost almost never fired for me. I've submitted a PR to fix that (owncloud/core#19460). If that's merged, then I'll be able to improve the scanner
@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2015

@oparoz
Q: will your ongoing changes also fix that temp files ceated are immediatley released after processing the file, pls see my comments about that owncloud/core#19444 (comment)

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2015

@mmattel I'm not sure anything clean can be done about this. GD seems to be using the temp folder to process images and I don't have a command or the filenames.
A hack could get rid of *.JPG files after each operation.

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2015

OK, so I've imported most of the Scanner class in order to customise the scan since the original class didn't give us access to the mount options before scanning that storage location.

Using the latest master, we can now listen to scanPostFile event (owncloud/core#19460) and the Image class cleans up after itself (owncloud/core#19471), so the command should behave as expected.

@mmattel Let us know how it goes with your 19K images :)

@mmattel

This comment has been minimized.

Copy link
Contributor

commented Sep 30, 2015

Just for the record, this is the fix to my comment above regarding temp space usage: owncloud/core#19471 (Clean temp files used for thumbnail generation, merged)

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2016

I'm glad it's made it to the base class :). Thanks for the heads up.

@oparoz oparoz modified the milestones: 9.1-next, 9.0-current Feb 11, 2016

@elpraga

This comment has been minimized.

Copy link

commented Mar 9, 2016

I just applied this patch on OC 9.0 (using the guide from http://blog.jospoortvliet.com/2016/01/patching-owncloud-get-your-fix-now.html?m=1).
I checked before applying the patch, the check went OK, I applied the patch with no error reported, but when I try to run the occ command, I get an error on line 93.

Does somebody know when I'm doing an error?

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Cool guide @jospoortvliet :)

@elpraga - The problem is that things have changed in core between the last commit in this PR and today, so the patch is sadly out of date and won't give you what you want.

@elpraga

This comment has been minimized.

Copy link

commented Mar 9, 2016

OK. Thanks for explaining! Are the changes big?

In other words, is there a chance that the branch gets updated and the patch will be applicable to 9.0?

I was waiting quite anxiously for 9.0 to try out the command for thumbnail generation and I was really sad it did not maker it after all.. :-(

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Are the changes big?

No idea. It could be relatively quick to just fix, but some refactoring will be needed on 9.0 as some of the features developed in here have been integrated in core.

It's been low on my list of priorities unfortunately as I can't justify spending 2-3 days to fix it.

@elpraga

This comment has been minimized.

Copy link

commented Mar 9, 2016

Thank you for clarifying! I understand that priorities are tough sometimes.

Unfortunately I cannot afford to pitch in some money right now nor offer my skills to write the fix as I lack them..

I'll have to wait then.. But thank you a lot anyways!

@Bugsbane

This comment has been minimized.

Copy link

commented Apr 16, 2016

Hoping that this will live to see OC 9+

@zbirmingham0001

This comment has been minimized.

Copy link

commented Apr 18, 2016

Agreed, Love to see this working with OC9. This is why I use OC but the thumbnail generation is holding us back.

@oparoz oparoz modified the milestones: 9.2-next, 9.1-current May 31, 2016

@oparoz oparoz removed this from the 9.2-next milestone Jun 16, 2016

@oparoz oparoz removed the 2 - Developing label Jun 16, 2016

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2016

Closing as this will not be added to Gallery

@oparoz oparoz closed this Jun 16, 2016

@elpraga

This comment has been minimized.

Copy link

commented Jun 17, 2016

So this functionality will not be added not even to 9.1? Really? That is really sad to hear, I was waiting for it for months...
Is there any chance for the decision to be reversed?

@jospoortvliet

This comment has been minimized.

Copy link

commented Jun 17, 2016

@elpraga if somebody writes the tests, I suppose ;-)

@elpraga

This comment has been minimized.

Copy link

commented Jun 17, 2016

Unfortunately, I don't know how to. Otherwise, I would offer to help.. :-/

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2016

Write tests, remove code made obsolete by changes in core, use the background queue if it ever gets documented (and is useful) plus much more I'm sure.

Since I'm going to focus on Gallery+, this may make a come back there.

@nksupport

This comment has been minimized.

Copy link

commented Jun 17, 2016

@oparoz, i'm sure there's a whole lot of us who will be happy with just an ugly hack to run in the shell, something that would call the thumb generation method directly, not integrated in core or anything. ditaW pasted a browser-side greasemonkey script here , but it seems broken already... any hints?

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

Form my pov this should go into core ....

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2016

@DeepDiver1975 - I agree, all the base components are there. This was sort of a crutch while waiting for some movement on a background task queue manager capable of generating thumbnails.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Jun 20, 2016

background task queue manager capable of generating thumbnails.

we should start up the discussion about this for the scope of 9.2 again

@oparoz

This comment has been minimized.

Copy link
Contributor Author

commented Jun 20, 2016

For reference this is the issue in core: Migrate preview generation to async job execution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.