[libcamera-devel,v2,1/2] libcamera: control: initialise control info to ControlTypeNone by default
diff mbox series

Message ID 20220830202124.457253-1-Rauch.Christian@gmx.de
State Superseded
Headers show
Series
  • [libcamera-devel,v2,1/2] libcamera: control: initialise control info to ControlTypeNone by default
Related show

Commit Message

Christian Rauch Aug. 30, 2022, 8:21 p.m. UTC
The default ControlInfo constructor allows to partially initialised the
min/max/def values. Uninitialised values are assigned to 0 by default. This
implicit initialisation makes it impossible to distinguish between and
uninitialised and an explicitly 0-initialised ControlValue.

Default construct the ControlValue in the ControlInfo default contructor to
explicitly represent uninitialised values by the ControlTypeNone type.

Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
---
 include/libcamera/controls.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--
2.34.1

Comments

Kieran Bingham Aug. 31, 2022, 11:51 a.m. UTC | #1
Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> The default ControlInfo constructor allows to partially initialised the
> min/max/def values. Uninitialised values are assigned to 0 by default. This
> implicit initialisation makes it impossible to distinguish between and
> uninitialised and an explicitly 0-initialised ControlValue.
> 
> Default construct the ControlValue in the ControlInfo default contructor to
> explicitly represent uninitialised values by the ControlTypeNone type.

This sounds ... a lot better !

Testing with this series applied (including the patch 2/2, which should
probably be squashed to one patch to maintain bisection) has the
following failures for me:

27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
stderr:
[148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
[148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
[148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
[148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
[148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
[148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
[148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
lhs:
- 1: [false..true]
- 7: [0..999999]
- 8: [1.000000..32.000000]
- 9: [-1.000000..1.000000]
- 15: [0.000000..32.000000]
rhs:
Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original

So I think we have to identify what impact this has on control
serialisation. I expect we haven't tried to serialize ControlTypeNone
before.

But I think this is a good path to take currently.

--
Kieran


> 
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  include/libcamera/controls.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ebc168fc..38d0a3e8 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -268,9 +268,9 @@ private:
>  class ControlInfo
>  {
>  public:
> -       explicit ControlInfo(const ControlValue &min = 0,
> -                            const ControlValue &max = 0,
> -                            const ControlValue &def = 0);
> +       explicit ControlInfo(const ControlValue &min = {},
> +                            const ControlValue &max = {},
> +                            const ControlValue &def = {});
>         explicit ControlInfo(Span<const ControlValue> values,
>                              const ControlValue &def = {});
>         explicit ControlInfo(std::set<bool> values, bool def);
> --
> 2.34.1
>
Christian Rauch Aug. 31, 2022, 8:45 p.m. UTC | #2
Hi Kieran,

This test (and others) are skipped with "exit status 77". When I try to
run "test/serialization/ipa_data_serializer_test" manually, it fails
with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
a virtual device. Is it somewhere documented how this is created? Could
this be created automatically for convenience?

Best,
Christian


Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>> The default ControlInfo constructor allows to partially initialised the
>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>> implicit initialisation makes it impossible to distinguish between and
>> uninitialised and an explicitly 0-initialised ControlValue.
>>
>> Default construct the ControlValue in the ControlInfo default contructor to
>> explicitly represent uninitialised values by the ControlTypeNone type.
>
> This sounds ... a lot better !
>
> Testing with this series applied (including the patch 2/2, which should
> probably be squashed to one patch to maintain bisection) has the
> following failures for me:
>
> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> lhs:
> - 1: [false..true]
> - 7: [0..999999]
> - 8: [1.000000..32.000000]
> - 9: [-1.000000..1.000000]
> - 15: [0.000000..32.000000]
> rhs:
> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>
> So I think we have to identify what impact this has on control
> serialisation. I expect we haven't tried to serialize ControlTypeNone
> before.
>
> But I think this is a good path to take currently.
>
> --
> Kieran
>
>
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>  include/libcamera/controls.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index ebc168fc..38d0a3e8 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -268,9 +268,9 @@ private:
>>  class ControlInfo
>>  {
>>  public:
>> -       explicit ControlInfo(const ControlValue &min = 0,
>> -                            const ControlValue &max = 0,
>> -                            const ControlValue &def = 0);
>> +       explicit ControlInfo(const ControlValue &min = {},
>> +                            const ControlValue &max = {},
>> +                            const ControlValue &def = {});
>>         explicit ControlInfo(Span<const ControlValue> values,
>>                              const ControlValue &def = {});
>>         explicit ControlInfo(std::set<bool> values, bool def);
>> --
>> 2.34.1
>>
Kieran Bingham Aug. 31, 2022, 9:07 p.m. UTC | #3
Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
> Hi Kieran,
> 
> This test (and others) are skipped with "exit status 77". When I try to
> run "test/serialization/ipa_data_serializer_test" manually, it fails
> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
> a virtual device. Is it somewhere documented how this is created? Could
> this be created automatically for convenience?

To load the modules for testing:

sudo modprobe vimc
sudo modprobe vim2m
sudo modrpobe vivid

We should really find somewhere suitable to document that.

--
Kieran


> 
> Best,
> Christian
> 
> 
> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> > Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> >> The default ControlInfo constructor allows to partially initialised the
> >> min/max/def values. Uninitialised values are assigned to 0 by default. This
> >> implicit initialisation makes it impossible to distinguish between and
> >> uninitialised and an explicitly 0-initialised ControlValue.
> >>
> >> Default construct the ControlValue in the ControlInfo default contructor to
> >> explicitly represent uninitialised values by the ControlTypeNone type.
> >
> > This sounds ... a lot better !
> >
> > Testing with this series applied (including the patch 2/2, which should
> > probably be squashed to one patch to maintain bisection) has the
> > following failures for me:
> >
> > 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> >>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > stderr:
> > [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> > [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> > [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> > [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> > [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> > [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> > [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> > lhs:
> > - 1: [false..true]
> > - 7: [0..999999]
> > - 8: [1.000000..32.000000]
> > - 9: [-1.000000..1.000000]
> > - 15: [0.000000..32.000000]
> > rhs:
> > Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> >
> > So I think we have to identify what impact this has on control
> > serialisation. I expect we haven't tried to serialize ControlTypeNone
> > before.
> >
> > But I think this is a good path to take currently.
> >
> > --
> > Kieran
> >
> >
> >>
> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >> ---
> >>  include/libcamera/controls.h | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> index ebc168fc..38d0a3e8 100644
> >> --- a/include/libcamera/controls.h
> >> +++ b/include/libcamera/controls.h
> >> @@ -268,9 +268,9 @@ private:
> >>  class ControlInfo
> >>  {
> >>  public:
> >> -       explicit ControlInfo(const ControlValue &min = 0,
> >> -                            const ControlValue &max = 0,
> >> -                            const ControlValue &def = 0);
> >> +       explicit ControlInfo(const ControlValue &min = {},
> >> +                            const ControlValue &max = {},
> >> +                            const ControlValue &def = {});
> >>         explicit ControlInfo(Span<const ControlValue> values,
> >>                              const ControlValue &def = {});
> >>         explicit ControlInfo(std::set<bool> values, bool def);
> >> --
> >> 2.34.1
> >>
Nicolas Dufresne via libcamera-devel Aug. 31, 2022, 9:09 p.m. UTC | #4
On Wed, Aug 31, 2022 at 12:51:48PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> > The default ControlInfo constructor allows to partially initialised the
> > min/max/def values. Uninitialised values are assigned to 0 by default. This
> > implicit initialisation makes it impossible to distinguish between and
> > uninitialised and an explicitly 0-initialised ControlValue.
> > 
> > Default construct the ControlValue in the ControlInfo default contructor to
> > explicitly represent uninitialised values by the ControlTypeNone type.
> 
> This sounds ... a lot better !
> 
> Testing with this series applied (including the patch 2/2, which should
> probably be squashed to one patch to maintain bisection) has the
> following failures for me:
> 
> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> >>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> lhs:
> - 1: [false..true]
> - 7: [0..999999]
> - 8: [1.000000..32.000000]
> - 9: [-1.000000..1.000000]
> - 15: [0.000000..32.000000]
> rhs:
> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> 
> So I think we have to identify what impact this has on control
> serialisation. I expect we haven't tried to serialize ControlTypeNone
> before.

This is probably related to bug 137 [1].

Maybe it's time to fix it :p


Paul


[1] https://bugs.libcamera.org/show_bug.cgi?id=137

> 
> But I think this is a good path to take currently.
> 
> --
> Kieran
> 
> 
> > 
> > Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > ---
> >  include/libcamera/controls.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > index ebc168fc..38d0a3e8 100644
> > --- a/include/libcamera/controls.h
> > +++ b/include/libcamera/controls.h
> > @@ -268,9 +268,9 @@ private:
> >  class ControlInfo
> >  {
> >  public:
> > -       explicit ControlInfo(const ControlValue &min = 0,
> > -                            const ControlValue &max = 0,
> > -                            const ControlValue &def = 0);
> > +       explicit ControlInfo(const ControlValue &min = {},
> > +                            const ControlValue &max = {},
> > +                            const ControlValue &def = {});
> >         explicit ControlInfo(Span<const ControlValue> values,
> >                              const ControlValue &def = {});
> >         explicit ControlInfo(std::set<bool> values, bool def);
> > --
> > 2.34.1
> >
Nicolas Dufresne via libcamera-devel Aug. 31, 2022, 9:12 p.m. UTC | #5
Hi Christian,

On Wed, Aug 31, 2022 at 10:45:31PM +0200, Christian Rauch via libcamera-devel wrote:
> Hi Kieran,
> 
> This test (and others) are skipped with "exit status 77". When I try to
> run "test/serialization/ipa_data_serializer_test" manually, it fails
> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
> a virtual device. Is it somewhere documented how this is created? Could
> this be created automatically for convenience?

It's a kernel module; you can load it like:

$ sudo modprobe vimc

Although iirc many distro kernels these days don't come with it? Or
maybe they fixed that [1]. Anyway it's not difficult to test.

[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1831482


Paul

> 
> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> > Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> >> The default ControlInfo constructor allows to partially initialised the
> >> min/max/def values. Uninitialised values are assigned to 0 by default. This
> >> implicit initialisation makes it impossible to distinguish between and
> >> uninitialised and an explicitly 0-initialised ControlValue.
> >>
> >> Default construct the ControlValue in the ControlInfo default contructor to
> >> explicitly represent uninitialised values by the ControlTypeNone type.
> >
> > This sounds ... a lot better !
> >
> > Testing with this series applied (including the patch 2/2, which should
> > probably be squashed to one patch to maintain bisection) has the
> > following failures for me:
> >
> > 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> >>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> > ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > stderr:
> > [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> > [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> > [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> > [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> > [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> > [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> > [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> > lhs:
> > - 1: [false..true]
> > - 7: [0..999999]
> > - 8: [1.000000..32.000000]
> > - 9: [-1.000000..1.000000]
> > - 15: [0.000000..32.000000]
> > rhs:
> > Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> >
> > So I think we have to identify what impact this has on control
> > serialisation. I expect we haven't tried to serialize ControlTypeNone
> > before.
> >
> > But I think this is a good path to take currently.
> >
> > --
> > Kieran
> >
> >
> >>
> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >> ---
> >>  include/libcamera/controls.h | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> index ebc168fc..38d0a3e8 100644
> >> --- a/include/libcamera/controls.h
> >> +++ b/include/libcamera/controls.h
> >> @@ -268,9 +268,9 @@ private:
> >>  class ControlInfo
> >>  {
> >>  public:
> >> -       explicit ControlInfo(const ControlValue &min = 0,
> >> -                            const ControlValue &max = 0,
> >> -                            const ControlValue &def = 0);
> >> +       explicit ControlInfo(const ControlValue &min = {},
> >> +                            const ControlValue &max = {},
> >> +                            const ControlValue &def = {});
> >>         explicit ControlInfo(Span<const ControlValue> values,
> >>                              const ControlValue &def = {});
> >>         explicit ControlInfo(std::set<bool> values, bool def);
> >> --
> >> 2.34.1
> >>
Christian Rauch Sept. 2, 2022, 10:52 p.m. UTC | #6
Hi Kieran,

Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>> The default ControlInfo constructor allows to partially initialised the
>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>> implicit initialisation makes it impossible to distinguish between and
>> uninitialised and an explicitly 0-initialised ControlValue.
>>
>> Default construct the ControlValue in the ControlInfo default contructor to
>> explicitly represent uninitialised values by the ControlTypeNone type.
>
> This sounds ... a lot better !
>
> Testing with this series applied (including the patch 2/2, which should
> probably be squashed to one patch to maintain bisection) has the
> following failures for me:
>
> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> stderr:
> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> lhs:
> - 1: [false..true]
> - 7: [0..999999]
> - 8: [1.000000..32.000000]
> - 9: [-1.000000..1.000000]
> - 15: [0.000000..32.000000]
> rhs:
> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>
> So I think we have to identify what impact this has on control
> serialisation. I expect we haven't tried to serialize ControlTypeNone
> before.

See v3. I added a commit that de-/serialises the type alongside the
value. This fixes the test for me. Can you test this again?

> But I think this is a good path to take currently.
>
> --
> Kieran
>
>
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>  include/libcamera/controls.h | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index ebc168fc..38d0a3e8 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -268,9 +268,9 @@ private:
>>  class ControlInfo
>>  {
>>  public:
>> -       explicit ControlInfo(const ControlValue &min = 0,
>> -                            const ControlValue &max = 0,
>> -                            const ControlValue &def = 0);
>> +       explicit ControlInfo(const ControlValue &min = {},
>> +                            const ControlValue &max = {},
>> +                            const ControlValue &def = {});
>>         explicit ControlInfo(Span<const ControlValue> values,
>>                              const ControlValue &def = {});
>>         explicit ControlInfo(std::set<bool> values, bool def);
>> --
>> 2.34.1
>>
Christian Rauch Sept. 2, 2022, 11:04 p.m. UTC | #7
Hi Paul,

Am 31.08.22 um 23:09 schrieb paul.elder@ideasonboard.com:
> On Wed, Aug 31, 2022 at 12:51:48PM +0100, Kieran Bingham via libcamera-devel wrote:
>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>>> The default ControlInfo constructor allows to partially initialised the
>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>>> implicit initialisation makes it impossible to distinguish between and
>>> uninitialised and an explicitly 0-initialised ControlValue.
>>>
>>> Default construct the ControlValue in the ControlInfo default contructor to
>>> explicitly represent uninitialised values by the ControlTypeNone type.
>>
>> This sounds ... a lot better !
>>
>> Testing with this series applied (including the patch 2/2, which should
>> probably be squashed to one patch to maintain bisection) has the
>> following failures for me:
>>
>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>> stderr:
>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
>> lhs:
>> - 1: [false..true]
>> - 7: [0..999999]
>> - 8: [1.000000..32.000000]
>> - 9: [-1.000000..1.000000]
>> - 15: [0.000000..32.000000]
>> rhs:
>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>>
>> So I think we have to identify what impact this has on control
>> serialisation. I expect we haven't tried to serialize ControlTypeNone
>> before.
>
> This is probably related to bug 137 [1].
>
> Maybe it's time to fix it :p
>

See my v3 for a fix and see [2] for a bit more details.

[2] https://bugs.libcamera.org/show_bug.cgi?id=137#c5

>
> Paul
>
>
> [1] https://bugs.libcamera.org/show_bug.cgi?id=137
>
>>
>> But I think this is a good path to take currently.
>>
>> --
>> Kieran
>>
>>
>>>
>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>> ---
>>>  include/libcamera/controls.h | 6 +++---
>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>> index ebc168fc..38d0a3e8 100644
>>> --- a/include/libcamera/controls.h
>>> +++ b/include/libcamera/controls.h
>>> @@ -268,9 +268,9 @@ private:
>>>  class ControlInfo
>>>  {
>>>  public:
>>> -       explicit ControlInfo(const ControlValue &min = 0,
>>> -                            const ControlValue &max = 0,
>>> -                            const ControlValue &def = 0);
>>> +       explicit ControlInfo(const ControlValue &min = {},
>>> +                            const ControlValue &max = {},
>>> +                            const ControlValue &def = {});
>>>         explicit ControlInfo(Span<const ControlValue> values,
>>>                              const ControlValue &def = {});
>>>         explicit ControlInfo(std::set<bool> values, bool def);
>>> --
>>> 2.34.1
>>>
Christian Rauch Sept. 4, 2022, 4:10 p.m. UTC | #8
Hi Kieran,

Am 31.08.22 um 23:07 schrieb Kieran Bingham:
> Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
>> Hi Kieran,
>>
>> This test (and others) are skipped with "exit status 77". When I try to
>> run "test/serialization/ipa_data_serializer_test" manually, it fails
>> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
>> a virtual device. Is it somewhere documented how this is created? Could
>> this be created automatically for convenience?
>
> To load the modules for testing:
>
> sudo modprobe vimc
> sudo modprobe vim2m
> sudo modrpobe vivid
>
> We should really find somewhere suitable to document that.

I managed to create that virtual device. This only exposes BGR888 and
RGB888 formats. Is it possible to simulate other formats, such as YUYV
or NV21?

> --
> Kieran
>
>
>>
>> Best,
>> Christian
>>
>>
>> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
>>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>>>> The default ControlInfo constructor allows to partially initialised the
>>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>>>> implicit initialisation makes it impossible to distinguish between and
>>>> uninitialised and an explicitly 0-initialised ControlValue.
>>>>
>>>> Default construct the ControlValue in the ControlInfo default contructor to
>>>> explicitly represent uninitialised values by the ControlTypeNone type.
>>>
>>> This sounds ... a lot better !
>>>
>>> Testing with this series applied (including the patch 2/2, which should
>>> probably be squashed to one patch to maintain bisection) has the
>>> following failures for me:
>>>
>>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
>>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>> stderr:
>>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
>>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
>>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
>>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
>>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
>>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
>>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
>>> lhs:
>>> - 1: [false..true]
>>> - 7: [0..999999]
>>> - 8: [1.000000..32.000000]
>>> - 9: [-1.000000..1.000000]
>>> - 15: [0.000000..32.000000]
>>> rhs:
>>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>>>
>>> So I think we have to identify what impact this has on control
>>> serialisation. I expect we haven't tried to serialize ControlTypeNone
>>> before.
>>>
>>> But I think this is a good path to take currently.
>>>
>>> --
>>> Kieran
>>>
>>>
>>>>
>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>> ---
>>>>  include/libcamera/controls.h | 6 +++---
>>>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>> index ebc168fc..38d0a3e8 100644
>>>> --- a/include/libcamera/controls.h
>>>> +++ b/include/libcamera/controls.h
>>>> @@ -268,9 +268,9 @@ private:
>>>>  class ControlInfo
>>>>  {
>>>>  public:
>>>> -       explicit ControlInfo(const ControlValue &min = 0,
>>>> -                            const ControlValue &max = 0,
>>>> -                            const ControlValue &def = 0);
>>>> +       explicit ControlInfo(const ControlValue &min = {},
>>>> +                            const ControlValue &max = {},
>>>> +                            const ControlValue &def = {});
>>>>         explicit ControlInfo(Span<const ControlValue> values,
>>>>                              const ControlValue &def = {});
>>>>         explicit ControlInfo(std::set<bool> values, bool def);
>>>> --
>>>> 2.34.1
>>>>
Kieran Bingham Sept. 27, 2022, 12:03 p.m. UTC | #9
Hi Christian,

Sorry - I missed this mail before.

Quoting Christian Rauch via libcamera-devel (2022-09-04 17:10:08)
> Hi Kieran,
> 
> Am 31.08.22 um 23:07 schrieb Kieran Bingham:
> > Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
> >> Hi Kieran,
> >>
> >> This test (and others) are skipped with "exit status 77". When I try to
> >> run "test/serialization/ipa_data_serializer_test" manually, it fails
> >> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
> >> a virtual device. Is it somewhere documented how this is created? Could
> >> this be created automatically for convenience?
> >
> > To load the modules for testing:
> >
> > sudo modprobe vimc
> > sudo modprobe vim2m
> > sudo modrpobe vivid
> >
> > We should really find somewhere suitable to document that.
> 
> I managed to create that virtual device. This only exposes BGR888 and
> RGB888 formats. Is it possible to simulate other formats, such as YUYV
> or NV21?

Not with VIMC. I'd like to see more formats supported by VIMC - but that
requires kernel work.

However, another option is the Vivid pipeline handler. This is a
pipeline handler for the vivid kernel test device. We keep this 'out of
tree' from libcamera, because it's not supposed to be a supported
device.

It is however really useful because the vivid device can generate all,
if not certainly most of the formats supported by V4L2.

To use it, merge the vivid pipeline handler into libcamera from:

  https://git.libcamera.org/libcamera/vivid.git/log/

This will then provide you with a new 'vivid' camera which can generate
many more formats.

It's constantly rebased with my local build scripts to maintain it on
top of tree, and also serves as some 'patch' documentation as to how to
add and introduce a new pipeline handler.

--
Kieran


> 
> > --
> > Kieran
> >
> >
> >>
> >> Best,
> >> Christian
> >>
> >>
> >> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> >>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> >>>> The default ControlInfo constructor allows to partially initialised the
> >>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
> >>>> implicit initialisation makes it impossible to distinguish between and
> >>>> uninitialised and an explicitly 0-initialised ControlValue.
> >>>>
> >>>> Default construct the ControlValue in the ControlInfo default contructor to
> >>>> explicitly represent uninitialised values by the ControlTypeNone type.
> >>>
> >>> This sounds ... a lot better !
> >>>
> >>> Testing with this series applied (including the patch 2/2, which should
> >>> probably be squashed to one patch to maintain bisection) has the
> >>> following failures for me:
> >>>
> >>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> >>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> >>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>> stderr:
> >>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> >>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> >>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> >>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> >>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> >>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> >>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> >>> lhs:
> >>> - 1: [false..true]
> >>> - 7: [0..999999]
> >>> - 8: [1.000000..32.000000]
> >>> - 9: [-1.000000..1.000000]
> >>> - 15: [0.000000..32.000000]
> >>> rhs:
> >>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> >>>
> >>> So I think we have to identify what impact this has on control
> >>> serialisation. I expect we haven't tried to serialize ControlTypeNone
> >>> before.
> >>>
> >>> But I think this is a good path to take currently.
> >>>
> >>> --
> >>> Kieran
> >>>
> >>>
> >>>>
> >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>>> ---
> >>>>  include/libcamera/controls.h | 6 +++---
> >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>>> index ebc168fc..38d0a3e8 100644
> >>>> --- a/include/libcamera/controls.h
> >>>> +++ b/include/libcamera/controls.h
> >>>> @@ -268,9 +268,9 @@ private:
> >>>>  class ControlInfo
> >>>>  {
> >>>>  public:
> >>>> -       explicit ControlInfo(const ControlValue &min = 0,
> >>>> -                            const ControlValue &max = 0,
> >>>> -                            const ControlValue &def = 0);
> >>>> +       explicit ControlInfo(const ControlValue &min = {},
> >>>> +                            const ControlValue &max = {},
> >>>> +                            const ControlValue &def = {});
> >>>>         explicit ControlInfo(Span<const ControlValue> values,
> >>>>                              const ControlValue &def = {});
> >>>>         explicit ControlInfo(std::set<bool> values, bool def);
> >>>> --
> >>>> 2.34.1
> >>>>
David Plowman Sept. 30, 2022, 11:01 a.m. UTC | #10
Hi Christian

I have a question about this because it's causing my Python code to
break. Not in any major way, we just need to decide what we want to do
about it.

The problem is that you can no longer fetch the default value for a
control, it causes a runtime error. The code in question is here:
https://git.linuxtv.org/libcamera.git/tree/src/py/libcamera/py_helpers.cpp#n57

There are a number of straightforward solutions, such as returning
"py::none()", for example. Does that sound like a reasonable thing to
do?

Thanks!
David

On Tue, 30 Aug 2022 at 21:21, Christian Rauch via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> The default ControlInfo constructor allows to partially initialised the
> min/max/def values. Uninitialised values are assigned to 0 by default. This
> implicit initialisation makes it impossible to distinguish between and
> uninitialised and an explicitly 0-initialised ControlValue.
>
> Default construct the ControlValue in the ControlInfo default contructor to
> explicitly represent uninitialised values by the ControlTypeNone type.
>
> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> ---
>  include/libcamera/controls.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> index ebc168fc..38d0a3e8 100644
> --- a/include/libcamera/controls.h
> +++ b/include/libcamera/controls.h
> @@ -268,9 +268,9 @@ private:
>  class ControlInfo
>  {
>  public:
> -       explicit ControlInfo(const ControlValue &min = 0,
> -                            const ControlValue &max = 0,
> -                            const ControlValue &def = 0);
> +       explicit ControlInfo(const ControlValue &min = {},
> +                            const ControlValue &max = {},
> +                            const ControlValue &def = {});
>         explicit ControlInfo(Span<const ControlValue> values,
>                              const ControlValue &def = {});
>         explicit ControlInfo(std::set<bool> values, bool def);
> --
> 2.34.1
>
Tomi Valkeinen Sept. 30, 2022, 11:14 a.m. UTC | #11
Hi,

On 30/09/2022 14:01, David Plowman wrote:
> Hi Christian
> 
> I have a question about this because it's causing my Python code to
> break. Not in any major way, we just need to decide what we want to do
> about it.
> 
> The problem is that you can no longer fetch the default value for a
> control, it causes a runtime error. The code in question is here:
> https://git.linuxtv.org/libcamera.git/tree/src/py/libcamera/py_helpers.cpp#n57
> 
> There are a number of straightforward solutions, such as returning
> "py::none()", for example. Does that sound like a reasonable thing to
> do?

Are we supposed to have uninitialized values? In that case py::none 
sounds fine. Or is it a bug if a control has an uninitialized default? 
Then throwing an exception, as we do now, sounds fine.

  Tomi


> Thanks!
> David
> 
> On Tue, 30 Aug 2022 at 21:21, Christian Rauch via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
>>
>> The default ControlInfo constructor allows to partially initialised the
>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>> implicit initialisation makes it impossible to distinguish between and
>> uninitialised and an explicitly 0-initialised ControlValue.
>>
>> Default construct the ControlValue in the ControlInfo default contructor to
>> explicitly represent uninitialised values by the ControlTypeNone type.
>>
>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>> ---
>>   include/libcamera/controls.h | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>> index ebc168fc..38d0a3e8 100644
>> --- a/include/libcamera/controls.h
>> +++ b/include/libcamera/controls.h
>> @@ -268,9 +268,9 @@ private:
>>   class ControlInfo
>>   {
>>   public:
>> -       explicit ControlInfo(const ControlValue &min = 0,
>> -                            const ControlValue &max = 0,
>> -                            const ControlValue &def = 0);
>> +       explicit ControlInfo(const ControlValue &min = {},
>> +                            const ControlValue &max = {},
>> +                            const ControlValue &def = {});
>>          explicit ControlInfo(Span<const ControlValue> values,
>>                               const ControlValue &def = {});
>>          explicit ControlInfo(std::set<bool> values, bool def);
>> --
>> 2.34.1
>>
Kieran Bingham Sept. 30, 2022, 11:59 a.m. UTC | #12
Quoting Tomi Valkeinen via libcamera-devel (2022-09-30 12:14:12)
> Hi,
> 
> On 30/09/2022 14:01, David Plowman wrote:
> > Hi Christian
> > 
> > I have a question about this because it's causing my Python code to
> > break. Not in any major way, we just need to decide what we want to do
> > about it.
> > 
> > The problem is that you can no longer fetch the default value for a
> > control, it causes a runtime error. The code in question is here:
> > https://git.linuxtv.org/libcamera.git/tree/src/py/libcamera/py_helpers.cpp#n57
> > 
> > There are a number of straightforward solutions, such as returning
> > "py::none()", for example. Does that sound like a reasonable thing to
> > do?
> 
> Are we supposed to have uninitialized values? In that case py::none 
> sounds fine. Or is it a bug if a control has an uninitialized default? 
> Then throwing an exception, as we do now, sounds fine.

Yes, I think the impact was we essentially introduced the abiltiy to
'not' specify a default value. There are controls, where it simply
doesn't make sense to have a default.

So I think returning py::none might be the most correct thing to do ?

--
Kieran


> 
>   Tomi
> 
> 
> > Thanks!
> > David
> > 
> > On Tue, 30 Aug 2022 at 21:21, Christian Rauch via libcamera-devel
> > <libcamera-devel@lists.libcamera.org> wrote:
> >>
> >> The default ControlInfo constructor allows to partially initialised the
> >> min/max/def values. Uninitialised values are assigned to 0 by default. This
> >> implicit initialisation makes it impossible to distinguish between and
> >> uninitialised and an explicitly 0-initialised ControlValue.
> >>
> >> Default construct the ControlValue in the ControlInfo default contructor to
> >> explicitly represent uninitialised values by the ControlTypeNone type.
> >>
> >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >> ---
> >>   include/libcamera/controls.h | 6 +++---
> >>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >> index ebc168fc..38d0a3e8 100644
> >> --- a/include/libcamera/controls.h
> >> +++ b/include/libcamera/controls.h
> >> @@ -268,9 +268,9 @@ private:
> >>   class ControlInfo
> >>   {
> >>   public:
> >> -       explicit ControlInfo(const ControlValue &min = 0,
> >> -                            const ControlValue &max = 0,
> >> -                            const ControlValue &def = 0);
> >> +       explicit ControlInfo(const ControlValue &min = {},
> >> +                            const ControlValue &max = {},
> >> +                            const ControlValue &def = {});
> >>          explicit ControlInfo(Span<const ControlValue> values,
> >>                               const ControlValue &def = {});
> >>          explicit ControlInfo(std::set<bool> values, bool def);
> >> --
> >> 2.34.1
> >>
>
David Plowman Sept. 30, 2022, 12:04 p.m. UTC | #13
On Fri, 30 Sept 2022 at 12:59, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Quoting Tomi Valkeinen via libcamera-devel (2022-09-30 12:14:12)
> > Hi,
> >
> > On 30/09/2022 14:01, David Plowman wrote:
> > > Hi Christian
> > >
> > > I have a question about this because it's causing my Python code to
> > > break. Not in any major way, we just need to decide what we want to do
> > > about it.
> > >
> > > The problem is that you can no longer fetch the default value for a
> > > control, it causes a runtime error. The code in question is here:
> > > https://git.linuxtv.org/libcamera.git/tree/src/py/libcamera/py_helpers.cpp#n57
> > >
> > > There are a number of straightforward solutions, such as returning
> > > "py::none()", for example. Does that sound like a reasonable thing to
> > > do?
> >
> > Are we supposed to have uninitialized values? In that case py::none
> > sounds fine. Or is it a bug if a control has an uninitialized default?
> > Then throwing an exception, as we do now, sounds fine.
>
> Yes, I think the impact was we essentially introduced the abiltiy to
> 'not' specify a default value. There are controls, where it simply
> doesn't make sense to have a default.
>
> So I think returning py::none might be the most correct thing to do ?

Works for me. Patch incoming...

Thanks!
David

>
> --
> Kieran
>
>
> >
> >   Tomi
> >
> >
> > > Thanks!
> > > David
> > >
> > > On Tue, 30 Aug 2022 at 21:21, Christian Rauch via libcamera-devel
> > > <libcamera-devel@lists.libcamera.org> wrote:
> > >>
> > >> The default ControlInfo constructor allows to partially initialised the
> > >> min/max/def values. Uninitialised values are assigned to 0 by default. This
> > >> implicit initialisation makes it impossible to distinguish between and
> > >> uninitialised and an explicitly 0-initialised ControlValue.
> > >>
> > >> Default construct the ControlValue in the ControlInfo default contructor to
> > >> explicitly represent uninitialised values by the ControlTypeNone type.
> > >>
> > >> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > >> ---
> > >>   include/libcamera/controls.h | 6 +++---
> > >>   1 file changed, 3 insertions(+), 3 deletions(-)
> > >>
> > >> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > >> index ebc168fc..38d0a3e8 100644
> > >> --- a/include/libcamera/controls.h
> > >> +++ b/include/libcamera/controls.h
> > >> @@ -268,9 +268,9 @@ private:
> > >>   class ControlInfo
> > >>   {
> > >>   public:
> > >> -       explicit ControlInfo(const ControlValue &min = 0,
> > >> -                            const ControlValue &max = 0,
> > >> -                            const ControlValue &def = 0);
> > >> +       explicit ControlInfo(const ControlValue &min = {},
> > >> +                            const ControlValue &max = {},
> > >> +                            const ControlValue &def = {});
> > >>          explicit ControlInfo(Span<const ControlValue> values,
> > >>                               const ControlValue &def = {});
> > >>          explicit ControlInfo(std::set<bool> values, bool def);
> > >> --
> > >> 2.34.1
> > >>
> >
Laurent Pinchart Oct. 4, 2022, 7:50 p.m. UTC | #14
Hi Kieran,

(CC'ing Hans Verkuil, in case he would happen to know of someone
interested in working on the vimc driver)

On Tue, Sep 27, 2022 at 01:03:32PM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Christian Rauch via libcamera-devel (2022-09-04 17:10:08)
> > Am 31.08.22 um 23:07 schrieb Kieran Bingham:
> > > Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
> > >> Hi Kieran,
> > >>
> > >> This test (and others) are skipped with "exit status 77". When I try to
> > >> run "test/serialization/ipa_data_serializer_test" manually, it fails
> > >> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
> > >> a virtual device. Is it somewhere documented how this is created? Could
> > >> this be created automatically for convenience?
> > >
> > > To load the modules for testing:
> > >
> > > sudo modprobe vimc
> > > sudo modprobe vim2m
> > > sudo modrpobe vivid
> > >
> > > We should really find somewhere suitable to document that.
> > 
> > I managed to create that virtual device. This only exposes BGR888 and
> > RGB888 formats. Is it possible to simulate other formats, such as YUYV
> > or NV21?
> 
> Not with VIMC. I'd like to see more formats supported by VIMC - but that
> requires kernel work.

I second that, it would be *really* useful.

> However, another option is the Vivid pipeline handler. This is a
> pipeline handler for the vivid kernel test device. We keep this 'out of
> tree' from libcamera, because it's not supposed to be a supported
> device.
> 
> It is however really useful because the vivid device can generate all,
> if not certainly most of the formats supported by V4L2.
> 
> To use it, merge the vivid pipeline handler into libcamera from:
> 
>   https://git.libcamera.org/libcamera/vivid.git/log/
> 
> This will then provide you with a new 'vivid' camera which can generate
> many more formats.
> 
> It's constantly rebased with my local build scripts to maintain it on
> top of tree, and also serves as some 'patch' documentation as to how to
> add and introduce a new pipeline handler.
> 
> > >> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> > >>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> > >>>> The default ControlInfo constructor allows to partially initialised the
> > >>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
> > >>>> implicit initialisation makes it impossible to distinguish between and
> > >>>> uninitialised and an explicitly 0-initialised ControlValue.
> > >>>>
> > >>>> Default construct the ControlValue in the ControlInfo default contructor to
> > >>>> explicitly represent uninitialised values by the ControlTypeNone type.
> > >>>
> > >>> This sounds ... a lot better !
> > >>>
> > >>> Testing with this series applied (including the patch 2/2, which should
> > >>> probably be squashed to one patch to maintain bisection) has the
> > >>> following failures for me:
> > >>>
> > >>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> > >>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> > >>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> > >>> stderr:
> > >>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> > >>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> > >>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> > >>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> > >>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> > >>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> > >>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> > >>> lhs:
> > >>> - 1: [false..true]
> > >>> - 7: [0..999999]
> > >>> - 8: [1.000000..32.000000]
> > >>> - 9: [-1.000000..1.000000]
> > >>> - 15: [0.000000..32.000000]
> > >>> rhs:
> > >>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> > >>>
> > >>> So I think we have to identify what impact this has on control
> > >>> serialisation. I expect we haven't tried to serialize ControlTypeNone
> > >>> before.
> > >>>
> > >>> But I think this is a good path to take currently.
> > >>>
> > >>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> > >>>> ---
> > >>>>  include/libcamera/controls.h | 6 +++---
> > >>>>  1 file changed, 3 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> > >>>> index ebc168fc..38d0a3e8 100644
> > >>>> --- a/include/libcamera/controls.h
> > >>>> +++ b/include/libcamera/controls.h
> > >>>> @@ -268,9 +268,9 @@ private:
> > >>>>  class ControlInfo
> > >>>>  {
> > >>>>  public:
> > >>>> -       explicit ControlInfo(const ControlValue &min = 0,
> > >>>> -                            const ControlValue &max = 0,
> > >>>> -                            const ControlValue &def = 0);
> > >>>> +       explicit ControlInfo(const ControlValue &min = {},
> > >>>> +                            const ControlValue &max = {},
> > >>>> +                            const ControlValue &def = {});
> > >>>>         explicit ControlInfo(Span<const ControlValue> values,
> > >>>>                              const ControlValue &def = {});
> > >>>>         explicit ControlInfo(std::set<bool> values, bool def);
Christian Rauch Oct. 8, 2022, 12:08 p.m. UTC | #15
Hi Kieran,

Am 27.09.22 um 14:03 schrieb Kieran Bingham:
> Hi Christian,
>
> Sorry - I missed this mail before.
>
> Quoting Christian Rauch via libcamera-devel (2022-09-04 17:10:08)
>> Hi Kieran,
>>
>> Am 31.08.22 um 23:07 schrieb Kieran Bingham:
>>> Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
>>>> Hi Kieran,
>>>>
>>>> This test (and others) are skipped with "exit status 77". When I try to
>>>> run "test/serialization/ipa_data_serializer_test" manually, it fails
>>>> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
>>>> a virtual device. Is it somewhere documented how this is created? Could
>>>> this be created automatically for convenience?
>>>
>>> To load the modules for testing:
>>>
>>> sudo modprobe vimc
>>> sudo modprobe vim2m
>>> sudo modrpobe vivid
>>>
>>> We should really find somewhere suitable to document that.
>>
>> I managed to create that virtual device. This only exposes BGR888 and
>> RGB888 formats. Is it possible to simulate other formats, such as YUYV
>> or NV21?
>
> Not with VIMC. I'd like to see more formats supported by VIMC - but that
> requires kernel work.
>
> However, another option is the Vivid pipeline handler. This is a
> pipeline handler for the vivid kernel test device. We keep this 'out of
> tree' from libcamera, because it's not supposed to be a supported
> device.
>
> It is however really useful because the vivid device can generate all,
> if not certainly most of the formats supported by V4L2.
>
> To use it, merge the vivid pipeline handler into libcamera from:
>
>    https://git.libcamera.org/libcamera/vivid.git/log/
>
> This will then provide you with a new 'vivid' camera which can generate
> many more formats.

I am probably doing something wrong. I checked out the 'vivid' branch
and just compiled with the default settings. I see that 'vivid' was
added to the 'pipelines' choices, but I don't see a new camera in 'cam'
or 'qcam'. Is there more documentation on how to use this?

>
> It's constantly rebased with my local build scripts to maintain it on
> top of tree, and also serves as some 'patch' documentation as to how to
> add and introduce a new pipeline handler.
>
> --
> Kieran
>
>
>>
>>> --
>>> Kieran
>>>
>>>
>>>>
>>>> Best,
>>>> Christian
>>>>
>>>>
>>>> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
>>>>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>>>>>> The default ControlInfo constructor allows to partially initialised the
>>>>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>>>>>> implicit initialisation makes it impossible to distinguish between and
>>>>>> uninitialised and an explicitly 0-initialised ControlValue.
>>>>>>
>>>>>> Default construct the ControlValue in the ControlInfo default contructor to
>>>>>> explicitly represent uninitialised values by the ControlTypeNone type.
>>>>>
>>>>> This sounds ... a lot better !
>>>>>
>>>>> Testing with this series applied (including the patch 2/2, which should
>>>>> probably be squashed to one patch to maintain bisection) has the
>>>>> following failures for me:
>>>>>
>>>>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
>>>>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>>>> stderr:
>>>>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
>>>>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
>>>>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
>>>>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
>>>>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
>>>>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
>>>>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
>>>>> lhs:
>>>>> - 1: [false..true]
>>>>> - 7: [0..999999]
>>>>> - 8: [1.000000..32.000000]
>>>>> - 9: [-1.000000..1.000000]
>>>>> - 15: [0.000000..32.000000]
>>>>> rhs:
>>>>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>>>>>
>>>>> So I think we have to identify what impact this has on control
>>>>> serialisation. I expect we haven't tried to serialize ControlTypeNone
>>>>> before.
>>>>>
>>>>> But I think this is a good path to take currently.
>>>>>
>>>>> --
>>>>> Kieran
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>>>> ---
>>>>>>   include/libcamera/controls.h | 6 +++---
>>>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>>>> index ebc168fc..38d0a3e8 100644
>>>>>> --- a/include/libcamera/controls.h
>>>>>> +++ b/include/libcamera/controls.h
>>>>>> @@ -268,9 +268,9 @@ private:
>>>>>>   class ControlInfo
>>>>>>   {
>>>>>>   public:
>>>>>> -       explicit ControlInfo(const ControlValue &min = 0,
>>>>>> -                            const ControlValue &max = 0,
>>>>>> -                            const ControlValue &def = 0);
>>>>>> +       explicit ControlInfo(const ControlValue &min = {},
>>>>>> +                            const ControlValue &max = {},
>>>>>> +                            const ControlValue &def = {});
>>>>>>          explicit ControlInfo(Span<const ControlValue> values,
>>>>>>                               const ControlValue &def = {});
>>>>>>          explicit ControlInfo(std::set<bool> values, bool def);
>>>>>> --
>>>>>> 2.34.1
>>>>>>
Laurent Pinchart Oct. 8, 2022, 1:33 p.m. UTC | #16
Hi Christian,

On Sat, Oct 08, 2022 at 02:08:27PM +0200, Christian Rauch via libcamera-devel wrote:
> Am 27.09.22 um 14:03 schrieb Kieran Bingham:
> > Quoting Christian Rauch via libcamera-devel (2022-09-04 17:10:08)
> >> Am 31.08.22 um 23:07 schrieb Kieran Bingham:
> >>> Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
> >>>> Hi Kieran,
> >>>>
> >>>> This test (and others) are skipped with "exit status 77". When I try to
> >>>> run "test/serialization/ipa_data_serializer_test" manually, it fails
> >>>> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
> >>>> a virtual device. Is it somewhere documented how this is created? Could
> >>>> this be created automatically for convenience?
> >>>
> >>> To load the modules for testing:
> >>>
> >>> sudo modprobe vimc
> >>> sudo modprobe vim2m
> >>> sudo modrpobe vivid
> >>>
> >>> We should really find somewhere suitable to document that.
> >>
> >> I managed to create that virtual device. This only exposes BGR888 and
> >> RGB888 formats. Is it possible to simulate other formats, such as YUYV
> >> or NV21?
> >
> > Not with VIMC. I'd like to see more formats supported by VIMC - but that
> > requires kernel work.
> >
> > However, another option is the Vivid pipeline handler. This is a
> > pipeline handler for the vivid kernel test device. We keep this 'out of
> > tree' from libcamera, because it's not supposed to be a supported
> > device.
> >
> > It is however really useful because the vivid device can generate all,
> > if not certainly most of the formats supported by V4L2.
> >
> > To use it, merge the vivid pipeline handler into libcamera from:
> >
> >    https://git.libcamera.org/libcamera/vivid.git/log/
> >
> > This will then provide you with a new 'vivid' camera which can generate
> > many more formats.
> 
> I am probably doing something wrong. I checked out the 'vivid' branch
> and just compiled with the default settings. I see that 'vivid' was
> added to the 'pipelines' choices, but I don't see a new camera in 'cam'
> or 'qcam'. Is there more documentation on how to use this?

Stupid question, have you loaded the vivid driver (modprobe vivid) ?

> > It's constantly rebased with my local build scripts to maintain it on
> > top of tree, and also serves as some 'patch' documentation as to how to
> > add and introduce a new pipeline handler.
> >
> >>>> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
> >>>>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
> >>>>>> The default ControlInfo constructor allows to partially initialised the
> >>>>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
> >>>>>> implicit initialisation makes it impossible to distinguish between and
> >>>>>> uninitialised and an explicitly 0-initialised ControlValue.
> >>>>>>
> >>>>>> Default construct the ControlValue in the ControlInfo default contructor to
> >>>>>> explicitly represent uninitialised values by the ControlTypeNone type.
> >>>>>
> >>>>> This sounds ... a lot better !
> >>>>>
> >>>>> Testing with this series applied (including the patch 2/2, which should
> >>>>> probably be squashed to one patch to maintain bisection) has the
> >>>>> following failures for me:
> >>>>>
> >>>>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
> >>>>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
> >>>>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
> >>>>> stderr:
> >>>>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
> >>>>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
> >>>>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
> >>>>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
> >>>>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
> >>>>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
> >>>>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
> >>>>> lhs:
> >>>>> - 1: [false..true]
> >>>>> - 7: [0..999999]
> >>>>> - 8: [1.000000..32.000000]
> >>>>> - 9: [-1.000000..1.000000]
> >>>>> - 15: [0.000000..32.000000]
> >>>>> rhs:
> >>>>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
> >>>>>
> >>>>> So I think we have to identify what impact this has on control
> >>>>> serialisation. I expect we haven't tried to serialize ControlTypeNone
> >>>>> before.
> >>>>>
> >>>>> But I think this is a good path to take currently.
> >>>>>
> >>>>> --
> >>>>> Kieran
> >>>>>
> >>>>>
> >>>>>>
> >>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
> >>>>>> ---
> >>>>>>   include/libcamera/controls.h | 6 +++---
> >>>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
> >>>>>>
> >>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
> >>>>>> index ebc168fc..38d0a3e8 100644
> >>>>>> --- a/include/libcamera/controls.h
> >>>>>> +++ b/include/libcamera/controls.h
> >>>>>> @@ -268,9 +268,9 @@ private:
> >>>>>>   class ControlInfo
> >>>>>>   {
> >>>>>>   public:
> >>>>>> -       explicit ControlInfo(const ControlValue &min = 0,
> >>>>>> -                            const ControlValue &max = 0,
> >>>>>> -                            const ControlValue &def = 0);
> >>>>>> +       explicit ControlInfo(const ControlValue &min = {},
> >>>>>> +                            const ControlValue &max = {},
> >>>>>> +                            const ControlValue &def = {});
> >>>>>>          explicit ControlInfo(Span<const ControlValue> values,
> >>>>>>                               const ControlValue &def = {});
> >>>>>>          explicit ControlInfo(std::set<bool> values, bool def);
Christian Rauch Oct. 9, 2022, 10:19 a.m. UTC | #17
Hi Laurent,

Am 08.10.22 um 15:33 schrieb Laurent Pinchart:
> Hi Christian,
>
> On Sat, Oct 08, 2022 at 02:08:27PM +0200, Christian Rauch via libcamera-devel wrote:
>> Am 27.09.22 um 14:03 schrieb Kieran Bingham:
>>> Quoting Christian Rauch via libcamera-devel (2022-09-04 17:10:08)
>>>> Am 31.08.22 um 23:07 schrieb Kieran Bingham:
>>>>> Quoting Christian Rauch via libcamera-devel (2022-08-31 21:45:31)
>>>>>> Hi Kieran,
>>>>>>
>>>>>> This test (and others) are skipped with "exit status 77". When I try to
>>>>>> run "test/serialization/ipa_data_serializer_test" manually, it fails
>>>>>> with "Can not find 'platform/vimc.0 Sensor B' camera". This seems to be
>>>>>> a virtual device. Is it somewhere documented how this is created? Could
>>>>>> this be created automatically for convenience?
>>>>>
>>>>> To load the modules for testing:
>>>>>
>>>>> sudo modprobe vimc
>>>>> sudo modprobe vim2m
>>>>> sudo modrpobe vivid
>>>>>
>>>>> We should really find somewhere suitable to document that.
>>>>
>>>> I managed to create that virtual device. This only exposes BGR888 and
>>>> RGB888 formats. Is it possible to simulate other formats, such as YUYV
>>>> or NV21?
>>>
>>> Not with VIMC. I'd like to see more formats supported by VIMC - but that
>>> requires kernel work.
>>>
>>> However, another option is the Vivid pipeline handler. This is a
>>> pipeline handler for the vivid kernel test device. We keep this 'out of
>>> tree' from libcamera, because it's not supposed to be a supported
>>> device.
>>>
>>> It is however really useful because the vivid device can generate all,
>>> if not certainly most of the formats supported by V4L2.
>>>
>>> To use it, merge the vivid pipeline handler into libcamera from:
>>>
>>>     https://git.libcamera.org/libcamera/vivid.git/log/
>>>
>>> This will then provide you with a new 'vivid' camera which can generate
>>> many more formats.
>>
>> I am probably doing something wrong. I checked out the 'vivid' branch
>> and just compiled with the default settings. I see that 'vivid' was
>> added to the 'pipelines' choices, but I don't see a new camera in 'cam'
>> or 'qcam'. Is there more documentation on how to use this?
>
> Stupid question, have you loaded the vivid driver (modprobe vivid) ?

No :-) I thought this was only required for the 'vimc' tests. It
correctly lists a "Virtual Video Device" after loading the kernel
module. I guess a development/debugging documentation would be really
useful :-)

>
>>> It's constantly rebased with my local build scripts to maintain it on
>>> top of tree, and also serves as some 'patch' documentation as to how to
>>> add and introduce a new pipeline handler.
>>>
>>>>>> Am 31.08.22 um 13:51 schrieb Kieran Bingham:
>>>>>>> Quoting Christian Rauch via libcamera-devel (2022-08-30 21:21:23)
>>>>>>>> The default ControlInfo constructor allows to partially initialised the
>>>>>>>> min/max/def values. Uninitialised values are assigned to 0 by default. This
>>>>>>>> implicit initialisation makes it impossible to distinguish between and
>>>>>>>> uninitialised and an explicitly 0-initialised ControlValue.
>>>>>>>>
>>>>>>>> Default construct the ControlValue in the ControlInfo default contructor to
>>>>>>>> explicitly represent uninitialised values by the ControlTypeNone type.
>>>>>>>
>>>>>>> This sounds ... a lot better !
>>>>>>>
>>>>>>> Testing with this series applied (including the patch 2/2, which should
>>>>>>> probably be squashed to one patch to maintain bisection) has the
>>>>>>> following failures for me:
>>>>>>>
>>>>>>> 27/70 libcamera:serialization / ipa_data_serializer_test                 FAIL            0.02s   (exit status 255 or signal 127 SIGinvalid)
>>>>>>>>>> MALLOC_PERTURB_=174 /home/kbingham/iob/libcamera/libcamera/build/gcc/test/serialization/ipa_data_serializer_test
>>>>>>> ―――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――― ✀  ――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――――
>>>>>>> stderr:
>>>>>>> [148:04:16.561414604] [830675]  INFO IPAManager ipa_manager.cpp:141 libcamera is not installed. Adding '/home/kbingham/iob/libcamera/libcamera/build/gcc/src/ipa' to the IPA search path
>>>>>>> [148:04:16.562718193] [830675]  INFO Camera camera_manager.cpp:293 libcamera v0.0.0+3856-6478a395
>>>>>>> [148:04:16.567590564] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:174 No static properties available for 'Sensor B'
>>>>>>> [148:04:16.567599841] [830679]  WARN CameraSensorProperties camera_sensor_properties.cpp:176 Please consider updating the camera sensor properties database
>>>>>>> [148:04:16.567605382] [830679]  WARN CameraSensor camera_sensor.cpp:411 'Sensor B': Failed to retrieve the camera location
>>>>>>> [148:04:16.568583439] [830679]  INFO IPAProxy ipa_proxy.cpp:130 libcamera is not installed. Loading IPA configuration from '/home/kbingham/iob/libcamera/libcamera/src/ipa/vimc/data'
>>>>>>> [148:04:16.569025699] [830675] ERROR Serializer control_serializer.cpp:509 Bad data, entry offset mismatch (entry 1)
>>>>>>> lhs:
>>>>>>> - 1: [false..true]
>>>>>>> - 7: [0..999999]
>>>>>>> - 8: [1.000000..32.000000]
>>>>>>> - 9: [-1.000000..1.000000]
>>>>>>> - 15: [0.000000..32.000000]
>>>>>>> rhs:
>>>>>>> Deserialized std::vector<libcamera::ControlInfoMap> doesn't match original
>>>>>>>
>>>>>>> So I think we have to identify what impact this has on control
>>>>>>> serialisation. I expect we haven't tried to serialize ControlTypeNone
>>>>>>> before.
>>>>>>>
>>>>>>> But I think this is a good path to take currently.
>>>>>>>
>>>>>>> --
>>>>>>> Kieran
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: Christian Rauch <Rauch.Christian@gmx.de>
>>>>>>>> ---
>>>>>>>>    include/libcamera/controls.h | 6 +++---
>>>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
>>>>>>>> index ebc168fc..38d0a3e8 100644
>>>>>>>> --- a/include/libcamera/controls.h
>>>>>>>> +++ b/include/libcamera/controls.h
>>>>>>>> @@ -268,9 +268,9 @@ private:
>>>>>>>>    class ControlInfo
>>>>>>>>    {
>>>>>>>>    public:
>>>>>>>> -       explicit ControlInfo(const ControlValue &min = 0,
>>>>>>>> -                            const ControlValue &max = 0,
>>>>>>>> -                            const ControlValue &def = 0);
>>>>>>>> +       explicit ControlInfo(const ControlValue &min = {},
>>>>>>>> +                            const ControlValue &max = {},
>>>>>>>> +                            const ControlValue &def = {});
>>>>>>>>           explicit ControlInfo(Span<const ControlValue> values,
>>>>>>>>                                const ControlValue &def = {});
>>>>>>>>           explicit ControlInfo(std::set<bool> values, bool def);
>

Patch
diff mbox series

diff --git a/include/libcamera/controls.h b/include/libcamera/controls.h
index ebc168fc..38d0a3e8 100644
--- a/include/libcamera/controls.h
+++ b/include/libcamera/controls.h
@@ -268,9 +268,9 @@  private:
 class ControlInfo
 {
 public:
-	explicit ControlInfo(const ControlValue &min = 0,
-			     const ControlValue &max = 0,
-			     const ControlValue &def = 0);
+	explicit ControlInfo(const ControlValue &min = {},
+			     const ControlValue &max = {},
+			     const ControlValue &def = {});
 	explicit ControlInfo(Span<const ControlValue> values,
 			     const ControlValue &def = {});
 	explicit ControlInfo(std::set<bool> values, bool def);