[libcamera-devel] [PATCH 1/2] libcamera: pipeline: simple: Set camera properties

[Laurent Pinchart]

Hi Andrey,

On Wed, Oct 21, 2020 at 05:05:01PM +0300, Andrey Konovalov wrote:
> On 21.10.2020 16:59, Andrey Konovalov wrote:
> > On 21.10.2020 16:16, Andrey Konovalov wrote:
> >> Hi Laurent,
> >>
> >> Thank you for the patch!
> >> Now I can see (on my setup using simple pipeline):
> >> -----8<-----
> >> Property: Rotation = 0
> >> Property: Location = 0
> >> Property: Model = imx290
> >> -----8<-----
> >> added to the 'cam -c 1 -p' output.
> > 
> > Also 'cam -l' gives now:
> > -----8<-----
> > Available cameras:
> > 1: Internal front camera (/base/soc/cci at 1b0c000/i2c-bus at 0/imx290 at 1a)
> > -----8<-----
> > instead of:
> > -----8<-----
> > 1: [0:12:22.274562882] [877] ERROR Controls controls.cpp:957 Control 0x00000001 not found
> > Internal front camera (/base/soc/cci at 1b0c000/i2c-bus at 0/imx290 at 1a)
> > -----8<-----
> > before the patch.
> > 
> > The ERROR was due to:
> > CamApp::cameraName() -> ControlList::get(properties::Location) -> ControlList::find() with the
> > list of properties supported by the camera not initialized.
> > 
> > And the old behavior seems to reveal a bug in ControlList::get() - if int32 control is not found,
> > ControlList::get() returns 0 which can be a valid control value like "internal front camera" in the
> > properties::Location case).
> 
> After reading the ControlList::get() documentation, I see that undefined behaviour is documented for that
> case. This is CamApp which should check that the control is present in the list before calling get().

I agree, but in this specific case, the Location property is meant to be
mandatory, so I don't think cam needs to check for it. We haven't
documented the property as mandatory yet, so that part has to be fixed
:-)

> Sorry,

Nothing to be sorry about.

> >> Tested-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> >>
> >> Thanks,
> >> Andrey
> >>
> >> On 21.10.2020 03:24, Laurent Pinchart wrote:
> >>> Initialize the CameraData properties with the properties exposed by the
> >>> sensor.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>   src/libcamera/pipeline/simple/simple.cpp | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >>> index 8868a43beeb4..4b6f708e8fee 100644
> >>> --- a/src/libcamera/pipeline/simple/simple.cpp
> >>> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >>> @@ -324,6 +324,8 @@ int SimpleCameraData::init()
> >>>           return -EINVAL;
> >>>       }
> >>> +    properties_ = sensor_->properties();
> >>> +
> >>>       return 0;
> >>>   }
> >>>

-- 
Regards,

Laurent Pinchart