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

decouple render core & hardware from UI #4580

Merged
merged 15 commits into from Mar 3, 2019

Conversation

Projects
None yet
3 participants
@houqp
Copy link
Member

houqp commented Feb 11, 2019

All hardware dependent settings are abstracted into a new module called runtimectl.

This PR is just an internal refactoring and adds no new functionality. It should have no end user impact (assuming no bug is introduced). For developers, you will be able to use koreader core as a library without its own UI framework & hardware handling code.

Once this is merged, i will send a follow up PR to enable building a headless koreader with RPC support.

This PR depends on koreader/koreader-base#822.

@houqp houqp requested review from poire-z , Frenzie and NiLuJe Feb 11, 2019

@houqp houqp changed the title decouple render core & hardware from UI [WIP] decouple render core & hardware from UI Feb 11, 2019

@houqp houqp force-pushed the houqp:decouple branch 5 times, most recently from 7c2d025 to 7198227 Feb 11, 2019

@houqp houqp changed the title [WIP] decouple render core & hardware from UI decouple render core & hardware from UI Feb 11, 2019

@Frenzie
Copy link
Member

Frenzie left a comment

Look like a good refactoring to me, just a couple of minor comments/questions.

end
end
local prefix = config_defaults.prefix.."_"
for key, value in pairs(config_defaults.options) do

This comment has been minimized.

@Frenzie

Frenzie Feb 11, 2019

Member

Note to self: think about this in more detail later.

Show resolved Hide resolved frontend/cache.lua Outdated
Show resolved Hide resolved frontend/document/document.lua Outdated
Show resolved Hide resolved frontend/ui/data/credefaults.lua Outdated
Show resolved Hide resolved frontend/ui/data/credefaults.lua Outdated
Show resolved Hide resolved frontend/ui/data/credefaults.lua Outdated
Show resolved Hide resolved frontend/ui/data/creoptions.lua Outdated
Show resolved Hide resolved frontend/ui/data/koptdefaults.lua Outdated
Show resolved Hide resolved kodev Outdated
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 11, 2019

I'm a bit confused about when I will have to use Screen:getWidth() or Runtimectl:getRenderWidth() :| And where should we add new stuff/features: in Screen/Device or Runtimectl?

Well, it looks like some of this stuff may have some sense, but it also looks like it adds indirection, complexity and shuffle a bit what I'm used to :)
Can you provide a bit of context for why that is needed? Some use cases, some hints about that secret project of yours that needed that refactoring? :) so at least we have some alternative real thing/device to think about/not forget when we'll modify stuff.

One thing that bothers me a bit is the separation of the creoptions/koptoptions defaults from the widget definitions and available values. Will make reading both less easy, and harder to notice when a default drift away from the available options (not that we change them that often, but still :).

And the name Runtimectl looks a bit ugly :) RuntimeCtl ? Does that imply we can change DPI at runtime now without the need to restart koreader? Still a bit of a too abstract name to me. Might be that name make sense in that context of yours, but currently not in mine :) A better more explicit name might just cllarify all that :)

@houqp houqp force-pushed the houqp:decouple branch 2 times, most recently from 0e613c9 to b134f18 Feb 12, 2019

@houqp

This comment has been minimized.

Copy link
Member Author

houqp commented Feb 12, 2019

I'm a bit confused about when I will have to use Screen:getWidth() or Runtimectl:getRenderWidth()

Short answer is use Screen for UI and RuntimeCtl for document rendering code. Right now, some of the document rendering code still lives with UI code, so it's confusing. It will be clear when we completely separate the UI code (frontend) and document handling code (backend) into two folders, then we can say only use Screen for code inside ui folder.

Well, it looks like some of this stuff may have some sense, but it also looks like it adds indirection, complexity and shuffle a bit what I'm used to :)

Same here, I hate unnecessary abstractions so I try to avoid it whenever I can. See below for why I added RuntimeCtl.

One thing that bothers me a bit is the separation of the creoptions/koptoptions defaults from the widget definitions and available values. Will make reading both less easy, and harder to notice when a default drift away from the available options (not that we change them that often, but still :).

I agree, I will see if there is better way to organize this. The problem here is available options are tightly coupled with UI texts, so we can't simply make it part of the defaults. Maybe the defaults should just be part of the UI and not provided by the core.

And the name Runtimectl looks a bit ugly :)

Yeah, naming is hard, not a fan of this name either. Would love to get some suggestions.

RuntimeCtl ? Does that imply we can change DPI at runtime now without the need to restart koreader? Still a bit of a too abstract name to me. Might be that name make sense in that context of yours, but currently not in mine :) A better more explicit name might just cllarify all that :)

Short answer is no, since RuntimeCtl is not supposed to be used by the UI so changing the value in this module will only affect document rendering result. This module is created to mainly abstract hardware settings. For example, mupdf has a function called layoutDocument, it just takes expected width and height as argument, not the actual screen object. It would be weird for mupdf to depend on hardware implementation details. Crengine does the same thing when you open a document. It's the same idea here, you can think of RuntimeCtl as a unified abstraction to provide device specific rendering setting to all rendering engines. All the device specific rendering settings are injected through the init method so RuntimeCtl doesn't care what device is being used. If fact, you don't even need to pass in a real device object, you can can just pass in a table with required rendering settings.

The goal of this PR is to make it possible to use the core document parsing & rendering logic as a lua library without having to spin up the whole builtin UI framework or go through any hardware handling code:

...
local Runtimectl = require("runtimectl")
local Device = require("device/dummy/device"):new{
  viewport = require("ui/geometry"):new{x=0, y=0, w=600, h=800}
}
Runtimectl:init(Device)
local doc = require("document/documentregistry"):openDocument("path.pdf")
print("page count", doc:getPageCount())
local tile = doc:renderPage(1, {x = 0, y = 0, w = 600, h = 800}, 1, 0, 0, 0)
...

With this, you can build powerful CLI tools in LUA to automate any kind of document processing tasks. For example, batch document reflow or metadata indexing.

Next PR would be adding headless koreader mode, which exposes the LUA API through RPC interfaces (gRPC for now). This makes is possible to build frontend in any language & framework. Also makes it very easy to run the core rendering logic in a separate process.

This is something I started a long time ago to solve two pain points:

  1. While trying to solve various memory leak issues that's causing reader crashes. I noticed no matter how good we are at catching those bugs, we won't be able to get rid of all of them. That means, for some books, an eventual reader crash is almost inevitable. The next best thing we can do is to run the rendering core in a separate process so that the frontend (UI) can just restart the core whenever it sees fit without user impact.

  2. I noticed I have spent way too much time fighting the builtin UI framework than I would like to. Almost every time I create a new UI component (e.g. network manager), I would run into bugs in the framework and got distracted by fixing bugs (lots of pixel alignment issues). And to be fair, the learning curve is also pretty high for new contributors because it's not very documented and apis are not very well designed (I take blame for this). For example, having to put Screen:scaleyBySize everywhere makes me sad ;(. Plus the builtin UI framework doesn't work well under environments where JIT can not function properly, for example: ios, android. If we really want to deliver a good cross platform experience, we need something better. I am hoping this change would make it easier for people to explore other frontend options.

I stopped working on this refactoring two years ago due to lack of free time. Recently, I have been thinking of writing a prototype VR reader for fun, so here I am, trying to finish up an old refactoring :)

Here is a screenshot of I what I built during the weekend:

demo reader_001

It's a 150 lines electron reader app powered by koreader core. Disclaimer: I am not a fan of electron at all, it's too bloated for my taste. I am only using it for fast prototyping because I am already very familiar with html canvas.

my ultimate goal is to play around with a prototype VR reader and I am definitely not interested in adding VR support to the existing UI framework :P

@houqp houqp changed the title decouple render core & hardware from UI [WIP] decouple render core & hardware from UI Feb 12, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 12, 2019

Yeah, naming is hard, not a fan of this name either. Would love to get some suggestions.

Something with a word like render or context?

I am only using it for fast prototyping because I am already very familiar with html canvas.

Funny, I considered replying with that exact example (HTML canvas, that is).

I've been entertaining the notion of creating a similar abstraction but I never got around to it. I was thinking less drastically, more in the direction of making the GPU take care of a bunch of stuff that it currently doesn't, using e.g. GL surface textures on Android and SDL, but essentially with the same UI framework even though the abstraction would've made that optional.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 12, 2019

Something with a word like render or context?

In fact I just noticed you suggestively changed the PR title to RenderCore.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 12, 2019

Thanks for the detailed answer and story.

Yeah, naming is hard, not a fan of this name either. Would love to get some suggestions.

Something with a word like render or context? [...] Funny, I considered replying with that exact example (HTML canvas, that is).

Why not Canvas then ? :) or DocumentCanvas?
That word does not exist in our code base, and I guess a canvas can have a width and height, some DPI, and be colored or not. Hardware screen vs Document canvas? Would talk to me when thinking about which to use!

Regarding your two old pain points (which might not be what you're focusing on these days, just updating you after your long absence):

various memory leak issues that's causing reader crashes [...] That means, for some books, an eventual reader crash is almost inevitable. The next best thing we can do is to run the rendering core in a separate process so that the frontend (UI) can just restart the core whenever it sees fit without user impact.

Memory usage growing is still happening. But I'm not sure it's always the rendering libraries or glue fault. It could very well be the UI/luajit code. Or (my intuition) the interleaving of both in a single process, making freed memory holes often not reused. Having these decoupled in 2 processes would help pinpoint which of them is the most reponsible for that, but I'm not sure a restart of the UI would not be needed at some point too.

got distracted by fixing bugs (lots of pixel alignment issues)

Spent a lot of time fixing these last year. I hope we are now past having that distraction.

having to put Screen:scaleyBySize everywhere

@Frenzie had shoved most of them in the Size.lua module a year ago too.

I am hoping this change would make it easier for people to explore other frontend options.

I understand. But we should try to not make the current real/existing/used/delivered/active lua/ui/widgets even more abstract (and so, even more hard to learn) in the hope to allow the birth of other frontend code (haven't read anyone wishing to become such a parent before now :)
Also, if you remove the UI code, the little glue to tweak/pilot the C libraries is very thin (except may be for our kopt stuff), and could very well be made into some C or those new frontends language, without having to bring luajit/luaffi in that new frontend environment. The added value of koreader on that front is tiny, and, if I had to build a new frontend in another language, I would use it as an inspiration, but not the code as is.

I am definitely not interested in adding VR support to the existing UI framework :P

Thanks ! :) (There is a huge PR in coolreader github repo about something similar, that must make the devs there quite perplexed.)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 12, 2019

@Frenzie had shoved most of them in the Size.lua module a year ago too.

My main motivation there was standardization of sizes for UI consistency, but yes, the basic idea is that 9 times out of 10 you shouldn't be touching scaleBySize directly.

@Frenzie Frenzie added the enhancement label Feb 17, 2019

@houqp houqp force-pushed the houqp:decouple branch from b134f18 to bb7d566 Feb 19, 2019

Show resolved Hide resolved frontend/ui/font.lua Outdated
@Frenzie
Copy link
Member

Frenzie left a comment

Assuming only the name changed in the most recent update, it's good to go in my book.

@houqp

This comment has been minimized.

Copy link
Member Author

houqp commented Feb 19, 2019

I haven't had time to write up the comment yet, you guys review too fast :P I will work on the new feedback later this week.

Summary for this round of update:

  • Renamed runtimectl to CanvasContext, I suffixed the name with Context to signal that this module does not has an buffer you can blit to (like html canvas).
  • Merged defaults back to creoptions & koptoptions
    • However, part of the kopt default options is duplicated in koptinterface. I am planning to import those defaults from koptoptions to avoid duplication.

Memory usage growing is still happening. But I'm not sure it's always the rendering libraries or glue fault. It could very well be the UI/luajit code. Or (my intuition) the interleaving of both in a single process, making freed memory holes often not reused. Having these decoupled in 2 processes would help pinpoint which of them is the most reponsible for that, but I'm not sure a restart of the UI would not be needed at some point too.

Yeah, I was referring to the nasty memory issues I ran into in the past, they were all from rendering libraries. I am hoping for a mature & battle tested UI library, we should be able to get rid most of the memory leaks from the UI side. Otherwise, we would be seeing memory leak issues from all other programs :)

Spent a lot of time fixing these last year. I hope we are now past having that distraction.

Yeah, we are definitely in a much better state today. I am very impressed by how fast you picked up the code base :)

I understand. But we should try to not make the current real/existing/used/delivered/active lua/ui/widgets even more abstract (and so, even more hard to learn) in the hope to allow the birth of other frontend code (haven't read anyone wishing to become such a parent before now :)
Also, if you remove the UI code, the little glue to tweak/pilot the C libraries is very thin (except may be for our kopt stuff), and could very well be made into some C or those new frontends language, without having to bring luajit/luaffi in that new frontend environment. The added value of koreader on that front is tiny, and, if I had to build a new frontend in another language, I would use it as an inspiration, but not the code as is.

Yes, my plan is to decouple the core from UI and keep the existing UI framework as is. I don't expect to introduce more refactoring to existing UI code base.

For the core, I think, we should consider slowly converting the glue code into C/Rust/C++ and get rid of Lua as dependency. The flexibility of Lua is useful for quickly customizing UIs and high level features with just an editor. The render core logic is fairly static compared to the rest.

@houqp houqp force-pushed the houqp:decouple branch 2 times, most recently from add3c9a to f42db6e Feb 19, 2019

@houqp houqp force-pushed the houqp:decouple branch from 429c7ae to 055c740 Feb 24, 2019

@houqp houqp force-pushed the houqp:decouple branch from 055c740 to 0e2a3f1 Mar 3, 2019

@houqp houqp changed the title [WIP] decouple render core & hardware from UI decouple render core & hardware from UI Mar 3, 2019

@houqp

This comment has been minimized.

Copy link
Member Author

houqp commented Mar 3, 2019

@Frenzie @poire-z ready for next round of review :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Mar 3, 2019

No more comment. Tested on my Kobo, no apparent issue.

@Frenzie Frenzie force-pushed the houqp:decouple branch from 60ad49b to 6680ae6 Mar 3, 2019

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

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
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.