jbaiter / gphoto2-cffi

Python bindings for libgphoto2 with an idiomatic API
GNU Lesser General Public License v3.0
38 stars 17 forks source link

Remove decorator `exit_after` from `Camera.get_preview()` #21

Closed define-private-public closed 7 years ago

define-private-public commented 7 years ago

https://github.com/jbaiter/gphoto2-cffi/blob/master/gphoto2cffi/gphoto2.py#L748

images that are returned from gp_camera_capture_preview() are meant to by tiny preview images, that can captured many times per second (e.g. 30 captures per second) to provide a sort of live feed from the camera.

The Camera.get_preview() method is functionally correct, except that the decorator @exit_after will put the camera in a state where it can't successively capture preview images.

Take this code snippet for example

#!/usr/bin/env python3
#
# A small test that will let the user view a live feed when pressing "p"

import sys
import tempfile
from PyQt5.QtCore import *
from PyQt5.QtGui import *
from PyQt5.QtWidgets import *

import gphoto2cffi as gp
from gphoto2cffi.backend import ffi as gpFFI
from gphoto2cffi.backend import lib as gpLIB

# Have to hack up the `get_preview` because of @exit_after
def get_preview(cam):
    camfile_p = gpFFI.new('CameraFile **')
    gpLIB.gp_file_new(camfile_p)
    gpLIB.gp_camera_capture_preview(cam._cam, camfile_p[0], cam._ctx)
    data_p = gpFFI.new('char **')
    length_p = gpFFI.new('unsigned long *')
    gpLIB.gp_file_get_data_and_size(camfile_p[0], data_p, length_p)
    return gpFFI.buffer(data_p[0], length_p[0])[:]

class PreviewWidget(QWidget):
    __slots__ = (
        'camera',   # gp.Camera
        'frame',    # QLabel, where picture goes
        'showingPreview', # bool, if showing a preview image or not
    )

    def __init__(self, parent=None):
        super(PreviewWidget, self).__init__(parent)

        self.showingPreview = False

        # Setup UI
        layout = QVBoxLayout(self)
        layout.setContentsMargins(0, 0, 0, 0)
        layout.setAlignment(Qt.AlignHCenter or Qt.AlignVCenter)
        self.setFixedSize(960, 540)
        self.setWindowTitle('Live Preview')

        # Create a label
        self.frame = QLabel(self)
        layout.addWidget(self.frame)

        # Get the camera
        # TODO check for no camera attached
        self.camera = gp.Camera()

        self.show()

    def keyPressEvent(self, event):
        key = event.key()

        # close on ESC press
        if key == Qt.Key_Escape:
            self.close()
        elif key == Qt.Key_P:
            self.showingPreview = not self.showingPreview
            self.showPreview()

    def showPreview(self):
        if self.showingPreview:
            preview = get_preview(self.camera)

            # TODO save to memory later, it's faster
            # Save the previewture to disk
            tf = tempfile.NamedTemporaryFile(suffix='.jpeg')
            tf.write(preview)
            tf.flush()

            # Load with QPixmap
            self.frame.setPixmap(QPixmap(tf.name).scaled(960, 540, Qt.KeepAspectRatio))

            # delete file
            tf.close()

            # Setup another show preview in 1/60 of a second
            QTimer.singleShot(1000 // 60, self.showPreview)
        else:
            pass

def main():
    app = QApplication(sys.argv)
    pw = PreviewWidget()
    sys.exit(app.exec_())

if __name__ == '__main__':
    main()

When pressing P, it will create a live camera feed, using the get_preview() that I defined at the top of the file.

Please remove the decorator from Camera.get_preview(). Is there some extra cleanup function that should be called for when to turn a preview off?

define-private-public commented 7 years ago

Also, I don't know too much about Python's GC, and how CFFI works with it when doing those new() calls, but I noticed that this program was slowly eating up more and more RAM when running, with no signs of decreasing. When I turned off of the preview, I noticed that it wasn't creeping up anymore.

I don't think it's my PyQt code, though I could be wrong.

define-private-public commented 7 years ago

My suspicions were correct, it was from the get_preview() code and the CFFI new allocations. With the one above, my system would jump from 0.6% memory usage to about 6% in five minutes.

I switched the code out with this:

camfile_p = gpFFI.new('CameraFile **')
gpLIB.gp_file_new(camfile_p)
data_p = gpFFI.new('char **')
length_p = gpFFI.new('unsigned long *')
def get_preview(cam):
    gpLIB.gp_camera_capture_preview(cam._cam, camfile_p[0], cam._ctx)
    gpLIB.gp_file_get_data_and_size(camfile_p[0], data_p, length_p)
    return gpFFI.buffer(data_p[0], length_p[0])[:]

After running the program for about 10 minutes memory usage flatlined at 0.6%.

I'm pretty sure there are other parts in the Camera wrapper that are making many more new allocations than needed. It would be best to give the Camera class some internal CFFI buffers so the methods themselves aren't doing the allocations; but that should be it's own separate ticket.

jbaiter commented 7 years ago

Thank you for the analysis! I was pretty inexperienced with CFFI when I wrote the code.

However, regarding your question:

Also, I don't know too much about Python's GC, and how CFFI works with it when doing those new() calls, but I noticed that this program was slowly eating up more and more RAM when running, with no signs of decreasing.

According to the CFFI docs, the underlying memory should be freed once the Python variable goes out of scope. Seems like this is not the case for the camfile_p and data_p, though :-/

I'm not sure when I'll be able to get to making the necessary changes, but you're welcome to submit a pull request, and I promise that I won't take as long for merging it as it took me to respond to this issue (sorry) ;-)

define-private-public commented 7 years ago

I might go ahead and do this soon enough, but not immediately.

As stated before, this might be a little more complex of a cleanup since I think there are a lot more memory leaks in Camera that need to be dealt with (.e.g, I think it's present for each time a picture is taken).

I think it would be best to first do an audit of where the leaks are.

jbaiter commented 7 years ago

Might be a good idea to set up a testing script that runs through all exposed functionality with real hardware and then run that through valgrind.