[libcamera-devel] [PATCH v4 4/6] libcamera: CameraSensor: Enable retrieving supported test pattern modes

[Hirokazu Honda]

Hi Jacopo,

On Thu, May 13, 2021 at 7:17 PM Jacopo Mondi <jacopo at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210514/c407f7f4/attachment-0001.htm>