[libcamera-devel] [RFC PATCH v2 2/5] libcamera: camera_sensor: Enable to set a test pattern mode

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, Jun 23, 2021 at 10:07:04AM +0200, Jacopo Mondi wrote:
> On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:
> > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi wrote:
> > > On Tue, Jun 22, 2021 at 11:36:51AM +0900, Hirokazu Honda wrote:
> > > > Provides a function to set the camera sensor a test pattern mode.
> > > >
> > > > Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> > > > ---
> > > >  include/libcamera/internal/camera_sensor.h |  1 +
> > > >  src/libcamera/camera_sensor.cpp            | 39 ++++++++++++++++++++++
> > > >  2 files changed, 40 insertions(+)
> > > >
> > > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > > index e133ebf4..8b9f84c9 100644
> > > > --- a/include/libcamera/internal/camera_sensor.h
> > > > +++ b/include/libcamera/internal/camera_sensor.h
> > > > @@ -43,6 +43,7 @@ public:
> > > >       {
> > > >               return testPatternModes_;
> > > >       }
> > > > +     int setTestPatternMode(uint8_t testPatternMode);
> > > >
> > > >       V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > > >                                     const Size &size) const;
> > > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > > index 70bbd97a..ce8ff274 100644
> > > > --- a/src/libcamera/camera_sensor.cpp
> > > > +++ b/src/libcamera/camera_sensor.cpp
> > > > @@ -507,6 +507,45 @@ Size CameraSensor::resolution() const
> > > >   * \return The list of test pattern modes
> > > >   */
> > > >
> > > > +/**
> > > > + * \brief Set the camera sensor a specified controls::TestPatternMode
> > >
> > > Doxygen is puzzling me recently... the testPatternMode paramter is not
> > > documented but I don't get a warning :/
> > >
> > > Also, I'm a bit debated about where to actually place this function
> > > (or better, it's initTestPatternModes which is misplaced)
> > > if we have to respect the declaration order or the way you sorted them
> > > here (which is more logical) is better.
> > >
> > > > + *
> > > > + * \return 0 on success or a negative error code otherwise
> > > > + */
> > > > +int CameraSensor::setTestPatternMode(uint8_t testPatternMode)
> > > > +{
> > > > +     if (std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > > > +                   testPatternMode) == testPatternModes_.end()) {
> > > > +             LOG(CameraSensor, Error) << "Unsupported test pattern mode: "
> > > > +                                      << testPatternMode;
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > Do we need this ? The testPatternModes_ map is constructed using the
> > > camera sensor properties, the below find() on the props->testPatternModes
> > > map should gurantee that is supported. Also, if a test pattern mode is
> > > not reported as part of testPatternModes_ applications shouldn't set
> > > it...
> > >
> > > > +
> > > > +     const CameraSensorProperties *props = CameraSensorProperties::get(model_);
> > > > +     if (!props)
> > > > +             return -EINVAL;

We already retrieve this in CameraSensor::initStaticProperties(), let's
cache the pointer in the CameraSensor class.

> > > > +
> > > > +     auto it = props->testPatternModes.find(testPatternMode);
> > > > +     ASSERT(it != props->testPatternModes.end());
> > >
> > > Yeah, I think the assertion here is more than enough..
> > >
> > > > +     const uint8_t index = it->second;
> > > > +
> > > > +     ControlList ctrls = getControls({ V4L2_CID_TEST_PATTERN });
> > > > +     if (ctrls.empty()) {
> > > > +             LOG(CameraSensor, Error)
> > > > +                     << "Failed to retrieve test pattern control";
> > > > +             return -EINVAL;
> > > > +     }
> > >
> > > We won't register the TestPatternMode control in first place the V4L2
> > > control is not supported..
> >
> > Replace this with ASSERT.
> >
> > > > +
> > > > +     ControlValue value = ctrls.get(V4L2_CID_TEST_PATTERN);
> > > > +     if (value.get<int32_t>() == index)
> > > > +             return 0;
> > >
> > > The V4L2 control framework should take care of this by itself, I think
> > > we can set the control regardless
> >
> > Yes, I put this in order to save a redundant Ioctl call.
> > Do you want to remove that?
> 
> Well, the ctrls ControlList you create with getControls() goes through
> an IOCTL :) We're talking details, but if we want to retain the
> state of the testPatterMode we can make it a class member, so that the
> code can be reduced to (not tested pseudo-code)
> 
> {
>      if (testPatternMode == testPatternMode_)
>            return 0;
> 
>      const CameraSensorProperties *props = CameraSensorProperties::get(model_);
>      if (!props)
>              return -EINVAL;
> 
>      auto it = props->testPatternModes.find(testPatternMode);
>      ASSERT(it != props->testPatternModes.end());
> 
>      ControlList ctrls(controls());
>      ctrls.set(V4L2_CID_TEST_PATTERN, it->second);
> 
>      int ret = setControls(&ctrls);
>      if (ret)
>             return ret;
> 
>      testPatternMode_ = testPatternMode;
> 
>      return 0;
> }
> 
> I also wonder if the ASSERT() is not too harsh and we shouldn't just
> error out. If the application tries to set an unsupported test pattern,
> the whole library would crash, maybe we should be a little more
> indulgent.

Yes, not crashing is a good idea :-)

> > > > +
> > > > +     value.set<int32_t>(index);
> > > > +     ctrls.set(V4L2_CID_TEST_PATTERN, value);
> > > > +
> > > > +     return setControls(&ctrls);
> > > > +}
> > > > +
> > > >  /**
> > > >   * \brief Retrieve the best sensor format for a desired output
> > > >   * \param[in] mbusCodes The list of acceptable media bus codes

-- 
Regards,

Laurent Pinchart