| Message ID | 20201021002418.21764-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On Wed, Oct 21, 2020 at 03:24:17AM +0300, Laurent Pinchart wrote: > Initialize the CameraData properties with the properties exposed by the > sensor. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, Thanks for your work. On 2020-10-21 03:24:17 +0300, Laurent Pinchart wrote: > Initialize the CameraData properties with the properties exposed by the > sensor. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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. Tested-by: Andrey Konovalov <andrey.konovalov@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@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; > } > >
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@1b0c000/i2c-bus@0/imx290@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@1b0c000/i2c-bus@0/imx290@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). Thanks, Andrey > Tested-by: Andrey Konovalov <andrey.konovalov@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@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; >> } >>
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@1b0c000/i2c-bus@0/imx290@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@1b0c000/i2c-bus@0/imx290@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(). Sorry, Andrey > Thanks, > Andrey > >> Tested-by: Andrey Konovalov <andrey.konovalov@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@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; >>> } >>>
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@1b0c000/i2c-bus@0/imx290@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@1b0c000/i2c-bus@0/imx290@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@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@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; > >>> } > >>>
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; }
Initialize the CameraData properties with the properties exposed by the sensor. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/pipeline/simple/simple.cpp | 2 ++ 1 file changed, 2 insertions(+)