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

[Jacopo Mondi]

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.

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
> > >