Fix memory leak when updating images.

randopix.nimble

@@ -12,7 +12,7 @@ bin           = @["randopix", "pixctrl"]
 requires "nim >= 1.0.0", "gintro >= 0.5.5", "argparse >=0.10.1"
 
 task debug, "Compile debug version":
-  exec "nim c -d:debug --debugger:native src/randopix.nim"
+  exec "nim c -d:debug --debugger:native --out:randopix src/randopix.nim"
 
 task release, "Compile release version":
   exec fmt"nim c -d:release --out:randopix-{version} src/randopix.nim"

src/commands.nim

@@ -1,17 +1,28 @@
 import json
+
 const
   defaultPort* = 5555     ## Default port at which the control server will run
 
 type
+  OpResult* = object of RootObj ## Result object for signalling failure state across proc calls
+    success*: bool              ## Indicating if the opration was successfull
+    errorMsg*: string           ## Error meassge in case the operation failed
+
   Command* = enum
     cClose = "close"       ## Closes the control server and exists the applicaiton
     cRefresh = "refresh"   ## Force refresh of the image now
     cTimeout = "timeout"   ## Set image timeout to a new value
 
-  CommandMessage* = object
+  CommandMessage* = object of RootObj
     command*: Command     ## Command that the application should execute
     parameter*: string    ## Optional parameter for the command
 
+proc newOpResult*(): OpResult =
+  OpResult(success: true)
+
+proc newOpResult*(msg: string): OpResult =
+  OpResult(success: false, errorMsg: msg)
+
 proc newCommand*(c: Command, p: string = ""): CommandMessage =
   CommandMessage(command: c, parameter: p)
 

src/providers.nim

@@ -1,17 +1,23 @@
-import os, sets, random, httpClient, json
+import os, sets, random, httpClient, json, strformat
-import gintro/[gdkpixbuf]
+import gintro/[gdkpixbuf, gobject, gtk]
+import commands
 
 const
   supportedExts = @[".png", ".jpg", ".jpeg"]
   foxesUrl = "https://randomfox.ca/floof/"
+  tmpFile = "/tmp/randopix_tmp.png"
 
 type
+  FileOpResult* = object of OpResult
+    file*: string
+
   ProviderKind* {.pure.} = enum
     Foxes = "foxes"       ## Some nice foxes
     Inspiro = "inspiro"   ## Inspiring nonsense
     File = "file"         ## Images from a local path
 
-  ImageProvider* = ref object
+  ImageProvider* = ref object of RootObj
+    verbose: bool
     case kind: ProviderKind
     of ProviderKind.Foxes, ProviderKind.Inspiro:
       url: string
@@ -20,49 +26,25 @@ type
       path*: string
       files*: seq[string]
 
-var client = newHttpClient()  ## For loading images from the web
+var
+  client = newHttpClient()  ## For loading images from the web
+  verbose = true
 
-proc downloadFox(ip: ImageProvider): Pixbuf =
+########################
-  ## Download image from the fox API
+# Constructors
-  let urlData = client.getContent(ip.url)
+########################
-  let info = parseJson(urlData)
-  let imageData = client.getContent(info["image"].getStr)
-  let loader = newPixbufLoader()
-  discard loader.write(imageData)
-  loader.getPixbuf()
-
-proc reloadFileList(ip: ImageProvider) =
-  ## Reload the file list
-  if ip.path == "":
-    return
-
-  for file in walkDirRec(ip.path):
-    let split = splitFile(file)
-    if ip.exts.contains(split.ext):
-      ip.files.add(file)
-  
-  randomize()
-  shuffle(ip.files)
-
-proc next*(ip: ImageProvider): Pixbuf =
-  ## Return a new image from the chosen image source
-  case ip.kind
-  of ProviderKind.Foxes, ProviderKind.Inspiro:
-    return ip.downloadFox
-  of ProviderKind.File:
-    if ip.files.len < 1:
-      ip.reloadFileList
-    result = ip.files[0].newPixbufFromFile
-    ip.files.delete(0)
 
 proc newFileProvider(path: string): ImageProvider =
+  ## Create an image provider to access images from the local file system 
+  randomize()
   result = ImageProvider(kind: ProviderKind.File, path: path, exts: supportedExts.toHashSet)
-  result.reloadFileList
 
-proc newFoxProvider(): ImageProvider = ImageProvider(kind: ProviderKind.Foxes, url: foxesUrl)
+proc newFoxProvider(): ImageProvider =
+  ## Create an image provider to access the API at "https://randomfox.ca/floof/".
+  ImageProvider(kind: ProviderKind.Foxes, url: foxesUrl)
 
 proc newImageProvider*(kind: ProviderKind, filePath: string = ""): ImageProvider =
-  ## Create a new `ImageProvider` for the API chosen with thge `kind` parameter 
+  ## Create a new `ImageProvider` for the API.
   case kind
   of ProviderKind.Foxes:
     newFoxProvider()
@@ -71,3 +53,96 @@ proc newImageProvider*(kind: ProviderKind, filePath: string = ""): ImageProvider
     newFoxProvider()
   of ProviderKind.File:
     newFileProvider(filePath)
+
+proc newFileOpResultError(msg: string): FileOpResult =
+  FileOpResult(success: false, errorMsg: msg)
+
+proc newFileOpResult(file: string): FileOpResult =
+  FileOpResult(success: true, file: file)
+
+########################
+# Utilities
+########################
+
+proc log(msg: string) =
+  if verbose: echo msg
+
+########################
+# Image Provider procs
+########################
+
+proc getFox(ip: ImageProvider): FileOpResult =
+  ## Download image from the fox API
+  try:
+    let urlData = client.getContent(ip.url)
+    let info = parseJson(urlData)
+    let imageData = client.getContent(info["image"].getStr)
+    let dlFile = fmt"{tmpFile}.download"
+    writeFile(dlFile, imageData)
+    return newFileOpResult(dlFile)
+  except JsonParsingError:
+    log fmt"Error while fetching from fox API: {getCurrentExceptionMsg()}"
+    return newFileOpResultError("Json parsing error")
+  except KeyError:
+    log fmt"No image in downloaded data: {getCurrentExceptionMsg()}"
+    return newFileOpResultError("No image from API")
+
+proc getLocalFile(ip: var ImageProvider): FileOpResult =
+  ## Provide an image from a local folder
+  # First, check if there are still images left to be loaded.
+  # If not reread all files from the path
+  if ip.files.len < 1:    
+    if ip.path == "":
+      return newFileOpResultError("No path for image loading")
+    log "Reloading file list..."
+    for file in walkDirRec(ip.path):
+      let split = splitFile(file)
+      if ip.exts.contains(split.ext):
+        ip.files.add(file)
+    log fmt"Loaded {ip.files.len} files"
+    shuffle(ip.files)
+  # Remove the current file after 
+  result = newFileOpResult(ip.files[0])
+  ip.files.delete(0)
+
+proc getFileName(ip: var ImageProvider): FileOpResult =
+  ## Get the temporary file name of the next file to display
+  case ip.kind
+  of ProviderKind.File:
+    result = ip.getLocalFile()
+  of ProviderKind.Foxes:
+    result = ip.getFox()
+  else:
+    result = newFileOpResultError("Not implemented")
+
+########################
+# Exported procs
+########################
+
+proc next*(ip: var ImageProvider, width, height: int): FileOpResult =
+  ## Uses the image provider to get a new image ready to display.
+  ## `width` and `height` should be the size of the window.
+  let op = ip.getFileName()
+  if not op.success: return op
+
+  var rawPixbuf = newPixbufFromFile(op.file)
+  # resize the pixbuf to best fit on screen
+  var w, h: int
+  if (width > height):
+    h = height
+    w = ((rawPixbuf.width * h) / rawPixbuf.height).toInt 
+  else:
+    w = width
+    h = ((rawPixbuf.height * w) / rawPixbuf.width).toInt
+  var pixbuf = rawPixbuf.scaleSimple(w, h, InterpType.bilinear)
+  # The pixbuf is written to disk and loaded again once because
+  # directly setting the image from a pixbuf will leak memory
+  let saved = pixbuf.savev(tmpFile, "png", @[])
+  if not saved:
+    return newFileOpResultError("Error while saving temporary image")
+
+  # GTK pixbuf leaks memory when not manually decreasing reference count
+  pixbuf.genericGObjectUnref()
+  rawPixbuf.genericGObjectUnref()
+
+  newFileOpResult(tmpFile)

src/randopix.nim

@@ -1,8 +1,6 @@
 import os, options, strformat
-import gintro/[glib, gobject, gdkpixbuf]
+import gintro/[glib, gobject, gdkpixbuf, gtk, gio]
 import gintro/gdk except Window
-import gintro/gtk except newSocket, Socket
-import gintro/gio except Socket
 import argparse except run
 import providers, server, commands
 
@@ -21,7 +19,6 @@ var
   args: Args                    ## The parsed command line args
   # Widgets
   window: ApplicationWindow
-  imageWidget: Image
   label: Label
   # Server vor recieving commands from external tools
   serverWorker: system.Thread[void]
@@ -67,37 +64,20 @@ proc newArgs(): Option[Args] =
     echo getCurrentExceptionMsg()
     echo p.help
 
-proc updateImage(): bool =
+proc updateImage(image: Image): bool =
   ## Updates the UI with a new image
-  # Loading new image
   try:
     if (args.verbose): echo "Refreshing..."
-    # TODO better error signalling from providers.nim
-    let data = some(imageProvider.next)
-    if data.isNone:
-      label.notify "No image to display..."
-      return false;
 
-    ## Resize image to best fit the window
+    var wWidth, wHeight: int
-    var pixbuf = data.get()
-    var wWidth, wHeight, width, height: int
     window.getSize(wWidth, wHeight)
-    if (wWidth > wHeight):
-      height = wHeight
-      width = ((pixbuf.width * height) / pixbuf.height).toInt 
-    else:
-      width = wWidth
-      height = ((pixbuf.height * width) / pixbuf.width).toInt
-    pixbuf = pixbuf.scaleSimple(width, height, InterpType.bilinear)
 
-    # Update the UI with the image
+    let op = imageProvider.next(wWidth, wHeight)
-    imageWidget.setFromPixbuf(pixbuf)
+    result = op.success
-    if (args.verbose):
+    if op.success:
-      label.notify "New image set"
+      image.setFromFile(op.file)
     else:
-      label.notify
+      label.notify op.errorMsg
-    return true
-
   except:
     let
       e = getCurrentException()
@@ -105,17 +85,19 @@ proc updateImage(): bool =
     echo "Got exception ", repr(e), " with message ", msg
     return false
 
-proc forceUpdate(action: SimpleAction; parameter: Variant;) =
+proc forceUpdate(action: SimpleAction; parameter: Variant; image: Image) =
-  discard updateImage()
+  discard updateImage(image)
 
-proc timedUpdate(image: Widget): bool = 
+proc timedUpdate(image: Image): bool = 
-  let ok = updateImage();
+  let ok = updateImage(image);
   if not ok:
     label.notify "Error while refreshing image, retrying..."
-  discard timeoutAdd(uint32(args.timeout), timedUpdate, imageWidget)
+  else:
+    label.notify
+  discard timeoutAdd(uint32(args.timeout), timedUpdate, image)
   return false
 
-proc checkServerChannel(parameter: string): bool =
+proc checkServerChannel(image: Image): bool =
   ## Check the channel from the control server for incomming commands
   let tried = chan.tryRecv()
 
@@ -125,7 +107,7 @@ proc checkServerChannel(parameter: string): bool =
 
     case msg.command
     of cRefresh:
-      discard updateImage()
+      discard updateImage(image)
     of cTimeout:
       let val = msg.parameter.parseInt * 1000
       echo "Setting timeout to ", val
@@ -134,8 +116,8 @@ proc checkServerChannel(parameter: string): bool =
       echo "Command ignored: ", msg.command
 
   sleep(100)
-  result = false
+  result = true
-  discard idleAdd(checkServerChannel, parameter)
+  # discard idleAdd(checkServerChannel, parameter)
 
 proc toggleFullscreen(action: SimpleAction; parameter: Variant; window: ApplicationWindow) =
   ## Fullscreen toggle event
@@ -158,25 +140,6 @@ proc quit(action: SimpleAction; parameter: Variant; app: Application) =
   ## Application quit event
   cleanUp(window, app)
   
-proc connectSignals(app: Application) =
-  ## Connect the GTK signals to the procs 
-  let fullscreenAction = newSimpleAction("fullscreen")
-  discard fullscreenAction.connect("activate", toggleFullscreen, window)
-  app.setAccelsForAction("win.fullscreen", "F")
-  window.actionMap.addAction(fullscreenAction)
-
-  let quitAction = newSimpleAction("quit")
-  discard quitAction.connect("activate", quit, app)
-  app.setAccelsForAction("win.quit", "Escape")
-  window.actionMap.addAction(quitAction)
-
-  let updateImageAction = newSimpleAction("update")
-  discard updateImageAction.connect("activate", forceUpdate)
-  app.setAccelsForAction("win.update", "U")
-  window.actionMap.addAction(updateImageAction)
-
-  window.connect("destroy", cleanUp, app)
-
 proc appActivate(app: Application) =
   # Parse arguments from the command line
   let parsed = newArgs()
@@ -196,31 +159,48 @@ proc appActivate(app: Application) =
   addProviderForScreen(getDefaultScreen(), provider, STYLE_PROVIDER_PRIORITY_USER)
 
   # Create all windgets we are gonna use
-  var overlay = newOverlay()
-  imageWidget = newImage()
   label = newLabel("Starting...")
   label.halign = Align.`end`
   label.valign = Align.`end`
 
-  overlay.addOverlay(label)
+  let container = newOverlay()
-  overlay.add(imageWidget)
+  container.addOverlay(label)
-  window.add(overlay)
+  window.add(container)
+  
+  let image = newImage()
+  container.add(image)
 
   if args.fullscreen:
     window.fullscreen
 
-  app.connectSignals
+  ## Connect the GTK signals to the procs 
+  let fullscreenAction = newSimpleAction("fullscreen")
+  discard fullscreenAction.connect("activate", toggleFullscreen, window)
+  app.setAccelsForAction("win.fullscreen", "F")
+  window.actionMap.addAction(fullscreenAction)
+
+  let quitAction = newSimpleAction("quit")
+  discard quitAction.connect("activate", quit, app)
+  app.setAccelsForAction("win.quit", "Escape")
+  window.actionMap.addAction(quitAction)
+
+  let updateImageAction = newSimpleAction("update")
+  discard updateImageAction.connect("activate", forceUpdate, image)
+  app.setAccelsForAction("win.update", "U")
+  window.actionMap.addAction(updateImageAction)
+
+  window.connect("destroy", cleanUp, app)
+
   window.showAll
 
-  discard timeoutAdd(500, timedUpdate, imageWidget)
+  var timerId = timeoutAdd(500, timedUpdate, image)
 
   ## open communication channel from the control server
   chan.open()
 
   ## Start the server for handling incoming commands
   createThread(serverWorker, runServer)
-  var tag = ""
+  discard idleAdd(checkServerChannel, image)
-  discard idleAdd(checkServerChannel, tag)
 
 when isMainModule:
   let app = newApplication("org.luxick.randopix")

src/randopix.nim.cfg

@@ -1,2 +1,3 @@
 threads:on
 d:ssl
+gc:arc