[libcamera-devel,v2,13/13] py: use geometry classes
diff mbox series

Message ID 20220517143325.71784-14-tomi.valkeinen@ideasonboard.com
State Superseded
Headers show
Series
  • Misc Python bindings patches
Related show

Commit Message

Tomi Valkeinen May 17, 2022, 2:33 p.m. UTC 
Now that we have proper geometry classes in the Python bindings, change
the existing bindings and the .py files accordingly.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/py/cam/cam.py           |  7 +++++--
 src/py/cam/cam_kms.py       |  3 ++-
 src/py/cam/cam_qt.py        |  7 ++++---
 src/py/cam/cam_qtgl.py      |  3 ++-
 src/py/libcamera/pymain.cpp | 41 ++++++++-----------------------------
 5 files changed, 22 insertions(+), 39 deletions(-)

Comments

Kieran Bingham May 17, 2022, 5:20 p.m. UTC | #1 
Quoting Tomi Valkeinen (2022-05-17 15:33:25)
> Now that we have proper geometry classes in the Python bindings, change
> the existing bindings and the .py files accordingly.
> 
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  src/py/cam/cam.py           |  7 +++++--
>  src/py/cam/cam_kms.py       |  3 ++-
>  src/py/cam/cam_qt.py        |  7 ++++---
>  src/py/cam/cam_qtgl.py      |  3 ++-
>  src/py/libcamera/pymain.cpp | 41 ++++++++-----------------------------
>  5 files changed, 22 insertions(+), 39 deletions(-)
> 
> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
> index 001fb9de..2f0690b5 100755
> --- a/src/py/cam/cam.py
> +++ b/src/py/cam/cam.py
> @@ -160,8 +160,11 @@ def configure(ctx):
>      for idx, stream_opts in enumerate(streams):
>          stream_config = camconfig.at(idx)
>  
> -        if 'width' in stream_opts and 'height' in stream_opts:
> -            stream_config.size = (stream_opts['width'], stream_opts['height'])
> +        if 'width' in stream_opts:
> +            stream_config.size.width = stream_opts['width']
> +
> +        if 'height' in stream_opts:
> +            stream_config.size.height = stream_opts['height']

What happens here if only one of width, or height is set. Can it be
enforced to set both? Actually - I suspect the validation phase should
choose a (probably small, but more correct) configuration.

It looks like a good cleanup otherwise though.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


>  
>          if 'pixelformat' in stream_opts:
>              stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat'])
> diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py
> index 04381da1..74cd3b38 100644
> --- a/src/py/cam/cam_kms.py
> +++ b/src/py/cam/cam_kms.py
> @@ -126,7 +126,8 @@ class KMSRenderer:
>                  })
>  
>                  for fb in ctx['allocator'].buffers(stream):
> -                    w, h = cfg.size
> +                    w = cfg.size.width
> +                    h = cfg.size.height
>                      fds = []
>                      strides = []
>                      offsets = []
> diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py
> index 45a30aeb..91be2a08 100644
> --- a/src/py/cam/cam_qt.py
> +++ b/src/py/cam/cam_qt.py
> @@ -84,8 +84,8 @@ def demosaic(data, r0, g0, g1, b0):
>  
>  
>  def to_rgb(fmt, size, data):
> -    w = size[0]
> -    h = size[1]
> +    w = size.width
> +    h = size.height
>  
>      fmt = str(fmt)
>  
> @@ -292,7 +292,8 @@ class MainWindow(QtWidgets.QWidget):
>      def buf_to_qpixmap(self, stream, fb):
>          with fb.mmap() as mfb:
>              cfg = stream.configuration
> -            w, h = cfg.size
> +            w = cfg.size.width
> +            h = cfg.size.height
>              pitch = cfg.stride
>  
>              if str(cfg.pixel_format) == 'MJPEG':
> diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py
> index 261accb8..4bbcda6c 100644
> --- a/src/py/cam/cam_qtgl.py
> +++ b/src/py/cam/cam_qtgl.py
> @@ -268,7 +268,8 @@ class MainWindow(QtWidgets.QWidget):
>      def create_texture(self, stream, fb):
>          cfg = stream.configuration
>          fmt = cfg.pixel_format.fourcc
> -        w, h = cfg.size
> +        w = cfg.size.width
> +        h = cfg.size.height
>  
>          attribs = [
>              EGL_WIDTH, w,
> diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
> index 96333ebc..ef3f157a 100644
> --- a/src/py/libcamera/pymain.cpp
> +++ b/src/py/libcamera/pymain.cpp
> @@ -6,7 +6,6 @@
>   */
>  
>  /*
> - * \todo Add geometry classes (Point, Rectangle...)
>   * \todo Add bindings for the ControlInfo class
>   */
>  
> @@ -61,11 +60,11 @@ static py::object controlValueToPy(const ControlValue &cv)
>                 return py::cast(cv.get<std::string>());
>         case ControlTypeRectangle: {
>                 const Rectangle *v = reinterpret_cast<const Rectangle *>(cv.data().data());
> -               return py::make_tuple(v->x, v->y, v->width, v->height);
> +               return py::cast(v);
>         }
>         case ControlTypeSize: {
>                 const Size *v = reinterpret_cast<const Size *>(cv.data().data());
> -               return py::make_tuple(v->width, v->height);
> +               return py::cast(v);
>         }
>         case ControlTypeNone:
>         default:
> @@ -99,14 +98,10 @@ static ControlValue pyToControlValue(const py::object &ob, ControlType type)
>                 return controlValueMaybeArray<float>(ob);
>         case ControlTypeString:
>                 return ControlValue(ob.cast<std::string>());
> -       case ControlTypeRectangle: {
> -               auto array = ob.cast<std::array<int32_t, 4>>();
> -               return ControlValue(Rectangle(array[0], array[1], array[2], array[3]));
> -       }
> -       case ControlTypeSize: {
> -               auto array = ob.cast<std::array<int32_t, 2>>();
> -               return ControlValue(Size(array[0], array[1]));
> -       }
> +       case ControlTypeRectangle:
> +               return ControlValue(ob.cast<Rectangle>());
> +       case ControlTypeSize:
> +               return ControlValue(ob.cast<Size>());
>         case ControlTypeNone:
>         default:
>                 throw std::runtime_error("Control type not implemented");
> @@ -397,15 +392,7 @@ PYBIND11_MODULE(_libcamera, m)
>                 .def("__str__", &StreamConfiguration::toString)
>                 .def_property_readonly("stream", &StreamConfiguration::stream,
>                                        py::return_value_policy::reference_internal)
> -               .def_property(
> -                       "size",
> -                       [](StreamConfiguration &self) {
> -                               return std::make_tuple(self.size.width, self.size.height);
> -                       },
> -                       [](StreamConfiguration &self, std::tuple<uint32_t, uint32_t> size) {
> -                               self.size.width = std::get<0>(size);
> -                               self.size.height = std::get<1>(size);
> -                       })
> +               .def_readwrite("size", &StreamConfiguration::size)
>                 .def_readwrite("pixel_format", &StreamConfiguration::pixelFormat)
>                 .def_readwrite("stride", &StreamConfiguration::stride)
>                 .def_readwrite("frame_size", &StreamConfiguration::frameSize)
> @@ -416,18 +403,8 @@ PYBIND11_MODULE(_libcamera, m)
>  
>         pyStreamFormats
>                 .def_property_readonly("pixel_formats", &StreamFormats::pixelformats)
> -               .def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) {
> -                       std::vector<std::tuple<uint32_t, uint32_t>> fmts;
> -                       for (const auto &s : self.sizes(pixelFormat))
> -                               fmts.push_back(std::make_tuple(s.width, s.height));
> -                       return fmts;
> -               })
> -               .def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) {
> -                       const auto &range = self.range(pixelFormat);
> -                       return make_tuple(std::make_tuple(range.hStep, range.vStep),
> -                                         std::make_tuple(range.min.width, range.min.height),
> -                                         std::make_tuple(range.max.width, range.max.height));
> -               });
> +               .def("sizes", &StreamFormats::sizes)
> +               .def("range", &StreamFormats::range);
>  
>         pyFrameBufferAllocator
>                 .def(py::init<std::shared_ptr<Camera>>(), py::keep_alive<1, 2>())
> -- 
> 2.34.1
>
Tomi Valkeinen May 18, 2022, 6:57 a.m. UTC | #2 
On 17/05/2022 20:20, Kieran Bingham wrote:
> Quoting Tomi Valkeinen (2022-05-17 15:33:25)
>> Now that we have proper geometry classes in the Python bindings, change
>> the existing bindings and the .py files accordingly.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   src/py/cam/cam.py           |  7 +++++--
>>   src/py/cam/cam_kms.py       |  3 ++-
>>   src/py/cam/cam_qt.py        |  7 ++++---
>>   src/py/cam/cam_qtgl.py      |  3 ++-
>>   src/py/libcamera/pymain.cpp | 41 ++++++++-----------------------------
>>   5 files changed, 22 insertions(+), 39 deletions(-)
>>
>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
>> index 001fb9de..2f0690b5 100755
>> --- a/src/py/cam/cam.py
>> +++ b/src/py/cam/cam.py
>> @@ -160,8 +160,11 @@ def configure(ctx):
>>       for idx, stream_opts in enumerate(streams):
>>           stream_config = camconfig.at(idx)
>>   
>> -        if 'width' in stream_opts and 'height' in stream_opts:
>> -            stream_config.size = (stream_opts['width'], stream_opts['height'])
>> +        if 'width' in stream_opts:
>> +            stream_config.size.width = stream_opts['width']
>> +
>> +        if 'height' in stream_opts:
>> +            stream_config.size.height = stream_opts['height']
> 
> What happens here if only one of width, or height is set. Can it be
> enforced to set both? Actually - I suspect the validation phase should
> choose a (probably small, but more correct) configuration.

Yes, I think it picks up some default value for the other parameter. 
Maybe whatever is currently configured.

  Tomi

Patch
diff mbox series 

diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
index 001fb9de..2f0690b5 100755
--- a/src/py/cam/cam.py
+++ b/src/py/cam/cam.py
@@ -160,8 +160,11 @@  def configure(ctx):
     for idx, stream_opts in enumerate(streams):
         stream_config = camconfig.at(idx)
 
-        if 'width' in stream_opts and 'height' in stream_opts:
-            stream_config.size = (stream_opts['width'], stream_opts['height'])
+        if 'width' in stream_opts:
+            stream_config.size.width = stream_opts['width']
+
+        if 'height' in stream_opts:
+            stream_config.size.height = stream_opts['height']
 
         if 'pixelformat' in stream_opts:
             stream_config.pixel_format = libcam.PixelFormat(stream_opts['pixelformat'])
diff --git a/src/py/cam/cam_kms.py b/src/py/cam/cam_kms.py
index 04381da1..74cd3b38 100644
--- a/src/py/cam/cam_kms.py
+++ b/src/py/cam/cam_kms.py
@@ -126,7 +126,8 @@  class KMSRenderer:
                 })
 
                 for fb in ctx['allocator'].buffers(stream):
-                    w, h = cfg.size
+                    w = cfg.size.width
+                    h = cfg.size.height
                     fds = []
                     strides = []
                     offsets = []
diff --git a/src/py/cam/cam_qt.py b/src/py/cam/cam_qt.py
index 45a30aeb..91be2a08 100644
--- a/src/py/cam/cam_qt.py
+++ b/src/py/cam/cam_qt.py
@@ -84,8 +84,8 @@  def demosaic(data, r0, g0, g1, b0):
 
 
 def to_rgb(fmt, size, data):
-    w = size[0]
-    h = size[1]
+    w = size.width
+    h = size.height
 
     fmt = str(fmt)
 
@@ -292,7 +292,8 @@  class MainWindow(QtWidgets.QWidget):
     def buf_to_qpixmap(self, stream, fb):
         with fb.mmap() as mfb:
             cfg = stream.configuration
-            w, h = cfg.size
+            w = cfg.size.width
+            h = cfg.size.height
             pitch = cfg.stride
 
             if str(cfg.pixel_format) == 'MJPEG':
diff --git a/src/py/cam/cam_qtgl.py b/src/py/cam/cam_qtgl.py
index 261accb8..4bbcda6c 100644
--- a/src/py/cam/cam_qtgl.py
+++ b/src/py/cam/cam_qtgl.py
@@ -268,7 +268,8 @@  class MainWindow(QtWidgets.QWidget):
     def create_texture(self, stream, fb):
         cfg = stream.configuration
         fmt = cfg.pixel_format.fourcc
-        w, h = cfg.size
+        w = cfg.size.width
+        h = cfg.size.height
 
         attribs = [
             EGL_WIDTH, w,
diff --git a/src/py/libcamera/pymain.cpp b/src/py/libcamera/pymain.cpp
index 96333ebc..ef3f157a 100644
--- a/src/py/libcamera/pymain.cpp
+++ b/src/py/libcamera/pymain.cpp
@@ -6,7 +6,6 @@ 
  */
 
 /*
- * \todo Add geometry classes (Point, Rectangle...)
  * \todo Add bindings for the ControlInfo class
  */
 
@@ -61,11 +60,11 @@  static py::object controlValueToPy(const ControlValue &cv)
 		return py::cast(cv.get<std::string>());
 	case ControlTypeRectangle: {
 		const Rectangle *v = reinterpret_cast<const Rectangle *>(cv.data().data());
-		return py::make_tuple(v->x, v->y, v->width, v->height);
+		return py::cast(v);
 	}
 	case ControlTypeSize: {
 		const Size *v = reinterpret_cast<const Size *>(cv.data().data());
-		return py::make_tuple(v->width, v->height);
+		return py::cast(v);
 	}
 	case ControlTypeNone:
 	default:
@@ -99,14 +98,10 @@  static ControlValue pyToControlValue(const py::object &ob, ControlType type)
 		return controlValueMaybeArray<float>(ob);
 	case ControlTypeString:
 		return ControlValue(ob.cast<std::string>());
-	case ControlTypeRectangle: {
-		auto array = ob.cast<std::array<int32_t, 4>>();
-		return ControlValue(Rectangle(array[0], array[1], array[2], array[3]));
-	}
-	case ControlTypeSize: {
-		auto array = ob.cast<std::array<int32_t, 2>>();
-		return ControlValue(Size(array[0], array[1]));
-	}
+	case ControlTypeRectangle:
+		return ControlValue(ob.cast<Rectangle>());
+	case ControlTypeSize:
+		return ControlValue(ob.cast<Size>());
 	case ControlTypeNone:
 	default:
 		throw std::runtime_error("Control type not implemented");
@@ -397,15 +392,7 @@  PYBIND11_MODULE(_libcamera, m)
 		.def("__str__", &StreamConfiguration::toString)
 		.def_property_readonly("stream", &StreamConfiguration::stream,
 				       py::return_value_policy::reference_internal)
-		.def_property(
-			"size",
-			[](StreamConfiguration &self) {
-				return std::make_tuple(self.size.width, self.size.height);
-			},
-			[](StreamConfiguration &self, std::tuple<uint32_t, uint32_t> size) {
-				self.size.width = std::get<0>(size);
-				self.size.height = std::get<1>(size);
-			})
+		.def_readwrite("size", &StreamConfiguration::size)
 		.def_readwrite("pixel_format", &StreamConfiguration::pixelFormat)
 		.def_readwrite("stride", &StreamConfiguration::stride)
 		.def_readwrite("frame_size", &StreamConfiguration::frameSize)
@@ -416,18 +403,8 @@  PYBIND11_MODULE(_libcamera, m)
 
 	pyStreamFormats
 		.def_property_readonly("pixel_formats", &StreamFormats::pixelformats)
-		.def("sizes", [](StreamFormats &self, const PixelFormat &pixelFormat) {
-			std::vector<std::tuple<uint32_t, uint32_t>> fmts;
-			for (const auto &s : self.sizes(pixelFormat))
-				fmts.push_back(std::make_tuple(s.width, s.height));
-			return fmts;
-		})
-		.def("range", [](StreamFormats &self, const PixelFormat &pixelFormat) {
-			const auto &range = self.range(pixelFormat);
-			return make_tuple(std::make_tuple(range.hStep, range.vStep),
-					  std::make_tuple(range.min.width, range.min.height),
-					  std::make_tuple(range.max.width, range.max.height));
-		});
+		.def("sizes", &StreamFormats::sizes)
+		.def("range", &StreamFormats::range);
 
 	pyFrameBufferAllocator
 		.def(py::init<std::shared_ptr<Camera>>(), py::keep_alive<1, 2>())