[libcamera-devel,v4,01/15] py: Use exceptions instead of returning error codes

18362 diff mbox series

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@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 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(-)

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@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@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