ImageViewer: Clamp zoom factor to sane values (#9529)

Should avoid egregious values that would potentially alloc insanely large buffers (and likely fail to do so).

In the process, tweak the scale_factor computations when zooming so as to produce slightly less annoying behavior.

frontend/cache.lua

@@ -6,6 +6,7 @@ local lfs = require("libs/libkoreader-lfs")
 local logger = require("logger")
 local lru = require("ffi/lru")
 local md5 = require("ffi/sha2").md5
+local util = require("util")
 
 local CanvasContext = require("document/canvascontext")
 if CanvasContext.should_restrict_JIT then
@@ -78,95 +79,6 @@ function Cache:_getDiskCache()
     return cached
 end
 
--- For documentation purposes, here's a battle-tested shell version of calcFreeMem
---[[
-    if grep -q 'MemAvailable' /proc/meminfo ; then
-        # We'll settle for 85% of available memory to leave a bit of breathing room
-        tmpfs_size="$(awk '/MemAvailable/ {printf "%d", $2 * 0.85}' /proc/meminfo)"
-    elif grep -q 'Inactive(file)' /proc/meminfo ; then
-        # Basically try to emulate the kernel's computation, c.f., https://unix.stackexchange.com/q/261247
-        # Again, 85% of available memory
-        tmpfs_size="$(awk -v low=$(grep low /proc/zoneinfo | awk '{k+=$2}END{printf "%d", k}') \
-            '{a[$1]=$2}
-            END{
-                printf "%d", (a["MemFree:"]+a["Active(file):"]+a["Inactive(file):"]+a["SReclaimable:"]-(12*low))*0.85;
-            }' /proc/meminfo)"
-    else
-        # Ye olde crap workaround of Free + Buffers + Cache...
-        # Take it with a grain of salt, and settle for 80% of that...
-        tmpfs_size="$(awk \
-            '{a[$1]=$2}
-            END{
-                printf "%d", (a["MemFree:"]+a["Buffers:"]+a["Cached:"])*0.80;
-            }' /proc/meminfo)"
-    fi
---]]
-
--- And here's our simplified Lua version...
-function Cache:_calcFreeMem()
-    local memtotal, memfree, memavailable, buffers, cached
-
-    local meminfo = io.open("/proc/meminfo", "r")
-    if meminfo then
-        for line in meminfo:lines() do
-            if not memtotal then
-                memtotal = line:match("^MemTotal:%s-(%d+) kB")
-                if memtotal then
-                    -- Next!
-                    goto continue
-                end
-            end
-
-            if not memfree then
-                memfree = line:match("^MemFree:%s-(%d+) kB")
-                if memfree then
-                    -- Next!
-                    goto continue
-                end
-            end
-
-            if not memavailable then
-                memavailable = line:match("^MemAvailable:%s-(%d+) kB")
-                if memavailable then
-                    -- Best case scenario, we're done :)
-                    break
-                end
-            end
-
-            if not buffers then
-                buffers = line:match("^Buffers:%s-(%d+) kB")
-                if buffers then
-                    -- Next!
-                    goto continue
-                end
-            end
-
-            if not cached then
-                cached = line:match("^Cached:%s-(%d+) kB")
-                if cached then
-                    -- Ought to be the last entry we care about, we're done
-                    break
-                end
-            end
-
-            ::continue::
-        end
-        meminfo:close()
-    else
-        -- Not on Linux?
-        return 0, 0
-    end
-
-    if memavailable then
-        -- Leave a bit of margin, and report 85% of that...
-        return math.floor(memavailable * 0.85) * 1024, memtotal * 1024
-    else
-        -- Crappy Free + Buffers + Cache version, because the zoneinfo approach is a tad hairy...
-        -- So, leave an even larger margin, and only report 75% of that...
-        return math.floor((memfree + buffers + cached) * 0.75) * 1024, memtotal * 1024
-    end
-end
-
 function Cache:insert(key, object)
     -- If this object is single-handledly too large for the cache, don't cache it.
     if not self:willAccept(object.size) then
@@ -240,10 +152,10 @@ end
 
 -- Terribly crappy workaround: evict half the cache if we appear to be redlining on free RAM...
 function Cache:memoryPressureCheck()
-    local memfree, memtotal = self:_calcFreeMem()
+    local memfree, memtotal = util.calcFreeMem()
 
     -- Nonsensical values? (!Linux), skip this.
-    if memtotal == 0 then
+    if memtotal == nil then
         return
     end
 
@@ -251,7 +163,9 @@ function Cache:memoryPressureCheck()
     local free_fraction = memfree / memtotal
     if free_fraction < 0.20 then
         logger.warn(string.format("Running low on memory (~%d%%, ~%.2f/%d MiB), evicting half of the cache...",
-                                  free_fraction * 100, memfree / 1024 / 1024, memtotal / 1024 / 1024))
+                                  free_fraction * 100,
+                                  memfree / (1024 * 1024),
+                                  memtotal / (1024 * 1024)))
         self.cache:chop()
 
         -- And finish by forcing a GC sweep now...

frontend/document/doccache.lua

@@ -8,11 +8,12 @@ local DataStorage = require("datastorage")
 local lfs = require("libs/libkoreader-lfs")
 local logger = require("logger")
 local md5 = require("ffi/sha2").md5
+local util = require("util")
 
 local function calcCacheMemSize()
     local min = DGLOBAL_CACHE_SIZE_MINIMUM
     local max = DGLOBAL_CACHE_SIZE_MAXIMUM
-    local calc = Cache:_calcFreeMem() * (DGLOBAL_CACHE_FREE_PROPORTION or 0)
+    local calc = util.calcFreeMem() * (DGLOBAL_CACHE_FREE_PROPORTION or 0)
     return math.min(max, math.max(min, calc))
 end
 local doccache_size = calcCacheMemSize()

frontend/ui/widget/imageviewer.lua

@@ -550,18 +550,19 @@ function ImageViewer:onSwipe(_, ges)
     local distance = ges.distance
     local sq_distance = math.sqrt(distance*distance/2)
     if direction == "north" then
-        if ges.pos.x < Screen:getWidth() * 1/16 or ges.pos.x > Screen:getWidth() * 15/16 then
+        if ges.pos.x < Screen:getWidth() * 1/8 or ges.pos.x > Screen:getWidth() * 7/8 then
             -- allow for zooming with vertical swipe on screen sides
             -- (for devices without multi touch where pinch and spread don't work)
-            local inc = ges.distance / Screen:getHeight()
+            -- c.f., onSpread for details about the choice between screen & scaled image height.
+            local inc = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
             self:onZoomIn(inc)
         else
             self:panBy(0, distance)
         end
     elseif direction == "south" then
-        if ges.pos.x < Screen:getWidth() * 1/16 or ges.pos.x > Screen:getWidth() * 15/16 then
+        if ges.pos.x < Screen:getWidth() * 1/8 or ges.pos.x > Screen:getWidth() * 7/8 then
             -- allow for zooming with vertical swipe on screen sides
-            local dec = ges.distance / Screen:getHeight()
+            local dec = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
             self:onZoomOut(dec)
         elseif self.scale_factor == 0 then
             -- When scaled to fit (on initial launch, or after one has tapped
@@ -640,13 +641,32 @@ function ImageViewer:onZoomIn(inc)
         -- Get the scale_factor made out for best fit
         self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
     end
-    if not inc then inc = 0.2 end -- default for key zoom event
-    self.scale_factor = self.scale_factor + inc
-    -- Avoid excessive zoom by halving the increase if we go too high, and clamp the result
-    if self.scale_factor > 100 then
-        self.scale_factor = math.min((self.scale_factor - inc) + inc/2, 100)
+
+    if not inc then
+        -- default for key zoom event
+        inc = 0.2
+    end
+
+    -- Compute new scale factor for rescaled image dimensions
+    local new_factor = self.scale_factor * (1 + inc)
+
+    -- We destroy ImageWidget on update, so only request this the first time,
+    -- in order to avoid jitter in the results given differing memory consumption at different zoom levels...
+    if not self._max_scale_factor then
+        self._min_scale_factor, self._max_scale_factor = self._image_wg:getScaleFactorExtrema()
+    end
+    -- Clamp to sane values
+    new_factor = math.min(new_factor, self._max_scale_factor)
+    if new_factor ~= self.scale_factor then
+        self.scale_factor = new_factor
+        self:update()
+    else
+        if self.scale_factor == self._max_scale_factor then
+            logger.dbg("ImageViewer:onZoomIn: Hit the max scaling factor:", self.scale_factor)
+        else
+            logger.dbg("ImageViewer:onZoomIn: No change in scaling factor:", self.scale_factor)
+        end
     end
-    self:update()
     return true
 end
 
@@ -655,13 +675,34 @@ function ImageViewer:onZoomOut(dec)
         -- Get the scale_factor made out for best fit
         self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
     end
-    if not dec then dec = 0.2 end -- default for key zoom event
-    self.scale_factor = self.scale_factor - dec
-    -- Avoid excessive unzoom by halving the decrease if we go too low, and clamp the result
-    if self.scale_factor < 0.01 then
-        self.scale_factor = math.max(0.01, (self.scale_factor + dec) - dec/2)
+
+    if not dec then
+        -- default for key zoom event
+        dec = 0.2
+    elseif dec >= 0.75 then
+        -- Larger reductions tend to be fairly jarring, so limit to 75%.
+        -- (Also, we can't go above 1 because maths).
+        dec = 0.75
+    end
+
+    -- Compute new scale factor for rescaled image dimensions
+    local new_factor = self.scale_factor * (1 - dec)
+
+    if not self._min_scale_factor then
+        self._min_scale_factor, self._max_scale_factor = self._image_wg:getScaleFactorExtrema()
+    end
+    -- Clamp to sane values
+    new_factor = math.max(new_factor, self._min_scale_factor)
+    if new_factor ~= self.scale_factor then
+        self.scale_factor = new_factor
+        self:update()
+    else
+        if self.scale_factor == self._min_scale_factor then
+            logger.dbg("ImageViewer:onZoomOut: Hit the min scaling factor:", self.scale_factor)
+        else
+            logger.dbg("ImageViewer:onZoomOut: No change in scaling factor:", self.scale_factor)
+        end
     end
-    self:update()
     return true
 end
 
@@ -672,8 +713,23 @@ function ImageViewer:onSpread(_, ges)
     if self._image_wg then
         self._center_x_ratio, self._center_y_ratio = self._image_wg:getPanByCenterRatio(ges.pos.x - Screen:getWidth()/2, ges.pos.y - Screen:getHeight()/2)
     end
-    -- Set some zoom increase value from pinch distance
-    local inc = ges.distance / Screen:getWidth()
+    -- Set some zoom increase value from pinch distance.
+    -- Making it relative to the smallest dimension between the currently scaled image or the Screen makes it less annoying
+    -- when approaching both very small scale factors (where the image dimensions are many times smaller than the screen),
+    -- meaning using the image dimensions here takes less zoom steps to get it back to a sensible size;
+    -- *and* large scale factors (where the image dimensions are larger than the screen),
+    -- meaning using the screen dimensions here makes zoom steps, again, slightly more potent.
+    local inc
+    if ges.direction == "vertical" then
+        inc = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
+    elseif ges.direction == "horizontal" then
+        inc = ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth())
+    else
+        local tl = Geom:new{ x = 0, y = 0 }
+        local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
+        local screen_diag = tl:distance(br)
+        inc = ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal())
+    end
     self:onZoomIn(inc)
     return true
 end
@@ -681,7 +737,17 @@ end
 function ImageViewer:onPinch(_, ges)
     -- With Pinch, unlike Spread, it feels more natural if we keep the same center point.
     -- Set some zoom decrease value from pinch distance
-    local dec = ges.distance / Screen:getWidth()
+    local dec
+    if ges.direction == "vertical" then
+        dec = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
+    elseif ges.direction == "horizontal" then
+        dec = ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth())
+    else
+        local tl = Geom:new{ x = 0, y = 0 }
+        local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
+        local screen_diag = tl:distance(br)
+        dec = ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal())
+    end
     self:onZoomOut(dec)
     return true
 end

frontend/ui/widget/imagewidget.lua

@@ -116,6 +116,8 @@ local ImageWidget = Widget:new{
 
     _bb = nil,
     _bb_disposable = true, -- whether we should free() our _bb
+    _img_w = nil,
+    _img_h = nil,
     _bb_w = nil,
     _bb_h = nil,
 }
@@ -292,6 +294,9 @@ function ImageWidget:_render()
     end
 
     local bb_w, bb_h = self._bb:getWidth(), self._bb:getHeight()
+    -- Store the dimensions of the actual image, before any kind of scaling
+    self._img_w = bb_w
+    self._img_h = bb_h
 
     -- scale_for_dpi setting: update scale_factor (even if not set) with it
     if self.scale_for_dpi and not self.already_scaled_for_dpi then
@@ -333,7 +338,7 @@ function ImageWidget:_render()
     elseif self.scale_factor ~= 1 then
         -- scale by scale_factor (not needed if scale_factor == 1)
         logger.dbg("ImageWidget: scaling by", self.scale_factor)
-        self._bb = RenderImage:scaleBlitBuffer(self._bb, bb_w * self.scale_factor, bb_h * self.scale_factor, self._bb_disposable)
+        self._bb = RenderImage:scaleBlitBuffer(self._bb, math.floor(bb_w * self.scale_factor), math.floor(bb_h * self.scale_factor), self._bb_disposable)
         self._bb_disposable = true -- new bb will have to be freed
     end
     bb_w, bb_h = self._bb:getWidth(), self._bb:getHeight()
@@ -360,8 +365,8 @@ function ImageWidget:_render()
             self.center_y_ratio = 0.5 + self._max_off_center_y_ratio
         end
         -- set offsets to reflect center ratio, whether oversized or not
-        self._offset_x = self.center_x_ratio * bb_w - self.width/2
-        self._offset_y = self.center_y_ratio * bb_h - self.height/2
+        self._offset_x = math.floor(self.center_x_ratio * bb_w - self.width/2)
+        self._offset_y = math.floor(self.center_y_ratio * bb_h - self.height/2)
         logger.dbg("ImageWidget: initial offsets", self._offset_x, self._offset_y)
     end
 
@@ -387,6 +392,75 @@ function ImageWidget:getScaleFactor()
     return self.scale_factor
 end
 
+function ImageWidget:getScaleFactorExtrema()
+    if self._min_scale_factor and self._max_scale_factor then
+        return self._min_scale_factor, self._max_scale_factor
+    end
+
+    -- Compute dynamic limits for the scale factor, based on the screen's area and available memory (if possible).
+    -- Extrema eyeballed to be somewhat sensible given our usual screen dimensions and available RAM.
+    local util = require("util")
+    local memfree, _ = util.calcFreeMem()
+
+    local screen_area = Screen:getWidth() * Screen:getHeight()
+    local min_area = math.ceil(screen_area / 10000)
+    local max_area
+    if memfree then
+        -- If we have access to memory statistics, limit the requested bb size to 25% of the available RAM.
+        local bbtype = self._bb:getType()
+        local bpp
+        if bbtype == Blitbuffer.TYPE_BB8 then
+            bpp = 1
+        elseif bbtype == Blitbuffer.TYPE_BB8A then
+            bpp = 2
+        elseif bbtype == Blitbuffer.TYPE_BBRGB24 then
+            bpp = 3
+        elseif bbtype == Blitbuffer.TYPE_BBRGB32 then
+            bpp = 4
+        elseif bbtype == Blitbuffer.TYPE_BB4 then
+            bpp = 1
+        else
+            bpp = 4
+        end
+
+        max_area = math.floor(0.25 * memfree / bpp)
+    else
+        -- Best effort, trying to account for DPI...
+        local dpi = Screen:getDPI()
+        if dpi <= 212 then
+            max_area = screen_area * 45
+        elseif dpi <= 265 then
+            max_area = screen_area * 25
+        else
+            max_area = screen_area * 15
+        end
+    end
+
+    local area = self._img_w * self._img_h
+    self._min_scale_factor = 1 / math.sqrt(area / min_area)
+    self._max_scale_factor = math.sqrt(max_area / area)
+
+    return self._min_scale_factor, self._max_scale_factor
+end
+
+-- As opposed to what we've stored in self._img_w & self._img_h on decode,
+-- which hold the source image dimensions (i.e., before scaling),
+-- these return the dimensions of the currently displayed bb (i.e., post scaling),
+-- and it is *not* constrained to the Screen dimensions (or this bb's viewport).
+function ImageWidget:getCurrentWidth()
+    return self._bb:getWidth()
+end
+
+function ImageWidget:getCurrentHeight()
+    return self._bb:getHeight()
+end
+
+function ImageWidget:getCurrentDiagonal()
+    local tl = Geom:new{ x = 0, y = 0 }
+    local br = Geom:new{ x = self._bb:getWidth() - 1, y = self._bb:getHeight() - 1}
+    return tl:distance(br)
+end
+
 function ImageWidget:getPanByCenterRatio(x, y)
     -- returns center ratio (without limits check) we would get with this panBy
     local center_x_ratio = (x + self._offset_x + self.width/2) / self._bb_w
@@ -410,8 +484,8 @@ function ImageWidget:panBy(x, y)
         self.center_y_ratio = 0.5 + self._max_off_center_y_ratio
     end
     -- new offsets that reflect this new center ratio
-    local new_offset_x = self.center_x_ratio * self._bb_w - self.width/2
-    local new_offset_y = self.center_y_ratio * self._bb_h - self.height/2
+    local new_offset_x = math.floor(self.center_x_ratio * self._bb_w - self.width/2)
+    local new_offset_y = math.floor(self.center_y_ratio * self._bb_h - self.height/2)
     -- only trigger screen refresh it we actually pan
     if new_offset_x ~= self._offset_x or new_offset_y ~= self._offset_y then
         self._offset_x = new_offset_x

frontend/util.lua

@@ -755,6 +755,97 @@ function util.getFilesystemType(path)
     return type
 end
 
+-- For documentation purposes, here's a battle-tested shell version of calcFreeMem,
+-- our simplified Lua version follows...
+--[[
+    if grep -q 'MemAvailable' /proc/meminfo ; then
+        # We'll settle for 85% of available memory to leave a bit of breathing room
+        tmpfs_size="$(awk '/MemAvailable/ {printf "%d", $2 * 0.85}' /proc/meminfo)"
+    elif grep -q 'Inactive(file)' /proc/meminfo ; then
+        # Basically try to emulate the kernel's computation, c.f., https://unix.stackexchange.com/q/261247
+        # Again, 85% of available memory
+        tmpfs_size="$(awk -v low=$(grep low /proc/zoneinfo | awk '{k+=$2}END{printf "%d", k}') \
+            '{a[$1]=$2}
+            END{
+                printf "%d", (a["MemFree:"]+a["Active(file):"]+a["Inactive(file):"]+a["SReclaimable:"]-(12*low))*0.85;
+            }' /proc/meminfo)"
+    else
+        # Ye olde crap workaround of Free + Buffers + Cache...
+        # Take it with a grain of salt, and settle for 80% of that...
+        tmpfs_size="$(awk \
+            '{a[$1]=$2}
+            END{
+                printf "%d", (a["MemFree:"]+a["Buffers:"]+a["Cached:"])*0.80;
+            }' /proc/meminfo)"
+    fi
+--]]
+
+--- Computes the currently available memory
+---- @treturn tuple of ints: memavailable, memtotal (or nil, nil on unsupported platforms).
+function util:calcFreeMem()
+    local memtotal, memfree, memavailable, buffers, cached
+
+    local meminfo = io.open("/proc/meminfo", "r")
+    if meminfo then
+        for line in meminfo:lines() do
+            if not memtotal then
+                memtotal = line:match("^MemTotal:%s-(%d+) kB")
+                if memtotal then
+                    -- Next!
+                    goto continue
+                end
+            end
+
+            if not memfree then
+                memfree = line:match("^MemFree:%s-(%d+) kB")
+                if memfree then
+                    -- Next!
+                    goto continue
+                end
+            end
+
+            if not memavailable then
+                memavailable = line:match("^MemAvailable:%s-(%d+) kB")
+                if memavailable then
+                    -- Best case scenario, we're done :)
+                    break
+                end
+            end
+
+            if not buffers then
+                buffers = line:match("^Buffers:%s-(%d+) kB")
+                if buffers then
+                    -- Next!
+                    goto continue
+                end
+            end
+
+            if not cached then
+                cached = line:match("^Cached:%s-(%d+) kB")
+                if cached then
+                    -- Ought to be the last entry we care about, we're done
+                    break
+                end
+            end
+
+            ::continue::
+        end
+        meminfo:close()
+    else
+        -- Not on Linux?
+        return nil, nil
+    end
+
+    if memavailable then
+        -- Leave a bit of margin, and report 85% of that...
+        return math.floor(memavailable * 0.85) * 1024, memtotal * 1024
+    else
+        -- Crappy Free + Buffers + Cache version, because the zoneinfo approach is a tad hairy...
+        -- So, leave an even larger margin, and only report 75% of that...
+        return math.floor((memfree + buffers + cached) * 0.75) * 1024, memtotal * 1024
+    end
+end
+
 --- Recursively scan directory for files inside
 -- @string path
 -- @func callback(fullpath, name, attr)