-
Notifications
You must be signed in to change notification settings - Fork 7
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
Minor fixes #9
Minor fixes #9
Conversation
NiLuJe
commented
May 20, 2019
- More robust "plugged in" detection
- Forma/H2O thumbnail detection tweaks
We can end up plugged in, but no longer charging (f.g., if the device has been plugged in for a while, it's obviously no longer charging, because battery chemistry would go boom otherwise, but it's still plugged in). For ref., on my H2O, online is 3 during charge, 1 in that "charged but connected" state, and 0 otherwise. TL;DR: For our purposes, we want it to be 0 ;).
* Detect the basic Forma * Use proper thumbnail dimensions on the Forma * Use proper thumbnail dimensions on the H2O
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.
Upon a closer look, the values seem wrong (including some already there). Here's what I found:
Type | Size |
---|---|
N3_LIBRARY_GRID | 149x223 |
N3_LIBRARY_LIST | 60x90 |
N3_LIBRARY_FULL | 355x530 |
MediumGridList | 135x205 |
iPhoneThumbnail | 104x158 |
NickelBookCover | 600x600 |
Default | 600x800 |
Device Class | N3_FULL |
---|---|
Daylight | 1404x1872 |
Alyssum / Nova | 1072x1448 |
Dragon | 1080x1440 |
Kraken / Star | 758x1024 |
Phoenix | 758x1014 |
Default | 600x800 |
The images are then scaled to fit within those bounds using Qt::SmoothTransformation (bilinear) and Qt::KeepAspectRatioQt::KeepAspectRatioByExpanding (see above comment thread).
Note that device class does not directly correspond to the device with that codename; it is the first in it's series.
Interesting. I wonder why the Calibre driver sets a different resolution per device? Calling @davidfor do you have any insights? |
Whoever came up with those probably tried putting a book on the device and looked at the resulting values, but they were inaccurate due to the aspect ratio preservation. |
Based on this info, I'd probably suggest simplifying the cover size stuff and changing the cover resize method to bilinear using the rez library (it is the second-fastest in my benchmarks, and it matches the Kobo output). Also, on a side note, I'm probably going to work on a utility to prerender covers to be distributed alongside seriesmeta or koboutils (I haven't decided yet). |
TL;DR: Yeah, that probably makes sense! Bonus point: since UNCaGED doesn't actually generate the full-screen cover for performance reasons, this would potentially allow one to chuck the whole device detection out the window ;). (... provided we figure out why Nickel's refusing to generate said full-screen version in my latest tests, c.f., #3 (comment)) |
Still need a model name, but no reason why that couldn't be passed in on the command line from the cli version of FBInk |
|
@geek1011's thumbnails findings pretty much confirm my hunche (aided by the fact that the Kobo specs used to officialy list the screen resolution with those 10 pixels chopped off). c.f., shermp/Kobo-UNCaGED#9 (review)
So, what have we decided on doing? Are we going to close this and redo the thumbnail code to match Kobo? |
I wouldn't mind this going in just for posterity's sake ;D. The first commit is at the very least completely orthogonal to the thumbnail issue, and good to go ;). |
OK, I agree. The second commit doesn't even necessarily break things or decrease quality per say, although it is technically incorrect. For the resizing, we can either stick with the imaging NearestNeighbor resizing (the fastest), switch to the rez Bilinear resizing (the second fastest by a small amount, but has some issues with gamma loss), or switch to the imaging Bilinear resizing (half the speed, but still 10x faster than the current). What do you think @shermp, @NiLuJe? |
Yeah, may as well merge this as is. We can sort out the thumbnail stuff in a separate PR. |
I'll also open a new issue to discuss the cover image/thumbnail stuff. No need to continue cluttering up this PR |