| Message ID | 20211202140317.3118364-5-hanlinchen@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Han-Lin, Thanks, I think this is a better place/way to model the Lens. Quoting Han-Lin Chen (2021-12-02 14:03:16) > Add CameraLens as a member of CameraSenosr. The patch does not implement how /CameraSenosr/CameraSensor/ > to link the lens to the specific sensor yet. Only to provide an interface for > pipeline handler's usage. And I think that's fine while we wait for the links to land. It means the plumbing is in place, and a CameraSensor just needs to construct/initialise a CameraLens when it is constructed based on its links. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > include/libcamera/internal/camera_sensor.h | 5 +++++ > src/libcamera/camera_sensor.cpp | 8 ++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index d25a1165..8e97a80e 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -24,6 +24,7 @@ > namespace libcamera { > > class BayerFormat; > +class CameraLens; > class MediaEntity; > > class CameraSensor : protected Loggable > @@ -60,6 +61,8 @@ public: > > void updateControlInfo(); > > + CameraLens *lens() { return lens_.get(); } > + > protected: > std::string logPrefix() const override; > > @@ -91,6 +94,8 @@ private: > const BayerFormat *bayerFormat_; > > ControlList properties_; > + > + std::unique_ptr<CameraLens> lens_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 6151b32e..b386e7b0 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -20,6 +20,7 @@ > #include <libcamera/base/utils.h> > > #include "libcamera/internal/bayer_format.h" > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor_properties.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/sysfs.h" > @@ -787,6 +788,13 @@ void CameraSensor::updateControlInfo() > subdev_->updateControlInfo(); > } > > +/** > + * \fn CameraSensor::lens() > + * \brief Retrieve the lens controller > + * > + * \return The lens controller. nullptr if no lens is connected to the sensor > + */ > + > std::string CameraSensor::logPrefix() const > { > return "'" + entity_->name() + "'"; > -- > 2.34.1.400.ga245620fadb-goog >
Hello, On 12/2/21 8:08 PM, Kieran Bingham wrote: > Hi Han-Lin, > > Thanks, I think this is a better place/way to model the Lens. Yeah, I was thinking of Pipeline-handlers but this seems okay to me as well, unless some pipeline-handler wants to manage it on its own (quirks). > > Quoting Han-Lin Chen (2021-12-02 14:03:16) >> Add CameraLens as a member of CameraSenosr. The patch does not implement how > /CameraSenosr/CameraSensor/ > >> to link the lens to the specific sensor yet. Only to provide an interface for >> pipeline handler's usage. > And I think that's fine while we wait for the links to land. It means > the plumbing is in place, and a CameraSensor just needs to > construct/initialise a CameraLens when it is constructed based on its > links. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > >> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> >> --- >> include/libcamera/internal/camera_sensor.h | 5 +++++ >> src/libcamera/camera_sensor.cpp | 8 ++++++++ >> 2 files changed, 13 insertions(+) >> >> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h >> index d25a1165..8e97a80e 100644 >> --- a/include/libcamera/internal/camera_sensor.h >> +++ b/include/libcamera/internal/camera_sensor.h >> @@ -24,6 +24,7 @@ >> namespace libcamera { >> >> class BayerFormat; >> +class CameraLens; >> class MediaEntity; >> >> class CameraSensor : protected Loggable >> @@ -60,6 +61,8 @@ public: >> >> void updateControlInfo(); >> >> + CameraLens *lens() { return lens_.get(); } >> + >> protected: >> std::string logPrefix() const override; >> >> @@ -91,6 +94,8 @@ private: >> const BayerFormat *bayerFormat_; >> >> ControlList properties_; >> + >> + std::unique_ptr<CameraLens> lens_; >> }; >> >> } /* namespace libcamera */ >> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp >> index 6151b32e..b386e7b0 100644 >> --- a/src/libcamera/camera_sensor.cpp >> +++ b/src/libcamera/camera_sensor.cpp >> @@ -20,6 +20,7 @@ >> #include <libcamera/base/utils.h> >> >> #include "libcamera/internal/bayer_format.h" >> +#include "libcamera/internal/camera_lens.h" >> #include "libcamera/internal/camera_sensor_properties.h" >> #include "libcamera/internal/formats.h" >> #include "libcamera/internal/sysfs.h" >> @@ -787,6 +788,13 @@ void CameraSensor::updateControlInfo() >> subdev_->updateControlInfo(); >> } >> >> +/** >> + * \fn CameraSensor::lens() >> + * \brief Retrieve the lens controller >> + * >> + * \return The lens controller. nullptr if no lens is connected to the sensor >> + */ >> + >> std::string CameraSensor::logPrefix() const >> { >> return "'" + entity_->name() + "'"; >> -- >> 2.34.1.400.ga245620fadb-goog >>
Hi Han-lin, Thank you for the patch. On Thu, Dec 02, 2021 at 10:03:16PM +0800, Han-Lin Chen wrote: > Add CameraLens as a member of CameraSenosr. The patch does not implement how s/CameraSenosr/CameraSensor/ > to link the lens to the specific sensor yet. Only to provide an interface for > pipeline handler's usage. It's a good idea, it can be merged right away, and allows rebasing ongoing developments on top. > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > include/libcamera/internal/camera_sensor.h | 5 +++++ > src/libcamera/camera_sensor.cpp | 8 ++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h > index d25a1165..8e97a80e 100644 > --- a/include/libcamera/internal/camera_sensor.h > +++ b/include/libcamera/internal/camera_sensor.h > @@ -24,6 +24,7 @@ > namespace libcamera { > > class BayerFormat; > +class CameraLens; > class MediaEntity; > > class CameraSensor : protected Loggable > @@ -60,6 +61,8 @@ public: > > void updateControlInfo(); > > + CameraLens *lens() { return lens_.get(); } > + > protected: > std::string logPrefix() const override; > > @@ -91,6 +94,8 @@ private: > const BayerFormat *bayerFormat_; > > ControlList properties_; > + > + std::unique_ptr<CameraLens> lens_; > }; > > } /* namespace libcamera */ > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp > index 6151b32e..b386e7b0 100644 > --- a/src/libcamera/camera_sensor.cpp > +++ b/src/libcamera/camera_sensor.cpp > @@ -20,6 +20,7 @@ > #include <libcamera/base/utils.h> > > #include "libcamera/internal/bayer_format.h" > +#include "libcamera/internal/camera_lens.h" > #include "libcamera/internal/camera_sensor_properties.h" > #include "libcamera/internal/formats.h" > #include "libcamera/internal/sysfs.h" > @@ -787,6 +788,13 @@ void CameraSensor::updateControlInfo() > subdev_->updateControlInfo(); > } > > +/** > + * \fn CameraSensor::lens() I wonder if we should call this focusLens() (and focusLens_, as well as s/lens/focus lens/ in the rest of this comment block) as zoom lenses are in scope too. > + * \brief Retrieve the lens controller > + * > + * \return The lens controller. nullptr if no lens is connected to the sensor s/no lens/no lens controller/ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + */ > + > std::string CameraSensor::logPrefix() const > { > return "'" + entity_->name() + "'";
diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h index d25a1165..8e97a80e 100644 --- a/include/libcamera/internal/camera_sensor.h +++ b/include/libcamera/internal/camera_sensor.h @@ -24,6 +24,7 @@ namespace libcamera { class BayerFormat; +class CameraLens; class MediaEntity; class CameraSensor : protected Loggable @@ -60,6 +61,8 @@ public: void updateControlInfo(); + CameraLens *lens() { return lens_.get(); } + protected: std::string logPrefix() const override; @@ -91,6 +94,8 @@ private: const BayerFormat *bayerFormat_; ControlList properties_; + + std::unique_ptr<CameraLens> lens_; }; } /* namespace libcamera */ diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index 6151b32e..b386e7b0 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -20,6 +20,7 @@ #include <libcamera/base/utils.h> #include "libcamera/internal/bayer_format.h" +#include "libcamera/internal/camera_lens.h" #include "libcamera/internal/camera_sensor_properties.h" #include "libcamera/internal/formats.h" #include "libcamera/internal/sysfs.h" @@ -787,6 +788,13 @@ void CameraSensor::updateControlInfo() subdev_->updateControlInfo(); } +/** + * \fn CameraSensor::lens() + * \brief Retrieve the lens controller + * + * \return The lens controller. nullptr if no lens is connected to the sensor + */ + std::string CameraSensor::logPrefix() const { return "'" + entity_->name() + "'";
Add CameraLens as a member of CameraSenosr. The patch does not implement how to link the lens to the specific sensor yet. Only to provide an interface for pipeline handler's usage. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- include/libcamera/internal/camera_sensor.h | 5 +++++ src/libcamera/camera_sensor.cpp | 8 ++++++++ 2 files changed, 13 insertions(+)