[libcamera-devel] [PATCH v5 1/3] Add Python bindings

[Tomi Valkeinen]

On 13/04/2022 12:53, Jacopo Mondi wrote:
> Hi
> 
> On Wed, Apr 13, 2022 at 12:10:45PM +0300, Tomi Valkeinen wrote:
>> On 13/04/2022 10:57, Kieran Bingham wrote:
>>> Quoting Jacopo Mondi (2022-04-12 18:49:53)
>>>> Hi Tomi,
>>>>      I've been using the bindings in the last days, they're nice to
>>>> work! Great job!
>>>>
>>>> Once the question about a Request belonging to a Camera is
>>>> clarified I think we should merge these soon, even if incomplete, to
>>>> build on top.
>>>>
>>>> My understanding of python is very limited so I have just a few minor
>>>> comments and one larger question about controls.
>>>>
>>>> On Mon, Mar 14, 2022 at 05:46:31PM +0200, Tomi Valkeinen via libcamera-devel wrote:
>>>>> Add libcamera Python bindings. pybind11 is used to generate the C++ <->
>>>>> Python layer.
>>>>>
>>>>> We use pybind11 'smart_holder' version to avoid issues with private
>>>>> destructors and shared_ptr. There is also an alternative solution here:
>>>>>
>>>>> https://github.com/pybind/pybind11/pull/2067
>>>
>>> Why is the 'smart_holder' a branch of pybind11 ? I can't see a PR or
>>> anything such that indicates how or when it might get merged.
>>>
> 
> [snip]
> 
>>>>> +
>>>>> +     pyRequest
>>>>> +             /* Fence is not supported, so we cannot expose addBuffer() directly */
>>>>> +             .def("addBuffer", [](Request &self, const Stream *stream, FrameBuffer *buffer) {
>>>>> +                     return self.addBuffer(stream, buffer);
>>>>> +             }, py::keep_alive<1, 3>()) /* Request keeps Framebuffer alive */
>>>>> +             .def_property_readonly("status", &Request::status)
>>>>> +             .def_property_readonly("buffers", &Request::buffers)
>>>>> +             .def_property_readonly("cookie", &Request::cookie)
>>>>> +             .def_property_readonly("hasPendingBuffers", &Request::hasPendingBuffers)
>>>>> +             .def("set_control", [](Request &self, ControlId& id, py::object value) {
>>>>> +                     self.controls().set(id.id(), PyToControlValue(value, id.type()));
>>>>> +             })
>>>>
>>>> Minor comments apart, there's a thing which is missing:
>>>> support for setting controls that accept a Span<> of values.
>>>>
>>>> I tried several solutions and I got the following to compile
>>>>
>>>>           .def("set_control_array", [](Request &self, ControlId& id, py::tuple &values) {
>>>>                   py::bytearray bytes = py::bytearray(values);
>>>>                   py::buffer_info info(py::buffer(bytes).request());
>>>>
>>>>                   const int *data = reinterpret_cast<const int *>(info.ptr);
>>>>                   size_t length = static_cast<size_t>(info.size);
>>>>
>>>>                   self.controls().set(id.id(), Span<const int>{data, length});
>>>>            })
>>>>
>>>> (All types in casts<> should depend on id.type(), but that's for later).
>>>>
>>>> Unfortunately, while length is correct, the values I access with
>>>> 'data' seems invalid, which makes me think
>>>>
>>>>                   py::bytearray bytes = py::bytearray(values);
>>>>
>>>> Dosn't do what I think it does, or maybe Tuple in Python are simply
>>>> not backed by a contigous memory buffer , hence there's no way to wrap
>>>> their memory in a Span<>).
>>>>
>>>> Please note I also tried to instrument:
>>>>
>>>>           .def("set_control_array", [](Request &self, ControlId& id, vector<int> values)
>>>>
>>>> Relying on pybind11 type casting, and it seems to work, but I see two issues:
>>>>
>>>> 1) Converting from python to C++ types goes through a copy.
>>>> Performances reasons apart, the vector lifetime is limited to the
>>>> function scope. This isn't be a problem for now, as control
>>>> values are copied in the Request's control list, but that would
>>>> prevent passing controls values as pointers, if a control transports a
>>>> large chunk of data (ie gamma tables). Not sure we'll ever want this
>>>> (I don't think so as it won't play well with serialization between the
>>>> IPA and the pipeline handler) but you never know.
>>>
>>> I don't think we could ever let an application pass a pointer to an
>>> arbitrary location into libcamera for us to parse.
>>>
>>>> 2) my understanding is that python does not support methods signature
>>>> overloading, hence we would have
>>>>
>>>>           .def("set_control_array_int", [](Request &self, ControlId& id, vector<int> values)
>>>>           .def("set_control_array_float", [](Request &self, ControlId& id, vector<float> values)
>>>>           ....
>>>>
>>>> there are surely smart ways to handle this, but in my quick experiment
>>>> I haven't found one yet :)
>>>
>>> Given how dynamically typed python is I would expect there would be some
>>> form of wrapping here that could be done?
>>
>> But the C++ templates are not dynamic, so we have to have all the possible
>> Span<T> variations here somehow so that we get the code for them.
>>
>>>> David: Is the issue with Span<> controls addressed by picamera2 ?
> 
> I have tested with picamera2, and this commit allows the usage of
> controls which accepts multiple values
> 
> https://github.com/raspberrypi/libcamera/commit/43cbfd5289aedafa40758312e944db13a5afdf14
> 
> and it works here.
> 
> It goes through a forceful ob.cast<vector>() which goes through a copy
> https://pybind11.readthedocs.io/en/stable/advanced/cast/stl.html
> 
> I had looked a bit into using PYBIND11_MAKE_OPAQUE() to define a
> custom caster, but if the raw memory of a Python Tuple cannot be
> accessed as a contiguous block, it won't help much.
> 
> Is this something that can be brought in already in your next series
> Tomi ?

I really don't like having exceptions thrown when everything goes fine. 
Unfortunately I haven't had time to look at how to do this properly.

  Tomi