ImageViewer: fix zoom in crash · Pull Request #9524 · koreader/koreader

ghost · 2022-09-16T15:32:24Z

With this PR and the following patch of base/ffi/blitbuffer.lua the crash is gone.

If the user zooms in too much an icon with a magnifying glass and an infomessage with the request to zoom out is shown.

Both crashes mentioned in #9510 (comment) are resolved.

Setting this as draft, as I think there will be a better solution (maybe just show the message and don't zoom further in).

--- a/ffi/blitbuffer.lua
+++ b/ffi/blitbuffer.lua
@@ -1902,7 +1902,10 @@ make a full copy of the current buffer, with its own memory
 function BB_mt.__index:copy()
     local mytype = ffi.typeof(self)
     local buffer = C.malloc(self.stride * self.h)
-    assert(buffer ~= nil, "cannot allocate bb copy buffer")
+ -- removing the assert does no harm here, only the crash might happen later.
+    if buffer == nil then
+        print("cannot allocate bb copy buffer of size " .. self.stride * self.h / 1000 .. "kB")
+        return nil
+    end
     ffi.copy(buffer, self.data, self.stride * self.h)
     local copy = mytype(self.w, self.pixel_stride, self.h, self.stride, buffer, self.config)
     copy:setAllocated(1)

This change is

NiLuJe · 2022-09-16T15:48:37Z

+            self._bb = _bb
+        else
+            -- Icon is from https://freesvg.org/tango-system-search-icon-vector-drawing
+            self._bb = RenderImage:renderImageFile("resources/icons/system-search.svg", false, 200, 200)


We already have a magnifying glass icon ;).

NiLuJe · 2022-09-16T15:59:15Z

I'm not sure I necessarily like returning nil in bb:copy and delaying the crash, but, eeeeh.

(Also: / 1024 ;)).

ghost · 2022-09-16T16:18:59Z

I'm not sure I necessarily like returning nil in bb:copy and delaying the crash, but, eeeeh.

I also don't like that (as I don't like crashes at all). But I think the danger for crashes from that side are not so big (at least I haven't seen that assertion up to now.) And we have the source of the crash in the log. BTW: Is it a good choice to use print in base for logging or do we have a better solution?

(Also: / 1024 ;)).

No problem, but then "KiB" :p

poire-z · 2022-09-16T16:42:01Z

-        self._bb = RenderImage:scaleBlitBuffer(self._bb, bb_w * self.scale_factor, bb_h * self.scale_factor, self._bb_disposable)
+        local _bb = RenderImage:scaleBlitBuffer(self._bb, bb_w * self.scale_factor, bb_h * self.scale_factor, self._bb_disposable)
+        if _bb then
+            self._bb = _bb
+        else
+            self._bb = RenderImage:renderImageFile("resources/icons/src/appbar.magnify.zoom.svg", false, 200, 200)
+
+            local InfoMessage = require("ui/widget/infomessage")
+            UIManager:show(InfoMessage:new{
+                show_icon = true,
+                text = _("Maximum zoom reached. Please zoom out again."),
+            })
+        end
+


The blank lines are a bit uneeded (they are used in the outer context to separate steps - no need to add them here as there are not really steps of processing).

Also, I'm not sure it's really needed for such a low level ImageWidget to use UIManager:show(InfoMessage).
I would either:

have a .svg with that message or symbol :)

or just keep using any previous self._bb : the user will continue trying to zoom in and see nothing changeing, and will guess maximum zoom is reached.

If going with the first solution, cleaner to not use local _bb:

self._bb = RenderImage... + if not self._bb then + -- couldn't make a bb: probably not enough memory for such a large bb + self._bb = RenderImage:renderImageFile("resources/icons/src/appbar.magnify.zoom.svg", false, 200, 200) + end

Also, I'm not sure it's really needed for such a low level ImageWidget to use UIManager:show(InfoMessage). I would either:

* have a .svg with that message or symbol :)

Good point.

* or just keep using any previous self._bb : the user will continue trying to zoom in and see nothing changeing, and will guess maximum zoom is reached.

Would be possible, but as there is no feedback, that maximum zoom is reached, the user could try to zoom in further. The zoom factor gets bigger and bigger with each try. If the user decides to zoom out again, on the first zoom out nothing happens ...

So I think it is better to have a message (svg-image with the message in).

If going with the first solution, cleaner to not use local _bb:

self._bb = RenderImage... + if not self._bb then + -- couldn't make a bb: probably not enough memory for such a large bb + self._bb = RenderImage:renderImageFile("resources/icons/src/appbar.magnify.zoom.svg", false, 200, 200) + end

OK

Yeah, the "no visible feedback" thing was what made me tweak the onZoom handlers because it was confusing as hell in practice.

poire-z · 2022-09-16T17:38:39Z

Thinking again, multiple remarks:

I'm not sure it's ImageWidget job to show something went bad and mention "too big zoom" : it's ImageViewer that is zooming, ImageWidget is just a library. So, it's ImageViewer that knows the user is zooming, that has to show a message if really needed.
you are dealing with bb:copy(). I guess you are testing with SVG images, and I think it's only with SVG images that I had to use :copy() - because the original bb is managed by LunaSVG. So, you're addressing one use case - but we have probably many others (zooming in a PNG), and possibly inner places where this may happen (LunaSVG, cre.cpp that can also malloc() and copy): if we are going to chase them all, we need to think more about how to handle this everywhere in the same way (returning nil+error message + logging could be that way, or using a fake image, dunno).
and users ImageWidget or ImageViewer would both handle "that way", and display something (fake image for ImageWidget, Notification that fail allocation, too large image... for ImageViever

The zoom factor gets bigger and bigger with each try. If the user decides to zoom out again, on the first zoom out nothing happens ...

In that case, ImageViewer should reset the zoom factor to the previous value if it didn't manage to get a bb for the wished zoom factor.

ghost · 2022-09-16T18:31:25Z

I guess you are testing with SVG images...

No, I am currently testing with the cover images of my regular e-pubs (png or jpg).

Thanks for the rest of your hints. I will sleep over it.

poire-z · 2022-09-16T18:42:12Z

I guess you are testing with SVG images...

No, I am currently testing with the cover images of my regular e-pubs (png or jpg).

Alright. And I was wrong: with SVG images, no copy is done - except for SVG cover images where the copy is done in cre.cpp - and there, I don't even check if the malloc succeeds :/ So, probably a few other places to visit and see if we can have them behave the same way on malloc failure.

NiLuJe · 2022-09-16T19:20:48Z

Is it a good choice to use print in base for logging or do we have a better solution?

The thought crossed my mind, but I don't think we have anything better for a plain module that's only essentially used as a toolbox of functions (as opposed to an instantiated class like framebuffer than inherits a dbg pointer from front that points to logger.dbg).

NiLuJe · 2022-09-16T19:23:54Z

if we are going to chase them all, we need to think more about how to handle this everywhere in the same way (returning nil+error message + logging could be that way, or using a fake image, dunno).

Ooh, making bb:new & bb:copy return a pointer or nil, error or nil tuple sounds good, yeah.

ghost · 2022-09-16T21:04:04Z

I have looked into ImageViewer, and I have to state that it is not easy to understand. In fact, it is to complicated for me.

As I will have not much time in the next few weeks, I will to let this PR to you. Please overtake it or close it, as you like.

NiLuJe · 2022-09-16T21:12:19Z

I think the only thing we really needed help for was the actual zoom scaling logic, we can probably deal with the rest one way or another ;).

ghost · 2022-09-16T21:21:48Z

Jepp, but I for my feelings the zoom scalimg feature works well, except the crashes. I never felt it does not work as expctd. Ok I dont use it often.

So closing this

NiLuJe · 2022-09-16T21:58:42Z

On a mildly related note, I'm slightly curious to know on what kind of input images and output devices you've managed to trigger these OOM crashes, because I tested a lot of pinches & spreads on quite a few devices when working on GestureDetector and I never saw it crash ;).

(In general, when we get OOM crash reports, they're either a kernel OOM-kill on Kindle or 256/512MB devices; or k2pdfopt processing related on difficult input).

ghost · 2022-09-17T08:30:59Z

I get the crashes on my mobile and my Sage. Maybe you can reproduce it with https://allesebook.de/ebooks-kostenlos/Krieg-und-Frieden-Leo-N-Tolstoi.epub Am 16. September 2022 23:58:52 MESZ schrieb NiLuJe ***@***.***>:

On a mildly related note, I'm slightly curious to know on what kind of input images and output devices you've managed to trigger these OOM crashes, because I tested *a lot* of pinches & spreads on quite a few devices and I never saw it crash ;). (In general, when we get OOM crash reports, they're either a kernel OOM-kill on Kindle or 256/512MB devices; or k2pdfopt processing related on difficult input). -- Reply to this email directly or view it on GitHub: #9524 (comment) You are receiving this because you modified the open/close state. Message ID: ***@***.***>

-- Diese Nachricht wurde von meinem Android-Gerät mit K-9 Mail gesendet.

Frenzie · 2022-09-17T09:59:05Z

Is that a particularly large image file?

Btw, I'm not sure if we're talking about malloc here or within LuaJIT itself? Strictly within LuaJIT we're limited to 2 GB atm, although we could build with the 64-bit flag on select platforms.

ghost · 2022-09-17T15:52:05Z

This is not a big image. The cover is more or less a blue background with some text in it. Open the cover with a long tap on the cover image and zoom in and in and in ... crash even on my 6GB mobile. Its a malloc priblem in blitbuffer line 1905.

Frenzie · 2022-09-17T16:06:47Z

Hm, if I zoom in 5 or 6 times it's extremely blurry and it doesn't seem to want to zoom in any further if you try.

(Also I didn't even realize you could zoom in, heh.)

NiLuJe · 2022-09-17T16:55:38Z

Okay, actually looking at the Sage crash you posted in #9510 (comment), a 131MB alloc failing is... kinda low, but still on the upper range of what I would consider acceptable.

As @Frenzie mentioned, we're limited by the 32bit architecture on how much we can alloc (at once and in total), so the actual device matters much less than expected in the end.

The Sage happens to be a fairly bad example because it suffers from higher memory pressure than other Kobo devices because of the terrible sunxi driver reserving a large chunk of memory, and NTX cheaping out on giving it the same amount of RAM than the Elipsa ;).

Anyway, I'm going to check if we're not simply leaking temporary bbs somewhere (and/or releasing 'em too late), and I plan to make the scaling cap dynamic: instead of being fixed factors, I want to make 'em dynamic based on the screen size, limiting zoom outs to a hundreth of the screen's area, and zoom in to ten times the screen's area.
On an hypothetically weak 600x800 devices, that would make the zoom-in cap out at 183MB for RGB32, which will probably fail on 256MB, but fuck those, but should work on an unpressured 512MB one.

EDIT: I can't maths. Numbers subject to alterations ;p.

Frenzie · 2022-09-17T17:07:05Z

As @Frenzie mentioned, we're limited by the 32bit architecture on how much we can alloc (at once and in total), so the actual device matters much less than expected in the end.

Like I said there is -DLUAJIT_ENABLE_GC64. I'm not sure if there's much added value to that; also we'd have to deal with a bunch of special cases (i.e., we'd want the emulator to be 32-bit even though we all use 64-bit).

It wouldn't make a difference in the standard Android build regardless of course.

NiLuJe · 2022-09-17T17:23:56Z

I'm... not entirely sure this would be safe on aarch32 ;).

Frenzie · 2022-09-17T18:31:33Z

I don't know what would happen if you tried to enable it on 32-bit but I only mean on (currently) some of the Debian builds. Hence not on Android.

1st try

8fda42b

NiLuJe reviewed Sep 16, 2022

View reviewed changes

Use the not so cool magnifying glass

060c82a

poire-z reviewed Sep 16, 2022

View reviewed changes

ghost closed this Sep 16, 2022

NiLuJe mentioned this pull request Sep 17, 2022

ImageViewer: Clamp zoom factor to sane values #9529

Merged

ghost deleted the fixZoomCrash1 branch November 4, 2022 21:52

This pull request was closed.

Uh oh!

Uh oh!

Conversation

ghost commented Sep 16, 2022 • edited by Frenzie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented Sep 16, 2022

Uh oh!

ghost commented Sep 16, 2022

Uh oh!

poire-z Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

ghost Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

NiLuJe Sep 16, 2022

Choose a reason for hiding this comment

Uh oh!

poire-z commented Sep 16, 2022

Uh oh!

ghost commented Sep 16, 2022

Uh oh!

poire-z commented Sep 16, 2022

Uh oh!

NiLuJe commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Sep 16, 2022

Uh oh!

ghost commented Sep 16, 2022

Uh oh!

NiLuJe commented Sep 16, 2022

Uh oh!

ghost commented Sep 16, 2022

Uh oh!

NiLuJe commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 17, 2022 via email

Uh oh!

Frenzie commented Sep 17, 2022

Uh oh!

ghost commented Sep 17, 2022 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented Sep 17, 2022

Uh oh!

Frenzie commented Sep 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ghost commented Sep 16, 2022 •

edited by Frenzie

Loading

NiLuJe commented Sep 16, 2022 •

edited

Loading

NiLuJe commented Sep 16, 2022 •

edited

Loading

ghost commented Sep 17, 2022 via email •

edited by ghost

Loading

Frenzie commented Sep 17, 2022 •

edited

Loading

NiLuJe commented Sep 17, 2022 •

edited

Loading

Frenzie commented Sep 17, 2022 •

edited

Loading