From 38919c22eb6e39c59d7efc12f5c83f397deafde1 Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Mon, 19 Sep 2022 23:25:18 +0200 Subject: [PATCH] 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 | 98 ++------------------------- frontend/document/doccache.lua | 3 +- frontend/ui/widget/imageviewer.lua | 104 +++++++++++++++++++++++------ frontend/ui/widget/imagewidget.lua | 84 +++++++++++++++++++++-- frontend/util.lua | 91 +++++++++++++++++++++++++ 5 files changed, 263 insertions(+), 117 deletions(-) diff --git a/frontend/cache.lua b/frontend/cache.lua index 1197b81de..bcb643292 100644 --- a/frontend/cache.lua +++ b/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... diff --git a/frontend/document/doccache.lua b/frontend/document/doccache.lua index e6d584e1f..29a8eea62 100644 --- a/frontend/document/doccache.lua +++ b/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() diff --git a/frontend/ui/widget/imageviewer.lua b/frontend/ui/widget/imageviewer.lua index 1c13cea8d..b6e62e084 100644 --- a/frontend/ui/widget/imageviewer.lua +++ b/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 diff --git a/frontend/ui/widget/imagewidget.lua b/frontend/ui/widget/imagewidget.lua index 237ee69a9..33e1319b0 100644 --- a/frontend/ui/widget/imagewidget.lua +++ b/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 diff --git a/frontend/util.lua b/frontend/util.lua index 08bcf186c..7c1d88d81 100644 --- a/frontend/util.lua +++ b/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)