FB: Refactor refresh rectangle bounding checks (#1718)

* BB: Implement getBoundedRect
  Replace our funky checkBounds use-cases where we actually want to
  fit a rectangle inside a bb, without actually relying on half the
  checkBounds feature, which are designed for ccordinates checking when
  blitting.
  Fold in support for alignment constraints, which allow us to make sure
  they don't do stupid things that would break OOB, as was experienced on
  Bellatrix3...
* FB: Never call fb:refresh* methods without arguments (i.e., make the rect mandatory)
  I hadn't consciously dropped the nil guards, but turns out it makes
  sense, because UIManager will *always* set them.
  Thanks to @mergen3107 for catching that one early ;)
* FB: Implement getScreenOrientation
  Which does what getScreenMode probably meant to be doing, i.e., returns
  the layout relative to the rotation, not how the buffer actually looks
  like ;).
* SDL: Move the getBoundedRect call earlier, to make the log message accurate
  It's also not actually the physical rect, as the rectangle honors rotation,
  while the physical buffer layout is always unrotated.

ffi/blitbuffer.lua

@@ -16,6 +16,7 @@ local min = math.min
 local rshift = bit.rshift
 local lshift = bit.lshift
 local band = bit.band
+local bnot = bit.bnot
 local bor = bit.bor
 local bxor = bit.bxor
 
@@ -1032,14 +1033,95 @@ function BB.checkBounds(length, target_offset, source_offset, target_size, sourc
         -- length is the smallest value
         return floor(length), floor(target_offset), floor(source_offset)
     elseif target_left < length and target_left < source_left then
-        -- target_left is the smalles value
+        -- target_left is the smallest value
         return floor(target_left), floor(target_offset), floor(source_offset)
     else
         -- source_left must be the smallest value
         return floor(source_left), floor(target_offset), floor(source_offset)
     end
 end
 
+-- A couple helper functions to compute aligned values...
+-- c.f., <linux/kernel.h> & ffi/framebuffer_linux.lua
+local function ALIGN_DOWN(x, a)
+    -- x & ~(a-1)
+    local mask = a - 1
+    return band(x, bnot(mask))
+end
+
+local function ALIGN_UP(x, a)
+    -- (x + (a-1)) & ~(a-1)
+    local mask = a - 1
+    return band(x + mask, bnot(mask))
+end
+
+--[[
+More specific checkBounds variant that will return the specified rect bounded inside the blitbuffer.
+i.e., make sure that x & y don't go OOB on either side, and that x+w & y+h don't go over their respective dimensions.
+
+@param x coordinate
+@param y coordinate
+@param w dimension
+@param h dimension
+@param alignment *optional* alignment constraint, if not nil, *must* be > 0 and a power of two!
+
+@return rect strictly bounded inside the bb
+--]]
+function BB_mt.__index:getBoundedRect(x, y, w, h, alignment)
+    local max_w = self:getWidth()
+    local max_h = self:getHeight()
+
+    -- Deal with OOB coordinates
+    if x >= max_w then
+        x = 0
+        w = 0
+    end
+    if y >= max_h then
+        y = 0
+        h = 0
+    end
+
+    -- Deal with negative coordinates
+    if x < 0 then
+        w = w + x
+        x = 0
+    end
+    if y < 0 then
+        h = h + y
+        y = 0
+    end
+
+    -- Align to pixel grid
+    x = floor(x)
+    y = floor(y)
+    w = ceil(w)
+    h = ceil(h)
+
+    -- Honor alignment constraints, if any.
+    -- coordinates can only go down, but never below 0, so there's no risk of it invalidating our previous OOB checks.
+    -- NOTE: c.f., dithering comments in framebuffer_mxcfb for a potential use-case
+    if alignment then
+        local x_orig = x
+        x = ALIGN_DOWN(x_orig, alignment)
+        local x_fixup = x_orig - x
+        w = ALIGN_UP(w + x_fixup, alignment)
+        local y_orig = y
+        y = ALIGN_DOWN(y_orig, alignment)
+        local y_fixup = y_orig - y
+        h = ALIGN_UP(h + y_fixup, alignment)
+    end
+
+    -- Make sure the rect fits strictly inside the bb
+    if x + w > max_w then
+        w = max_w - x
+    end
+    if y + h > max_h then
+        h = max_h - y
+    end
+
+    return x, y, w, h
+end
+
 function BB_mt.__index:blitDefault(dest, dest_x, dest_y, offs_x, offs_y, width, height, setter, set_param)
     -- slow default variant:
     local o_y = offs_y
@@ -1386,8 +1468,7 @@ invert a rectangle within the buffer
 @param h height
 --]]
 function BB_mt.__index:invertRect(x, y, w, h)
-    w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+    x, y, w, h = self:getBoundedRect(x, y, w, h)
     if w <= 0 or h <= 0 then return end
     if self:canUseCbb() then
         cblitbuffer.BB_invert_rect(ffi.cast(P_BlitBuffer, self),
@@ -1493,8 +1574,7 @@ paint a rectangle onto this buffer
 function BB_mt.__index:paintRect(x, y, w, h, value, setter)
     setter = setter or self.setPixel
     value = value or Color8(0)
-    w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+    x, y, w, h = self:getBoundedRect(x, y, w, h)
     if w <= 0 or h <= 0 then return end
     if self:canUseCbb() and setter == self.setPixel then
         cblitbuffer.BB_fill_rect(ffi.cast(P_BlitBuffer, self),
@@ -1602,8 +1682,7 @@ end
 function BB4_mt.__index:paintRect(x, y, w, h, value, setter)
     setter = setter or self.setPixel
     value = value or Color8(0)
-    w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+    x, y, w, h = self:getBoundedRect(x, y, w, h)
     if w <= 0 or h <= 0 then return end
     for tmp_y = y, y+h-1 do
         for tmp_x = x, x+w-1 do
@@ -1844,8 +1923,7 @@ Paint hatches in a rectangle
 @a:  alpha
 --]]
 function BB_mt.__index:hatchRect(x, y, w, h, sw, c, a)
-    w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+    x, y, w, h = self:getBoundedRect(x, y, w, h)
     if w <= 0 or h <= 0 then return end
     a = (a or 1)*0xFF
     if a <= 0 then return end -- fully transparent
@@ -1909,8 +1987,7 @@ dim color values in rectangular area
 function BB_mt.__index:dimRect(x, y, w, h, by)
     local color = Color8A(0xFF, 0xFF*(by or 0.5))
     if self:canUseCbb() then
-        w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-        h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+        x, y, w, h = self:getBoundedRect(x, y, w, h)
         if w <= 0 or h <= 0 then return end
         cblitbuffer.BB_blend_rect(ffi.cast(P_BlitBuffer, self),
             x, y, w, h, color)
@@ -1931,8 +2008,7 @@ lighten color values in rectangular area
 function BB_mt.__index:lightenRect(x, y, w, h, by)
     local color = Color8A(0, 0xFF*(by or 0.5))
     if self:canUseCbb() then
-        w, x = BB.checkBounds(w, x, 0, self:getWidth(), 0xFFFF)
-        h, y = BB.checkBounds(h, y, 0, self:getHeight(), 0xFFFF)
+        x, y, w, h = self:getBoundedRect(x, y, w, h)
         if w <= 0 or h <= 0 then return end
         cblitbuffer.BB_blend_rect(ffi.cast(P_BlitBuffer, self),
             x, y, w, h, color)

ffi/framebuffer.lua

@@ -13,7 +13,7 @@ local fb = {
     debug = function(...) --[[ NOP ]] end,
 
     bb = nil, -- should be set by implementations
-    full_bb = nil, -- will hold a saved reference when a viewport is set
+    full_bb = nil, -- will hold a reference to the full-size native buffer when a viewport is set
     viewport = nil,
     screen_size = nil,
     native_rotation_mode = nil,
@@ -201,6 +201,7 @@ function fb:refreshWaitForLastImp()
 end
 
 -- these should not be overridden, they provide the external refresh API:
+--- @note: x, y, w, h are *mandatory*, even for refreshFull! (UIManager guarantees it).
 function fb:refreshFull(x, y, w, h, d)
     x, y, w, h = self:calculateRealCoordinates(x, y, w, h)
     return self:refreshFullImp(x, y, w, h, d)
@@ -270,18 +271,16 @@ function fb:setViewport(viewport)
         viewport.w, viewport.h)
     self.viewport = viewport
     self.full_bb:fill(Blitbuffer.COLOR_WHITE)
-    self:refreshFull()
+    -- We want the *viewport's* dimensions, as calculateRealCoordinates will adjust the coordinates later.
+    self:refreshFull(0, 0, self:getWidth(), self:getHeight())
 end
 
+-- If there is a viewport in place, spit out adjusted coordinates for the native buffer that account for it
 function fb:calculateRealCoordinates(x, y, w, h)
-    local mode = self:getRotationMode()
-    if not (x and y and w and h) then return end
-
-    -- TODO: May need to implement refresh translations for HW rotate on broken drivers.
-    -- For now those should just avoid using HW mode altogether.
-
     if not self.viewport then return x, y, w, h end
 
+    -- TODO: May need to implement refresh translations for HW rotate on broken drivers.
+    --       For now those should just avoid using HW mode altogether.
     --[[
         we need to adapt the coordinates when we have a viewport.
         this adaptation depends on the rotation:
@@ -309,23 +308,30 @@ function fb:calculateRealCoordinates(x, y, w, h)
         recalculate the offsets.
     --]]
 
+    -- Ensure OOB coordinates do not allow requesting a refresh large enough to spill *outside* of the viewport...
+    -- NOTE: This is done here, instead of in the public refresh*() methods (where we'd explicitly run it on the viewport bb),
+    --       because implementations may prefer to bound to the *full* screen, instead of the viewport's dimensions,
+    --       especially if an alignment constraint is at play...
+    x, y, w, h = self.bb:getBoundedRect(x, y, w, h)
+
+    local mode = self:getRotationMode()
     local vx2 = self.screen_size.w - (self.viewport.x + self.viewport.w)
     local vy2 = self.screen_size.h - (self.viewport.y + self.viewport.h)
 
     if mode == self.DEVICE_ROTATED_UPRIGHT then
-        -- (0,0) is at top left of screen
+        -- (0, 0) is at top left of screen
         x = x + self.viewport.x
         y = y + self.viewport.y
     elseif mode == self.DEVICE_ROTATED_CLOCKWISE then
-        -- (0,0) is at bottom left of screen
+        -- (0, 0) is at bottom left of screen
         x = x + vy2
         y = y + self.viewport.x
     elseif mode == self.DEVICE_ROTATED_UPSIDE_DOWN then
-        -- (0,0) is at bottom right of screen
+        -- (0, 0) is at bottom right of screen
         x = x + vx2
         y = y + vy2
     else -- self.DEVICE_ROTATED_COUNTER_CLOCKWISE
-        -- (0,0) is at top right of screen
+        -- (0, 0) is at top right of screen
         x = x + self.viewport.y
         y = y + vx2
     end
@@ -421,6 +427,7 @@ function fb:getRotationMode()
     return self.cur_rotation_mode
 end
 
+--- This reflects how the screen *looks* like, not the screen layout relative to its native orientation...
 function fb:getScreenMode()
     if self:getWidth() > self:getHeight() then
         return "landscape"
@@ -429,6 +436,17 @@ function fb:getScreenMode()
     end
 end
 
+--- This reflects the current layout in terms of *rotation* modes
+function fb:getScreenOrientation()
+    if bit.band(self:getRotationMode(), 1) == 1 then
+        -- LinuxFB constants, Landscapes are odds
+        return "landscape"
+    else
+        -- And Portraits are even
+        return "portrait"
+    end
+end
+
 -- Configure desired rotation. By default, we setup the blitter to do rotations for us, but a subclass
 -- may implement rotation via hardware (android with hasNativeRotation, linuxfb with forced_rotation).
 function fb:setRotationMode(mode)

ffi/framebuffer_SDL2_0.lua

@@ -24,7 +24,7 @@ function framebuffer:init()
     end
 
     self.bb:fill(BB.COLOR_WHITE)
-    self:refreshFull()
+    self:refreshFull(0, 0, self:getWidth(), self:getHeight())
 
     framebuffer.parent.init(self)
 end
@@ -63,7 +63,7 @@ function framebuffer:resize(w, h)
     SDL.texture = SDL.createTexture(w, h)
 
     self.bb:fill(BB.COLOR_WHITE)
-    self:refreshFull()
+    self:refreshFull(0, 0, self:getWidth(), self:getHeight())
 end
 
 local bb_emu = os.getenv("EMULATE_BB_TYPE")
@@ -105,9 +105,6 @@ function framebuffer:_newBB(w, h)
 end
 
 function framebuffer:_render(bb, x, y, w, h)
-    w, x = BB.checkBounds(w or bb:getWidth(), x or 0, 0, bb:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h or bb:getHeight(), y or 0, 0, bb:getHeight(), 0xFFFF)
-
     -- x, y, w, h without rotation for SDL rectangle
     local px, py, pw, ph = bb:getPhysicalRect(x, y, w, h)
 
@@ -140,23 +137,19 @@ function framebuffer:refreshFullImp(x, y, w, h)
     if self.dummy then return end
 
     local bb = self.full_bb or self.bb
-
-    if not (x and y and w and h) then
-        x = 0
-        y = 0
-        w = bb:getWidth()
-        h = bb:getHeight()
-    end
-
-    self.debug("refresh on physical rectangle", x, y, w, h)
+    x, y, w, h = bb:getBoundedRect(x, y, w, h)
+    self.debug(string.format("refresh on logical rectangle %dx%d+%d+%d", w, h, x, y))
 
     local flash = os.getenv("EMULATE_READER_FLASH")
     if flash then
+        -- Match the rotation of the screen buffer now
+        self.sdl_bb:setRotation(bb:getRotation())
+        self.sdl_bb:setInverse(bb:getInverse())
+        -- This ensures invertRect will not paint to now potentially off-screen regions (e.g., because of a viewport)
         self.sdl_bb:invertRect(x, y, w, h)
         self:_render(self.sdl_bb, x, y, w, h)
         util.usleep(tonumber(flash)*1000)
-        self.sdl_bb:setRotation(bb:getRotation())
-        self.sdl_bb:setInverse(bb:getInverse())
+        -- Resync the shadow buffer with the new content
         self.sdl_bb:blitFrom(bb, x, y, x, y, w, h)
     end
     self:_render(bb, x, y, w, h)

ffi/framebuffer_android.lua

@@ -19,8 +19,7 @@ local framebuffer = {}
 -- update a region of the screen
 function framebuffer:_updatePartial(mode, delay, x, y, w, h)
     local bb = self.full_bb or self.bb
-    w, x = BB.checkBounds(w or bb:getWidth(), x or 0, 0, bb:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h or bb:getHeight(), y or 0, 0, bb:getHeight(), 0xFFFF)
+    x, y, w, h = bb:getBoundedRect(x, y, w, h)
     x, y, w, h = bb:getPhysicalRect(x, y, w, h)
 
     android.einkUpdate(mode, delay, x, y, (x + w), (y + h))

ffi/framebuffer_einkfb.lua

@@ -1,14 +1,12 @@
 local ffi = require("ffi")
-local BB = require("ffi/blitbuffer")
 require("ffi/posix_h")
 require("ffi/einkfb_h")
 local C = ffi.C
 
 local framebuffer = {}
 
 local function einkfb_update(fb, refreshtype, x, y, w, h)
-    w, x = BB.checkBounds(w or fb.bb:getWidth(), x or 0, 0, fb.bb:getWidth(), 0xFFFF)
-    h, y = BB.checkBounds(h or fb.bb:getHeight(), y or 0, 0, fb.bb:getHeight(), 0xFFFF)
+    x, y, w, h = fb.bb:getBoundedRect(x, y, w, h)
     x, y, w, h = fb.bb:getPhysicalRect(x, y, w, h)
 
     local refarea = ffi.new("struct update_area_t[1]")