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

[Hirokazu Honda]

Hi Jacopo,

On Wed, Jun 23, 2021 at 5:06 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Hiro,
>
> On Wed, Jun 23, 2021 at 04:49:46PM +0900, Hirokazu Honda wrote:
> > Hi Jacopo, thank you for reviewing.
> >
> > On Tue, Jun 22, 2021 at 7:30 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Hi Hiro,
> > >
> > > 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;
> > > > +
> > > > +     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.
>

I see what you meant. Thanks!

> Thanks
>    j
>
> >
> > -Hiro
> > > > +
> > > > +     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
> > > > --
> > > > 2.32.0.288.g62a8d224e6-goog
> > > >