-
-
Notifications
You must be signed in to change notification settings - Fork 172
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
Thumbnail Improvements #2
Conversation
Fantastic! Thank you for your work -- I'll try to review everything by Sunday (max 3 days). |
4092b3e
to
3f92922
Compare
How does the demo content work/is it relevant? 😄 |
Demo content was meant to be an online version of the app -- as a demo of how users can interact with it. This example from February 2018: http://dev.yboris.com/vh/ I am thinking showing an online demo to users might cause more confusion, so I'm not sharing it publicly (and the demo is of the app version 1.0.0). Feel free to ignore these files without any updating. Ps - to get the demo to work, just run |
I'm loving so much about this! One minor note - the new screenshots are much larger than the sum of the previous files. In one randomly-chosen example the previous 10 screenshots weighed in at 214kb, while the new one was 1,150kb (I checked the file, the JPEG quality was set to 100). I suspect we just need to pass in a lower jpg compression setting somehow 👍 I looked at merge-img but it doesn't seem to have any such options. It uses jimp which seems to have something related to quality but I don't know how to access it. Perhaps, we'll have to import |
I love that both thumbnails and filmstrip works so well with the new system! Could you please add |
Compared to the master and angular7 branch, for some reason the App uses a lot more RAM (500mb+ compared to ~130mb) on startup. |
While some people have requested automatic updating of screenshots on start, I would prefer it would not be the default, and only available (if you think it's worth it) with a setting in the menu. In part it's a personal preference, but in part it could cause some trouble: making the app unresponsive/slow while a user just wants to use it. There are likely other scenarios where auto-update is not preferred. Do you think it would be possible to have this behavior disabled until a user enables it in the settings? |
I suspect it's possible to bypass writing the 10 images to disk, and then reading the 10 images from disk to create the single image.
You said you've found a way to not use the What are your thoughts on this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is so good! You accomplished many of the things that were on my todo list and improved some things I didn't know I could improve! Thank you very much 👍
I'm eager to get angular7 merged in -- I tried your branch on top of angular7 and it ran great 🙌 Could you review #1 if you have time, and I'll merge it in once we're both happy with it. Then we could merge this PR on top (after a few minor changes).
I suspect there's some tasks the app running your branch performs on start-up that cause the RAM spike (over 1GB and CPU as high as 100%). Perhaps those can be commented out at least for now, or placed behind a setting that's off by default unless a user changes?
At work we sometimes add a feature toggle across code we disable, e.g.
// FEATURE_TOGGLE - DISABLE_AUTO_UPDATE
So that later when we're ready to enable it, we can find all instances to toggle quickly and easily.
Please let me know 🥇
This should be fixed now with the new ffmpeg - I'm getting 61kb files here, can you confirm? |
Can you confirm if this is still an issue? |
I've opened #15 to track this! 👍 |
Haha, thanks for this! 👍 This prompted me to go and do some more research into ffmpeg, and I now have both image and video thumbnails being extracted with a single command into a single file! 🎉 The image command has been added to this PR! Thanks again! 😄 |
Amazing! I will have time after work on Friday (tomorrow) to try this out. |
Don't merge yet, optimising thumbnails! |
That last commit majorly improves thumbnail extraction speed! 👍 |
Confirming that your branch at this moment works on top of angular7 and is fast at extracting screenshots. Confirming that I wonder if this could be solved with a newer version of |
I wonder if subtitles have something to do with the The 1min-long video extracted the first screenshot, just before any subtitles kicked in. I haven't confirmed, but I think all the mkv files I tried this on (just a few in my case) - they all had subtitles! maybe the |
Can you confirm this works for you too? 😄 |
Fantastic! mkv works perfectly now! Details: 43 videos imported in 1min 23sec! Average jpg weight is 420kb (at 300px height per screenshot). App uses around 780mb of RAM instead of the usual ~150mb. I suspect it has to do with multiple loading of the images. I have ideas about getting that number down - can be done after this PR merges. |
If you have the time, could you spot-check the angular7 #1 PR - once you approve it I'll merge it in, and then we can merge this one on top. I expect only about 2 lines of merge conflicts then 👌 |
In the middle of it right now! 😉 |
I'd assume this has to do with:
I'm sure there's some optimisation we could do, but merging first would be nice! 👍 |
It looks like when running the app in development mode the RAM usage is higher. I built the app and installed it with your branch, and it looks to be around ~165MB of RAM -- sorry for the false alarm 😓 |
Oh nice! 👍 Should be good to go then! 😄 |
I'm happy to merge this PR once the merge conflicts have been resolved 😁 Once merged in, would you mind if I fix up some minor things (e.g. |
Now just hashing file name and file size for reduced reads. Also using ffprobe to get the file size, removing a `stat` call per file, and saving the full file size in bytes rather than the rounded mb value, and converting on the fly later.
…ipping any already existing thumbnails.
Rebased onto master! 👍 Feel free to fix those minor things before merging! I think you can push to this branch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
I'll merge in and create a small PR for format changes - just things like var
-> let
, etc 👌
👍 Ready to go!
This PR will add many improvements to the thumbnail system:
Add backwards compatibility to previous versions. (?)See Implement Backwards Compatibility #10.Fix the demo content.Some notes about this PR:
.bak
would be a good way to go.Any and all comments accepted! 😄