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

On Kobo, drop fb to 8bpp on startup #4637

Merged
merged 16 commits into from Mar 3, 2019

Conversation

Projects
None yet
3 participants
@NiLuJe
Copy link
Member

NiLuJe commented Feb 21, 2019

Here be dragons!

8bpp in and of itself should not be an issue, as that's what Kindle & PB use, and it should, for most things, make things faster to blit.

(There's still a few things using non optimal BB types I have to go over).

The issue lies in the actual switch: on some devices, the Kernel does a counter-rotation when you send the ioctl.
I'm actively counteracting that, but I can only vouch for that on my own devices (a H2O and a Forma). Thankfully, both of them are quirky in that respect.
My main concern is, as always, with the H2O², and possibly the original Touch.

I've added a switch in Developer Options to disable it to test on demand (it's picked up on restart, no need for a full stop/start cycle), but that implies you actually can access that menu, which might be tricky if the rotation is fucked up.

(I've also added a toggle for debug/verbose debug mode in there, because I'm lazy :D).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 21, 2019

Also re-did a processing pass on the icons, as some of them were still not Grayscale, and a few could benefit from proper dithering.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 21, 2019

Ah, finally :)

Btw, I've been using this to turn on/off debug for small sequences of needs, without needing a restart (because a restart will flood the crash.log with debugs).

{
    text = _("Debug mode"),
    checked_func = function() return G_reader_settings:isTrue("debug") end,
    callback = function()
        G_reader_settings:flipTrue("debug")
        local logger = require("logger")
        local dbg = require("dbg")
        if G_reader_settings:isTrue("debug") then
            logger.info("DEBUG MODE ENABLED")
            dbg:turnOn()
        else
            dbg:turnOff()
            logger.info("DEBUG MODE DISABLED")
        end
    end,
},

because we already have that:

koreader/reader.lua

Lines 62 to 65 in a607629

-- should check DEBUG option in arg and turn on DEBUG before loading other
-- modules, otherwise DEBUG in some modules may not be printed.
local dbg = require("dbg")
if G_reader_settings:readSetting("debug") then dbg:turnOn() end

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 22, 2019

Good point, I should add something like that to the callback ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 22, 2019

On a slightly related matter, I have WIP-ish cbb tweaks in https://github.com/NiLuJe/koreader-base/tree/bbtype (and I think I finally groked why nightmode doesn't work w/ cbb on eInk ;)).

I still have some other stuff to play with (there's a RGB32 left in MuPDF's draw_next, IIRC; and the CRe RGB565 thingy for images; and a couple things in textwidget & htmlwidget), but that'll have to wait, as I'm going to be away next week ;).

(TL;DR: alpha is evil, RG565 is evil. :D).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 22, 2019

Also, ultimately, add that debug thingy to other platform's startup scripts (kindle/cervantes/pb/sony).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 22, 2019

@poire-z : Ooh, I missed the bit in reader.lua, I'll tweak the settings variable name.

NiLuJe added some commits Feb 14, 2019

The Great 8bpp Experiment
Swap to 8bpp on Kobo, because we're 'effing grayscale, for pete's sake!
Always swap to 8bpp, no matter the launch method.
Because it turned out that, even when restarting Nickel, we had to
restore the expected bitdepth ourselves, because pickel/Nickel didn't do
the job completely.

(I'm going to guess the grayscale flag wasn't getting flipped properly).
Allow the fbdepth switch to be disabled (in Developer settings).
Also, allow setting debug mode that way.

Pick those changes up on *restart* (nop need for a full stop/start
cycle).
As @poire-z helpfully reminded me, we can toggle debug mode right away
;).

Also, forcibly disable verbose logging when disabling debug.

@NiLuJe NiLuJe force-pushed the NiLuJe:road-runner branch from 1b313b4 to b93ad5b Feb 22, 2019

@Frenzie
Copy link
Member

Frenzie left a comment

If we're changing these binary resources anyway… do we, in fact, want to keep them as they are?

@Frenzie
Copy link
Member

Frenzie left a comment

What's the ImageMagick (I assume) command involved? Is it different than this one?

https://github.com/koreader/koreader/blob/b48b0d243f35f8b0f1157c1abea596dc466c0a36/resources/icons/README.md

end,
})
table.insert(self.menu_items.developer_options.sub_item_table, {
text = _("Don't drop fb bitdepth to 8bpp"),

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

Disable forced 8-bit color space?

# Do the fb depth switch, unless it's been disabled
ko_do_fbdepth
# Check if we wanted to enable debug logging...
DEV_ARGS=""

This comment has been minimized.

@Frenzie

Frenzie Feb 22, 2019

Member

Since we start with our settings before even parsing the command line arguments, this solution strikes me as somewhat overcomplicated?

koreader/reader.lua

Lines 29 to 30 in cfa946a

G_reader_settings = require("luasettings"):open(
DataStorage:getDataDir().."/settings.reader.lua")

koreader/reader.lua

Lines 82 to 85 in cfa946a

elseif arg == "-d" then
dbg:turnOn()
elseif arg == "-v" then
dbg:setVerbose(true)

This comment has been minimized.

@NiLuJe

NiLuJe Feb 22, 2019

Author Member

Yeah, that makes more sense, I wasn't aware of the reader.lua bit when I came up with that ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 22, 2019

If we're changing these binary resources anyway… do we, in fact, want to keep them as they are?

Meaning?

What's the ImageMagick (I assume) command involved? Is it different than this one?

A variant of https://www.mobileread.com/forums/showpost.php?p=3728291&postcount=17 (no scaling, and some icons were grayscaled in a gamma-corrected color space instead of in Linear space (ie. *Luma instead of *Luminance).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 22, 2019

Meaning?

Just a passing thought. The same source of these icons has come out with at least one other nice icon set over the years:

http://templarian.com/

https://github.com/Templarian/MaterialDesign

http://templarian.com/content/images/2017/07/Poster-Fixed.png

A variant of

Might want to add that to the resources README file. ;-)

@Frenzie Frenzie added the Kobo label Feb 22, 2019

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 22, 2019

(If we ever feel like updating icons, please choose some without big solid dark shape - I had to change the one shown on InfoMessage, because it was always letting some ghosting in the middle of the screen, for something like this one).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 22, 2019

There's one like that in this set: https://openclipart.org/tags/SFB-icons

mono-gnome-info

That user created a great many: https://openclipart.org/user-cliparts/dannya

Show resolved Hide resolved platform/kobo/koreader.sh Outdated
@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 23, 2019

@Frenzie Frenzie added this to the 2019.03 milestone Feb 23, 2019

Update platform/kobo/koreader.sh
Co-Authored-By: NiLuJe <ninuje@gmail.com>
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 23, 2019

Btw, where do we use stuff like this jpg icon thingy?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 23, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 23, 2019

Maybe — after due diligence of course — we should just delete those instead. It looks like they go back to 2012, presumably to be replaced by our current icons.

@Frenzie Frenzie modified the milestones: 2019.03, 2019.04 Mar 2, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Mar 3, 2019

Okay, I'm back :).

This should be ready, and I have an updated fbdepth w/ more aggressive rotation shenanigan recovery at the ready if the need arises ;).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 3, 2019

@NiLuJe If you're available for quick fixes should they be necessary then we can merge it for the current release window. I'm thinking 3-7 days from now for stability testing, translation updates, and dotting some is.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Mar 3, 2019

Yep, sounds good to me :).

@Frenzie Frenzie merged commit bb3f49a into koreader:master Mar 3, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 3, 2019

Alright, thanks!

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 3, 2019

Probably unrelated, might be a glitch or @NiLuJe updating its tree,, but the pipeline build triggered one hour ago failed - but only the Kobo one - with:

Cloning into 'libunibreak'...
Submodule path 'libunibreak': checked out '9c13f23169670adf50309ce23ece70bb858bf7d5'
Cloning into 'stb'...
fatal: reference is not a tree: 8eb119f31b9ba1939cbbc1cf41500f8d269c5d29
Unable to checkout '8eb119f31b9ba1939cbbc1cf41500f8d269c5d29' in submodule path 'stb'
CMake Error at /builds/koreader/nightly-builds/koreader/base/thirdparty/fbink/build/arm-kobo-linux-gnueabihf/git_checkout/tmp/fbink-gitclone-v1.11.1.cmake:132 (message):
  Failed to update submodules in:
  '/builds/koreader/nightly-builds/koreader/base/thirdparty/fbink/build/git_checkout/fbink'
@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Mar 3, 2019

Yeah, I stupidly force-pushed a submodule again, I'll fix it later tonight.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 3, 2019

@poire-z I only manually triggered a Kobo build. I thought it might be useful to have separate easily available builds of the Kobo 8-bit patch and @houqp 's decoupling just in case.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Mar 3, 2019

I can expedite a quick hotfix tag if need be.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Mar 3, 2019

Nah, I already burned that bridge after that fbink failure since the decoupling was really solid in my testing. ;-)

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.