[libcamera-devel] [PATCH v4 01/15] py: Use exceptions instead of returning error codes
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Mar 9 16:35:25 CET 2023
Hi Tomi,
Thank you for the patch.
On Thu, Mar 09, 2023 at 04:25:47PM +0200, Tomi Valkeinen via libcamera-devel wrote:
> We have multiple methods which return an error code, mimicking the C++
> API. Using exceptions is more natural in the Python API, so change all
> those methods to raise an Exception instead.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/py/cam/cam.py | 16 +----
> src/py/examples/simple-cam.py | 15 +---
> src/py/examples/simple-capture.py | 21 ++----
> src/py/examples/simple-continuous-capture.py | 21 ++----
> src/py/libcamera/py_main.cpp | 60 ++++++++++++----
> test/py/unittests.py | 76 +++++++-------------
> 6 files changed, 91 insertions(+), 118 deletions(-)
>
> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
> index 967a72f5..a2a115c1 100755
> --- a/src/py/cam/cam.py
> +++ b/src/py/cam/cam.py
> @@ -158,9 +158,7 @@ class CameraContext:
>
> print('Camera configuration adjusted')
>
> - r = self.camera.configure(camconfig)
> - if r != 0:
> - raise Exception('Configure failed')
> + self.camera.configure(camconfig)
>
> self.stream_names = {}
> self.streams = []
> @@ -175,12 +173,7 @@ class CameraContext:
> allocator = libcam.FrameBufferAllocator(self.camera)
>
> for stream in self.streams:
> - ret = allocator.allocate(stream)
> - if ret < 0:
> - print('Cannot allocate buffers')
> - exit(-1)
> -
> - allocated = len(allocator.buffers(stream))
> + allocated = allocator.allocate(stream)
>
> print('{}-{}: Allocated {} buffers'.format(self.id, self.stream_names[stream], allocated))
>
> @@ -205,10 +198,7 @@ class CameraContext:
> buffers = self.allocator.buffers(stream)
> buffer = buffers[buf_num]
>
> - ret = request.add_buffer(stream, buffer)
> - if ret < 0:
> - print('Can not set buffer for request')
> - exit(-1)
> + request.add_buffer(stream, buffer)
>
> requests.append(request)
>
> diff --git a/src/py/examples/simple-cam.py b/src/py/examples/simple-cam.py
> index b3e97ca7..1cd1019d 100755
> --- a/src/py/examples/simple-cam.py
> +++ b/src/py/examples/simple-cam.py
> @@ -259,12 +259,7 @@ def main():
> allocator = libcam.FrameBufferAllocator(camera)
>
> for cfg in config:
> - ret = allocator.allocate(cfg.stream)
> - if ret < 0:
> - print('Can\'t allocate buffers')
> - return -1
> -
> - allocated = len(allocator.buffers(cfg.stream))
> + allocated = allocator.allocate(cfg.stream)
> print(f'Allocated {allocated} buffers for stream')
>
> # --------------------------------------------------------------------
> @@ -289,15 +284,9 @@ def main():
> requests = []
> for i in range(len(buffers)):
> request = camera.create_request()
> - if not request:
> - print('Can\'t create request')
> - return -1
>
> buffer = buffers[i]
> - ret = request.add_buffer(stream, buffer)
> - if ret < 0:
> - print('Can\'t set buffer for request')
> - return -1
> + request.add_buffer(stream, buffer)
>
> # Controls can be added to a request on a per frame basis.
> request.set_control(libcam.controls.Brightness, 0.5)
> diff --git a/src/py/examples/simple-capture.py b/src/py/examples/simple-capture.py
> index 5f93574f..4b85408f 100755
> --- a/src/py/examples/simple-capture.py
> +++ b/src/py/examples/simple-capture.py
> @@ -43,8 +43,7 @@ def main():
>
> # Acquire the camera for our use
>
> - ret = cam.acquire()
> - assert ret == 0
> + cam.acquire()
>
> # Configure the camera
>
> @@ -60,8 +59,7 @@ def main():
> w, h = [int(v) for v in args.size.split('x')]
> stream_config.size = libcam.Size(w, h)
>
> - ret = cam.configure(cam_config)
> - assert ret == 0
> + cam.configure(cam_config)
>
> print(f'Capturing {TOTAL_FRAMES} frames with {stream_config}')
>
> @@ -83,15 +81,13 @@ def main():
> req = cam.create_request(i)
>
> buffer = allocator.buffers(stream)[i]
> - ret = req.add_buffer(stream, buffer)
> - assert ret == 0
> + req.add_buffer(stream, buffer)
>
> reqs.append(req)
>
> # Start the camera
>
> - ret = cam.start()
> - assert ret == 0
> + cam.start()
>
> # frames_queued and frames_done track the number of frames queued and done
>
> @@ -101,8 +97,7 @@ def main():
> # Queue the requests to the camera
>
> for req in reqs:
> - ret = cam.queue_request(req)
> - assert ret == 0
> + cam.queue_request(req)
> frames_queued += 1
>
> # The main loop. Wait for the queued Requests to complete, process them,
> @@ -155,13 +150,11 @@ def main():
>
> # Stop the camera
>
> - ret = cam.stop()
> - assert ret == 0
> + cam.stop()
>
> # Release the camera
>
> - ret = cam.release()
> - assert ret == 0
> + cam.release()
>
> return 0
>
> diff --git a/src/py/examples/simple-continuous-capture.py b/src/py/examples/simple-continuous-capture.py
> index 26a8060b..e1cb931e 100755
> --- a/src/py/examples/simple-continuous-capture.py
> +++ b/src/py/examples/simple-continuous-capture.py
> @@ -28,8 +28,7 @@ class CameraCaptureContext:
>
> # Acquire the camera for our use
>
> - ret = cam.acquire()
> - assert ret == 0
> + cam.acquire()
>
> # Configure the camera
>
> @@ -37,8 +36,7 @@ class CameraCaptureContext:
>
> stream_config = cam_config.at(0)
>
> - ret = cam.configure(cam_config)
> - assert ret == 0
> + cam.configure(cam_config)
>
> stream = stream_config.stream
>
> @@ -62,8 +60,7 @@ class CameraCaptureContext:
> req = cam.create_request(idx)
>
> buffer = allocator.buffers(stream)[i]
> - ret = req.add_buffer(stream, buffer)
> - assert ret == 0
> + req.add_buffer(stream, buffer)
>
> self.reqs.append(req)
>
> @@ -73,13 +70,11 @@ class CameraCaptureContext:
> def uninit_camera(self):
> # Stop the camera
>
> - ret = self.cam.stop()
> - assert ret == 0
> + self.cam.stop()
>
> # Release the camera
>
> - ret = self.cam.release()
> - assert ret == 0
> + self.cam.release()
>
>
> # A container class for our state
> @@ -145,8 +140,7 @@ class CaptureContext:
>
> for cam_ctx in self.camera_contexts:
> for req in cam_ctx.reqs:
> - ret = cam_ctx.cam.queue_request(req)
> - assert ret == 0
> + cam_ctx.cam.queue_request(req)
>
> # Use Selector to wait for events from the camera and from the keyboard
>
> @@ -177,8 +171,7 @@ def main():
> # Start the cameras
>
> for cam_ctx in ctx.camera_contexts:
> - ret = cam_ctx.cam.start()
> - assert ret == 0
> + cam_ctx.cam.start()
>
> ctx.capture()
>
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index d14e18e2..1d4c23b3 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -114,8 +114,18 @@ PYBIND11_MODULE(_libcamera, m)
>
> pyCamera
> .def_property_readonly("id", &Camera::id)
> - .def("acquire", &Camera::acquire)
> - .def("release", &Camera::release)
> + .def("acquire", [](Camera &self) {
> + int ret = self.acquire();
> + if (ret)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to acquire camera");
> + })
> + .def("release", [](Camera &self) {
> + int ret = self.release();
> + if (ret)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to release camera");
> + })
> .def("start", [](Camera &self,
> const std::unordered_map<const ControlId *, py::object> &controls) {
> /* \todo What happens if someone calls start() multiple times? */
> @@ -135,20 +145,19 @@ PYBIND11_MODULE(_libcamera, m)
> int ret = self.start(&controlList);
> if (ret) {
> self.requestCompleted.disconnect();
> - return ret;
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to start camera");
> }
> -
> - return 0;
> }, py::arg("controls") = std::unordered_map<const ControlId *, py::object>())
>
> .def("stop", [](Camera &self) {
> int ret = self.stop();
> - if (ret)
> - return ret;
>
> self.requestCompleted.disconnect();
>
> - return 0;
> + if (ret)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to stop camera");
> })
>
> .def("__str__", [](Camera &self) {
> @@ -157,9 +166,20 @@ PYBIND11_MODULE(_libcamera, m)
>
> /* Keep the camera alive, as StreamConfiguration contains a Stream* */
> .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
> - .def("configure", &Camera::configure)
> + .def("configure", [](Camera &self, CameraConfiguration *config) {
> + int ret = self.configure(config);
> + if (ret)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to configure camera");
> + })
>
> - .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
> + .def("create_request", [](Camera &self, uint64_t cookie) {
> + std::unique_ptr<Request> req = self.createRequest(cookie);
> + if (!req)
> + throw std::system_error(ENOMEM, std::generic_category(),
> + "Failed to create request");
> + return req;
> + }, py::arg("cookie") = 0)
>
> .def("queue_request", [](Camera &self, Request *req) {
> py::object py_req = py::cast(req);
> @@ -172,10 +192,11 @@ PYBIND11_MODULE(_libcamera, m)
> py_req.inc_ref();
>
> int ret = self.queueRequest(req);
> - if (ret)
> + if (ret) {
> py_req.dec_ref();
> -
> - return ret;
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to queue request");
> + }
> })
>
> .def_property_readonly("streams", [](Camera &self) {
> @@ -253,7 +274,13 @@ PYBIND11_MODULE(_libcamera, m)
>
> pyFrameBufferAllocator
> .def(py::init<std::shared_ptr<Camera>>(), py::keep_alive<1, 2>())
> - .def("allocate", &FrameBufferAllocator::allocate)
> + .def("allocate", [](FrameBufferAllocator &self, Stream *stream) {
> + int ret = self.allocate(stream);
> + if (ret < 0)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to allocate buffers");
> + return ret;
> + })
> .def_property_readonly("allocated", &FrameBufferAllocator::allocated)
> /* Create a list of FrameBuffers, where each FrameBuffer has a keep-alive to FrameBufferAllocator */
> .def("buffers", [](FrameBufferAllocator &self, Stream *stream) {
> @@ -330,7 +357,10 @@ PYBIND11_MODULE(_libcamera, m)
> pyRequest
> /* \todo Fence is not supported, so we cannot expose addBuffer() directly */
> .def("add_buffer", [](Request &self, const Stream *stream, FrameBuffer *buffer) {
> - return self.addBuffer(stream, buffer);
> + int ret = self.addBuffer(stream, buffer);
> + if (ret)
> + throw std::system_error(-ret, std::generic_category(),
> + "Failed to add buffer");
> }, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */
> .def_property_readonly("status", &Request::status)
> .def_property_readonly("buffers", &Request::buffers)
> diff --git a/test/py/unittests.py b/test/py/unittests.py
> index 6dea80fc..794e46be 100755
> --- a/test/py/unittests.py
> +++ b/test/py/unittests.py
> @@ -42,31 +42,26 @@ class SimpleTestMethods(BaseTestCase):
> cam = cm.get('platform/vimc.0 Sensor B')
> self.assertIsNotNone(cam)
>
> - ret = cam.acquire()
> - self.assertZero(ret)
> + cam.acquire()
>
> - ret = cam.release()
> - self.assertZero(ret)
> + cam.release()
>
> def test_double_acquire(self):
> cm = libcam.CameraManager.singleton()
> cam = cm.get('platform/vimc.0 Sensor B')
> self.assertIsNotNone(cam)
>
> - ret = cam.acquire()
> - self.assertZero(ret)
> + cam.acquire()
>
> libcam.log_set_level('Camera', 'FATAL')
> - ret = cam.acquire()
> - self.assertEqual(ret, -errno.EBUSY)
> + with self.assertRaises(RuntimeError):
> + cam.acquire()
> libcam.log_set_level('Camera', 'ERROR')
>
> - ret = cam.release()
> - self.assertZero(ret)
> + cam.release()
>
> - ret = cam.release()
> - # I expected EBUSY, but looks like double release works fine
> - self.assertZero(ret)
> + # I expected exception here, but looks like double release works fine
> + cam.release()
>
>
> class CameraTesterBase(BaseTestCase):
> @@ -80,11 +75,7 @@ class CameraTesterBase(BaseTestCase):
> self.cm = None
> self.skipTest('No vimc found')
>
> - ret = self.cam.acquire()
> - if ret != 0:
> - self.cam = None
> - self.cm = None
> - raise Exception('Failed to acquire camera')
> + self.cam.acquire()
>
> self.wr_cam = weakref.ref(self.cam)
> self.wr_cm = weakref.ref(self.cm)
> @@ -93,9 +84,7 @@ class CameraTesterBase(BaseTestCase):
> # If a test fails, the camera may be in running state. So always stop.
> self.cam.stop()
>
> - ret = self.cam.release()
> - if ret != 0:
> - raise Exception('Failed to release camera')
> + self.cam.release()
>
> self.cam = None
> self.cm = None
> @@ -115,8 +104,7 @@ class AllocatorTestMethods(CameraTesterBase):
> streamconfig = camconfig.at(0)
> wr_streamconfig = weakref.ref(streamconfig)
>
> - ret = cam.configure(camconfig)
> - self.assertZero(ret)
> + cam.configure(camconfig)
>
> stream = streamconfig.stream
> wr_stream = weakref.ref(stream)
> @@ -129,8 +117,8 @@ class AllocatorTestMethods(CameraTesterBase):
> self.assertIsNotNone(wr_streamconfig())
>
> allocator = libcam.FrameBufferAllocator(cam)
> - ret = allocator.allocate(stream)
> - self.assertTrue(ret > 0)
> + num_bufs = allocator.allocate(stream)
> + self.assertTrue(num_bufs > 0)
> wr_allocator = weakref.ref(allocator)
>
> buffers = allocator.buffers(stream)
> @@ -173,14 +161,13 @@ class SimpleCaptureMethods(CameraTesterBase):
> self.assertIsNotNone(fmts)
> fmts = None
>
> - ret = cam.configure(camconfig)
> - self.assertZero(ret)
> + cam.configure(camconfig)
>
> stream = streamconfig.stream
>
> allocator = libcam.FrameBufferAllocator(cam)
> - ret = allocator.allocate(stream)
> - self.assertTrue(ret > 0)
> + num_bufs = allocator.allocate(stream)
> + self.assertTrue(num_bufs > 0)
>
> num_bufs = len(allocator.buffers(stream))
>
> @@ -190,19 +177,16 @@ class SimpleCaptureMethods(CameraTesterBase):
> self.assertIsNotNone(req)
>
> buffer = allocator.buffers(stream)[i]
> - ret = req.add_buffer(stream, buffer)
> - self.assertZero(ret)
> + req.add_buffer(stream, buffer)
>
> reqs.append(req)
>
> buffer = None
>
> - ret = cam.start()
> - self.assertZero(ret)
> + cam.start()
>
> for req in reqs:
> - ret = cam.queue_request(req)
> - self.assertZero(ret)
> + cam.queue_request(req)
>
> reqs = None
> gc.collect()
> @@ -230,8 +214,7 @@ class SimpleCaptureMethods(CameraTesterBase):
> reqs = None
> gc.collect()
>
> - ret = cam.stop()
> - self.assertZero(ret)
> + cam.stop()
>
> def test_select(self):
> cm = self.cm
> @@ -245,14 +228,13 @@ class SimpleCaptureMethods(CameraTesterBase):
> self.assertIsNotNone(fmts)
> fmts = None
>
> - ret = cam.configure(camconfig)
> - self.assertZero(ret)
> + cam.configure(camconfig)
>
> stream = streamconfig.stream
>
> allocator = libcam.FrameBufferAllocator(cam)
> - ret = allocator.allocate(stream)
> - self.assertTrue(ret > 0)
> + num_bufs = allocator.allocate(stream)
> + self.assertTrue(num_bufs > 0)
>
> num_bufs = len(allocator.buffers(stream))
>
> @@ -262,19 +244,16 @@ class SimpleCaptureMethods(CameraTesterBase):
> self.assertIsNotNone(req)
>
> buffer = allocator.buffers(stream)[i]
> - ret = req.add_buffer(stream, buffer)
> - self.assertZero(ret)
> + req.add_buffer(stream, buffer)
>
> reqs.append(req)
>
> buffer = None
>
> - ret = cam.start()
> - self.assertZero(ret)
> + cam.start()
>
> for req in reqs:
> - ret = cam.queue_request(req)
> - self.assertZero(ret)
> + cam.queue_request(req)
>
> reqs = None
> gc.collect()
> @@ -303,8 +282,7 @@ class SimpleCaptureMethods(CameraTesterBase):
> reqs = None
> gc.collect()
>
> - ret = cam.stop()
> - self.assertZero(ret)
> + cam.stop()
>
>
> # Recursively expand slist's objects into olist, using seen to track already
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list