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

A couple of fixes & tweaks #8250

Merged
merged 5 commits into from
Sep 25, 2021
Merged

A couple of fixes & tweaks #8250

merged 5 commits into from
Sep 25, 2021

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Sep 23, 2021

  • TextBoxWidget stuff for Crash after press button next page in wikipedia box search #8241
  • Runtime selection of online CPU core count on Kobo (Elipsa) to allow spending most of our time with only a single core online.
    (The 2 core switch is actually probably mostly overkill still, but, oh, well ;)).
  • BookInfoManager: Avoid undefined behavior in collectSubprocesses .

This change is Reviewable

@Frenzie Frenzie added this to the 2021.10 milestone Sep 23, 2021
@@ -381,6 +390,10 @@ function KoptInterface:renderOptimizedPage(doc, pageno, rect, zoom, rotation, re

local cached = DocCache:check(hash, TileCacheItem)
if not cached then
if hinting then
Device:enableCPUCores(2)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that all that's actually useful in theory or just all that's actually useful in current practice?

Copy link
Member Author

@NiLuJe NiLuJe Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already barely useful here, as we're heavily single-threaded ;p.

It's mainly for the sake of the eInk driver itself, which is fairly terrible (read: resource intensive ;p).

* Make sure we have a BB to measure in getSize, in case the instance is
  recycled. (fix koreader#8241)

* nil `line_num_to_image` early in `:free`

* Hide the _renderText calls that are used across the whole module to
  simply update the text layout & instantiate the inner bb behind a
  wrapper function with a slightly less obscure name.
cores

* Only keep a single core online most of the time.
* Device: Add an enableCPUCores method to allow controlling the amount of
  online CPU cores.
* Move the initial core onlining setup to Kobo:init, instead of the startup script.
* Enable two CPU cores while hinting new (e.g., cache miss) pages in PDF land.
* Enable two CPU cores while processing book metadata.
* Drive-by fix to isolate the DocCache pressure check to KoptInterface
  and actually apply it when it matters most (e.g., k2pdfopt stuff).
@NiLuJe NiLuJe force-pushed the master branch 2 times, most recently from a9c1041 to 1950ad1 Compare September 23, 2021 23:38
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 23, 2021

(Now includes UB cleanup in BookInfoManager:collectSubprocesses, because I saw weird things when testing this ;p).

@poire-z
Copy link
Contributor

poire-z commented Sep 24, 2021

Now includes UB cleanup

"UB" = undefined behaviour ?
It feels it wasn't undefined, but quite defined: we allowed having multiple collectSubprocesses at the same time :)
Or is it something else?

* Don't run multiple collectSubprocesses tasks in parallel.
  UIManager:scheduleIn doesn't return anything, using that as a boolean trap made no sense.

* Make the in-place removal of collected pids use a slightly more common idiom
  (table.remove in a reverse iteration of said array).
@@ -1122,6 +1134,7 @@ function TextBoxWidget:free(full)
self.cursor_restore_bb:free()
self.cursor_restore_bb = nil
end
self.line_num_to_image = nil
Copy link
Contributor

@poire-z poire-z Sep 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad idea: thumbnails in Wikipedia results and full articles are not shown after this (the room for the image is still there, but not image displayed, and long press in the hold select texts around).
Remove this line and all works again.
(Except for the numerous segfault I get when existing KOreader :/ I get segfaults with or without this line. Somehow, I can't get any when running under gdb...)

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 27, 2021
Turns out we can't release line_num_to_image early, so,
delay it until the final, full widget teardown.

(Regression since koreader#8250, c.f.,
koreader#8250 (comment))
NiLuJe added a commit that referenced this pull request Sep 27, 2021
Turns out we can't release line_num_to_image early, so, delay it until the final, full widget teardown.

(Regression since #8250, c.f., #8250 (comment))
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Oct 26, 2021
The second argument is a ddjvu_render_mode_t
Try to actually honor the user settings instead of enforcing COLOR
while we're there.

Fix koreader#8376
Regression since koreader#8250
Frenzie pushed a commit that referenced this pull request Oct 26, 2021
The second argument is a ddjvu_render_mode_t
Try to actually honor the user settings instead of enforcing COLOR
while we're there.

Fix #8376
Regression since #8250
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants