[libcamera-devel,v4,4/6] libcamera: CameraSensor: Enable retrieving supported test pattern modes
diff mbox series

Message ID 20210506075449.1761752-5-hiroh@chromium.org
State Superseded
Headers show
Series
  • [libcamera-devel,v4,1/6] libcamera: controls: Add sensor test pattern mode
Related show

Commit Message

Hirokazu Honda May 6, 2021, 7:54 a.m. UTC 
This enables retrieving supported test pattern modes through
CameraSensorInfo.

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 include/libcamera/internal/camera_sensor.h |  5 ++++
 src/libcamera/camera_sensor.cpp            | 28 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)

Comments

Jacopo Mondi May 13, 2021, 10:17 a.m. UTC | #1 
Hi Hiro,

On Thu, May 06, 2021 at 04:54:47PM +0900, Hirokazu Honda wrote:
> This enables retrieving supported test pattern modes through
> CameraSensorInfo.

I'm afraid this hits a shortcoming of the current CameraSensor class.

In order:
- The CameraSensor class accepts lists V4L2 controls in get/setControls
- The CameraSensor class exposes libcamera::properties through
  properties()
- The CameraSensor class exposes the -subdev- V4L2 controls through
  controls()
- The CameraSensor class allows to get a snapshot of the current
  sensor configuration through CameraSensorInfo (mostly for IPAs, in
  facts the structure definition is in the process of being moved to
  the IPA headers).

So we have quite some confusion there, and we need to design better
the control interface for the class.

In the meantime, items like test patters have not a real place where
they belong:

- We can't expose the raw V4L2 control indices and let the ph
  translate them to libcamera controls like we do with exposures, crop
  rectangles and such, as the translation requires accessing the
  sensor database, which is not (ideally) exposed to ph
- We can't model them as properties, the supported test patters are static
  information but they can be enabled/disabled, something we don't
  allow for properties (at least conceptually).
- CameraSensorInfo reports the current configuration parameters, it
  would be fair to report if test pattern is enabled or disabled, but
  the list of supported patterns does not really belong there.

we've discussed the issue with the group, and for the time being it is
probably easier to create and ad-hoc methods along the lines of
CameraSensor::testPatternModes() to retrieve them. It's not ideal but
it's probably the easiest decision to safely back-track later on.

Sorry for conflicting directions, the CameraSensor class control
interface will need to be re-thought with this new use-case in mind.

Thanks
   j


>
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  5 ++++
>  src/libcamera/camera_sensor.cpp            | 28 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 3fa3a419..115e266d 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -38,6 +38,8 @@ struct CameraSensorInfo {
>
>  	uint32_t minFrameLength;
>  	uint32_t maxFrameLength;
> +
> +	std::vector<int32_t> testPatternModes;
>  };
>
>  class CameraSensor : protected Loggable
> @@ -79,6 +81,8 @@ private:
>  	void initVimcDefaultProperties();
>  	void initStaticProperties();
>  	int initProperties();
> +	void initTestPatternModes(
> +		const std::map<int32_t, int32_t> &testPatternModeMap);
>
>  	const MediaEntity *entity_;
>  	std::unique_ptr<V4L2Subdevice> subdev_;
> @@ -90,6 +94,7 @@ private:
>  	V4L2Subdevice::Formats formats_;
>  	std::vector<unsigned int> mbusCodes_;
>  	std::vector<Size> sizes_;
> +	std::vector<int32_t> testPatternModes_;
>
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 1db263cf..eedd2f89 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
>  	activeArea_ = Rectangle(pixelArraySize_);
>  }
>
> +void CameraSensor::initTestPatternModes(
> +	const std::map<int32_t, int32_t> &testPatternModeMap)
> +{
> +	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
> +	if (v4l2TestPattern == controls().end()) {
> +		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
> +					 << model() << "\'";
> +		return;
> +	}
> +
> +	for (const ControlValue &value : v4l2TestPattern->second.values()) {
> +		const int32_t index = value.get<int32_t>();
> +
> +		const auto it = testPatternModeMap.find(index);
> +		if (it != testPatternModeMap.end()) {
> +			LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +						 << index << ") ignored";
> +			continue;
> +		}
> +
> +		LOG(CameraSensor, Debug) << "Test pattern mode (index="
> +					 << index << ") added";
> +		testPatternModes_.push_back(it->second);
> +	}
> +}
> +
>  void CameraSensor::initStaticProperties()
>  {
>  	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
>
>  	/* Register the properties retrieved from the sensor database. */
>  	properties_.set(properties::UnitCellSize, props->unitCellSize);
> +
> +	initTestPatternModes(props->testPatternModeMap);
>  }
>
>  int CameraSensor::initProperties()
> --
> 2.31.1.607.g51e8a6a459-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda May 14, 2021, 3:19 a.m. UTC | #2 
Hi Jacopo,

On Thu, May 13, 2021 at 7:17 PM Jacopo Mondi <jacopo@jmondi.org> wrote:

> Hi Hiro,
>
> On Thu, May 06, 2021 at 04:54:47PM +0900, Hirokazu Honda wrote:
> > This enables retrieving supported test pattern modes through
> > CameraSensorInfo.
>
> I'm afraid this hits a shortcoming of the current CameraSensor class.
>
> In order:
> - The CameraSensor class accepts lists V4L2 controls in get/setControls
> - The CameraSensor class exposes libcamera::properties through
>   properties()
> - The CameraSensor class exposes the -subdev- V4L2 controls through
>   controls()
> - The CameraSensor class allows to get a snapshot of the current
>   sensor configuration through CameraSensorInfo (mostly for IPAs, in
>   facts the structure definition is in the process of being moved to
>   the IPA headers).
>
> So we have quite some confusion there, and we need to design better
> the control interface for the class.
>
> In the meantime, items like test patters have not a real place where
> they belong:
>
> - We can't expose the raw V4L2 control indices and let the ph
>   translate them to libcamera controls like we do with exposures, crop
>   rectangles and such, as the translation requires accessing the
>   sensor database, which is not (ideally) exposed to ph
> - We can't model them as properties, the supported test patters are static
>   information but they can be enabled/disabled, something we don't
>   allow for properties (at least conceptually).
> - CameraSensorInfo reports the current configuration parameters, it
>   would be fair to report if test pattern is enabled or disabled, but
>   the list of supported patterns does not really belong there.
>
> we've discussed the issue with the group, and for the time being it is
> probably easier to create and ad-hoc methods along the lines of
> CameraSensor::testPatternModes() to retrieve them. It's not ideal but
> it's probably the easiest decision to safely back-track later on.
>
> Sorry for conflicting directions, the CameraSensor class control
> interface will need to be re-thought with this new use-case in mind.
>
>
I see. Can you review other patches in this series assuming
CameraSensor::testPatternModes()?

 -Hiro

Thanks
>    j
>
>
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  5 ++++
> >  src/libcamera/camera_sensor.cpp            | 28 ++++++++++++++++++++++
> >  2 files changed, 33 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h
> b/include/libcamera/internal/camera_sensor.h
> > index 3fa3a419..115e266d 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -38,6 +38,8 @@ struct CameraSensorInfo {
> >
> >       uint32_t minFrameLength;
> >       uint32_t maxFrameLength;
> > +
> > +     std::vector<int32_t> testPatternModes;
> >  };
> >
> >  class CameraSensor : protected Loggable
> > @@ -79,6 +81,8 @@ private:
> >       void initVimcDefaultProperties();
> >       void initStaticProperties();
> >       int initProperties();
> > +     void initTestPatternModes(
> > +             const std::map<int32_t, int32_t> &testPatternModeMap);
> >
> >       const MediaEntity *entity_;
> >       std::unique_ptr<V4L2Subdevice> subdev_;
> > @@ -90,6 +94,7 @@ private:
> >       V4L2Subdevice::Formats formats_;
> >       std::vector<unsigned int> mbusCodes_;
> >       std::vector<Size> sizes_;
> > +     std::vector<int32_t> testPatternModes_;
> >
> >       Size pixelArraySize_;
> >       Rectangle activeArea_;
> > diff --git a/src/libcamera/camera_sensor.cpp
> b/src/libcamera/camera_sensor.cpp
> > index 1db263cf..eedd2f89 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -408,6 +408,32 @@ void CameraSensor::initVimcDefaultProperties()
> >       activeArea_ = Rectangle(pixelArraySize_);
> >  }
> >
> > +void CameraSensor::initTestPatternModes(
> > +     const std::map<int32_t, int32_t> &testPatternModeMap)
> > +{
> > +     const auto &v4l2TestPattern =
> controls().find(V4L2_CID_TEST_PATTERN);
> > +     if (v4l2TestPattern == controls().end()) {
> > +             LOG(CameraSensor, Debug) << "No static test pattern map
> for \'"
> > +                                      << model() << "\'";
> > +             return;
> > +     }
> > +
> > +     for (const ControlValue &value : v4l2TestPattern->second.values())
> {
> > +             const int32_t index = value.get<int32_t>();
> > +
> > +             const auto it = testPatternModeMap.find(index);
> > +             if (it != testPatternModeMap.end()) {
> > +                     LOG(CameraSensor, Debug) << "Test pattern mode
> (index="
> > +                                              << index << ") ignored";
> > +                     continue;
> > +             }
> > +
> > +             LOG(CameraSensor, Debug) << "Test pattern mode (index="
> > +                                      << index << ") added";
> > +             testPatternModes_.push_back(it->second);
> > +     }
> > +}
> > +
> >  void CameraSensor::initStaticProperties()
> >  {
> >       const CameraSensorProperties *props =
> CameraSensorProperties::get(model_);
> > @@ -416,6 +442,8 @@ void CameraSensor::initStaticProperties()
> >
> >       /* Register the properties retrieved from the sensor database. */
> >       properties_.set(properties::UnitCellSize, props->unitCellSize);
> > +
> > +     initTestPatternModes(props->testPatternModeMap);
> >  }
> >
> >  int CameraSensor::initProperties()
> > --
> > 2.31.1.607.g51e8a6a459-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 3fa3a419..115e266d 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -38,6 +38,8 @@  struct CameraSensorInfo {
 
 	uint32_t minFrameLength;
 	uint32_t maxFrameLength;
+
+	std::vector<int32_t> testPatternModes;
 };
 
 class CameraSensor : protected Loggable
@@ -79,6 +81,8 @@  private:
 	void initVimcDefaultProperties();
 	void initStaticProperties();
 	int initProperties();
+	void initTestPatternModes(
+		const std::map<int32_t, int32_t> &testPatternModeMap);
 
 	const MediaEntity *entity_;
 	std::unique_ptr<V4L2Subdevice> subdev_;
@@ -90,6 +94,7 @@  private:
 	V4L2Subdevice::Formats formats_;
 	std::vector<unsigned int> mbusCodes_;
 	std::vector<Size> sizes_;
+	std::vector<int32_t> testPatternModes_;
 
 	Size pixelArraySize_;
 	Rectangle activeArea_;
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 1db263cf..eedd2f89 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -408,6 +408,32 @@  void CameraSensor::initVimcDefaultProperties()
 	activeArea_ = Rectangle(pixelArraySize_);
 }
 
+void CameraSensor::initTestPatternModes(
+	const std::map<int32_t, int32_t> &testPatternModeMap)
+{
+	const auto &v4l2TestPattern = controls().find(V4L2_CID_TEST_PATTERN);
+	if (v4l2TestPattern == controls().end()) {
+		LOG(CameraSensor, Debug) << "No static test pattern map for \'"
+					 << model() << "\'";
+		return;
+	}
+
+	for (const ControlValue &value : v4l2TestPattern->second.values()) {
+		const int32_t index = value.get<int32_t>();
+
+		const auto it = testPatternModeMap.find(index);
+		if (it != testPatternModeMap.end()) {
+			LOG(CameraSensor, Debug) << "Test pattern mode (index="
+						 << index << ") ignored";
+			continue;
+		}
+
+		LOG(CameraSensor, Debug) << "Test pattern mode (index="
+					 << index << ") added";
+		testPatternModes_.push_back(it->second);
+	}
+}
+
 void CameraSensor::initStaticProperties()
 {
 	const CameraSensorProperties *props = CameraSensorProperties::get(model_);
@@ -416,6 +442,8 @@  void CameraSensor::initStaticProperties()
 
 	/* Register the properties retrieved from the sensor database. */
 	properties_.set(properties::UnitCellSize, props->unitCellSize);
+
+	initTestPatternModes(props->testPatternModeMap);
 }
 
 int CameraSensor::initProperties()