[libcamera-devel,v5,0/3] Python bindings
mbox series

Message ID 20220314154633.506026-1-tomi.valkeinen@ideasonboard.com
Headers show
Series
  • Python bindings
Related show

Message

Nicolas Dufresne via libcamera-devel March 14, 2022, 3:46 p.m. UTC
Hi,

This is v5 of the series. The changes to v4:

- Changed to pybind11 'smart_holders' branch which allows us to drop the
  HACK for Camera public destructor
- Changed Request.set_control() so that we can drop the HACK for
  exposing Camera from Request
- Moved Python enum defs to a separate file for clarity
- "Forward" declare Python classes to fix docstring issues.

The set_control change breaks the current users of that function.
Previously you could do this:

req.set_control("Brightness", 1)

Now you need to do:

cid = camera.find_control("Brightness")
req.set_control(cid, 1)

 Tomi

Tomi Valkeinen (3):
  Add Python bindings
  py: add unittests.py
  py: Add cam.py

 meson.build                  |   1 +
 meson_options.txt            |   5 +
 src/meson.build              |   1 +
 src/py/cam/cam.py            | 461 +++++++++++++++++++++++++++++++++++
 src/py/cam/cam_kms.py        | 183 ++++++++++++++
 src/py/cam/cam_null.py       |  46 ++++
 src/py/cam/cam_qt.py         | 355 +++++++++++++++++++++++++++
 src/py/cam/cam_qtgl.py       | 386 +++++++++++++++++++++++++++++
 src/py/cam/gl_helpers.py     |  67 +++++
 src/py/libcamera/__init__.py |  10 +
 src/py/libcamera/meson.build |  43 ++++
 src/py/libcamera/pyenums.cpp |  53 ++++
 src/py/libcamera/pymain.cpp  | 453 ++++++++++++++++++++++++++++++++++
 src/py/meson.build           |   1 +
 src/py/test/unittests.py     | 364 +++++++++++++++++++++++++++
 subprojects/.gitignore       |   3 +-
 subprojects/pybind11.wrap    |   6 +
 17 files changed, 2437 insertions(+), 1 deletion(-)
 create mode 100755 src/py/cam/cam.py
 create mode 100644 src/py/cam/cam_kms.py
 create mode 100644 src/py/cam/cam_null.py
 create mode 100644 src/py/cam/cam_qt.py
 create mode 100644 src/py/cam/cam_qtgl.py
 create mode 100644 src/py/cam/gl_helpers.py
 create mode 100644 src/py/libcamera/__init__.py
 create mode 100644 src/py/libcamera/meson.build
 create mode 100644 src/py/libcamera/pyenums.cpp
 create mode 100644 src/py/libcamera/pymain.cpp
 create mode 100644 src/py/meson.build
 create mode 100755 src/py/test/unittests.py
 create mode 100644 subprojects/pybind11.wrap

Comments

Nicolas Dufresne via libcamera-devel March 17, 2022, 9:44 a.m. UTC | #1
Hi everyone

Thanks for the update!

On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> Hi,
>
> This is v5 of the series. The changes to v4:
>
> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>   HACK for Camera public destructor
> - Changed Request.set_control() so that we can drop the HACK for
>   exposing Camera from Request
> - Moved Python enum defs to a separate file for clarity
> - "Forward" declare Python classes to fix docstring issues.
>
> The set_control change breaks the current users of that function.
> Previously you could do this:
>
> req.set_control("Brightness", 1)
>
> Now you need to do:
>
> cid = camera.find_control("Brightness")
> req.set_control(cid, 1)
>

I see the need for this, and I guess it's fine - obviously we'll hide
that under our Picamera2 API as we wouldn't want our users to have to
do that!

There is one further thing I wanted to raise in relation to the Python
bindings for controls, now that a few folks are trying out Picamera2.

I've had feedback that they don't like the fact that all the "enum"
control values (exposure modes, AWB modes, that kind of thing) are
simple integers up in the Python world. They complain that this makes
them quite unfriendly to use, and I'm inclined to agree! We could
always create Python-level definitions for them, and possibly add code
to convert stuff on the fly, but I wonder if Pybind11 can do anything
more automatic for us?

Thanks!
David

>  Tomi
>
> Tomi Valkeinen (3):
>   Add Python bindings
>   py: add unittests.py
>   py: Add cam.py
>
>  meson.build                  |   1 +
>  meson_options.txt            |   5 +
>  src/meson.build              |   1 +
>  src/py/cam/cam.py            | 461 +++++++++++++++++++++++++++++++++++
>  src/py/cam/cam_kms.py        | 183 ++++++++++++++
>  src/py/cam/cam_null.py       |  46 ++++
>  src/py/cam/cam_qt.py         | 355 +++++++++++++++++++++++++++
>  src/py/cam/cam_qtgl.py       | 386 +++++++++++++++++++++++++++++
>  src/py/cam/gl_helpers.py     |  67 +++++
>  src/py/libcamera/__init__.py |  10 +
>  src/py/libcamera/meson.build |  43 ++++
>  src/py/libcamera/pyenums.cpp |  53 ++++
>  src/py/libcamera/pymain.cpp  | 453 ++++++++++++++++++++++++++++++++++
>  src/py/meson.build           |   1 +
>  src/py/test/unittests.py     | 364 +++++++++++++++++++++++++++
>  subprojects/.gitignore       |   3 +-
>  subprojects/pybind11.wrap    |   6 +
>  17 files changed, 2437 insertions(+), 1 deletion(-)
>  create mode 100755 src/py/cam/cam.py
>  create mode 100644 src/py/cam/cam_kms.py
>  create mode 100644 src/py/cam/cam_null.py
>  create mode 100644 src/py/cam/cam_qt.py
>  create mode 100644 src/py/cam/cam_qtgl.py
>  create mode 100644 src/py/cam/gl_helpers.py
>  create mode 100644 src/py/libcamera/__init__.py
>  create mode 100644 src/py/libcamera/meson.build
>  create mode 100644 src/py/libcamera/pyenums.cpp
>  create mode 100644 src/py/libcamera/pymain.cpp
>  create mode 100644 src/py/meson.build
>  create mode 100755 src/py/test/unittests.py
>  create mode 100644 subprojects/pybind11.wrap
>
> --
> 2.25.1
>
Nicolas Dufresne via libcamera-devel March 17, 2022, 9:55 a.m. UTC | #2
On 17/03/2022 11:44, David Plowman wrote:
> Hi everyone
> 
> Thanks for the update!
> 
> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> Hi,
>>
>> This is v5 of the series. The changes to v4:
>>
>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>>    HACK for Camera public destructor
>> - Changed Request.set_control() so that we can drop the HACK for
>>    exposing Camera from Request
>> - Moved Python enum defs to a separate file for clarity
>> - "Forward" declare Python classes to fix docstring issues.
>>
>> The set_control change breaks the current users of that function.
>> Previously you could do this:
>>
>> req.set_control("Brightness", 1)
>>
>> Now you need to do:
>>
>> cid = camera.find_control("Brightness")
>> req.set_control(cid, 1)
>>
> 
> I see the need for this, and I guess it's fine - obviously we'll hide
> that under our Picamera2 API as we wouldn't want our users to have to
> do that!

Yes, it's not very nice. Then again, having to do a string search every 
time we call set_control is not good either, at least if we can avoid it 
(and we can, as above). But I agree that in a higher level API things 
should be easy. Of course, it would be nice to have both versions even 
in this low level API, but... if Request doesn't expose camera, I don't 
think we can do it in a simple way.

> There is one further thing I wanted to raise in relation to the Python
> bindings for controls, now that a few folks are trying out Picamera2.
> 
> I've had feedback that they don't like the fact that all the "enum"
> control values (exposure modes, AWB modes, that kind of thing) are
> simple integers up in the Python world. They complain that this makes
> them quite unfriendly to use, and I'm inclined to agree! We could
> always create Python-level definitions for them, and possibly add code
> to convert stuff on the fly, but I wonder if Pybind11 can do anything
> more automatic for us?

Can you elaborate? Which enums are these?

  Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 10:03 a.m. UTC | #3
On 17/03/2022 11:55, Tomi Valkeinen wrote:
> On 17/03/2022 11:44, David Plowman wrote:
>> Hi everyone
>>
>> Thanks for the update!
>>
>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>
>>> Hi,
>>>
>>> This is v5 of the series. The changes to v4:
>>>
>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>>>    HACK for Camera public destructor
>>> - Changed Request.set_control() so that we can drop the HACK for
>>>    exposing Camera from Request
>>> - Moved Python enum defs to a separate file for clarity
>>> - "Forward" declare Python classes to fix docstring issues.
>>>
>>> The set_control change breaks the current users of that function.
>>> Previously you could do this:
>>>
>>> req.set_control("Brightness", 1)
>>>
>>> Now you need to do:
>>>
>>> cid = camera.find_control("Brightness")
>>> req.set_control(cid, 1)
>>>
>>
>> I see the need for this, and I guess it's fine - obviously we'll hide
>> that under our Picamera2 API as we wouldn't want our users to have to
>> do that!

I think the core question is whether a Request is tied to a Camera. I 
thought it is, as it is created from the Camera. And if that's the case, 
I don't see why the Camera cannot be exposed from the Request.

Maybe Laurent can explain the reasons why not to expose it.

> Yes, it's not very nice. Then again, having to do a string search every 
> time we call set_control is not good either, at least if we can avoid it 
> (and we can, as above). But I agree that in a higher level API things 
> should be easy. Of course, it would be nice to have both versions even 
> in this low level API, but... if Request doesn't expose camera, I don't 
> think we can do it in a simple way.

Note that there are multiple ways to approach this. E.g.:

req.set_control(camera.controls["Brightness"], 1)

or:

req.set_control(camera.controls.Brightness, 1)

or even (although this could perhaps conflict too easily):

req.set_control(camera.Brightness, 1)

Or we could go the other way:

camera.set_control(req, "Brightness", 1)

or:

camera.controls.Brightness.set(req, 1)

  Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 10:06 a.m. UTC | #4
Hi Tomi

On Thu, 17 Mar 2022 at 10:03, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 17/03/2022 11:55, Tomi Valkeinen wrote:
> > On 17/03/2022 11:44, David Plowman wrote:
> >> Hi everyone
> >>
> >> Thanks for the update!
> >>
> >> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> >> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This is v5 of the series. The changes to v4:
> >>>
> >>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>>    HACK for Camera public destructor
> >>> - Changed Request.set_control() so that we can drop the HACK for
> >>>    exposing Camera from Request
> >>> - Moved Python enum defs to a separate file for clarity
> >>> - "Forward" declare Python classes to fix docstring issues.
> >>>
> >>> The set_control change breaks the current users of that function.
> >>> Previously you could do this:
> >>>
> >>> req.set_control("Brightness", 1)
> >>>
> >>> Now you need to do:
> >>>
> >>> cid = camera.find_control("Brightness")
> >>> req.set_control(cid, 1)
> >>>
> >>
> >> I see the need for this, and I guess it's fine - obviously we'll hide
> >> that under our Picamera2 API as we wouldn't want our users to have to
> >> do that!
>
> I think the core question is whether a Request is tied to a Camera. I
> thought it is, as it is created from the Camera. And if that's the case,
> I don't see why the Camera cannot be exposed from the Request.
>
> Maybe Laurent can explain the reasons why not to expose it.

I wouldn't want to worry too much about this! I'll just hide it under
the Picamera2 API and that's fine...

David

>
> > Yes, it's not very nice. Then again, having to do a string search every
> > time we call set_control is not good either, at least if we can avoid it
> > (and we can, as above). But I agree that in a higher level API things
> > should be easy. Of course, it would be nice to have both versions even
> > in this low level API, but... if Request doesn't expose camera, I don't
> > think we can do it in a simple way.
>
> Note that there are multiple ways to approach this. E.g.:
>
> req.set_control(camera.controls["Brightness"], 1)
>
> or:
>
> req.set_control(camera.controls.Brightness, 1)
>
> or even (although this could perhaps conflict too easily):
>
> req.set_control(camera.Brightness, 1)
>
> Or we could go the other way:
>
> camera.set_control(req, "Brightness", 1)
>
> or:
>
> camera.controls.Brightness.set(req, 1)
>
>   Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 10:12 a.m. UTC | #5
Hi again

Just to answer the about about the "enum" type controls:

On Thu, 17 Mar 2022 at 09:55, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 17/03/2022 11:44, David Plowman wrote:
> > Hi everyone
> >
> > Thanks for the update!
> >
> > On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> Hi,
> >>
> >> This is v5 of the series. The changes to v4:
> >>
> >> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>    HACK for Camera public destructor
> >> - Changed Request.set_control() so that we can drop the HACK for
> >>    exposing Camera from Request
> >> - Moved Python enum defs to a separate file for clarity
> >> - "Forward" declare Python classes to fix docstring issues.
> >>
> >> The set_control change breaks the current users of that function.
> >> Previously you could do this:
> >>
> >> req.set_control("Brightness", 1)
> >>
> >> Now you need to do:
> >>
> >> cid = camera.find_control("Brightness")
> >> req.set_control(cid, 1)
> >>
> >
> > I see the need for this, and I guess it's fine - obviously we'll hide
> > that under our Picamera2 API as we wouldn't want our users to have to
> > do that!
>
> Yes, it's not very nice. Then again, having to do a string search every
> time we call set_control is not good either, at least if we can avoid it
> (and we can, as above). But I agree that in a higher level API things
> should be easy. Of course, it would be nice to have both versions even
> in this low level API, but... if Request doesn't expose camera, I don't
> think we can do it in a simple way.
>
> > There is one further thing I wanted to raise in relation to the Python
> > bindings for controls, now that a few folks are trying out Picamera2.
> >
> > I've had feedback that they don't like the fact that all the "enum"
> > control values (exposure modes, AWB modes, that kind of thing) are
> > simple integers up in the Python world. They complain that this makes
> > them quite unfriendly to use, and I'm inclined to agree! We could
> > always create Python-level definitions for them, and possibly add code
> > to convert stuff on the fly, but I wonder if Pybind11 can do anything
> > more automatic for us?
>
> Can you elaborate? Which enums are these?

So users would prefer to write something like

picamera2.set_controls({"AwbMode": AwbMode.Indoor})

rather than

picamera2.set_controls({"AwbMode": 4})

The same applies to all the controls of this type, there including

AeMeteringMode
AeConstraintMode
AeExposureMode
AwbMode
NoiseReductionMode
there will at some point be an AfMode and probably other AF ones
and so on.

Basically it looks to be mostly anything with "Mode" on the end,
though I think we will encounter some others too. Also anything with
"State" on the end is probably similar, though in this case we're
talking about reported values, not ones you can set.

Does that make sense?

Thanks!
David

>
>   Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 10:41 a.m. UTC | #6
On 17/03/2022 12:12, David Plowman wrote:
> Hi again
> 
> Just to answer the about about the "enum" type controls:
> 
> On Thu, 17 Mar 2022 at 09:55, Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> On 17/03/2022 11:44, David Plowman wrote:
>>> Hi everyone
>>>
>>> Thanks for the update!
>>>
>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> This is v5 of the series. The changes to v4:
>>>>
>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>>>>     HACK for Camera public destructor
>>>> - Changed Request.set_control() so that we can drop the HACK for
>>>>     exposing Camera from Request
>>>> - Moved Python enum defs to a separate file for clarity
>>>> - "Forward" declare Python classes to fix docstring issues.
>>>>
>>>> The set_control change breaks the current users of that function.
>>>> Previously you could do this:
>>>>
>>>> req.set_control("Brightness", 1)
>>>>
>>>> Now you need to do:
>>>>
>>>> cid = camera.find_control("Brightness")
>>>> req.set_control(cid, 1)
>>>>
>>>
>>> I see the need for this, and I guess it's fine - obviously we'll hide
>>> that under our Picamera2 API as we wouldn't want our users to have to
>>> do that!
>>
>> Yes, it's not very nice. Then again, having to do a string search every
>> time we call set_control is not good either, at least if we can avoid it
>> (and we can, as above). But I agree that in a higher level API things
>> should be easy. Of course, it would be nice to have both versions even
>> in this low level API, but... if Request doesn't expose camera, I don't
>> think we can do it in a simple way.
>>
>>> There is one further thing I wanted to raise in relation to the Python
>>> bindings for controls, now that a few folks are trying out Picamera2.
>>>
>>> I've had feedback that they don't like the fact that all the "enum"
>>> control values (exposure modes, AWB modes, that kind of thing) are
>>> simple integers up in the Python world. They complain that this makes
>>> them quite unfriendly to use, and I'm inclined to agree! We could
>>> always create Python-level definitions for them, and possibly add code
>>> to convert stuff on the fly, but I wonder if Pybind11 can do anything
>>> more automatic for us?
>>
>> Can you elaborate? Which enums are these?
> 
> So users would prefer to write something like
> 
> picamera2.set_controls({"AwbMode": AwbMode.Indoor})
> 
> rather than
> 
> picamera2.set_controls({"AwbMode": 4})
> 
> The same applies to all the controls of this type, there including
> 
> AeMeteringMode
> AeConstraintMode
> AeExposureMode
> AwbMode
> NoiseReductionMode
> there will at some point be an AfMode and probably other AF ones
> and so on.
> 
> Basically it looks to be mostly anything with "Mode" on the end,
> though I think we will encounter some others too. Also anything with
> "State" on the end is probably similar, though in this case we're
> talking about reported values, not ones you can set.

Looks like these are defined in src/libcamera/control_ids.yaml, so we 
could have a script that generates pybind11 code to create the enums. Or 
we could just do it by hand.

Is that your question, how and where to define the python enums? Or do 
you think there's some issue using those?

  Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 11:01 a.m. UTC | #7
Hi again

On Thu, 17 Mar 2022 at 10:41, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 17/03/2022 12:12, David Plowman wrote:
> > Hi again
> >
> > Just to answer the about about the "enum" type controls:
> >
> > On Thu, 17 Mar 2022 at 09:55, Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> On 17/03/2022 11:44, David Plowman wrote:
> >>> Hi everyone
> >>>
> >>> Thanks for the update!
> >>>
> >>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> >>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> This is v5 of the series. The changes to v4:
> >>>>
> >>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>>>     HACK for Camera public destructor
> >>>> - Changed Request.set_control() so that we can drop the HACK for
> >>>>     exposing Camera from Request
> >>>> - Moved Python enum defs to a separate file for clarity
> >>>> - "Forward" declare Python classes to fix docstring issues.
> >>>>
> >>>> The set_control change breaks the current users of that function.
> >>>> Previously you could do this:
> >>>>
> >>>> req.set_control("Brightness", 1)
> >>>>
> >>>> Now you need to do:
> >>>>
> >>>> cid = camera.find_control("Brightness")
> >>>> req.set_control(cid, 1)
> >>>>
> >>>
> >>> I see the need for this, and I guess it's fine - obviously we'll hide
> >>> that under our Picamera2 API as we wouldn't want our users to have to
> >>> do that!
> >>
> >> Yes, it's not very nice. Then again, having to do a string search every
> >> time we call set_control is not good either, at least if we can avoid it
> >> (and we can, as above). But I agree that in a higher level API things
> >> should be easy. Of course, it would be nice to have both versions even
> >> in this low level API, but... if Request doesn't expose camera, I don't
> >> think we can do it in a simple way.
> >>
> >>> There is one further thing I wanted to raise in relation to the Python
> >>> bindings for controls, now that a few folks are trying out Picamera2.
> >>>
> >>> I've had feedback that they don't like the fact that all the "enum"
> >>> control values (exposure modes, AWB modes, that kind of thing) are
> >>> simple integers up in the Python world. They complain that this makes
> >>> them quite unfriendly to use, and I'm inclined to agree! We could
> >>> always create Python-level definitions for them, and possibly add code
> >>> to convert stuff on the fly, but I wonder if Pybind11 can do anything
> >>> more automatic for us?
> >>
> >> Can you elaborate? Which enums are these?
> >
> > So users would prefer to write something like
> >
> > picamera2.set_controls({"AwbMode": AwbMode.Indoor})
> >
> > rather than
> >
> > picamera2.set_controls({"AwbMode": 4})
> >
> > The same applies to all the controls of this type, there including
> >
> > AeMeteringMode
> > AeConstraintMode
> > AeExposureMode
> > AwbMode
> > NoiseReductionMode
> > there will at some point be an AfMode and probably other AF ones
> > and so on.
> >
> > Basically it looks to be mostly anything with "Mode" on the end,
> > though I think we will encounter some others too. Also anything with
> > "State" on the end is probably similar, though in this case we're
> > talking about reported values, not ones you can set.
>
> Looks like these are defined in src/libcamera/control_ids.yaml, so we
> could have a script that generates pybind11 code to create the enums. Or
> we could just do it by hand.

Defining them by hand is fine with me, at least for the time being. I
just wonder whether it would get annoying in the longer run, so some
kind of script in the build system might be helpful.

>
> Is that your question, how and where to define the python enums? Or do
> you think there's some issue using those?

I don't think there's any problem, I guess I'm just flagging that
there's something we probably need to do. I could define them all in
Picamera2, but it seems like everyone using these Python bindings
would appreciate them, so they probably belong in libcamera somewhere.

Thanks!
David

>
>   Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 11:36 a.m. UTC | #8
On 17/03/2022 13:01, David Plowman wrote:
> Hi again
> 
> On Thu, 17 Mar 2022 at 10:41, Tomi Valkeinen
> <tomi.valkeinen@ideasonboard.com> wrote:
>>
>> On 17/03/2022 12:12, David Plowman wrote:
>>> Hi again
>>>
>>> Just to answer the about about the "enum" type controls:
>>>
>>> On Thu, 17 Mar 2022 at 09:55, Tomi Valkeinen
>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>
>>>> On 17/03/2022 11:44, David Plowman wrote:
>>>>> Hi everyone
>>>>>
>>>>> Thanks for the update!
>>>>>
>>>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
>>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> This is v5 of the series. The changes to v4:
>>>>>>
>>>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>>>>>>      HACK for Camera public destructor
>>>>>> - Changed Request.set_control() so that we can drop the HACK for
>>>>>>      exposing Camera from Request
>>>>>> - Moved Python enum defs to a separate file for clarity
>>>>>> - "Forward" declare Python classes to fix docstring issues.
>>>>>>
>>>>>> The set_control change breaks the current users of that function.
>>>>>> Previously you could do this:
>>>>>>
>>>>>> req.set_control("Brightness", 1)
>>>>>>
>>>>>> Now you need to do:
>>>>>>
>>>>>> cid = camera.find_control("Brightness")
>>>>>> req.set_control(cid, 1)
>>>>>>
>>>>>
>>>>> I see the need for this, and I guess it's fine - obviously we'll hide
>>>>> that under our Picamera2 API as we wouldn't want our users to have to
>>>>> do that!
>>>>
>>>> Yes, it's not very nice. Then again, having to do a string search every
>>>> time we call set_control is not good either, at least if we can avoid it
>>>> (and we can, as above). But I agree that in a higher level API things
>>>> should be easy. Of course, it would be nice to have both versions even
>>>> in this low level API, but... if Request doesn't expose camera, I don't
>>>> think we can do it in a simple way.
>>>>
>>>>> There is one further thing I wanted to raise in relation to the Python
>>>>> bindings for controls, now that a few folks are trying out Picamera2.
>>>>>
>>>>> I've had feedback that they don't like the fact that all the "enum"
>>>>> control values (exposure modes, AWB modes, that kind of thing) are
>>>>> simple integers up in the Python world. They complain that this makes
>>>>> them quite unfriendly to use, and I'm inclined to agree! We could
>>>>> always create Python-level definitions for them, and possibly add code
>>>>> to convert stuff on the fly, but I wonder if Pybind11 can do anything
>>>>> more automatic for us?
>>>>
>>>> Can you elaborate? Which enums are these?
>>>
>>> So users would prefer to write something like
>>>
>>> picamera2.set_controls({"AwbMode": AwbMode.Indoor})
>>>
>>> rather than
>>>
>>> picamera2.set_controls({"AwbMode": 4})
>>>
>>> The same applies to all the controls of this type, there including
>>>
>>> AeMeteringMode
>>> AeConstraintMode
>>> AeExposureMode
>>> AwbMode
>>> NoiseReductionMode
>>> there will at some point be an AfMode and probably other AF ones
>>> and so on.
>>>
>>> Basically it looks to be mostly anything with "Mode" on the end,
>>> though I think we will encounter some others too. Also anything with
>>> "State" on the end is probably similar, though in this case we're
>>> talking about reported values, not ones you can set.
>>
>> Looks like these are defined in src/libcamera/control_ids.yaml, so we
>> could have a script that generates pybind11 code to create the enums. Or
>> we could just do it by hand.
> 
> Defining them by hand is fine with me, at least for the time being. I
> just wonder whether it would get annoying in the longer run, so some
> kind of script in the build system might be helpful.
> 
>>
>> Is that your question, how and where to define the python enums? Or do
>> you think there's some issue using those?
> 
> I don't think there's any problem, I guess I'm just flagging that
> there's something we probably need to do. I could define them all in
> Picamera2, but it seems like everyone using these Python bindings
> would appreciate them, so they probably belong in libcamera somewhere.

Ok. I hacked a quick script. Here's a snippet to add to the py .c file if you want to try them:


	py::enum_<libcamera::controls::AeMeteringModeEnum>(m, "AeMeteringMode")
		.value("CentreWeighted", libcamera::controls::MeteringCentreWeighted)
		.value("Spot", libcamera::controls::MeteringSpot)
		.value("Matrix", libcamera::controls::MeteringMatrix)
		.value("Custom", libcamera::controls::MeteringCustom)
	;
	py::enum_<libcamera::controls::AeConstraintModeEnum>(m, "AeConstraintMode")
		.value("Normal", libcamera::controls::ConstraintNormal)
		.value("Highlight", libcamera::controls::ConstraintHighlight)
		.value("Shadows", libcamera::controls::ConstraintShadows)
		.value("Custom", libcamera::controls::ConstraintCustom)
	;
	py::enum_<libcamera::controls::AeExposureModeEnum>(m, "AeExposureMode")
		.value("Normal", libcamera::controls::ExposureNormal)
		.value("Short", libcamera::controls::ExposureShort)
		.value("Long", libcamera::controls::ExposureLong)
		.value("Custom", libcamera::controls::ExposureCustom)
	;
	py::enum_<libcamera::controls::AwbModeEnum>(m, "AwbMode")
		.value("Auto", libcamera::controls::AwbAuto)
		.value("Incandescent", libcamera::controls::AwbIncandescent)
		.value("Tungsten", libcamera::controls::AwbTungsten)
		.value("Fluorescent", libcamera::controls::AwbFluorescent)
		.value("Indoor", libcamera::controls::AwbIndoor)
		.value("Daylight", libcamera::controls::AwbDaylight)
		.value("Cloudy", libcamera::controls::AwbCloudy)
		.value("Custom", libcamera::controls::AwbCustom)
	;
	py::enum_<libcamera::controls::draft::AePrecaptureTriggerEnum>(m, "AePrecaptureTrigger")
		.value("Idle", libcamera::controls::draft::AePrecaptureTriggerIdle)
		.value("Start", libcamera::controls::draft::AePrecaptureTriggerStart)
		.value("Cancel", libcamera::controls::draft::AePrecaptureTriggerCancel)
	;
	py::enum_<libcamera::controls::draft::AfTriggerEnum>(m, "AfTrigger")
		.value("Idle", libcamera::controls::draft::AfTriggerIdle)
		.value("Start", libcamera::controls::draft::AfTriggerStart)
		.value("Cancel", libcamera::controls::draft::AfTriggerCancel)
	;
	py::enum_<libcamera::controls::draft::NoiseReductionModeEnum>(m, "NoiseReductionMode")
		.value("Off", libcamera::controls::draft::NoiseReductionModeOff)
		.value("Fast", libcamera::controls::draft::NoiseReductionModeFast)
		.value("HighQuality", libcamera::controls::draft::NoiseReductionModeHighQuality)
		.value("Minimal", libcamera::controls::draft::NoiseReductionModeMinimal)
		.value("ZSL", libcamera::controls::draft::NoiseReductionModeZSL)
	;
	py::enum_<libcamera::controls::draft::ColorCorrectionAberrationModeEnum>(m, "ColorCorrectionAberrationMode")
		.value("Off", libcamera::controls::draft::ColorCorrectionAberrationOff)
		.value("Fast", libcamera::controls::draft::ColorCorrectionAberrationFast)
		.value("HighQuality", libcamera::controls::draft::ColorCorrectionAberrationHighQuality)
	;
	py::enum_<libcamera::controls::draft::AeStateEnum>(m, "AeState")
		.value("Inactive", libcamera::controls::draft::AeStateInactive)
		.value("Searching", libcamera::controls::draft::AeStateSearching)
		.value("Converged", libcamera::controls::draft::AeStateConverged)
		.value("Locked", libcamera::controls::draft::AeStateLocked)
		.value("FlashRequired", libcamera::controls::draft::AeStateFlashRequired)
		.value("Precapture", libcamera::controls::draft::AeStatePrecapture)
	;
	py::enum_<libcamera::controls::draft::AfStateEnum>(m, "AfState")
		.value("Inactive", libcamera::controls::draft::AfStateInactive)
		.value("PassiveScan", libcamera::controls::draft::AfStatePassiveScan)
		.value("PassiveFocused", libcamera::controls::draft::AfStatePassiveFocused)
		.value("ActiveScan", libcamera::controls::draft::AfStateActiveScan)
		.value("FocusedLock", libcamera::controls::draft::AfStateFocusedLock)
		.value("NotFocusedLock", libcamera::controls::draft::AfStateNotFocusedLock)
		.value("PassiveUnfocused", libcamera::controls::draft::AfStatePassiveUnfocused)
	;
	py::enum_<libcamera::controls::draft::AwbStateEnum>(m, "AwbState")
		.value("StateInactive", libcamera::controls::draft::AwbStateInactive)
		.value("StateSearching", libcamera::controls::draft::AwbStateSearching)
		.value("Converged", libcamera::controls::draft::AwbConverged)
		.value("Locked", libcamera::controls::draft::AwbLocked)
	;
	py::enum_<libcamera::controls::draft::LensShadingMapModeEnum>(m, "LensShadingMapMode")
		.value("Off", libcamera::controls::draft::LensShadingMapModeOff)
		.value("On", libcamera::controls::draft::LensShadingMapModeOn)
	;
	py::enum_<libcamera::controls::draft::SceneFlickerEnum>(m, "SceneFlicker")
		.value("Off", libcamera::controls::draft::SceneFickerOff)
		.value("50Hz", libcamera::controls::draft::SceneFicker50Hz)
		.value("60Hz", libcamera::controls::draft::SceneFicker60Hz)
	;
	py::enum_<libcamera::controls::draft::TestPatternModeEnum>(m, "TestPatternMode")
		.value("Off", libcamera::controls::draft::TestPatternModeOff)
		.value("SolidColor", libcamera::controls::draft::TestPatternModeSolidColor)
		.value("ColorBars", libcamera::controls::draft::TestPatternModeColorBars)
		.value("ColorBarsFadeToGray", libcamera::controls::draft::TestPatternModeColorBarsFadeToGray)
		.value("Pn9", libcamera::controls::draft::TestPatternModePn9)
		.value("Custom1", libcamera::controls::draft::TestPatternModeCustom1)
	;
Nicolas Dufresne via libcamera-devel March 17, 2022, 11:46 a.m. UTC | #9
Cool, thanks, I'll try those and let you know!

David

On Thu, 17 Mar 2022 at 11:36, Tomi Valkeinen
<tomi.valkeinen@ideasonboard.com> wrote:
>
> On 17/03/2022 13:01, David Plowman wrote:
> > Hi again
> >
> > On Thu, 17 Mar 2022 at 10:41, Tomi Valkeinen
> > <tomi.valkeinen@ideasonboard.com> wrote:
> >>
> >> On 17/03/2022 12:12, David Plowman wrote:
> >>> Hi again
> >>>
> >>> Just to answer the about about the "enum" type controls:
> >>>
> >>> On Thu, 17 Mar 2022 at 09:55, Tomi Valkeinen
> >>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>>
> >>>> On 17/03/2022 11:44, David Plowman wrote:
> >>>>> Hi everyone
> >>>>>
> >>>>> Thanks for the update!
> >>>>>
> >>>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> >>>>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> This is v5 of the series. The changes to v4:
> >>>>>>
> >>>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>>>>>      HACK for Camera public destructor
> >>>>>> - Changed Request.set_control() so that we can drop the HACK for
> >>>>>>      exposing Camera from Request
> >>>>>> - Moved Python enum defs to a separate file for clarity
> >>>>>> - "Forward" declare Python classes to fix docstring issues.
> >>>>>>
> >>>>>> The set_control change breaks the current users of that function.
> >>>>>> Previously you could do this:
> >>>>>>
> >>>>>> req.set_control("Brightness", 1)
> >>>>>>
> >>>>>> Now you need to do:
> >>>>>>
> >>>>>> cid = camera.find_control("Brightness")
> >>>>>> req.set_control(cid, 1)
> >>>>>>
> >>>>>
> >>>>> I see the need for this, and I guess it's fine - obviously we'll hide
> >>>>> that under our Picamera2 API as we wouldn't want our users to have to
> >>>>> do that!
> >>>>
> >>>> Yes, it's not very nice. Then again, having to do a string search every
> >>>> time we call set_control is not good either, at least if we can avoid it
> >>>> (and we can, as above). But I agree that in a higher level API things
> >>>> should be easy. Of course, it would be nice to have both versions even
> >>>> in this low level API, but... if Request doesn't expose camera, I don't
> >>>> think we can do it in a simple way.
> >>>>
> >>>>> There is one further thing I wanted to raise in relation to the Python
> >>>>> bindings for controls, now that a few folks are trying out Picamera2.
> >>>>>
> >>>>> I've had feedback that they don't like the fact that all the "enum"
> >>>>> control values (exposure modes, AWB modes, that kind of thing) are
> >>>>> simple integers up in the Python world. They complain that this makes
> >>>>> them quite unfriendly to use, and I'm inclined to agree! We could
> >>>>> always create Python-level definitions for them, and possibly add code
> >>>>> to convert stuff on the fly, but I wonder if Pybind11 can do anything
> >>>>> more automatic for us?
> >>>>
> >>>> Can you elaborate? Which enums are these?
> >>>
> >>> So users would prefer to write something like
> >>>
> >>> picamera2.set_controls({"AwbMode": AwbMode.Indoor})
> >>>
> >>> rather than
> >>>
> >>> picamera2.set_controls({"AwbMode": 4})
> >>>
> >>> The same applies to all the controls of this type, there including
> >>>
> >>> AeMeteringMode
> >>> AeConstraintMode
> >>> AeExposureMode
> >>> AwbMode
> >>> NoiseReductionMode
> >>> there will at some point be an AfMode and probably other AF ones
> >>> and so on.
> >>>
> >>> Basically it looks to be mostly anything with "Mode" on the end,
> >>> though I think we will encounter some others too. Also anything with
> >>> "State" on the end is probably similar, though in this case we're
> >>> talking about reported values, not ones you can set.
> >>
> >> Looks like these are defined in src/libcamera/control_ids.yaml, so we
> >> could have a script that generates pybind11 code to create the enums. Or
> >> we could just do it by hand.
> >
> > Defining them by hand is fine with me, at least for the time being. I
> > just wonder whether it would get annoying in the longer run, so some
> > kind of script in the build system might be helpful.
> >
> >>
> >> Is that your question, how and where to define the python enums? Or do
> >> you think there's some issue using those?
> >
> > I don't think there's any problem, I guess I'm just flagging that
> > there's something we probably need to do. I could define them all in
> > Picamera2, but it seems like everyone using these Python bindings
> > would appreciate them, so they probably belong in libcamera somewhere.
>
> Ok. I hacked a quick script. Here's a snippet to add to the py .c file if you want to try them:
>
>
>         py::enum_<libcamera::controls::AeMeteringModeEnum>(m, "AeMeteringMode")
>                 .value("CentreWeighted", libcamera::controls::MeteringCentreWeighted)
>                 .value("Spot", libcamera::controls::MeteringSpot)
>                 .value("Matrix", libcamera::controls::MeteringMatrix)
>                 .value("Custom", libcamera::controls::MeteringCustom)
>         ;
>         py::enum_<libcamera::controls::AeConstraintModeEnum>(m, "AeConstraintMode")
>                 .value("Normal", libcamera::controls::ConstraintNormal)
>                 .value("Highlight", libcamera::controls::ConstraintHighlight)
>                 .value("Shadows", libcamera::controls::ConstraintShadows)
>                 .value("Custom", libcamera::controls::ConstraintCustom)
>         ;
>         py::enum_<libcamera::controls::AeExposureModeEnum>(m, "AeExposureMode")
>                 .value("Normal", libcamera::controls::ExposureNormal)
>                 .value("Short", libcamera::controls::ExposureShort)
>                 .value("Long", libcamera::controls::ExposureLong)
>                 .value("Custom", libcamera::controls::ExposureCustom)
>         ;
>         py::enum_<libcamera::controls::AwbModeEnum>(m, "AwbMode")
>                 .value("Auto", libcamera::controls::AwbAuto)
>                 .value("Incandescent", libcamera::controls::AwbIncandescent)
>                 .value("Tungsten", libcamera::controls::AwbTungsten)
>                 .value("Fluorescent", libcamera::controls::AwbFluorescent)
>                 .value("Indoor", libcamera::controls::AwbIndoor)
>                 .value("Daylight", libcamera::controls::AwbDaylight)
>                 .value("Cloudy", libcamera::controls::AwbCloudy)
>                 .value("Custom", libcamera::controls::AwbCustom)
>         ;
>         py::enum_<libcamera::controls::draft::AePrecaptureTriggerEnum>(m, "AePrecaptureTrigger")
>                 .value("Idle", libcamera::controls::draft::AePrecaptureTriggerIdle)
>                 .value("Start", libcamera::controls::draft::AePrecaptureTriggerStart)
>                 .value("Cancel", libcamera::controls::draft::AePrecaptureTriggerCancel)
>         ;
>         py::enum_<libcamera::controls::draft::AfTriggerEnum>(m, "AfTrigger")
>                 .value("Idle", libcamera::controls::draft::AfTriggerIdle)
>                 .value("Start", libcamera::controls::draft::AfTriggerStart)
>                 .value("Cancel", libcamera::controls::draft::AfTriggerCancel)
>         ;
>         py::enum_<libcamera::controls::draft::NoiseReductionModeEnum>(m, "NoiseReductionMode")
>                 .value("Off", libcamera::controls::draft::NoiseReductionModeOff)
>                 .value("Fast", libcamera::controls::draft::NoiseReductionModeFast)
>                 .value("HighQuality", libcamera::controls::draft::NoiseReductionModeHighQuality)
>                 .value("Minimal", libcamera::controls::draft::NoiseReductionModeMinimal)
>                 .value("ZSL", libcamera::controls::draft::NoiseReductionModeZSL)
>         ;
>         py::enum_<libcamera::controls::draft::ColorCorrectionAberrationModeEnum>(m, "ColorCorrectionAberrationMode")
>                 .value("Off", libcamera::controls::draft::ColorCorrectionAberrationOff)
>                 .value("Fast", libcamera::controls::draft::ColorCorrectionAberrationFast)
>                 .value("HighQuality", libcamera::controls::draft::ColorCorrectionAberrationHighQuality)
>         ;
>         py::enum_<libcamera::controls::draft::AeStateEnum>(m, "AeState")
>                 .value("Inactive", libcamera::controls::draft::AeStateInactive)
>                 .value("Searching", libcamera::controls::draft::AeStateSearching)
>                 .value("Converged", libcamera::controls::draft::AeStateConverged)
>                 .value("Locked", libcamera::controls::draft::AeStateLocked)
>                 .value("FlashRequired", libcamera::controls::draft::AeStateFlashRequired)
>                 .value("Precapture", libcamera::controls::draft::AeStatePrecapture)
>         ;
>         py::enum_<libcamera::controls::draft::AfStateEnum>(m, "AfState")
>                 .value("Inactive", libcamera::controls::draft::AfStateInactive)
>                 .value("PassiveScan", libcamera::controls::draft::AfStatePassiveScan)
>                 .value("PassiveFocused", libcamera::controls::draft::AfStatePassiveFocused)
>                 .value("ActiveScan", libcamera::controls::draft::AfStateActiveScan)
>                 .value("FocusedLock", libcamera::controls::draft::AfStateFocusedLock)
>                 .value("NotFocusedLock", libcamera::controls::draft::AfStateNotFocusedLock)
>                 .value("PassiveUnfocused", libcamera::controls::draft::AfStatePassiveUnfocused)
>         ;
>         py::enum_<libcamera::controls::draft::AwbStateEnum>(m, "AwbState")
>                 .value("StateInactive", libcamera::controls::draft::AwbStateInactive)
>                 .value("StateSearching", libcamera::controls::draft::AwbStateSearching)
>                 .value("Converged", libcamera::controls::draft::AwbConverged)
>                 .value("Locked", libcamera::controls::draft::AwbLocked)
>         ;
>         py::enum_<libcamera::controls::draft::LensShadingMapModeEnum>(m, "LensShadingMapMode")
>                 .value("Off", libcamera::controls::draft::LensShadingMapModeOff)
>                 .value("On", libcamera::controls::draft::LensShadingMapModeOn)
>         ;
>         py::enum_<libcamera::controls::draft::SceneFlickerEnum>(m, "SceneFlicker")
>                 .value("Off", libcamera::controls::draft::SceneFickerOff)
>                 .value("50Hz", libcamera::controls::draft::SceneFicker50Hz)
>                 .value("60Hz", libcamera::controls::draft::SceneFicker60Hz)
>         ;
>         py::enum_<libcamera::controls::draft::TestPatternModeEnum>(m, "TestPatternMode")
>                 .value("Off", libcamera::controls::draft::TestPatternModeOff)
>                 .value("SolidColor", libcamera::controls::draft::TestPatternModeSolidColor)
>                 .value("ColorBars", libcamera::controls::draft::TestPatternModeColorBars)
>                 .value("ColorBarsFadeToGray", libcamera::controls::draft::TestPatternModeColorBarsFadeToGray)
>                 .value("Pn9", libcamera::controls::draft::TestPatternModePn9)
>                 .value("Custom1", libcamera::controls::draft::TestPatternModeCustom1)
>         ;
>
Nicolas Dufresne via libcamera-devel March 17, 2022, 11:50 a.m. UTC | #10
Quoting Tomi Valkeinen (2022-03-17 10:03:25)
> On 17/03/2022 11:55, Tomi Valkeinen wrote:
> > On 17/03/2022 11:44, David Plowman wrote:
> >> Hi everyone
> >>
> >> Thanks for the update!
> >>
> >> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> >> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>> This is v5 of the series. The changes to v4:
> >>>
> >>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>>    HACK for Camera public destructor
> >>> - Changed Request.set_control() so that we can drop the HACK for
> >>>    exposing Camera from Request
> >>> - Moved Python enum defs to a separate file for clarity
> >>> - "Forward" declare Python classes to fix docstring issues.
> >>>
> >>> The set_control change breaks the current users of that function.
> >>> Previously you could do this:
> >>>
> >>> req.set_control("Brightness", 1)
> >>>
> >>> Now you need to do:
> >>>
> >>> cid = camera.find_control("Brightness")
> >>> req.set_control(cid, 1)
> >>>
> >>
> >> I see the need for this, and I guess it's fine - obviously we'll hide
> >> that under our Picamera2 API as we wouldn't want our users to have to
> >> do that!
> 
> I think the core question is whether a Request is tied to a Camera. I 
> thought it is, as it is created from the Camera. And if that's the case, 
> I don't see why the Camera cannot be exposed from the Request.

This is how I understand it, and the patch I have/had locally to add and
expose this is followed by an addition of an error check to make sure
that requests queued to a camera that were not created by that camera get
rejected.

I could probably still do that using the private class infrastructure
now though. (re-sketching up that patch now).

> Maybe Laurent can explain the reasons why not to expose it.
> 
> > Yes, it's not very nice. Then again, having to do a string search every 
> > time we call set_control is not good either, at least if we can avoid it 
> > (and we can, as above). But I agree that in a higher level API things 
> > should be easy. Of course, it would be nice to have both versions even 
> > in this low level API, but... if Request doesn't expose camera, I don't 
> > think we can do it in a simple way.
> 
> Note that there are multiple ways to approach this. E.g.:
> 
> req.set_control(camera.controls["Brightness"], 1)
> 
> or:
> 
> req.set_control(camera.controls.Brightness, 1)
> 
> or even (although this could perhaps conflict too easily):
> 
> req.set_control(camera.Brightness, 1)
> 
> Or we could go the other way:
> 
> camera.set_control(req, "Brightness", 1)
> 
> or:
> 
> camera.controls.Brightness.set(req, 1)
> 
>   Tomi
Nicolas Dufresne via libcamera-devel March 17, 2022, 2:03 p.m. UTC | #11
Hi Tomi

Thanks for those py::enum definitions - they work great, for example:
"picamera2.set_controls({"AwbMode": libcamera.AwbMode.Indoor})"

David

On Thu, 17 Mar 2022 at 11:50, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Tomi Valkeinen (2022-03-17 10:03:25)
> > On 17/03/2022 11:55, Tomi Valkeinen wrote:
> > > On 17/03/2022 11:44, David Plowman wrote:
> > >> Hi everyone
> > >>
> > >> Thanks for the update!
> > >>
> > >> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> > >> <tomi.valkeinen@ideasonboard.com> wrote:
> > >>>
> > >>> Hi,
> > >>>
> > >>> This is v5 of the series. The changes to v4:
> > >>>
> > >>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> > >>>    HACK for Camera public destructor
> > >>> - Changed Request.set_control() so that we can drop the HACK for
> > >>>    exposing Camera from Request
> > >>> - Moved Python enum defs to a separate file for clarity
> > >>> - "Forward" declare Python classes to fix docstring issues.
> > >>>
> > >>> The set_control change breaks the current users of that function.
> > >>> Previously you could do this:
> > >>>
> > >>> req.set_control("Brightness", 1)
> > >>>
> > >>> Now you need to do:
> > >>>
> > >>> cid = camera.find_control("Brightness")
> > >>> req.set_control(cid, 1)
> > >>>
> > >>
> > >> I see the need for this, and I guess it's fine - obviously we'll hide
> > >> that under our Picamera2 API as we wouldn't want our users to have to
> > >> do that!
> >
> > I think the core question is whether a Request is tied to a Camera. I
> > thought it is, as it is created from the Camera. And if that's the case,
> > I don't see why the Camera cannot be exposed from the Request.
>
> This is how I understand it, and the patch I have/had locally to add and
> expose this is followed by an addition of an error check to make sure
> that requests queued to a camera that were not created by that camera get
> rejected.
>
> I could probably still do that using the private class infrastructure
> now though. (re-sketching up that patch now).
>
> > Maybe Laurent can explain the reasons why not to expose it.
> >
> > > Yes, it's not very nice. Then again, having to do a string search every
> > > time we call set_control is not good either, at least if we can avoid it
> > > (and we can, as above). But I agree that in a higher level API things
> > > should be easy. Of course, it would be nice to have both versions even
> > > in this low level API, but... if Request doesn't expose camera, I don't
> > > think we can do it in a simple way.
> >
> > Note that there are multiple ways to approach this. E.g.:
> >
> > req.set_control(camera.controls["Brightness"], 1)
> >
> > or:
> >
> > req.set_control(camera.controls.Brightness, 1)
> >
> > or even (although this could perhaps conflict too easily):
> >
> > req.set_control(camera.Brightness, 1)
> >
> > Or we could go the other way:
> >
> > camera.set_control(req, "Brightness", 1)
> >
> > or:
> >
> > camera.controls.Brightness.set(req, 1)
> >
> >   Tomi
Tomi Valkeinen April 5, 2022, 6:47 a.m. UTC | #12
Hi Laurent,

On 17/03/2022 13:50, Kieran Bingham wrote:
> Quoting Tomi Valkeinen (2022-03-17 10:03:25)
>> On 17/03/2022 11:55, Tomi Valkeinen wrote:
>>> On 17/03/2022 11:44, David Plowman wrote:
>>>> Hi everyone
>>>>
>>>> Thanks for the update!
>>>>
>>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
>>>> <tomi.valkeinen@ideasonboard.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> This is v5 of the series. The changes to v4:
>>>>>
>>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
>>>>>     HACK for Camera public destructor
>>>>> - Changed Request.set_control() so that we can drop the HACK for
>>>>>     exposing Camera from Request
>>>>> - Moved Python enum defs to a separate file for clarity
>>>>> - "Forward" declare Python classes to fix docstring issues.
>>>>>
>>>>> The set_control change breaks the current users of that function.
>>>>> Previously you could do this:
>>>>>
>>>>> req.set_control("Brightness", 1)
>>>>>
>>>>> Now you need to do:
>>>>>
>>>>> cid = camera.find_control("Brightness")
>>>>> req.set_control(cid, 1)
>>>>>
>>>>
>>>> I see the need for this, and I guess it's fine - obviously we'll hide
>>>> that under our Picamera2 API as we wouldn't want our users to have to
>>>> do that!
>>
>> I think the core question is whether a Request is tied to a Camera. I
>> thought it is, as it is created from the Camera. And if that's the case,
>> I don't see why the Camera cannot be exposed from the Request.
> 
> This is how I understand it, and the patch I have/had locally to add and
> expose this is followed by an addition of an error check to make sure
> that requests queued to a camera that were not created by that camera get
> rejected.
> 
> I could probably still do that using the private class infrastructure
> now though. (re-sketching up that patch now).
> 
>> Maybe Laurent can explain the reasons why not to expose it.

Any comment on this?

  Tomi
Laurent Pinchart April 6, 2022, 1:55 a.m. UTC | #13
Hi Tomi,

On Tue, Apr 05, 2022 at 09:47:16AM +0300, Tomi Valkeinen wrote:
> On 17/03/2022 13:50, Kieran Bingham wrote:
> > Quoting Tomi Valkeinen (2022-03-17 10:03:25)
> >> On 17/03/2022 11:55, Tomi Valkeinen wrote:
> >>> On 17/03/2022 11:44, David Plowman wrote:
> >>>> Hi everyone
> >>>>
> >>>> Thanks for the update!
> >>>>
> >>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> >>>> <tomi.valkeinen@ideasonboard.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> This is v5 of the series. The changes to v4:
> >>>>>
> >>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> >>>>>     HACK for Camera public destructor
> >>>>> - Changed Request.set_control() so that we can drop the HACK for
> >>>>>     exposing Camera from Request
> >>>>> - Moved Python enum defs to a separate file for clarity
> >>>>> - "Forward" declare Python classes to fix docstring issues.
> >>>>>
> >>>>> The set_control change breaks the current users of that function.
> >>>>> Previously you could do this:
> >>>>>
> >>>>> req.set_control("Brightness", 1)
> >>>>>
> >>>>> Now you need to do:
> >>>>>
> >>>>> cid = camera.find_control("Brightness")
> >>>>> req.set_control(cid, 1)
> >>>>>
> >>>>
> >>>> I see the need for this, and I guess it's fine - obviously we'll hide
> >>>> that under our Picamera2 API as we wouldn't want our users to have to
> >>>> do that!
> >>
> >> I think the core question is whether a Request is tied to a Camera. I
> >> thought it is, as it is created from the Camera. And if that's the case,
> >> I don't see why the Camera cannot be exposed from the Request.
> > 
> > This is how I understand it, and the patch I have/had locally to add and
> > expose this is followed by an addition of an error check to make sure
> > that requests queued to a camera that were not created by that camera get
> > rejected.
> > 
> > I could probably still do that using the private class infrastructure
> > now though. (re-sketching up that patch now).
> > 
> >> Maybe Laurent can explain the reasons why not to expose it.
> 
> Any comment on this?

Added on my todo list, I'll sleep over that and try to reply before the
end of the week.
Kieran Bingham April 13, 2022, 8:06 a.m. UTC | #14
Quoting Laurent Pinchart (2022-04-06 02:55:59)
> Hi Tomi,
> 
> On Tue, Apr 05, 2022 at 09:47:16AM +0300, Tomi Valkeinen wrote:
> > On 17/03/2022 13:50, Kieran Bingham wrote:
> > > Quoting Tomi Valkeinen (2022-03-17 10:03:25)
> > >> On 17/03/2022 11:55, Tomi Valkeinen wrote:
> > >>> On 17/03/2022 11:44, David Plowman wrote:
> > >>>> Hi everyone
> > >>>>
> > >>>> Thanks for the update!
> > >>>>
> > >>>> On Mon, 14 Mar 2022 at 15:46, Tomi Valkeinen
> > >>>> <tomi.valkeinen@ideasonboard.com> wrote:
> > >>>>>
> > >>>>> Hi,
> > >>>>>
> > >>>>> This is v5 of the series. The changes to v4:
> > >>>>>
> > >>>>> - Changed to pybind11 'smart_holders' branch which allows us to drop the
> > >>>>>     HACK for Camera public destructor
> > >>>>> - Changed Request.set_control() so that we can drop the HACK for
> > >>>>>     exposing Camera from Request
> > >>>>> - Moved Python enum defs to a separate file for clarity
> > >>>>> - "Forward" declare Python classes to fix docstring issues.
> > >>>>>
> > >>>>> The set_control change breaks the current users of that function.
> > >>>>> Previously you could do this:
> > >>>>>
> > >>>>> req.set_control("Brightness", 1)
> > >>>>>
> > >>>>> Now you need to do:
> > >>>>>
> > >>>>> cid = camera.find_control("Brightness")
> > >>>>> req.set_control(cid, 1)
> > >>>>>
> > >>>>
> > >>>> I see the need for this, and I guess it's fine - obviously we'll hide
> > >>>> that under our Picamera2 API as we wouldn't want our users to have to
> > >>>> do that!
> > >>
> > >> I think the core question is whether a Request is tied to a Camera. I
> > >> thought it is, as it is created from the Camera. And if that's the case,
> > >> I don't see why the Camera cannot be exposed from the Request.
> > > 
> > > This is how I understand it, and the patch I have/had locally to add and
> > > expose this is followed by an addition of an error check to make sure
> > > that requests queued to a camera that were not created by that camera get
> > > rejected.
> > > 
> > > I could probably still do that using the private class infrastructure
> > > now though. (re-sketching up that patch now).
> > > 
> > >> Maybe Laurent can explain the reasons why not to expose it.
> > 
> > Any comment on this?
> 
> Added on my todo list, I'll sleep over that and try to reply before the
> end of the week.

Note that now I've now merged my "Ensure requests belong to the camera"
patch, so the libcamera API rejects any Request which is not queued
directly to it's camera.

I would believe that makes it 'safe' to obtain the camera directly from
a request, and trust that it's properties are directly associated with
the correct camera.

--
Kieran

> -- 
> Regards,
> 
> Laurent Pinchart