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

Enable HW dithering in a few key places #4541

Merged
merged 92 commits into from Feb 7, 2019

Conversation

Projects
None yet
4 participants
@NiLuJe
Copy link
Member

NiLuJe commented Feb 1, 2019

Depends on BASE#800 (in particular for more details about HW support).

  • ScreenSaver
  • ImageViewer
  • BookStatus
  • History (only the initial display, I couldn't track down what's triggering the refresh on pagination, I can only tell that what wins is an "all", "ui").
@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

Stupid query @ @poire-z: I imagine it'd be fairly complex to get CRe to tell us that the "to be shown" page contains an image, right?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

I'm open to suggestion, ideas for stuff that mainly show images that I may have missed, and if someone can figure out what's triggering refreshes on page switches in the History menu ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

(This doesn't apply to thumbnails in the FM proper, as those are too small to matter).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

Re #1833

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

Would probably be nice to have, albeit in a user-configurable manner (since content may already be properly pre-processed) in the CBZ codepath (re: #4290)?

Except I have no idea what handles CBZ on our end :D MµPDF?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 1, 2019

CBZ is MuPDF, yes.

registry:addProvider("cbz", "application/vnd.comicbook+zip", self, 100)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 1, 2019

I imagine it'd be fairly complex to get CRe to tell us that the "to be shown" page contains an image, right?

May be not. The places where an image is drawn to a buffer are not legion.
But there are lots of recursive calls from the doc->text_view->Draw(drawBuf, false) in base/cre.cpp down to them.
So, passing thru each a by-ref parameter seems heavy, but why not have the initial Draw reset a global variable to 0, and any Draw image set it to 1?
I can have a look over the weekend if you wish and that sounds good enough.

(This doesn't apply to thumbnails in the FM proper, as those are too small to matter).

You can have them as big as in History if you set Display mode to Mosaic with cover images for both.

History (only the initial display, I couldn't track down what's triggering the refresh on pagination, I can only tell that what wins is an "all", "ui").

The next displays, when the book is not yet cached in the db, must (it's been a while :) be done in:

self.items_update_action = function()
logger.dbg("Scheduled items update:", #self.items_to_update, "waiting")
local is_still_extracting = BookInfoManager:isExtractingInBackground()
local i = 1
while i <= #self.items_to_update do -- process and clean in-place
local item = self.items_to_update[i]
item:update()
if item.bookinfo_found then
logger.dbg(" found", item.text)
local refreshfunc = function()
if item.refresh_dimen then
-- MosaicMenuItem may exceed its own dimen in its paintTo
-- with its "description" hint
return "ui", item.refresh_dimen
else
return "ui", item[1].dimen
end
end
UIManager:setDirty(self.show_parent, refreshfunc)
table.remove(self.items_to_update, i)

I'm totally unfamiliar with dithering :)
So, how does it help? Some pictures may be for those who don't have these high-end devices? :)
And why not do it everytime? Does it have a bad effect if used on text-only pages?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

@poire-z : If the CRe thing is not too much trouble, sure, that'd be nice to have, although it's fairly low priority ;).

I'll take another look at covermenu, thanks ;). (I did play a bit with it, but what I tried was affecting every FM refresh, even when only showing toplevel directories with no images, which is a tad too aggressive ;)).

As for Mosaic mode, that's another widget though, right? (Ideally, the same as the History? ;)). I was vaguely hoping that handling History properly would take care of Mosaic ;).

I can't really show screenshots, because the processing pass is done by the display controller, the OS doesn't get any of it back (i.e., fb content is untouched).
It's a somewhat irregular square-ish ordered dither pattern.
(c.f., IM's doc for examples of traditional patterns).
But, yes, quantization is technically a highly destructive process, so you don't want to do it when you don't have to ;). (Plus, there may be some power consumption considerations, although I suspect those would be fairly insignificant).

I'll try to see how that looks when taking pictures of the screen tomorrow (need sunlight ;)).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 1, 2019

I'll take another look at covermenu, thanks ;). (I did play a bit with it, but what I tried was affecting every FM refresh, even when only showing toplevel directories with no images, which is a tad too aggressive ;)).

As for Mosaic mode, that's another widget though, right? (Ideally, the same as the History? ;)). I was vaguely hoping that handling History properly would take care of Mosaic ;).

Look at the comment at the top of plugins/coverbrowser/covermenu.lua for some details.
main.lua just plug and replace existing methods of FM and History (History is a bit different as a new instance is created each time).
covermenu.lua implement the common methods, and CoverMenu:updateItems() does a setDirty(), which might be the only one you need to tweak.
mosaicmenu.lua and listmenu.lua implements some others, but are mainly wrapped by covermenu.lua stuff: they deal with the different layouts, and each has its own little widget for each book vignette: MosaicMenuItem and ListMenuItem. These are drawn directly on the initial view, and may be updated later (their :update() called by CoverMenu when the cover has been fetched, and that xxMenuItem can redraw itself.
I guess you (or I) should add somewhere a flag if there are xxMenuItem with images, or none (or just loop thru the items to check that) so you can decide (in CoverMenu:updateItems() for the initial display, and for the individual XXMenuItem) if dithering should be used.

I guess you may then not have to touch at the frontend FM or History code, as the "classic" display mode they themselves implement doesn't need dithering.

(Also, I'm not used seeing UIManager:show() with refresh parameters - I usually see them only in setDirty())

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 1, 2019

Thanks, I'll take another look at it tomorrow ;).

(Yeah, definitely agree that those UIManager:show() look clunky. Hell, I got it wrong the first time around ;)).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 2, 2019

If the CRe thing is not too much trouble, sure, that'd be nice to have

Quite simple and clean in the end:

--- a/crengine/include/lvdrawbuf.h
+++ b/crengine/include/lvdrawbuf.h
@@ -228,6 +228,8 @@ protected:
     lUInt32 _backgroundColor;
     lUInt32 _textColor;
     bool _hidePartialGlyphs;
+    int _drawnImagesCount;
+    int _drawnImagesSurface;
 public:
     virtual void setHidePartialGlyphs( bool hide ) { _hidePartialGlyphs = hide; }
     /// returns current background color
@@ -262,8 +264,14 @@ public:
     */
     /// draws formatted text
     //virtual void DrawFormattedText( formatted_text_fragment_t * text, int x, int y );
+
+    /// Get nb of images drawn on buffer
+    int getDrawnImagesCount() { return _drawnImagesCount; }
+    /// Get surface of images drawn on buffer
+    int getDrawnImagesSurface() { return _drawnImagesSurface; }

-    LVBaseDrawBuf() : _dx(0), _dy(0), _rowsize(0), _data(NULL), _hidePartialGlyphs(true) { }
+    LVBaseDrawBuf() : _dx(0), _dy(0), _rowsize(0), _data(NULL), _hidePartialGlyphs(true),
+                        _drawnImagesCount(0), _drawnImagesSurface(0) { }
     virtual ~LVBaseDrawBuf() { }
 };

--- a/crengine/src/lvdrawbuf.cpp
+++ b/crengine/src/lvdrawbuf.cpp
@@ -709,6 +709,8 @@ void LVGrayDrawBuf::Draw( LVImageSourceRef img, int x, int y, int width, int hei
         return;
     LVImageScaledDrawCallback drawcb( this, img, x, y, width, height, dither );
     img->Decode( &drawcb );
+    _drawnImagesCount++;
+    _drawnImagesSurface += width*height;
 }


@@ -1299,6 +1301,8 @@ void LVColorDrawBuf::Draw( LVImageSourceRef img, int x, int y, int width, int he
     //fprintf( stderr, "LVColorDrawBuf::Draw( img(%d, %d), %d, %d, %d, %d\n", img->GetWidth(), img->GetHeight(), x, y, width, height );
     LVImageScaledDrawCallback drawcb( this, img, x, y, width, height, dither );
     img->Decode( &drawcb );
+    _drawnImagesCount++;
+    _drawnImagesSurface += width*height;
 }

 /// fills buffer with specified color
--- a/cre.cpp
+++ b/cre.cpp
@@ -2294,6 +2294,9 @@ static int drawCurrentPage(lua_State *L) {
        int w = bb->w,
                h = bb->h;

+       int drawn_images_count;
+       int drawn_images_surface;
+
        doc->text_view->Resize(w, h);
        doc->text_view->Render();
        if (color) {
@@ -2302,14 +2305,20 @@ static int drawCurrentPage(lua_State *L) {
                  * for why not TYPE_BBRGB32) */
                LVColorDrawBuf drawBuf(w, h, bb->data, 16);
                doc->text_view->Draw(drawBuf, false);
+               drawn_images_count = drawBuf.getDrawnImagesCount();
+               drawn_images_surface = drawBuf.getDrawnImagesSurface();
        }
        else {
                /* Set DrawBuf to 8bpp */
                LVGrayDrawBuf drawBuf(w, h, 8, bb->data);
                doc->text_view->Draw(drawBuf, false);
+               drawn_images_count = drawBuf.getDrawnImagesCount();
+               drawn_images_surface = drawBuf.getDrawnImagesSurface();
        }

-       return 0;
+       lua_pushinteger(L, drawn_images_count);
+       lua_pushnumber(L, (float)drawn_images_surface/(w*h));
+       return 2;
 }

 static int drawCoverPage(lua_State *L) {
--- a/frontend/document/credocument.lua
+++ b/frontend/document/credocument.lua
@@ -323,7 +323,7 @@ function CreDocument:drawCurrentView(target, x, y, rect, pos)
     -- change has been made, to avoid having crengine redraw the exact
     -- same buffer.
     -- And it could only change when some other methods from here are called
-    self._document:drawCurrentPage(self.buffer, self.render_color)
+    self._drawn_images_count, self._drawn_images_surface = self._document:drawCurrentPage(self.buffer, self.render_color)
     target:blitFrom(self.buffer, x, y, 0, 0, rect.w, rect.h)
 end

--- a/frontend/document/document.lua
+++ b/frontend/document/document.lua
@@ -402,6 +402,11 @@ function Document:drawPage(target, x, y, rect, pageno, zoom, rotation, gamma, re
         rect.w, rect.h)
 end

+function Document:getDrawnImagesStatistics()
+    -- Only implemented for credocument
+    return self._drawn_images_count, self._drawn_images_surface
+end
+
 function Document:getPageText(pageno)
     -- is this worth caching? not done yet.
     local page = self._document:openPage(pageno)

And the fun part probably in ReaderView, not sure how you want to go from there:

--- a/frontend/apps/reader/modules/readerview.lua
+++ b/frontend/apps/reader/modules/readerview.lua
@@ -213,6 +213,8 @@ function ReaderView:paintTo(bb, x, y)
     end
     -- stop activity indicator
     self.ui:handleEvent(Event:new("StopActivityIndicator"))
+    -- Here we have that info, but might be too late
+    logger.info("drawn images statistics:", self.ui.document:getDrawnImagesStatistics())
 end

 --[[
@@ -579,6 +581,8 @@ function ReaderView:recalculate()
         self.state.offset.x = (self.dimen.w - self.visible_area.w) / 2
     end
     -- flag a repaint so self:paintTo will be called
+    -- Here is too early to get that info :(
+    -- logger.info("drawn images statistics:", self.ui.document:getDrawnImagesStatistics())
     UIManager:setDirty(self.dialog, "partial")
 end
Wikipedia page with many small png (from converted SVG math stuff):
02/02/19-09:28:53 INFO  drawn images statistics: 9 0.017349727451801 (9 images on 1.7% of screen)
02/02/19-09:28:54 INFO  drawn images statistics: 1 0.0026775957085192
02/02/19-09:28:55 INFO  drawn images statistics: 1 0.13198360800743
02/02/19-09:28:55 INFO  drawn images statistics: 1 0.0026775957085192
02/02/19-09:28:56 INFO  drawn images statistics: 9 0.017349727451801
02/02/19-09:28:56 INFO  drawn images statistics: 7 0.2694316804409
02/02/19-09:28:57 INFO  drawn images statistics: 5 0.041595626622438
02/02/19-09:28:58 INFO  drawn images statistics: 11 0.065131150186062
Wikipedia page with pictures and high Zoom/DPI:
02/02/19-09:29:37 INFO  drawn images statistics: 1 0.42317485809326 (1 image on 42% of screen)
02/02/19-09:29:37 INFO  drawn images statistics: 0 0
02/02/19-09:29:37 INFO  drawn images statistics: 0 0
02/02/19-09:29:37 INFO  drawn images statistics: 1 0.6373907327652
02/02/19-09:29:38 INFO  drawn images statistics: 0 0
02/02/19-09:29:38 INFO  drawn images statistics: 0 0
02/02/19-09:29:38 INFO  drawn images statistics: 1 0.43631693720818
02/02/19-09:29:38 INFO  drawn images statistics: 1 0.54013931751251

We can trash the surface stuff if you don't need it.
crengine_drawn_images_stats.diff.txt
base_drawn_images_stats.diff.txt
frontend_drawn_images_stats.diff.txt

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

@poire-z : 👍

And, yeah, the surface stats might come in handy, I was already thinking about something like that w/ regions in UIManager to avoid triggering dithering when there's only really tiny stuff ;).

Just have to find the right place in the frontend to make the decision... :D.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

As far as pictures go, this is far from accurate, but does give a rough idea ;).

Original:
original

Properly Preprocessed via IM:
im_dithered

Original w/ HW dithering:
hw_dithered

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 2, 2019

IM definitely looks better on the picture than the HW implementation. ;-)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 2, 2019

(Properly Preprocessed via IM displayed on your kobo: is this with or without the HW dithering ?! I guess still with - otherwise we could just do the preprocessing in software if it would display as good.)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 2, 2019

Yeah, but that's probably a touch slow. :-)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

IM is entirely SW (and fairly expensive) preprocessing, displayed without HW dithering ;).

i.e., the source looks like this (as opposed to the original):
forma_im

(That goes to show that even on an LCD screen, you'd be hard pressed to tell that there's only 16 shades of grey in there ;)).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

So, yeah, it looks better, but that was to be expected ;).

On the other hand, the HW result, which we essentially get for free, is more than good enough, even on a worse than worst case scenario like this test ;).

EDIT: And on the upside, it doesn't horribly mangle properly preprocessed content either, so, that's good, too ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

@poire-z : No luck yet on the FM/History front, but good news about CRe:

Sticking a

-- If we're attempting to show a large enough amount of image data, request dithering (without triggering another repaint ;)).
    UIManager:setDirty(nil, "partial", nil, true)

at the bottom of ReaderView:paintTo appears to do the trick ;). (Excerpt, err, I'm enforcing it just to test, obviously, real code would check the data and do that only if, say > 50% of the screen is image).

TL;DR: If you're happy about your CRe changes, and we want to go forward with this, you can go ahead with it, and I'll make use of it here ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 2, 2019

I'm OK with the cre stuff.
@Frenzie : it's early february, so... are you in a release state of mind and we should hold a bit ? or shall we go ahead? (I don't want to commit stuff to base if you ever need to bump base again for some android last-minute tweaks.)

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 2, 2019

Oh yeah, sure. I'll probably wait until tomorrow to give translators a little more time with the latest strings. (And I just finished up German myself.)

Android is fine.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

Okay, handled CoverMenu w/ the large hammer approach, it dithers a tiny bit too much, but there's probably not much we can do about it short of uglier hacks.

Speaking of, I tried to honor the dithering status of widgets across time, so that, f.g., when you close the menu on top of the FM, that refresh, which is usually not dithered (as the menu isn't), will now be dithered (as the FM is).
It's a bit tricky because for some reason I can't consume refreshtype in setDirty when it's a function without ruining it, so I have to do some guesswork in _repaint (when refreshtype gets consumed for real) instead.
I'm not a fan of both the guesswork, and the extra window stack walk :/

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 2, 2019

I think you can drop that hammer with the following (your setDirty are just after my logger.info).
(And you can may be now drop your change in filemanagerhistory.lua?)

--- a/plugins/coverbrowser.koplugin/covermenu.lua
+++ b/plugins/coverbrowser.koplugin/covermenu.lua
@@ -77,7 +77,9 @@ function CoverMenu:updateItems(select_number)
     collectgarbage()

     -- Specific UI building implementation (defined in some other module)
+    self._has_cover_images = false
     self:_updateItemsBuildUI()
+    logger.info("covermenu has cover images?", self._has_cover_images)

     -- Set the local variables with the things we know
     current_path = self.path
@@ -137,6 +139,7 @@ function CoverMenu:updateItems(select_number)
                 item:update()
                 if item.bookinfo_found then
                     logger.dbg("  found", item.text)
+                    logger.info("covermenu updated item has cover image?", item._has_cover_image)
                     local refreshfunc = function()
                         if item.refresh_dimen then
                             -- MosaicMenuItem may exceed its own dimen in its paintTo
--- a/plugins/coverbrowser.koplugin/listmenu.lua
+++ b/plugins/coverbrowser.koplugin/listmenu.lua
@@ -288,6 +288,9 @@ function ListMenuItem:update()
                             wimage,
                         }
                     }
+                    -- Let menu know it has some item with images
+                    self.menu._has_cover_images = true
+                    self._has_cover_image = true
                 else
                     -- empty element the size of an image
                     wleft = CenterContainer:new{
--- a/plugins/coverbrowser.koplugin/mosaicmenu.lua
+++ b/plugins/coverbrowser.koplugin/mosaicmenu.lua
@@ -498,6 +498,9 @@ function MosaicMenuItem:update()
                         image,
                     }
                 }
+                -- Let menu know it has some item with images
+                self.menu._has_cover_images = true
+                self._has_cover_image = true
             else
                 -- add Series metadata if requested
                 if bookinfo.series then

if img_count > 0 and img_coverage >= 0.50 then 50% feels a bit too high! Some images in wikipedia pages with the normal 96dpi probably use less than 10%, so I guess I'd go with as low as 5% :)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 2, 2019

@poire-z : Eh, I'll check it out ;). [Yup, that works! 👍]

In other news, implemented the toggle fairly generically for KOpt via a per-document user-configurable switch.
That made me look into why panning wasn't using "fast"... and it turns out we can't really, there's always something that gets coalesced into it, upgrading that to at least "ui".
Anyway, not a huge fan of either 2bit (or semi half-toned w/ HW dithering) panning anyway, it's too jarring, and doesn't net us much reactivity anyway.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 2, 2019

Cool (don't forget to remove my logger.info()s when done testing :)

@@ -254,7 +256,8 @@ function BookStatusWidget:setStar(num)

table.insert(self.stars_container, stars_group)

UIManager:setDirty(nil, "ui")
-- Individual stars are Buttons, which'll trigger a flash on their own, we want to refresh the group *after* that...
UIManager:scheduleIn(0.5, function() UIManager:setDirty(self.stars_container, "ui", nil, true) end)

This comment has been minimized.

@Frenzie

Frenzie Feb 6, 2019

Member

@NiLuJe While looking over it quickly (not in detail) only this jumps out, but given the context it can't be too bad. ;-)

This comment has been minimized.

@NiLuJe

NiLuJe Feb 6, 2019

Author Member

Yeah, AFAICT, it's a bit of a race condition...

I'll double-check how it behaves with a tickAfterNext, though ;). (With a bit of luck, that'll get merged with the one from Button).

This comment has been minimized.

@NiLuJe

NiLuJe Feb 6, 2019

Author Member

Huh, turned out to be fairly broken, in various respects ;D.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 6, 2019

@poire-z : Yep, I did ;).

Nothing's changed, both effectively, and in the code, as that's behind a flash_ui config branch ;).

NiLuJe added some commits Feb 6, 2019

Fix BookStatusWidgets stars
Turned out to be broken when testing on a Touch.
Either there's a weird quirk on my Forma or my Touch,
or I've fixed spurious redraws since?

In any case, stars_container isn't a window, so the setDirty call wa
sbroken anyways.

Can't do targetet repaints, because we don't have the coordinates of the
stars container, AFAICT.

So just repaint the full widget.
No need for any delay, it should get merged with the Button unhilight w/
flash_ui, like everywhere else with that sort of interaction.
Much like ImageViewer, switch ImageWidget panning to "ui" to avoid REAGL
on image content.

(Even on non-REAGL devices, on Kindle, GC16_FAST is better suited to
image content than GL16_FAST).
Revert "Targeted repaint in ImageWidget panning?"
This reverts commit 4967ed2.

Not quite sure it won't have any adverse effect somewhere...

@NiLuJe NiLuJe merged commit 812e595 into koreader:master Feb 7, 2019

1 check passed

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

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 7, 2019

Just realized there's a cosmetic issue with the highlight being on in a single button's frame in Dialogs.

The button redraw also redraws a button with rounded corners (i.e., it's pill shaped), which is usually fine, except when the corners of the frame aren't redrawn, too (c.f., Wikipedia button with WiFi off).

It took me three different devices for it to register that that wasn't normal behavior :D.

NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Feb 7, 2019

NiLuJe added a commit that referenced this pull request Feb 7, 2019

A few graphics fixes after #4541 (#4554)
* Various FocusManager related tweaks to limit its usage to devices with a DPad, and prevent initial button highlights in Dialogs on devices where it makes no sense (i.e., those without a DPad. And even on DPad devices, I'm not even sure how we'd go about making one of those pop up anyway, because no Touch ;)!).
* One mysterious fix to text-only Buttons so that the flash_ui highlight always works, and always honors `FrameContainer`'s pill shape. (Before that, an unhighlight on a text button with a callback that didn't repaint anything [say, the find first/find last buttons in the Reader's search bar when you're already on the first/last match] would do a square black highlight, and a white pill-shaped unhighlight (leaving the black corners visible)).
The workaround makes *absolutely* no sense to me (as `self[1] -> self.frame`, AFAICT), but it works, and ensures all highlights/unhighlights are pill-shaped, so at least we're not doing maths for rounded corners for nothing ;).
@ptrm

This comment has been minimized.

Copy link

ptrm commented Feb 8, 2019

Works great on my Clara HD, thanks :)

One question, though: when opening the test png direct from file manager, no dithering is present (on screensavers it's there alright). Is it supposed to be so?

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 8, 2019

This morning Android nightly crashes on start (may be because I had a PDF as last open document) on my phone (Android6). In logcat:
Failed to run script: frontend/ui/data/koptoptions.lua:261: attempt to call method "hasEinkScreen' (a boolean value)

Before that, I have 2 unrelated warnings, just for info:
.../files/common/seralize.so: unused DT entry: type 0xf arg 0x2ca .../files/common/seralize.so: is missing DT_SONAME will use basename as a replacement: "serialize.so".
and before that, the same 2 warnings for libluacompat52.so.

@ptrm

This comment has been minimized.

Copy link

ptrm commented Feb 8, 2019

This morning Android nightly crashes on start (may be because I had a PDF as last open document) on my phone (Android6). In logcat:
Failed to run script: frontend/ui/data/koptoptions.lua:261: attempt to call method "hasEinkScreen' (a boolean value)

@poire-z Isn't that #4552 related?

EDIT: Anyways, crashing on Android 4.2.2 as well, last file was an epub.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 8, 2019

@ptrm In a manner of speaking. The crash is directly caused by a line added in this PR for dithering, but the deeper cause is that this line should be a function instead:

hasEinkScreen = android.isEink(),

(i.e., function() return android.isEink() end)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Feb 8, 2019

I haven't followed the recent android enhancements, but I don't think so. Line 261 of koptoptions.lua has been added by this PR.
But yes, most probably #4517 as @Frenzie pointed out.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 8, 2019

I'll fix it up.

Frenzie added a commit to Frenzie/koreader that referenced this pull request Feb 8, 2019

Frenzie added a commit that referenced this pull request Feb 8, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 8, 2019

I manually triggered new builds, so within half an hour there'll be new Android nightlies that don't crash on paged media.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Feb 8, 2019

D'oh, I forgot about the fact that the boolean is used elsewhere. Serves me right for doing a 10-second fix. >_> Unfortunately I failed to recognize the boolean vs function problem a few weeks ago during code review.

Anyway, that means everyone will see the E Ink settings screen in that nightly. Surely that's a better bug than crashing.

@ptrm

This comment has been minimized.

Copy link

ptrm commented Feb 8, 2019

Regarding the removal of refresh on resume, I noticed some ghosting when waking Kobo Clara HD after long inactivity. I have quite an intricate Dürer woodcut print for a wallpaper, maybe that's the reason, anyway it forces me to manually refresh the screen. I also noticed the screen inverting once when showing screensaver (before the 812e595 commit it inverted twice), but not at all when resuming.

Anyway, quicker wallpaper appearance looks cool and smooth :)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

NiLuJe commented Feb 8, 2019

@ptrm : Yep, forgot to re-check Images after re-factoring stuff, I'll look into it.

And the ScreenSaver should definitely flash on resume, I missed that one, thanks ;).

NiLuJe added a commit that referenced this pull request Feb 8, 2019

A few minor fixes after #4541 (#4561)
* Enforce dithering in PicDocument
* Ensure we'll get a flashing update on ScreenSaver exit
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.