Screensaver image sometimes garbled

[Frenzie]

• KOReader version: up to 6f037ce
• Device: Kobo Aura H2O, but also reported on Aura One and Kindle PW3

Issue

When you suspend, occasionally a cover image will end up pure black or garbled. Here is a screenshot taken on my H2O by inserting the following after this line:

image:writePNG("/mnt/onboard/.adds/koreader/screenshots/blabla-png")

Here are another couple of pictures, this time in real life:

Steps to reproduce

I've only seen this in PDFs myself, and I've been able to trigger it the most frequently with this book: http://www.oapen.org/search?identifier=626372;keyword=orthographies

Edit: okay, you can reproduce this reliably by showing book status twice (it renders the cover image just like the screensaver does). It must be a cache issue.

[houqp]

@Frenzie and I looked into this. The root cause is double free from imagewidget and tile cache for those cover image blitbuffers. I believe this is also what's causing random segfaults for me on suspend.

It should be a pretty straight forward fix. However, I won't have time to work on it until next week. So if anyone is interested in this, please feel free to send a PR.

[poire-z]

I witnessed a crash while suspending with this pdf on my GloHD this morning. I tried to reproduce this on the emulator (adding a way to trigger screensaver.show()) but everything worked ok (no crash, cover image clean).

Anyway, looking at the code, and the BlitBuffer memory leaks hunt i did some months ago, ImageWidget now does free() blitbuffers (unless explicitely requested not to with image_disposable = false).
It's fine in most cases, except when the blitbuffer may be cached in a TileCacheItem, where the bb may still me kept and reused later.
As getCoverImage in credocument do not use tile cache, and is used only for the ImageWidget, it's good to still have ImageWidget do the free() itself.
For pdfdocument/koptdocument, the tile (and its bb) may or may not be cached (depending on size, cache size available...), so we would need to propagate the fact that the bb is disposable or not from there up to the ImageWidget, which adds some bits to a few calls.

The other simplest solution is, in KoptInterface:getCoverPageImage(), to return a copy of tile.bb (whether it's cached or not), so that ImageWidget can free this copy every time, and leave the free'ing of the original tile to TileCacheItem or the GC. To the expense of having temporarily 2 times the coverimage blitbuffer in memory (1 copy shoud be freed immediately when out of screensaver).

@Frenzie : can u try this simple patch and see if if you still get a garbled cover image ?

--- a/frontend/document/koptinterface.lua
+++ b/frontend/document/koptinterface.lua
@@ -263,7 +263,7 @@ function KoptInterface:getCoverPageImage(doc)
     local zoom = math.min(screen_size.w / native_size.w, screen_size.h / native_size.h)
     local tile = Document.renderPage(doc, 1, nil, zoom, 0, 1, 0)
     if tile then
-        return tile.bb
+        return tile.bb:copy()
     end
 end

[Frenzie]

I'll give it a go. Btw, in our discussion we concluded that it'd probably be simplest to always load the screensaver from file (saved to the sidecar dir), which is something we'll need for fancied up history display anyway. That being said, while typing this I realized it probably makes more sense to save a thumbnail-sized image for that purpose, whereas for the screensaver we definitely want the full size.

Anyway, I'll get back to you in a bit. I'm at home and I've got an hour. ;-)

[Frenzie]

On a related note, trying the book status trick on my H2O (pre-patch) results in a segmentation fault rather than garbled display like on my desktop.

[Edit]

I witnessed a crash while suspending with this pdf on my GloHD this morning. I tried to reproduce this on the emulator (adding a way to trigger screensaver.show()) but everything worked ok (no crash, cover image clean).

Oh btw, you'll have to do it at least twice. The first time it'll show up clean.
[/Edit]

The issue in the screensaver, book status, and probably a few other places that use ImageWidget (like GoodReads) indeed seem to be solved by that simple patch. Will you stick it in a PR or shall I? This garbled screensaver business is leaving a bad first impression, after all.

I don't think having two copies in memory should be much of an issue provided GC picks 'em up. I didn't see any memory leaks while testing, just an expected single-cover increase of some 5MB.

[poire-z]

Will you stick it in a PR or shall I?

Feel free to do it if you have time (if not, i'll do it this evening).

[ersi-dnd]

I get those garbled screensaver images in Pocketbook also.